diff --git a/internal/controller/helper.go b/internal/controller/helper.go index 3dfb12c..6efefda 100644 --- a/internal/controller/helper.go +++ b/internal/controller/helper.go @@ -17,9 +17,48 @@ limitations under the License. package controller import ( + "encoding/json" + corev1 "k8s.io/api/core/v1" ) +const ( + // bootstrapStateAnnotation is the node annotation key that stores a JSON map + // of rule names to their bootstrap completion status. + // Using a single annotation with a structured payload avoids Kubernetes' + // 63-character name-segment limit on annotation keys and keeps all rule + // state visible in one place — the same pattern kubectl uses for + // kubectl.kubernetes.io/last-applied-configuration. + bootstrapStateAnnotation = "readiness.k8s.io/bootstrap-state" +) + +// getBootstrapState deserializes the bootstrap state map from the node annotation. +// It returns an empty map if the annotation is absent or unparseable. +func getBootstrapState(node *corev1.Node) map[string]bool { + state := make(map[string]bool) + if node.Annotations == nil { + return state + } + raw, ok := node.Annotations[bootstrapStateAnnotation] + if !ok || raw == "" { + return state + } + // Best-effort parse; ignore malformed values. + _ = json.Unmarshal([]byte(raw), &state) + return state +} + +// serializeBootstrapState serializes the bootstrap state map to a JSON string +// for storage in the node annotation. Errors are silently ignored and return "{}". +func serializeBootstrapState(state map[string]bool) string { + b, err := json.Marshal(state) + if err != nil { + return "{}" + } + return string(b) +} + + // conditionsEqual checks if two condition slices are equal. func conditionsEqual(a, b []corev1.NodeCondition) bool { if len(a) != len(b) { diff --git a/internal/controller/helper_unit_test.go b/internal/controller/helper_unit_test.go new file mode 100644 index 0000000..7b13e12 --- /dev/null +++ b/internal/controller/helper_unit_test.go @@ -0,0 +1,113 @@ +/* +Copyright 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 controller + +import ( + "testing" + + . "github.com/onsi/gomega" + corev1 "k8s.io/api/core/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" +) + +func TestGetBootstrapState(t *testing.T) { + g := NewWithT(t) + + tests := []struct { + name string + annotations map[string]string + expected map[string]bool + }{ + { + name: "nil annotations returns empty map", + annotations: nil, + expected: map[string]bool{}, + }, + { + name: "missing annotation key returns empty map", + annotations: map[string]string{"other-key": "other-value"}, + expected: map[string]bool{}, + }, + { + name: "empty annotation value returns empty map", + annotations: map[string]string{bootstrapStateAnnotation: ""}, + expected: map[string]bool{}, + }, + { + name: "malformed JSON returns empty map", + annotations: map[string]string{bootstrapStateAnnotation: "not-json"}, + expected: map[string]bool{}, + }, + { + name: "valid JSON with one rule", + annotations: map[string]string{bootstrapStateAnnotation: `{"my-rule":true}`}, + expected: map[string]bool{"my-rule": true}, + }, + { + name: "valid JSON with multiple rules", + annotations: map[string]string{bootstrapStateAnnotation: `{"rule-a":true,"rule-b":true}`}, + expected: map[string]bool{"rule-a": true, "rule-b": true}, + }, + { + name: "very long rule name stored as map key — no length constraint", + annotations: map[string]string{ + bootstrapStateAnnotation: `{"my-very-long-rule-name-that-is-definitely-going-to-exceed-the-63-character-annotation-key-limit":true}`, + }, + expected: map[string]bool{ + "my-very-long-rule-name-that-is-definitely-going-to-exceed-the-63-character-annotation-key-limit": true, + }, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + node := &corev1.Node{ + ObjectMeta: metav1.ObjectMeta{ + Annotations: tt.annotations, + }, + } + result := getBootstrapState(node) + g.Expect(result).To(Equal(tt.expected)) + }) + } +} + +func TestSerializeBootstrapState(t *testing.T) { + g := NewWithT(t) + + t.Run("empty map serializes to {}", func(t *testing.T) { + result := serializeBootstrapState(map[string]bool{}) + g.Expect(result).To(Equal("{}")) + }) + + t.Run("round-trip: serialize then deserialize preserves entries", func(t *testing.T) { + original := map[string]bool{ + "rule-a": true, + "my-very-long-rule-name-that-exceeds-any-annotation-key-limit-by-far": true, + } + serialized := serializeBootstrapState(original) + + node := &corev1.Node{ + ObjectMeta: metav1.ObjectMeta{ + Annotations: map[string]string{bootstrapStateAnnotation: serialized}, + }, + } + recovered := getBootstrapState(node) + g.Expect(recovered).To(Equal(original)) + }) +} + diff --git a/internal/controller/node_controller.go b/internal/controller/node_controller.go index a3e8aac..c9fabf7 100644 --- a/internal/controller/node_controller.go +++ b/internal/controller/node_controller.go @@ -324,23 +324,16 @@ func (r *RuleReadinessController) removeTaintBySpec(ctx context.Context, node *c }) } -// Bootstrap completion tracking. func (r *RuleReadinessController) isBootstrapCompleted(ctx context.Context, nodeName, ruleName string) bool { - // Check node annotation node := &corev1.Node{} if err := r.Get(ctx, client.ObjectKey{Name: nodeName}, node); err != nil { return false } - - annotationKey := fmt.Sprintf("readiness.k8s.io/bootstrap-completed-%s", ruleName) - _, exists := node.Annotations[annotationKey] - return exists + return getBootstrapState(node)[ruleName] } func (r *RuleReadinessController) markBootstrapCompleted(ctx context.Context, nodeName, ruleName string) { log := ctrl.LoggerFrom(ctx) - - annotationKey := fmt.Sprintf("readiness.k8s.io/bootstrap-completed-%s", ruleName) marked := false // retry to handle conflict with concurrent node updates @@ -350,21 +343,23 @@ func (r *RuleReadinessController) markBootstrapCompleted(ctx context.Context, no return err } - // Check if already marked to avoid unnecessary updates - if node.Annotations != nil { - if _, exists := node.Annotations[annotationKey]; exists { - return nil - } + // Decode the existing bootstrap state map. + state := getBootstrapState(node) + + // Check if already marked to avoid unnecessary updates. + if state[ruleName] { + return nil } patch := client.MergeFrom(node.DeepCopy()) - // Initialize annotations if nil + // Initialize annotations map if nil. if node.Annotations == nil { node.Annotations = make(map[string]string) } - node.Annotations[annotationKey] = "true" + state[ruleName] = true + node.Annotations[bootstrapStateAnnotation] = serializeBootstrapState(state) if err := r.Patch(ctx, node, patch); err != nil { return err } diff --git a/internal/controller/node_controller_reproduction_test.go b/internal/controller/node_controller_reproduction_test.go new file mode 100644 index 0000000..cd810e7 --- /dev/null +++ b/internal/controller/node_controller_reproduction_test.go @@ -0,0 +1,132 @@ +/* +Copyright 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 controller + +import ( + "context" + + . "github.com/onsi/ginkgo/v2" + . "github.com/onsi/gomega" + corev1 "k8s.io/api/core/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/types" + "k8s.io/client-go/kubernetes/fake" + "k8s.io/client-go/tools/record" + "sigs.k8s.io/controller-runtime/pkg/reconcile" + + nodereadinessiov1alpha1 "sigs.k8s.io/node-readiness-controller/api/v1alpha1" +) + +var _ = Describe("Node Controller Reproduction", func() { + Context("when reconciling a node with a very long rule name", func() { + var ( + ctx context.Context + readinessController *RuleReadinessController + nodeReconciler *NodeReconciler + fakeClientset *fake.Clientset + node *corev1.Node + rule *nodereadinessiov1alpha1.NodeReadinessRule + longRuleName = "my-very-long-rule-name-that-exceeds-the-annotation-key-limit-strictly" + ) + + BeforeEach(func() { + ctx = context.Background() + + fakeClientset = fake.NewSimpleClientset() + readinessController = &RuleReadinessController{ + Client: k8sClient, + Scheme: k8sClient.Scheme(), + clientset: fakeClientset, + ruleCache: make(map[string]*nodereadinessiov1alpha1.NodeReadinessRule), + EventRecorder: record.NewFakeRecorder(10), + } + + nodeReconciler = &NodeReconciler{ + Client: k8sClient, + Scheme: k8sClient.Scheme(), + Controller: readinessController, + } + + node = &corev1.Node{ + ObjectMeta: metav1.ObjectMeta{ + Name: "repro-node", + Labels: map[string]string{"env": "repro"}, + }, + Status: corev1.NodeStatus{ + Conditions: []corev1.NodeCondition{ + {Type: "ReproCondition", Status: corev1.ConditionTrue}, + }, + }, + } + + rule = &nodereadinessiov1alpha1.NodeReadinessRule{ + ObjectMeta: metav1.ObjectMeta{ + Name: longRuleName, + }, + Spec: nodereadinessiov1alpha1.NodeReadinessRuleSpec{ + Conditions: []nodereadinessiov1alpha1.ConditionRequirement{ + {Type: "ReproCondition", RequiredStatus: corev1.ConditionTrue}, + }, + Taint: corev1.Taint{ + Key: "readiness.k8s.io/repro-taint", + Effect: corev1.TaintEffectNoSchedule, + }, + NodeSelector: metav1.LabelSelector{ + MatchLabels: map[string]string{"env": "repro"}, + }, + EnforcementMode: nodereadinessiov1alpha1.EnforcementModeBootstrapOnly, + }, + } + }) + + JustBeforeEach(func() { + Expect(k8sClient.Create(ctx, node)).To(Succeed()) + Expect(k8sClient.Create(ctx, rule)).To(Succeed()) + readinessController.updateRuleCache(ctx, rule) + }) + + AfterEach(func() { + _ = k8sClient.Delete(ctx, node) + updatedRule := &nodereadinessiov1alpha1.NodeReadinessRule{} + if err := k8sClient.Get(ctx, types.NamespacedName{Name: longRuleName}, updatedRule); err == nil { + updatedRule.Finalizers = nil + _ = k8sClient.Update(ctx, updatedRule) + _ = k8sClient.Delete(ctx, updatedRule) + } + readinessController.removeRuleFromCache(ctx, longRuleName) + }) + + It("should successfully mark bootstrap completed using the JSON payload annotation for long rule names", func() { + // Trigger reconciliation + _, err := nodeReconciler.Reconcile(ctx, reconcile.Request{NamespacedName: types.NamespacedName{Name: "repro-node"}}) + Expect(err).NotTo(HaveOccurred()) + + recheckedNode := &corev1.Node{} + Expect(k8sClient.Get(ctx, types.NamespacedName{Name: "repro-node"}, recheckedNode)).To(Succeed()) + + // The bootstrap state should be stored as a JSON map in a single annotation, + // with the full rule name as the key — no length constraint applies. + Expect(recheckedNode.Annotations).To(HaveKey(bootstrapStateAnnotation), + "bootstrap-state annotation should be present on the node") + + state := getBootstrapState(recheckedNode) + Expect(state).To(HaveKeyWithValue(longRuleName, true), + "bootstrap state JSON should contain the full rule name marked as completed") + }) + }) +}) +