Skip to content

Commit

Permalink
Refactor options handling
Browse files Browse the repository at this point in the history
- Adds validation
- Improve code documentation and add tests
  • Loading branch information
olivierlemasle committed Dec 8, 2022
1 parent af23382 commit 3debc97
Show file tree
Hide file tree
Showing 4 changed files with 177 additions and 32 deletions.
1 change: 1 addition & 0 deletions .gitignore
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
*.swp
*~
_output
apiserver.local.config/
.idea/
24 changes: 14 additions & 10 deletions pkg/cmd/builder.go
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ import (
"github.com/spf13/pflag"

apimeta "k8s.io/apimachinery/pkg/api/meta"
utilerrors "k8s.io/apimachinery/pkg/util/errors"
openapinamer "k8s.io/apiserver/pkg/endpoints/openapi"
genericapiserver "k8s.io/apiserver/pkg/server"
"k8s.io/client-go/discovery"
Expand All @@ -35,7 +36,7 @@ import (
openapicommon "k8s.io/kube-openapi/pkg/common"

"sigs.k8s.io/custom-metrics-apiserver/pkg/apiserver"
"sigs.k8s.io/custom-metrics-apiserver/pkg/cmd/server"
"sigs.k8s.io/custom-metrics-apiserver/pkg/cmd/options"
"sigs.k8s.io/custom-metrics-apiserver/pkg/dynamicmapper"
generatedcore "sigs.k8s.io/custom-metrics-apiserver/pkg/generated/openapi/core"
generatedcustommetrics "sigs.k8s.io/custom-metrics-apiserver/pkg/generated/openapi/custommetrics"
Expand All @@ -56,7 +57,7 @@ import (
// Methods on this struct are not safe to call from multiple goroutines without
// external synchronization.
type AdapterBase struct {
*server.CustomMetricsAdapterServerOptions
*options.CustomMetricsAdapterServerOptions

// Name is the name of the API server. It defaults to custom-metrics-adapter
Name string
Expand Down Expand Up @@ -97,14 +98,10 @@ func (b *AdapterBase) InstallFlags() {
b.initFlagSet()
b.flagOnce.Do(func() {
if b.CustomMetricsAdapterServerOptions == nil {
b.CustomMetricsAdapterServerOptions = server.NewCustomMetricsAdapterServerOptions()
b.CustomMetricsAdapterServerOptions = options.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 "+
Expand Down Expand Up @@ -265,11 +262,18 @@ func (b *AdapterBase) Config() (*apiserver.Config, error) {
}
b.CustomMetricsAdapterServerOptions.OpenAPIConfig = b.OpenAPIConfig

config, err := b.CustomMetricsAdapterServerOptions.Config()
if errList := b.CustomMetricsAdapterServerOptions.Validate(); len(errList) > 0 {
return nil, utilerrors.NewAggregate(errList)
}

serverConfig := genericapiserver.NewConfig(apiserver.Codecs)
err := b.CustomMetricsAdapterServerOptions.ApplyTo(serverConfig)
if err != nil {
return nil, err
}
b.config = config
b.config = &apiserver.Config{
GenericConfig: serverConfig,
}
}

return b.config, nil
Expand Down
56 changes: 34 additions & 22 deletions pkg/cmd/server/start.go → pkg/cmd/options/options.go
Original file line number Diff line number Diff line change
Expand Up @@ -14,31 +14,36 @@ See the License for the specific language governing permissions and
limitations under the License.
*/

package server
// Package options provides configuration options for the metrics API server.
package options

import (
"fmt"
"net"

"github.com/spf13/pflag"

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(),
Expand All @@ -51,43 +56,50 @@ 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 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)
}

func (o CustomMetricsAdapterServerOptions) Config() (*apiserver.Config, error) {
// ApplyTo applies CustomMetricsAdapterServerOptions to the server configuration.
func (o *CustomMetricsAdapterServerOptions) ApplyTo(serverConfig *genericapiserver.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 {
return nil, fmt.Errorf("error creating self-signed certificates: %v", err)
return fmt.Errorf("error creating self-signed certificates: %v", err)
}

serverConfig := genericapiserver.NewConfig(apiserver.Codecs)
if err := o.SecureServing.ApplyTo(&serverConfig.SecureServing, &serverConfig.LoopbackClientConfig); err != nil {
return nil, err
return err
}

if err := o.Authentication.ApplyTo(&serverConfig.Authentication, serverConfig.SecureServing, nil); err != nil {
return nil, err
return err
}
if err := o.Authorization.ApplyTo(&serverConfig.Authorization); err != nil {
return nil, err
return err
}

if err := o.Audit.ApplyTo(serverConfig); err != nil {
return nil, err
return err
}

// enable OpenAPI schemas
if o.OpenAPIConfig != nil {
serverConfig.OpenAPIConfig = o.OpenAPIConfig
}

config := &apiserver.Config{
GenericConfig: serverConfig,
}
return config, nil
return nil
}
128 changes: 128 additions & 0 deletions pkg/cmd/options/options_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,128 @@
/*
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 options

import (
"testing"

"github.com/spf13/pflag"
"github.com/stretchr/testify/assert"

utilerrors "k8s.io/apimachinery/pkg/util/errors"
genericapiserver "k8s.io/apiserver/pkg/server"

"sigs.k8s.io/custom-metrics-apiserver/pkg/apiserver"
)

func TestValidate(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()

flagSet := pflag.NewFlagSet("", pflag.PanicOnError)
o.AddFlags(flagSet)
err := flagSet.Parse(c.args)
assert.NoErrorf(t, err, "Error while parsing flags")

errList := o.Validate()
err = utilerrors.NewAggregate(errList)
if c.shouldErr {
assert.Errorf(t, err, "Expected error while validating options")
} else {
assert.NoErrorf(t, err, "Error while validating options")
}
})
}
}

func TestApplyTo(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
},
{
testName: "secure-port-0",
args: []string{"--secure-port=0"}, // means: "don't serve HTTPS at all"
shouldErr: false,
},
}

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")

serverConfig := genericapiserver.NewConfig(apiserver.Codecs)
err = o.ApplyTo(serverConfig)

defer func() {
// Close the listener, if any
if serverConfig.SecureServing != nil && serverConfig.SecureServing.Listener != nil {
err := serverConfig.SecureServing.Listener.Close()
assert.NoError(t, err)
}
}()

assert.NoErrorf(t, err, "Error while applying options")
})
}
}

0 comments on commit 3debc97

Please sign in to comment.