diff --git a/internal/controller/helper.go b/internal/controller/helper.go index 7765fcf..9e9456d 100644 --- a/internal/controller/helper.go +++ b/internal/controller/helper.go @@ -17,12 +17,48 @@ limitations under the License. package controller import ( + "crypto/sha256" + "encoding/hex" "fmt" corev1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" ) +const ( + // bootstrapAnnotationPrefix is the prefix used for node annotations tracking bootstrap completion. + bootstrapAnnotationPrefix = "readiness.k8s.io/bootstrap-completed-" +) + +// getBootstrapAnnotationKey generates a valid Kubernetes annotation key for a given rule name. +// It handles rule names longer than 43 characters by using a hybrid approach: +// it truncates the name and appends a short hash. This keeps the key human-readable +// while staying within the 63-character limit for the name part. +func getBootstrapAnnotationKey(ruleName string) string { + const maxNamePartLen = 63 + const namePartPrefix = "bootstrap-completed-" + // availableSpace is the space left for the ruleName in the annotation name part (after "bootstrap-completed-") + availableSpace := maxNamePartLen - len(namePartPrefix) // 43 + + if len(ruleName) <= availableSpace { + return fmt.Sprintf("%s%s", bootstrapAnnotationPrefix, ruleName) + } + + // For names longer than 43 chars, use a hybrid approach: truncated-name-shorthash + const hashLen = 8 + // -1 for the hyphen separator + humanPartLen := availableSpace - hashLen - 1 // 34 + + hash := sha256.Sum256([]byte(ruleName)) + shortHash := hex.EncodeToString(hash[:])[:hashLen] + + // Use first 34 chars of ruleName, then a hyphen, then 8 chars of hash. + // Total name part: 34 + 1 + 8 = 43. + // This fits well within the 63 char limit and leaves room for the prefix. + namePart := fmt.Sprintf("%s-%s", ruleName[:humanPartLen], shortHash) + return fmt.Sprintf("%s%s", bootstrapAnnotationPrefix, namePart) +} + // nodeSelectorChanged checks if nodeSelector has changed. func nodeSelectorChanged(current, previous metav1.LabelSelector) bool { // Compare matchLabels diff --git a/internal/controller/helper_unit_test.go b/internal/controller/helper_unit_test.go new file mode 100644 index 0000000..67602c1 --- /dev/null +++ b/internal/controller/helper_unit_test.go @@ -0,0 +1,78 @@ +/* +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" +) + +func TestGetBootstrapAnnotationKey(t *testing.T) { + g := NewWithT(t) + + tests := []struct { + name string + ruleName string + expected string + }{ + { + name: "Short name", + ruleName: "short-rule", + expected: "readiness.k8s.io/bootstrap-completed-short-rule", + }, + { + name: "Exactly 43 characters", + ruleName: "this-rule-name-is-exactly-43-chars-long-123", + expected: "readiness.k8s.io/bootstrap-completed-this-rule-name-is-exactly-43-chars-long-123", + }, + { + name: "Long name (44 characters)", + ruleName: "this-rule-name-is-exactly-44-chars-long-1234", + }, + { + name: "Very long name", + ruleName: "my-very-long-rule-name-that-is-definitely-going-to-exceed-the-limit-of-sixty-three-characters-in-the-annotation-key-name-part", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + result := getBootstrapAnnotationKey(tt.ruleName) + + // Verify prefix + g.Expect(result).To(HavePrefix(bootstrapAnnotationPrefix)) + + if len(tt.ruleName) <= 43 { + g.Expect(result).To(Equal(bootstrapAnnotationPrefix + tt.ruleName)) + } else { + // Verify name part length is exactly 20 (prefix) + 34 (human) + 1 (hyphen) + 8 (hash) = 63 + // Total key length: 17 (domain) + 63 (name part) = 80 + g.Expect(len(result)).To(Equal(80)) + // Verify it has the hyphen separator before the hash (last 9 characters are -XXXXXXXX) + g.Expect(result[len(result)-9 : len(result)-8]).To(Equal("-")) + } + + // Verify name part length is <= 63 + namePart := result[len("readiness.k8s.io/"):] + g.Expect(len(namePart)).To(BeNumerically("<=", 63)) + + // Verify determinism + g.Expect(getBootstrapAnnotationKey(tt.ruleName)).To(Equal(result)) + }) + } +} diff --git a/internal/controller/node_controller.go b/internal/controller/node_controller.go index 9dc596a..c9b67d5 100644 --- a/internal/controller/node_controller.go +++ b/internal/controller/node_controller.go @@ -316,7 +316,6 @@ 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{} @@ -324,7 +323,7 @@ func (r *RuleReadinessController) isBootstrapCompleted(ctx context.Context, node return false } - annotationKey := fmt.Sprintf("readiness.k8s.io/bootstrap-completed-%s", ruleName) + annotationKey := getBootstrapAnnotationKey(ruleName) _, exists := node.Annotations[annotationKey] return exists } @@ -332,7 +331,7 @@ func (r *RuleReadinessController) isBootstrapCompleted(ctx context.Context, node func (r *RuleReadinessController) markBootstrapCompleted(ctx context.Context, nodeName, ruleName string) { log := ctrl.LoggerFrom(ctx) - annotationKey := fmt.Sprintf("readiness.k8s.io/bootstrap-completed-%s", ruleName) + annotationKey := getBootstrapAnnotationKey(ruleName) marked := false // retry to handle conflict with concurrent node updates diff --git a/internal/controller/node_controller_reproduction_test.go b/internal/controller/node_controller_reproduction_test.go new file mode 100644 index 0000000..e0c02b3 --- /dev/null +++ b/internal/controller/node_controller_reproduction_test.go @@ -0,0 +1,131 @@ +/* +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 by using a hashed annotation key 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 key should now be the hashed version: readiness.k8s.io/bootstrap-completed- + expectedKey := getBootstrapAnnotationKey(longRuleName) + Expect(recheckedNode.Annotations).To(HaveKey(expectedKey), + "Annotation SHOULD be present with the hashed key") + + // Verify the length of the name part is exactly 63 + // Total length: 17 (domain) + 63 (name part) = 80 + Expect(len(expectedKey)).To(Equal(17 + 63)) + }) + }) +})