From c7e4e831dd580961130750467bc35bfb871bc748 Mon Sep 17 00:00:00 2001 From: Abhijit Hoskeri Date: Tue, 23 Jul 2024 22:14:36 +0000 Subject: [PATCH] node self-register continue if CREATE is 403. tryRegisterWithAPIServer issues node create requests regardless of whether the Node already exists. If there's an admission policy forbidding the creation of new nodes, the policy would return 403 even if the Node actually exists. This change makes Kubelet proceed to attempt to GET the node in case the Create returns 403. The feature `KubeletRegistrationGetOnExistsOnly` restores the previous behavior. --- pkg/features/kube_features.go | 7 + pkg/features/versioned_kube_features.go | 3 + pkg/kubelet/kubelet_node_status.go | 14 +- pkg/kubelet/kubelet_node_status_test.go | 152 +++++++++++------- .../test_data/versioned_feature_list.yaml | 6 + 5 files changed, 126 insertions(+), 56 deletions(-) diff --git a/pkg/features/kube_features.go b/pkg/features/kube_features.go index c30588fb4bb..fe474402f96 100644 --- a/pkg/features/kube_features.go +++ b/pkg/features/kube_features.go @@ -219,6 +219,13 @@ const ( // Remove in v1.33 AllowInsecureKubeletCertificateSigningRequests featuregate.Feature = "AllowInsecureKubeletCertificateSigningRequests" + // owner: @hoskeri + // Deprecated: v1.32 + // + // Restores previous behavior where Kubelet fails self registration if node create returns 403 Forbidden. + // Remove in v1.34 + KubeletRegistrationGetOnExistsOnly featuregate.Feature = "KubeletRegistrationGetOnExistsOnly" + // owner: @HirazawaUi // kep: http://kep.k8s.io/4004 // Deprecated: v1.29 (default off) diff --git a/pkg/features/versioned_kube_features.go b/pkg/features/versioned_kube_features.go index e88426ed0e7..48b2aac3eb6 100644 --- a/pkg/features/versioned_kube_features.go +++ b/pkg/features/versioned_kube_features.go @@ -443,4 +443,7 @@ var defaultVersionedKubernetesFeatureGates = map[featuregate.Feature]featuregate ImageVolume: { {Version: version.MustParse("1.31"), Default: false, PreRelease: featuregate.Alpha}, }, + KubeletRegistrationGetOnExistsOnly: { + {Version: version.MustParse("1.32"), Default: false, PreRelease: featuregate.Deprecated}, + }, } diff --git a/pkg/kubelet/kubelet_node_status.go b/pkg/kubelet/kubelet_node_status.go index ce3b3afa83f..6298144d333 100644 --- a/pkg/kubelet/kubelet_node_status.go +++ b/pkg/kubelet/kubelet_node_status.go @@ -32,12 +32,14 @@ import ( metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/types" "k8s.io/apimachinery/pkg/util/sets" + utilfeature "k8s.io/apiserver/pkg/util/feature" cloudprovider "k8s.io/cloud-provider" cloudproviderapi "k8s.io/cloud-provider/api" nodeutil "k8s.io/component-helpers/node/util" "k8s.io/klog/v2" kubeletapis "k8s.io/kubelet/pkg/apis" v1helper "k8s.io/kubernetes/pkg/apis/core/v1/helper" + "k8s.io/kubernetes/pkg/features" "k8s.io/kubernetes/pkg/kubelet/events" "k8s.io/kubernetes/pkg/kubelet/nodestatus" taintutil "k8s.io/kubernetes/pkg/util/taints" @@ -91,7 +93,16 @@ func (kl *Kubelet) tryRegisterWithAPIServer(node *v1.Node) bool { return true } - if !apierrors.IsAlreadyExists(err) { + switch { + case apierrors.IsAlreadyExists(err): + // Node already exists, proceed to reconcile node. + case apierrors.IsForbidden(err): + // Creating nodes is forbidden, but node may still exist, attempt to get the node. + if utilfeature.DefaultFeatureGate.Enabled(features.KubeletRegistrationGetOnExistsOnly) { + klog.ErrorS(err, "Unable to register node with API server, reason is forbidden", "node", klog.KObj(node)) + return false + } + default: klog.ErrorS(err, "Unable to register node with API server", "node", klog.KObj(node)) return false } @@ -101,6 +112,7 @@ func (kl *Kubelet) tryRegisterWithAPIServer(node *v1.Node) bool { klog.ErrorS(err, "Unable to register node with API server, error getting existing node", "node", klog.KObj(node)) return false } + if existingNode == nil { klog.InfoS("Unable to register node with API server, no node instance returned", "node", klog.KObj(node)) return false diff --git a/pkg/kubelet/kubelet_node_status_test.go b/pkg/kubelet/kubelet_node_status_test.go index 2cee198ecf7..2df53690a8c 100644 --- a/pkg/kubelet/kubelet_node_status_test.go +++ b/pkg/kubelet/kubelet_node_status_test.go @@ -45,13 +45,17 @@ import ( "k8s.io/apimachinery/pkg/util/rand" "k8s.io/apimachinery/pkg/util/strategicpatch" "k8s.io/apimachinery/pkg/util/uuid" + utilversion "k8s.io/apimachinery/pkg/util/version" "k8s.io/apimachinery/pkg/util/wait" + utilfeature "k8s.io/apiserver/pkg/util/feature" clientset "k8s.io/client-go/kubernetes" "k8s.io/client-go/kubernetes/fake" "k8s.io/client-go/rest" core "k8s.io/client-go/testing" + featuregatetesting "k8s.io/component-base/featuregate/testing" "k8s.io/component-base/version" kubeletapis "k8s.io/kubelet/pkg/apis" + "k8s.io/kubernetes/pkg/features" cadvisortest "k8s.io/kubernetes/pkg/kubelet/cadvisor/testing" "k8s.io/kubernetes/pkg/kubelet/cm" kubecontainer "k8s.io/kubernetes/pkg/kubelet/container" @@ -1386,6 +1390,10 @@ func TestTryRegisterWithApiServer(t *testing.T) { ErrStatus: metav1.Status{Reason: metav1.StatusReasonConflict}, } + forbidden := &apierrors.StatusError{ + ErrStatus: metav1.Status{Reason: metav1.StatusReasonForbidden}, + } + newNode := func(cmad bool) *v1.Node { node := &v1.Node{ ObjectMeta: metav1.ObjectMeta{ @@ -1408,18 +1416,19 @@ func TestTryRegisterWithApiServer(t *testing.T) { } cases := []struct { - name string - newNode *v1.Node - existingNode *v1.Node - createError error - getError error - patchError error - deleteError error - expectedResult bool - expectedActions int - testSavedNode bool - savedNodeIndex int - savedNodeCMAD bool + name string + newNode *v1.Node + existingNode *v1.Node + createError error + getError error + patchError error + deleteError error + expectedResult bool + expectedActions int + testSavedNode bool + getOnForbiddenDisabled bool + savedNodeIndex int + savedNodeCMAD bool }{ { name: "success case - new node", @@ -1435,6 +1444,25 @@ func TestTryRegisterWithApiServer(t *testing.T) { expectedResult: true, expectedActions: 2, }, + { + name: "success case - existing node - create forbidden - no change in CMAD", + newNode: newNode(true), + createError: forbidden, + existingNode: newNode(true), + expectedResult: true, + expectedActions: 2, + }, + { + name: "success case - existing node - create forbidden - CMAD disabled", + newNode: newNode(false), + createError: forbidden, + existingNode: newNode(true), + expectedResult: true, + expectedActions: 3, + testSavedNode: true, + savedNodeIndex: 2, + savedNodeCMAD: false, + }, { name: "success case - existing node - CMAD disabled", newNode: newNode(false), @@ -1464,6 +1492,14 @@ func TestTryRegisterWithApiServer(t *testing.T) { expectedResult: false, expectedActions: 1, }, + { + name: "create failed with forbidden - get-on-forbidden feature is disabled", + newNode: newNode(false), + getOnForbiddenDisabled: true, + createError: forbidden, + expectedResult: false, + expectedActions: 1, + }, { name: "get existing node failed", newNode: newNode(false), @@ -1484,55 +1520,61 @@ func TestTryRegisterWithApiServer(t *testing.T) { } for _, tc := range cases { - testKubelet := newTestKubelet(t, false /* controllerAttachDetachEnabled is a don't-care for this test */) - defer testKubelet.Cleanup() - kubelet := testKubelet.kubelet - kubeClient := testKubelet.fakeKubeClient - - kubeClient.AddReactor("create", "nodes", func(action core.Action) (bool, runtime.Object, error) { - return true, nil, tc.createError - }) - kubeClient.AddReactor("get", "nodes", func(action core.Action) (bool, runtime.Object, error) { - // Return an existing (matching) node on get. - return true, tc.existingNode, tc.getError - }) - kubeClient.AddReactor("patch", "nodes", func(action core.Action) (bool, runtime.Object, error) { - if action.GetSubresource() == "status" { - return true, nil, tc.patchError + t.Run(tc.name, func(t *testing.T) { + if tc.getOnForbiddenDisabled { + featuregatetesting.SetFeatureGateEmulationVersionDuringTest(t, utilfeature.DefaultFeatureGate, utilversion.MustParse("1.32")) + featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.KubeletRegistrationGetOnExistsOnly, true) } - return notImplemented(action) - }) - kubeClient.AddReactor("delete", "nodes", func(action core.Action) (bool, runtime.Object, error) { - return true, nil, tc.deleteError - }) - addNotImplatedReaction(kubeClient) + testKubelet := newTestKubelet(t, false /* controllerAttachDetachEnabled is a don't-care for this test */) + defer testKubelet.Cleanup() + kubelet := testKubelet.kubelet + kubeClient := testKubelet.fakeKubeClient - result := kubelet.tryRegisterWithAPIServer(tc.newNode) - require.Equal(t, tc.expectedResult, result, "test [%s]", tc.name) + kubeClient.AddReactor("create", "nodes", func(action core.Action) (bool, runtime.Object, error) { + return true, nil, tc.createError + }) + kubeClient.AddReactor("get", "nodes", func(action core.Action) (bool, runtime.Object, error) { + // Return an existing (matching) node on get. + return true, tc.existingNode, tc.getError + }) + kubeClient.AddReactor("patch", "nodes", func(action core.Action) (bool, runtime.Object, error) { + if action.GetSubresource() == "status" { + return true, nil, tc.patchError + } + return notImplemented(action) + }) + kubeClient.AddReactor("delete", "nodes", func(action core.Action) (bool, runtime.Object, error) { + return true, nil, tc.deleteError + }) + addNotImplatedReaction(kubeClient) - actions := kubeClient.Actions() - assert.Len(t, actions, tc.expectedActions, "test [%s]", tc.name) + result := kubelet.tryRegisterWithAPIServer(tc.newNode) + require.Equal(t, tc.expectedResult, result, "test [%s]", tc.name) - if tc.testSavedNode { - var savedNode *v1.Node + actions := kubeClient.Actions() + assert.Len(t, actions, tc.expectedActions, "test [%s]", tc.name) - t.Logf("actions: %v: %+v", len(actions), actions) - action := actions[tc.savedNodeIndex] - if action.GetVerb() == "create" { - createAction := action.(core.CreateAction) - obj := createAction.GetObject() - require.IsType(t, &v1.Node{}, obj) - savedNode = obj.(*v1.Node) - } else if action.GetVerb() == "patch" { - patchAction := action.(core.PatchActionImpl) - var err error - savedNode, err = applyNodeStatusPatch(tc.existingNode, patchAction.GetPatch()) - require.NoError(t, err) + if tc.testSavedNode { + var savedNode *v1.Node + + t.Logf("actions: %v: %+v", len(actions), actions) + action := actions[tc.savedNodeIndex] + if action.GetVerb() == "create" { + createAction := action.(core.CreateAction) + obj := createAction.GetObject() + require.IsType(t, &v1.Node{}, obj) + savedNode = obj.(*v1.Node) + } else if action.GetVerb() == "patch" { + patchAction := action.(core.PatchActionImpl) + var err error + savedNode, err = applyNodeStatusPatch(tc.existingNode, patchAction.GetPatch()) + require.NoError(t, err) + } + + actualCMAD, _ := strconv.ParseBool(savedNode.Annotations[util.ControllerManagedAttachAnnotation]) + assert.Equal(t, tc.savedNodeCMAD, actualCMAD, "test [%s]", tc.name) } - - actualCMAD, _ := strconv.ParseBool(savedNode.Annotations[util.ControllerManagedAttachAnnotation]) - assert.Equal(t, tc.savedNodeCMAD, actualCMAD, "test [%s]", tc.name) - } + }) } } diff --git a/test/featuregates_linter/test_data/versioned_feature_list.yaml b/test/featuregates_linter/test_data/versioned_feature_list.yaml index 2a5653ac3ba..3461c147927 100644 --- a/test/featuregates_linter/test_data/versioned_feature_list.yaml +++ b/test/featuregates_linter/test_data/versioned_feature_list.yaml @@ -362,6 +362,12 @@ lockToDefault: false preRelease: Alpha version: "1.27" +- name: KubeletRegistrationGetOnExistsOnly + versionedSpecs: + - default: false + lockToDefault: false + preRelease: Deprecated + version: "1.32" - name: KubeletSeparateDiskGC versionedSpecs: - default: false