From 1eab8be822b08d74e315ba4aef1959ba202a802a Mon Sep 17 00:00:00 2001 From: James Munnelly Date: Mon, 5 Feb 2024 16:36:38 +0000 Subject: [PATCH 1/6] KEP-4193: promote ServiceAccountTokenJTI, ServiceAccountTokenPodNodeInfo and ServiceAccountTokenNodeBindingValidation to beta --- pkg/features/kube_features.go | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/pkg/features/kube_features.go b/pkg/features/kube_features.go index 977bc9807ef..16d1aaee4e9 100644 --- a/pkg/features/kube_features.go +++ b/pkg/features/kube_features.go @@ -694,6 +694,7 @@ const ( // owner: @munnerz // kep: http://kep.k8s.io/4193 // alpha: v1.29 + // beta: v1.30 // // Controls whether JTIs (UUIDs) are embedded into generated service account tokens, and whether these JTIs are // recorded into the audit log for future requests made by these tokens. @@ -709,6 +710,7 @@ const ( // owner: @munnerz // kep: http://kep.k8s.io/4193 // alpha: v1.29 + // beta: v1.30 // // Controls whether the apiserver will validate Node claims in service account tokens. ServiceAccountTokenNodeBindingValidation featuregate.Feature = "ServiceAccountTokenNodeBindingValidation" @@ -716,6 +718,7 @@ const ( // owner: @munnerz // kep: http://kep.k8s.io/4193 // alpha: v1.29 + // beta: v1.30 // // Controls whether the apiserver embeds the node name and uid for the associated node when issuing // service account tokens bound to Pod objects. @@ -1101,13 +1104,13 @@ var defaultKubernetesFeatureGates = map[featuregate.Feature]featuregate.FeatureS SeparateTaintEvictionController: {Default: true, PreRelease: featuregate.Beta}, - ServiceAccountTokenJTI: {Default: false, PreRelease: featuregate.Alpha}, + ServiceAccountTokenJTI: {Default: true, PreRelease: featuregate.Beta}, - ServiceAccountTokenPodNodeInfo: {Default: false, PreRelease: featuregate.Alpha}, + ServiceAccountTokenPodNodeInfo: {Default: true, PreRelease: featuregate.Beta}, ServiceAccountTokenNodeBinding: {Default: false, PreRelease: featuregate.Alpha}, - ServiceAccountTokenNodeBindingValidation: {Default: false, PreRelease: featuregate.Alpha}, + ServiceAccountTokenNodeBindingValidation: {Default: true, PreRelease: featuregate.Beta}, ServiceNodePortStaticSubrange: {Default: true, PreRelease: featuregate.GA, LockToDefault: true}, // GA in 1.29; remove in 1.31 From dfc20d19c8a1f337f913960a8844690cb3b8d0cc Mon Sep 17 00:00:00 2001 From: James Munnelly Date: Mon, 5 Feb 2024 18:11:50 +0000 Subject: [PATCH 2/6] fix integration tests now JTI feature is enabled by default --- test/integration/auth/svcaccttoken_test.go | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/test/integration/auth/svcaccttoken_test.go b/test/integration/auth/svcaccttoken_test.go index 7cb849494a5..ba5896cd862 100644 --- a/test/integration/auth/svcaccttoken_test.go +++ b/test/integration/auth/svcaccttoken_test.go @@ -306,6 +306,8 @@ func TestServiceAccountTokenCreate(t *testing.T) { checkPayload(t, treq.Status.Token, "null", "kubernetes.io", "node") info := doTokenReview(t, cs, treq, false) + // we are not testing the credential-id feature, so delete this value from the returned extra info map + delete(info.Extra, apiserverserviceaccount.CredentialIDKey) if len(info.Extra) != 2 { t.Fatalf("expected Extra have length of 2 but was length %d: %#v", len(info.Extra), info.Extra) } @@ -400,6 +402,8 @@ func TestServiceAccountTokenCreate(t *testing.T) { } info := doTokenReview(t, cs, treq, false) + // we are not testing the credential-id feature, so delete this value from the returned extra info map + delete(info.Extra, apiserverserviceaccount.CredentialIDKey) if len(info.Extra) != len(expectedExtraValues) { t.Fatalf("expected Extra have length of %d but was length %d: %#v", len(expectedExtraValues), len(info.Extra), info.Extra) } From e087acc7917d77bd3a54c1f3b77188fe30b0d7ca Mon Sep 17 00:00:00 2001 From: James Munnelly Date: Tue, 6 Feb 2024 14:03:50 +0000 Subject: [PATCH 3/6] refuse to allow apiserver to startup if ServiceAccountTokenNodeBinding is enabled without ServiceAccountTokenNodeBindingValidation --- pkg/kubeapiserver/options/authentication.go | 5 ++++ .../options/authentication_test.go | 28 ++++++++++++++----- 2 files changed, 26 insertions(+), 7 deletions(-) diff --git a/pkg/kubeapiserver/options/authentication.go b/pkg/kubeapiserver/options/authentication.go index 11501a2f3cb..a7914af535d 100644 --- a/pkg/kubeapiserver/options/authentication.go +++ b/pkg/kubeapiserver/options/authentication.go @@ -260,6 +260,11 @@ func (o *BuiltInAuthenticationOptions) Validate() []error { } } + // verify that if ServiceAccountTokenNodeBinding is enabled, ServiceAccountTokenNodeBindingValidation is also enabled. + if utilfeature.DefaultFeatureGate.Enabled(features.ServiceAccountTokenNodeBinding) && !utilfeature.DefaultFeatureGate.Enabled(features.ServiceAccountTokenNodeBindingValidation) { + allErrors = append(allErrors, fmt.Errorf("the %q feature gate can only be enabled if the %q feature gate is also enabled", features.ServiceAccountTokenNodeBinding, features.ServiceAccountTokenNodeBindingValidation)) + } + if o.WebHook != nil { retryBackoff := o.WebHook.RetryBackoff if retryBackoff != nil && retryBackoff.Steps <= 0 { diff --git a/pkg/kubeapiserver/options/authentication_test.go b/pkg/kubeapiserver/options/authentication_test.go index eed32f1cab8..e2a8f5b874e 100644 --- a/pkg/kubeapiserver/options/authentication_test.go +++ b/pkg/kubeapiserver/options/authentication_test.go @@ -35,19 +35,22 @@ import ( "k8s.io/apiserver/pkg/features" apiserveroptions "k8s.io/apiserver/pkg/server/options" utilfeature "k8s.io/apiserver/pkg/util/feature" + "k8s.io/component-base/featuregate" featuregatetesting "k8s.io/component-base/featuregate/testing" + kubefeatures "k8s.io/kubernetes/pkg/features" kubeauthenticator "k8s.io/kubernetes/pkg/kubeapiserver/authenticator" "k8s.io/utils/pointer" ) func TestAuthenticationValidate(t *testing.T) { testCases := []struct { - name string - testOIDC *OIDCAuthenticationOptions - testSA *ServiceAccountAuthenticationOptions - testWebHook *WebHookAuthenticationOptions - testAuthenticationConfigFile string - expectErr string + name string + testOIDC *OIDCAuthenticationOptions + testSA *ServiceAccountAuthenticationOptions + testWebHook *WebHookAuthenticationOptions + testAuthenticationConfigFile string + expectErr string + enabledFeatures, disabledFeatures []featuregate.Feature }{ { name: "test when OIDC and ServiceAccounts are nil", @@ -226,6 +229,12 @@ func TestAuthenticationValidate(t *testing.T) { }, expectErr: "authentication-config file and oidc-* flags are mutually exclusive", }, + { + name: "fails to validate if ServiceAccountTokenNodeBindingValidation is disabled and ServiceAccountTokenNodeBinding is enabled", + enabledFeatures: []featuregate.Feature{kubefeatures.ServiceAccountTokenNodeBinding}, + disabledFeatures: []featuregate.Feature{kubefeatures.ServiceAccountTokenNodeBindingValidation}, + expectErr: "the \"ServiceAccountTokenNodeBinding\" feature gate can only be enabled if the \"ServiceAccountTokenNodeBindingValidation\" feature gate is also enabled", + }, } for _, testcase := range testCases { @@ -235,7 +244,12 @@ func TestAuthenticationValidate(t *testing.T) { options.ServiceAccounts = testcase.testSA options.WebHook = testcase.testWebHook options.AuthenticationConfigFile = testcase.testAuthenticationConfigFile - + for _, f := range testcase.enabledFeatures { + defer featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, f, true)() + } + for _, f := range testcase.disabledFeatures { + defer featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, f, false)() + } errs := options.Validate() if len(errs) > 0 && (!strings.Contains(utilerrors.NewAggregate(errs).Error(), testcase.expectErr) || testcase.expectErr == "") { t.Errorf("Got err: %v, Expected err: %s", errs, testcase.expectErr) From 4d8c3530f57e8b6f72d544207948952307808a15 Mon Sep 17 00:00:00 2001 From: James Munnelly Date: Wed, 7 Feb 2024 12:30:33 +0000 Subject: [PATCH 4/6] fix regular bound service account token test --- test/integration/auth/svcaccttoken_test.go | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/test/integration/auth/svcaccttoken_test.go b/test/integration/auth/svcaccttoken_test.go index ba5896cd862..ab0be808c3f 100644 --- a/test/integration/auth/svcaccttoken_test.go +++ b/test/integration/auth/svcaccttoken_test.go @@ -237,8 +237,14 @@ func TestServiceAccountTokenCreate(t *testing.T) { checkPayload(t, treq.Status.Token, `"test-svcacct"`, "kubernetes.io", "serviceaccount", "name") info := doTokenReview(t, cs, treq, false) + // we are not testing the credential-id feature, so delete this value from the returned extra info map if info.Extra != nil { - t.Fatalf("expected Extra to be nil but got: %#v", info.Extra) + if _, ok := info.Extra[apiserverserviceaccount.CredentialIDKey]; ok { + delete(info.Extra, apiserverserviceaccount.CredentialIDKey) + } + } + if len(info.Extra) > 0 { + t.Fatalf("expected Extra to be empty but got: %#v", info.Extra) } delSvcAcct() doTokenReview(t, cs, treq, true) From 852c03a49b96d0737830ed8fa63d3614276deba8 Mon Sep 17 00:00:00 2001 From: James Munnelly Date: Wed, 7 Feb 2024 12:31:42 +0000 Subject: [PATCH 5/6] check key is set before deleting from map --- test/integration/auth/svcaccttoken_test.go | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/test/integration/auth/svcaccttoken_test.go b/test/integration/auth/svcaccttoken_test.go index ab0be808c3f..471470dedd7 100644 --- a/test/integration/auth/svcaccttoken_test.go +++ b/test/integration/auth/svcaccttoken_test.go @@ -313,7 +313,9 @@ func TestServiceAccountTokenCreate(t *testing.T) { info := doTokenReview(t, cs, treq, false) // we are not testing the credential-id feature, so delete this value from the returned extra info map - delete(info.Extra, apiserverserviceaccount.CredentialIDKey) + if _, ok := info.Extra[apiserverserviceaccount.CredentialIDKey]; ok { + delete(info.Extra, apiserverserviceaccount.CredentialIDKey) + } if len(info.Extra) != 2 { t.Fatalf("expected Extra have length of 2 but was length %d: %#v", len(info.Extra), info.Extra) } @@ -409,7 +411,9 @@ func TestServiceAccountTokenCreate(t *testing.T) { info := doTokenReview(t, cs, treq, false) // we are not testing the credential-id feature, so delete this value from the returned extra info map - delete(info.Extra, apiserverserviceaccount.CredentialIDKey) + if _, ok := info.Extra[apiserverserviceaccount.CredentialIDKey]; ok { + delete(info.Extra, apiserverserviceaccount.CredentialIDKey) + } if len(info.Extra) != len(expectedExtraValues) { t.Fatalf("expected Extra have length of %d but was length %d: %#v", len(expectedExtraValues), len(info.Extra), info.Extra) } From 105ec3d48fba00c3e2edfa17d3b4e443856ae15c Mon Sep 17 00:00:00 2001 From: James Munnelly Date: Wed, 7 Feb 2024 12:57:05 +0000 Subject: [PATCH 6/6] fix linter failures --- test/integration/auth/svcaccttoken_test.go | 12 +++--------- 1 file changed, 3 insertions(+), 9 deletions(-) diff --git a/test/integration/auth/svcaccttoken_test.go b/test/integration/auth/svcaccttoken_test.go index 471470dedd7..813310d6c4c 100644 --- a/test/integration/auth/svcaccttoken_test.go +++ b/test/integration/auth/svcaccttoken_test.go @@ -239,9 +239,7 @@ func TestServiceAccountTokenCreate(t *testing.T) { info := doTokenReview(t, cs, treq, false) // we are not testing the credential-id feature, so delete this value from the returned extra info map if info.Extra != nil { - if _, ok := info.Extra[apiserverserviceaccount.CredentialIDKey]; ok { - delete(info.Extra, apiserverserviceaccount.CredentialIDKey) - } + delete(info.Extra, apiserverserviceaccount.CredentialIDKey) } if len(info.Extra) > 0 { t.Fatalf("expected Extra to be empty but got: %#v", info.Extra) @@ -313,9 +311,7 @@ func TestServiceAccountTokenCreate(t *testing.T) { info := doTokenReview(t, cs, treq, false) // we are not testing the credential-id feature, so delete this value from the returned extra info map - if _, ok := info.Extra[apiserverserviceaccount.CredentialIDKey]; ok { - delete(info.Extra, apiserverserviceaccount.CredentialIDKey) - } + delete(info.Extra, apiserverserviceaccount.CredentialIDKey) if len(info.Extra) != 2 { t.Fatalf("expected Extra have length of 2 but was length %d: %#v", len(info.Extra), info.Extra) } @@ -411,9 +407,7 @@ func TestServiceAccountTokenCreate(t *testing.T) { info := doTokenReview(t, cs, treq, false) // we are not testing the credential-id feature, so delete this value from the returned extra info map - if _, ok := info.Extra[apiserverserviceaccount.CredentialIDKey]; ok { - delete(info.Extra, apiserverserviceaccount.CredentialIDKey) - } + delete(info.Extra, apiserverserviceaccount.CredentialIDKey) if len(info.Extra) != len(expectedExtraValues) { t.Fatalf("expected Extra have length of %d but was length %d: %#v", len(expectedExtraValues), len(info.Extra), info.Extra) }