Refactor options handling

- Adds validation
- Improve code documentation and add tests

Author: olivierlemasle

.gitignore

@@ -1,4 +1,5 @@
 *.swp
 *~
 _output
+apiserver.local.config/
 .idea/

pkg/cmd/builder.go

@@ -100,11 +100,7 @@ func (b *AdapterBase) InstallFlags() {
 			b.CustomMetricsAdapterServerOptions = server.NewCustomMetricsAdapterServerOptions()
 		}
 
-		b.SecureServing.AddFlags(b.FlagSet)
-		b.Authentication.AddFlags(b.FlagSet)
-		b.Authorization.AddFlags(b.FlagSet)
-		b.Audit.AddFlags(b.FlagSet)
-		b.Features.AddFlags(b.FlagSet)
+		b.CustomMetricsAdapterServerOptions.AddFlags(b.FlagSet)
 
 		b.FlagSet.StringVar(&b.RemoteKubeConfigFile, "lister-kubeconfig", b.RemoteKubeConfigFile,
 			"kubeconfig file pointing at the 'core' kubernetes server with enough rights to list "+
@@ -265,6 +261,10 @@ func (b *AdapterBase) Config() (*apiserver.Config, error) {
 		}
 		b.CustomMetricsAdapterServerOptions.OpenAPIConfig = b.OpenAPIConfig
 
+		if err := b.CustomMetricsAdapterServerOptions.Validate(); err != nil {
+			return nil, err
+		}
+
 		config, err := b.CustomMetricsAdapterServerOptions.Config()
 		if err != nil {
 			return nil, err

pkg/cmd/server/start.go renamed to pkg/cmd/server/options.go

@@ -14,31 +14,39 @@ See the License for the specific language governing permissions and
 limitations under the License.
 */
 
+// Package server provides configuration options for the metrics API server.
 package server
 
 import (
 	"fmt"
 	"net"
 
+	"github.com/spf13/pflag"
+
+	utilerrors "k8s.io/apimachinery/pkg/util/errors"
 	genericapiserver "k8s.io/apiserver/pkg/server"
 	genericoptions "k8s.io/apiserver/pkg/server/options"
 	openapicommon "k8s.io/kube-openapi/pkg/common"
 
 	"sigs.k8s.io/custom-metrics-apiserver/pkg/apiserver"
 )
 
+// CustomMetricsAdapterServerOptions contains the of options used to configure
+// the metrics API server.
+//
+// It is based on a subset of [genericoptions.RecommendedOptions].
 type CustomMetricsAdapterServerOptions struct {
-	// genericoptions.ReccomendedOptions - EtcdOptions
 	SecureServing  *genericoptions.SecureServingOptionsWithLoopback
 	Authentication *genericoptions.DelegatingAuthenticationOptions
 	Authorization  *genericoptions.DelegatingAuthorizationOptions
 	Audit          *genericoptions.AuditOptions
 	Features       *genericoptions.FeatureOptions
 
-	// OpenAPIConfig
 	OpenAPIConfig *openapicommon.Config
 }
 
+// NewCustomMetricsAdapterServerOptions creates a new instance of
+// CustomMetricsAdapterServerOptions with its default values.
 func NewCustomMetricsAdapterServerOptions() *CustomMetricsAdapterServerOptions {
 	o := &CustomMetricsAdapterServerOptions{
 		SecureServing:  genericoptions.NewSecureServingOptions().WithLoopback(),
@@ -51,14 +59,27 @@ func NewCustomMetricsAdapterServerOptions() *CustomMetricsAdapterServerOptions {
 	return o
 }
 
-func (o CustomMetricsAdapterServerOptions) Validate(args []string) error {
-	return nil
+// Validate validates CustomMetricsAdapterServerOptions
+func (o CustomMetricsAdapterServerOptions) Validate() error {
+	errors := []error{}
+	errors = append(errors, o.SecureServing.Validate()...)
+	errors = append(errors, o.Authentication.Validate()...)
+	errors = append(errors, o.Authorization.Validate()...)
+	errors = append(errors, o.Audit.Validate()...)
+	errors = append(errors, o.Features.Validate()...)
+	return utilerrors.NewAggregate(errors)
 }
 
-func (o *CustomMetricsAdapterServerOptions) Complete() error {
-	return nil
+// AddFlags adds the flags defined for the options, to the given flagset.
+func (o *CustomMetricsAdapterServerOptions) AddFlags(fs *pflag.FlagSet) {
+	o.SecureServing.AddFlags(fs)
+	o.Authentication.AddFlags(fs)
+	o.Authorization.AddFlags(fs)
+	o.Audit.AddFlags(fs)
+	o.Features.AddFlags(fs)
 }
 
+// Config returns configuration for the API server given CustomMetricsAdapterServerOptions
 func (o CustomMetricsAdapterServerOptions) Config() (*apiserver.Config, error) {
 	// TODO have a "real" external address (have an AdvertiseAddress?)
 	if err := o.SecureServing.MaybeDefaultWithSelfSignedCerts("localhost", nil, []net.IP{net.ParseIP("127.0.0.1")}); err != nil {
@@ -69,14 +90,12 @@ func (o CustomMetricsAdapterServerOptions) Config() (*apiserver.Config, error) {
 	if err := o.SecureServing.ApplyTo(&serverConfig.SecureServing, &serverConfig.LoopbackClientConfig); err != nil {
 		return nil, err
 	}
-
 	if err := o.Authentication.ApplyTo(&serverConfig.Authentication, serverConfig.SecureServing, nil); err != nil {
 		return nil, err
 	}
 	if err := o.Authorization.ApplyTo(&serverConfig.Authorization); err != nil {
 		return nil, err
 	}
-
 	if err := o.Audit.ApplyTo(serverConfig); err != nil {
 		return nil, err
 	}

pkg/cmd/server/options_test.go

@@ -0,0 +1,94 @@
+/*
+Copyright 2022 The Kubernetes Authors.
+
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+
+    http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+
+package server
+
+import (
+	"testing"
+
+	"github.com/spf13/pflag"
+	"github.com/stretchr/testify/assert"
+)
+
+func TestOptions(t *testing.T) {
+	cases := []struct {
+		testName  string
+		args      []string
+		shouldErr bool
+	}{
+		{
+			testName:  "only-secure-port",
+			args:      []string{"--secure-port=6443"}, // default is 443, which requires privileges
+			shouldErr: false,
+		},
+		{
+			testName:  "secure-port-0",
+			args:      []string{"--secure-port=0"}, // means: "don't serve HTTPS at all"
+			shouldErr: false,
+		},
+		{
+			testName:  "invalid-secure-port",
+			args:      []string{"--secure-port=-1"},
+			shouldErr: true,
+		},
+		{
+			testName:  "empty-header",
+			args:      []string{"--secure-port=6443", "--requestheader-username-headers=\" \""},
+			shouldErr: true,
+		},
+		{
+			testName:  "invalid-audit-log-format",
+			args:      []string{"--secure-port=6443", "--audit-log-path=file", "--audit-log-format=txt"},
+			shouldErr: true,
+		},
+	}
+
+	for _, c := range cases {
+		t.Run(c.testName, func(t *testing.T) {
+			o := NewCustomMetricsAdapterServerOptions()
+
+			// Unit tests have no Kubernetes cluster access
+			o.Authentication.RemoteKubeConfigFileOptional = true
+			o.Authorization.RemoteKubeConfigFileOptional = true
+
+			flagSet := pflag.NewFlagSet("", pflag.PanicOnError)
+			o.AddFlags(flagSet)
+			err := flagSet.Parse(c.args)
+			assert.NoErrorf(t, err, "Error while parsing flags")
+
+			err = o.Validate()
+			if c.shouldErr {
+				assert.Errorf(t, err, "Expected error while validating options")
+			} else {
+				assert.NoErrorf(t, err, "Error while validating options")
+			}
+
+			if err != nil || c.shouldErr {
+				return
+			}
+
+			config, err := o.Config()
+			defer func() {
+				// Close the listener, if any
+				if config != nil && config.GenericConfig.SecureServing != nil && config.GenericConfig.SecureServing.Listener != nil {
+					err := config.GenericConfig.SecureServing.Listener.Close()
+					assert.NoError(t, err)
+				}
+			}()
+			assert.NoErrorf(t, err, "Error creating config")
+		})
+	}
+}