fix(controller): handle long rule names in bootstrap annotation keys

[vishnukothakapu]

Description

This PR fixes a bug where NodeReadinessRule resources with long names (longer than 43 characters) caused the controller to fail when patching Node annotations. Kubernetes strictly limits the name part of an annotation key to 63 characters. Since our key pattern was readiness.k8s.io/bootstrap-completed-<rule-name>, long rule names resulted in invalid annotation keys.

Instead of a hash-based workaround, this PR adopts a structured JSON payload annotation on the node, the same pattern kubectl uses for kubectl.kubernetes.io/last-applied-configuration. A single annotation key stores a JSON map of all bootstrap-completed rules:

readiness.k8s.io/bootstrap-state → {"rule-a": true, "rule-b": true, ...}

This eliminates the key length constraint at the root by moving rule names into the annotation value rather than the key, and also addresses the stale annotation issue from #247, a deleted and recreated rule is no longer silently skipped due to a persisting annotation key.

Related Issue

Fixes #223

Type of Change

/kind bug

Testing

• Added internal/controller/helper_unit_test.go: Unit tests for getBootstrapState and serializeBootstrapState, covering nil annotations, missing/empty/malformed values, single/multiple rules, and arbitrarily long rule names stored as map keys with no length constraint.
• Added internal/controller/node_controller_reproduction_test.go: Reproduction test that confirms the controller successfully reconciles rules with very long names, verifying the full rule name appears in the JSON payload annotation.
• Verified with go build ./... and go vet ./internal/controller/....

Checklist

• make test passes
• make lint passes

Does this PR introduce a user-facing change?

Nodes that previously had readiness.k8s.io/bootstrap-completed-* annotations will retain them as harmless no-ops. Bootstrap state is now tracked under the single annotation readiness.k8s.io/bootstrap-state.

[netlify]

✅ Deploy Preview for node-readiness-controller canceled.

Name	Link
🔨 Latest commit	86fbd4d
🔍 Latest deploy log	https://app.netlify.com/projects/node-readiness-controller/deploys/6a1032d328530c000867e0df

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: vishnukothakapu
Once this PR has been reviewed and has the lgtm label, please assign ajaysundark for approval. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Details

Needs approval from an approver in each of these files:

• OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[k8s-ci-robot]

Welcome @vishnukothakapu!

It looks like this is your first PR to kubernetes-sigs/node-readiness-controller 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes-sigs/node-readiness-controller has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

[k8s-ci-robot]

Hi @vishnukothakapu. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work.

Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

[ajaysundark]

Thanks for catching this. My only thoughts on this is that it takes away the human observability on this when a bootsrap-rule is done. :/

[AvineshTripathi]

 should we go with a hybrid approach something like below

availableSpace := maxK8sAnnotationNameLen - len(bootstrapAnnotationPrefix)

const hashLen = 8
humanPartLen := availableSpace - hashLen - 1

hashBytes := sha256.Sum256([]byte(ruleName))
shortHash := hex.EncodeToString(hashBytes[:])[:hashLen]

namePart := fmt.Sprintf("%s-%s", ruleName[:humanPartLen], shortHash)


return fmt.Sprintf("%s%s", bootstrapAnnotationPrefix, namePart)

this way we keep the ruleName

[ajaysundark]

we have a lot of options to handle this. I added some different approach as well. We could discuss further over the meeting, feel free to join if you are available, @vishnukothakapu

[ajaysundark]

The annotation restrictions are <dns-prefix: 253>/<name: 63>; metadata.name can also be 253 chars in length.

Hashing the rule name is one option, couple of alternatives to consider:

1. restricting the length of the rule-name to 63 (we can also move the "bootstrap-completed" to the value?)
2. alternatively, safer to use a standard key like "readiness.k8s.io/rule-status" and go with a json payload for value. Value doesnt seem to have a restriction. it'll allow us to capture the rule-name fully inside the payload.

We need to evaluate the pros/cons on the implementation. @vishnukothakapu Do you want to evaluate the alternatives and propose a plan here?

[vishnukothakapu]

Thanks for catching this. My only thoughts on this is that it takes away the human observability on this when a bootsrap-rule is done. :/

Good point. I agree the full hash reduces readability during debugging. I’ll explore the hybrid approach with a readable prefix + short hash and compare it with the other alternatives discussed.

The annotation restrictions are <dns-prefix: 253>/<name: 63>; metadata.name can also be 253 chars in length.

Hashing the rule name is one option, couple of alternatives to consider:

1. restricting the length of the rule-name to 63 (we can also move the "bootstrap-completed" to the value?)
2. alternatively, safer to use a standard key like "readiness.k8s.io/rule-status" and go with a json payload for value. Value doesnt seem to have a restriction. it'll allow us to capture the rule-name fully inside the payload.

We need to evaluate the pros/cons on the implementation. @vishnukothakapu Do you want to evaluate the alternatives and propose a plan here?

Thanks @ajaysundark , these are good points. I’ll evaluate the tradeoffs between the current hashing approach, the hybrid readable-prefix approach, and the single annotation JSON payload design, then propose a direction based on readability, implementation complexity, and backward compatibility.

[ajaysundark]

/assign @vishnukothakapu

[vishnukothakapu]

Hi @AvineshTripathi & @ajaysundark,
Thanks for the suggestion! I decided to go with the hybrid approach (truncated-name + short-hash) instead of the JSON payload approach because it preserves Kubernetes native selector support while still avoiding annotation length issues.
This keeps the annotations readable for debugging, ensures uniqueness with a deterministic hash, and stays compatible with the current architecture and existing workflows. I have updated the implementation and adjusted the related unit and integration tests accordingly.

[ajaysundark]

instead of the JSON payload approach because it preserves Kubernetes native selector support

Could you clarify your thoughts further on this? What are the downsides of using a json payload. This is also how kubectl saves last applied configurations in objects today - ref: https://kubernetes.io/docs/tasks/manage-kubernetes-objects/declarative-config/#how-to-create-objects

@AvineshTripathi / @Karthik-K-N I think fixing this short-term with a hash based approach for length immunity doesnt feel right. A more reliable long term solution would be to maintain the rule-status inside a JSON payload to track individual rule evaluation data. It would also address concerns such as #247

[vishnukothakapu]

Hi @ajaysundark, thanks for the clarification, that makes sense. I can see how the structured JSON payload approach becomes more reliable long-term, especially with concerns like stale rule state and rule recreation handling from #247.

My initial preference for the hybrid approach was mainly to keep the fix minimal and avoid larger behavioral changes in this PR. But I agree the stable key + structured payload model feels more extensible and better suited for tracking rule lifecycle/state going forward.

[vishnukothakapu]

instead of the JSON payload approach because it preserves Kubernetes native selector support

Could you clarify your thoughts further on this? What are the downsides of using a json payload. This is also how kubectl saves last applied configurations in objects today - ref: https://kubernetes.io/docs/tasks/manage-kubernetes-objects/declarative-config/#how-to-create-objects

@AvineshTripathi / @Karthik-K-N I think fixing this short-term with a hash based approach for length immunity doesnt feel right. A more reliable long term solution would be to maintain the rule-status inside a JSON payload to track individual rule evaluation data. It would also address concerns such as #247

@ajaysundark, the JSON payload approach is the right long-term design. It eliminates the key length issue at the root, preserves full observability (complete rule name in the value), and cleanly resolves #247, a deleted+recreated rule would no longer be blocked by a stale annotation. The main considerations are:

1. handling read-modify-write races via RetryOnConflict (same pattern already used in addTaintBySpec), and
2. documenting existing per-rule annotations as no-op leftovers in the changelog. Happy to update this PR to implement this if the direction is agreed.