pkg/kubeapiserver/options: Improving test coverage (#114234)

* pkg/kubeapiserver/options: Improving test coverage

Signed-off-by: TommyStarK <thomasmilox@gmail.com>

* pkg/kubeapiserver/options: Improving test coverage

Add a snippet of the expected error string related to the aspect being tested

Signed-off-by: TommyStarK <thomasmilox@gmail.com>

Signed-off-by: TommyStarK <thomasmilox@gmail.com>

pkg/kubeapiserver/options/admission_test.go

@@ -19,6 +19,9 @@ package options
 import (
 	"reflect"
 	"testing"
+
+	"github.com/spf13/pflag"
+	"github.com/stretchr/testify/assert"
 )
 
 func TestValidate(t *testing.T) {
@@ -51,6 +54,12 @@ func TestValidate(t *testing.T) {
 	if errs := options.Validate(); len(errs) > 0 {
 		t.Errorf("Unexpected err: %v", errs)
 	}
+
+	// nil pointer
+	options = nil
+	if errs := options.Validate(); errs != nil {
+		t.Errorf("expected no errors, error found %+v", errs)
+	}
 }
 
 func TestComputeEnabledAdmission(t *testing.T) {
@@ -86,3 +95,22 @@ func TestComputeEnabledAdmission(t *testing.T) {
 		})
 	}
 }
+
+func TestAdmissionOptionsAddFlags(t *testing.T) {
+	var args = []string{
+		"--enable-admission-plugins=foo,bar,baz",
+		"--admission-control-config-file=admission_control_config.yaml",
+	}
+
+	opts := NewAdmissionOptions()
+	pf := pflag.NewFlagSet("test-admission-opts", pflag.ContinueOnError)
+	opts.AddFlags(pf)
+
+	if err := pf.Parse(args); err != nil {
+		t.Fatal(err)
+	}
+
+	// using assert because cannot compare neither pointer nor function of underlying GenericAdmission
+	assert.Equal(t, opts.GenericAdmission.ConfigFile, "admission_control_config.yaml")
+	assert.Equal(t, opts.GenericAdmission.EnablePlugins, []string{"foo", "bar", "baz"})
+}

pkg/kubeapiserver/options/authentication_test.go

@@ -23,8 +23,10 @@ import (
 	"time"
 
 	"github.com/google/go-cmp/cmp"
+	"github.com/spf13/pflag"
 
 	utilerrors "k8s.io/apimachinery/pkg/util/errors"
+	"k8s.io/apimachinery/pkg/util/wait"
 	"k8s.io/apiserver/pkg/authentication/authenticator"
 	"k8s.io/apiserver/pkg/authentication/authenticatorfactory"
 	"k8s.io/apiserver/pkg/authentication/request/headerrequest"
@@ -34,10 +36,11 @@ import (
 
 func TestAuthenticationValidate(t *testing.T) {
 	testCases := []struct {
-		name      string
-		testOIDC  *OIDCAuthenticationOptions
-		testSA    *ServiceAccountAuthenticationOptions
-		expectErr string
+		name        string
+		testOIDC    *OIDCAuthenticationOptions
+		testSA      *ServiceAccountAuthenticationOptions
+		testWebHook *WebHookAuthenticationOptions
+		expectErr   string
 	}{
 		{
 			name: "test when OIDC and ServiceAccounts are nil",
@@ -133,6 +136,62 @@ func TestAuthenticationValidate(t *testing.T) {
 			},
 			expectErr: "service-account-issuer \"http://[::1]:namedport\" contained a ':' but was not a valid URL",
 		},
+		{
+			name: "test when ServiceAccounts has invalid JWKSURI",
+			testOIDC: &OIDCAuthenticationOptions{
+				UsernameClaim: "sub",
+				SigningAlgs:   []string{"RS256"},
+				IssuerURL:     "testIssuerURL",
+				ClientID:      "testClientID",
+			},
+			testSA: &ServiceAccountAuthenticationOptions{
+				KeyFiles: []string{"cert", "key"},
+				Issuers:  []string{"http://foo.bar.com"},
+				JWKSURI:  "https://host:port",
+			},
+			expectErr: "service-account-jwks-uri must be a valid URL: parse \"https://host:port\": invalid port \":port\" after host",
+		},
+		{
+			name: "test when ServiceAccounts has invalid JWKSURI (not https scheme)",
+			testOIDC: &OIDCAuthenticationOptions{
+				UsernameClaim: "sub",
+				SigningAlgs:   []string{"RS256"},
+				IssuerURL:     "testIssuerURL",
+				ClientID:      "testClientID",
+			},
+			testSA: &ServiceAccountAuthenticationOptions{
+				KeyFiles: []string{"cert", "key"},
+				Issuers:  []string{"http://foo.bar.com"},
+				JWKSURI:  "http://baz.com",
+			},
+			expectErr: "service-account-jwks-uri requires https scheme, parsed as: http://baz.com",
+		},
+		{
+			name: "test when WebHook has invalid retry attempts",
+			testOIDC: &OIDCAuthenticationOptions{
+				UsernameClaim: "sub",
+				SigningAlgs:   []string{"RS256"},
+				IssuerURL:     "testIssuerURL",
+				ClientID:      "testClientID",
+			},
+			testSA: &ServiceAccountAuthenticationOptions{
+				KeyFiles: []string{"cert", "key"},
+				Issuers:  []string{"http://foo.bar.com"},
+				JWKSURI:  "https://baz.com",
+			},
+			testWebHook: &WebHookAuthenticationOptions{
+				ConfigFile: "configfile",
+				Version:    "v1",
+				CacheTTL:   60 * time.Second,
+				RetryBackoff: &wait.Backoff{
+					Duration: 500 * time.Millisecond,
+					Factor:   1.5,
+					Jitter:   0.2,
+					Steps:    0,
+				},
+			},
+			expectErr: "number of webhook retry attempts must be greater than 0, but is: 0",
+		},
 	}
 
 	for _, testcase := range testCases {
@@ -140,6 +199,7 @@ func TestAuthenticationValidate(t *testing.T) {
 			options := NewBuiltInAuthenticationOptions()
 			options.OIDC = testcase.testOIDC
 			options.ServiceAccounts = testcase.testSA
+			options.WebHook = testcase.testWebHook
 
 			errs := options.Validate()
 			if len(errs) > 0 && (!strings.Contains(utilerrors.NewAggregate(errs).Error(), testcase.expectErr) || testcase.expectErr == "") {
@@ -239,3 +299,89 @@ func TestToAuthenticationConfig(t *testing.T) {
 		t.Error(cmp.Diff(resultConfig, expectConfig))
 	}
 }
+
+func TestBuiltInAuthenticationOptionsAddFlags(t *testing.T) {
+	var args = []string{
+		"--api-audiences=foo",
+		"--anonymous-auth=true",
+		"--enable-bootstrap-token-auth=true",
+		"--oidc-issuer-url=https://baz.com",
+		"--oidc-client-id=client-id",
+		"--oidc-ca-file=cert",
+		"--oidc-username-prefix=-",
+		"--client-ca-file=client-cacert",
+		"--requestheader-client-ca-file=testdata/root.pem",
+		"--requestheader-username-headers=x-remote-user-custom",
+		"--requestheader-group-headers=x-remote-group-custom",
+		"--requestheader-allowed-names=kube-aggregator",
+		"--service-account-key-file=cert",
+		"--service-account-key-file=key",
+		"--service-account-issuer=http://foo.bar.com",
+		"--service-account-jwks-uri=https://qux.com",
+		"--token-auth-file=tokenfile",
+		"--authentication-token-webhook-config-file=webhook_config.yaml",
+		"--authentication-token-webhook-cache-ttl=180s",
+	}
+
+	expected := &BuiltInAuthenticationOptions{
+		APIAudiences: []string{"foo"},
+		Anonymous: &AnonymousAuthenticationOptions{
+			Allow: true,
+		},
+		BootstrapToken: &BootstrapTokenAuthenticationOptions{
+			Enable: true,
+		},
+		ClientCert: &apiserveroptions.ClientCertAuthenticationOptions{
+			ClientCA: "client-cacert",
+		},
+		OIDC: &OIDCAuthenticationOptions{
+			CAFile:         "cert",
+			ClientID:       "client-id",
+			IssuerURL:      "https://baz.com",
+			UsernameClaim:  "sub",
+			UsernamePrefix: "-",
+			SigningAlgs:    []string{"RS256"},
+		},
+		RequestHeader: &apiserveroptions.RequestHeaderAuthenticationOptions{
+			ClientCAFile:    "testdata/root.pem",
+			UsernameHeaders: []string{"x-remote-user-custom"},
+			GroupHeaders:    []string{"x-remote-group-custom"},
+			AllowedNames:    []string{"kube-aggregator"},
+		},
+		ServiceAccounts: &ServiceAccountAuthenticationOptions{
+			KeyFiles:         []string{"cert", "key"},
+			Lookup:           true,
+			Issuers:          []string{"http://foo.bar.com"},
+			JWKSURI:          "https://qux.com",
+			ExtendExpiration: true,
+		},
+		TokenFile: &TokenFileAuthenticationOptions{
+			TokenFile: "tokenfile",
+		},
+		WebHook: &WebHookAuthenticationOptions{
+			ConfigFile: "webhook_config.yaml",
+			Version:    "v1beta1",
+			CacheTTL:   180 * time.Second,
+			RetryBackoff: &wait.Backoff{
+				Duration: 500 * time.Millisecond,
+				Factor:   1.5,
+				Jitter:   0.2,
+				Steps:    5,
+			},
+		},
+		TokenSuccessCacheTTL: 10 * time.Second,
+		TokenFailureCacheTTL: 0 * time.Second,
+	}
+
+	opts := NewBuiltInAuthenticationOptions().WithAll()
+	pf := pflag.NewFlagSet("test-builtin-authentication-opts", pflag.ContinueOnError)
+	opts.AddFlags(pf)
+
+	if err := pf.Parse(args); err != nil {
+		t.Fatal(err)
+	}
+
+	if !reflect.DeepEqual(opts, expected) {
+		t.Error(cmp.Diff(opts, expected))
+	}
+}

pkg/kubeapiserver/options/authorization_test.go

@@ -17,8 +17,17 @@ limitations under the License.
 package options
 
 import (
+	"fmt"
+	"reflect"
+	"strings"
 	"testing"
+	"time"
 
+	"github.com/google/go-cmp/cmp"
+	"github.com/spf13/pflag"
+
+	utilerrors "k8s.io/apimachinery/pkg/util/errors"
+	"k8s.io/apimachinery/pkg/util/wait"
 	"k8s.io/kubernetes/pkg/kubeapiserver/authorizer/modes"
 )
 
@@ -26,21 +35,31 @@ func TestAuthzValidate(t *testing.T) {
 	examplePolicyFile := "../../auth/authorizer/abac/example_policy_file.jsonl"
 
 	testCases := []struct {
-		name              string
-		modes             []string
-		policyFile        string
-		webhookConfigFile string
-		expectErr         bool
+		name                 string
+		modes                []string
+		policyFile           string
+		webhookConfigFile    string
+		webhookRetryBackoff  *wait.Backoff
+		expectErr            bool
+		expectErrorSubString string
 	}{
 		{
-			name:      "Unknown modes should return errors",
-			modes:     []string{"DoesNotExist"},
-			expectErr: true,
+			name:                 "Unknown modes should return errors",
+			modes:                []string{"DoesNotExist"},
+			expectErr:            true,
+			expectErrorSubString: "is not a valid mode",
 		},
 		{
-			name:      "At least one authorizationMode is necessary",
-			modes:     []string{},
-			expectErr: true,
+			name:                 "At least one authorizationMode is necessary",
+			modes:                []string{},
+			expectErr:            true,
+			expectErrorSubString: "at least one authorization-mode must be passed",
+		},
+		{
+			name:                 "ModeAlwaysAllow specified more than once",
+			modes:                []string{modes.ModeAlwaysAllow, modes.ModeAlwaysAllow},
+			expectErr:            true,
+			expectErrorSubString: "has mode specified more than once",
 		},
 		{
 			name:      "ModeAlwaysAllow and ModeAlwaysDeny should return without authorizationPolicyFile",
@@ -48,16 +67,18 @@ func TestAuthzValidate(t *testing.T) {
 			expectErr: false,
 		},
 		{
-			name:      "ModeABAC requires a policy file",
-			modes:     []string{modes.ModeAlwaysAllow, modes.ModeAlwaysDeny, modes.ModeABAC},
-			expectErr: true,
+			name:                 "ModeABAC requires a policy file",
+			modes:                []string{modes.ModeAlwaysAllow, modes.ModeAlwaysDeny, modes.ModeABAC},
+			expectErr:            true,
+			expectErrorSubString: "authorization-mode ABAC's authorization policy file not passed",
 		},
 		{
-			name:              "Authorization Policy file cannot be used without ModeABAC",
-			modes:             []string{modes.ModeAlwaysAllow, modes.ModeAlwaysDeny},
-			policyFile:        examplePolicyFile,
-			webhookConfigFile: "",
-			expectErr:         true,
+			name:                 "Authorization Policy file cannot be used without ModeABAC",
+			modes:                []string{modes.ModeAlwaysAllow, modes.ModeAlwaysDeny},
+			policyFile:           examplePolicyFile,
+			webhookConfigFile:    "",
+			expectErr:            true,
+			expectErrorSubString: "cannot specify --authorization-policy-file without mode ABAC",
 		},
 		{
 			name:              "ModeABAC should not error if a valid policy path is provided",
@@ -67,15 +88,17 @@ func TestAuthzValidate(t *testing.T) {
 			expectErr:         false,
 		},
 		{
-			name:      "ModeWebhook requires a config file",
-			modes:     []string{modes.ModeWebhook},
-			expectErr: true,
+			name:                 "ModeWebhook requires a config file",
+			modes:                []string{modes.ModeWebhook},
+			expectErr:            true,
+			expectErrorSubString: "authorization-mode Webhook's authorization config file not passed",
 		},
 		{
-			name:              "Cannot provide webhook config file without ModeWebhook",
-			modes:             []string{modes.ModeAlwaysAllow},
-			webhookConfigFile: "authz_webhook_config.yaml",
-			expectErr:         true,
+			name:                 "Cannot provide webhook config file without ModeWebhook",
+			modes:                []string{modes.ModeAlwaysAllow},
+			webhookConfigFile:    "authz_webhook_config.yaml",
+			expectErr:            true,
+			expectErrorSubString: "cannot specify --authorization-webhook-config-file without mode Webhook",
 		},
 		{
 			name:              "ModeWebhook should not error if a valid config file is provided",
@@ -83,6 +106,14 @@ func TestAuthzValidate(t *testing.T) {
 			webhookConfigFile: "authz_webhook_config.yaml",
 			expectErr:         false,
 		},
+		{
+			name:                 "ModeWebhook should error if an invalid number of webhook retry attempts is provided",
+			modes:                []string{modes.ModeWebhook},
+			webhookConfigFile:    "authz_webhook_config.yaml",
+			webhookRetryBackoff:  &wait.Backoff{Steps: 0},
+			expectErr:            true,
+			expectErrorSubString: "number of webhook retry attempts must be greater than 0",
+		},
 	}
 
 	for _, testcase := range testCases {
@@ -90,6 +121,7 @@ func TestAuthzValidate(t *testing.T) {
 			options := NewBuiltInAuthorizationOptions()
 			options.Modes = testcase.modes
 			options.WebhookConfigFile = testcase.webhookConfigFile
+			options.WebhookRetryBackoff = testcase.webhookRetryBackoff
 			options.PolicyFile = testcase.policyFile
 
 			errs := options.Validate()
@@ -99,6 +131,49 @@ func TestAuthzValidate(t *testing.T) {
 			if testcase.expectErr && len(errs) == 0 {
 				t.Errorf("should return an error")
 			}
+			if len(errs) > 0 && testcase.expectErr {
+				if !strings.Contains(utilerrors.NewAggregate(errs).Error(), testcase.expectErrorSubString) {
+					t.Errorf("exepected to found error: %s, but no error found", testcase.expectErrorSubString)
+				}
+			}
 		})
 	}
 }
+
+func TestBuiltInAuthorizationOptionsAddFlags(t *testing.T) {
+	var args = []string{
+		fmt.Sprintf("--authorization-mode=%s,%s,%s,%s", modes.ModeAlwaysAllow, modes.ModeAlwaysDeny, modes.ModeABAC, modes.ModeWebhook),
+		"--authorization-policy-file=policy_file.json",
+		"--authorization-webhook-config-file=webhook_config_file.yaml",
+		"--authorization-webhook-version=v1",
+		"--authorization-webhook-cache-authorized-ttl=60s",
+		"--authorization-webhook-cache-unauthorized-ttl=30s",
+	}
+
+	expected := &BuiltInAuthorizationOptions{
+		Modes:                       []string{modes.ModeAlwaysAllow, modes.ModeAlwaysDeny, modes.ModeABAC, modes.ModeWebhook},
+		PolicyFile:                  "policy_file.json",
+		WebhookConfigFile:           "webhook_config_file.yaml",
+		WebhookVersion:              "v1",
+		WebhookCacheAuthorizedTTL:   60 * time.Second,
+		WebhookCacheUnauthorizedTTL: 30 * time.Second,
+		WebhookRetryBackoff: &wait.Backoff{
+			Duration: 500 * time.Millisecond,
+			Factor:   1.5,
+			Jitter:   0.2,
+			Steps:    5,
+		},
+	}
+
+	opts := NewBuiltInAuthorizationOptions()
+	pf := pflag.NewFlagSet("test-builtin-authorization-opts", pflag.ContinueOnError)
+	opts.AddFlags(pf)
+
+	if err := pf.Parse(args); err != nil {
+		t.Fatal(err)
+	}
+
+	if !reflect.DeepEqual(opts, expected) {
+		t.Error(cmp.Diff(opts, expected))
+	}
+}