Add opt-in ValidatingAdmissionPolicy for node-data-broker

[Shankar-v27]

Description

Closes #388.

This PR adds an opt-in ValidatingAdmissionPolicy and ValidatingAdmissionPolicyBinding to restrict the node-data-broker ServiceAccount to updating only its own Node resource.

Changes

• Add nodeDataBroker.validatingAdmissionPolicy.enabled (default: false)
• Add schema validation for the new configuration
• Add ValidatingAdmissionPolicy and ValidatingAdmissionPolicyBinding templates
• Add Helm unit tests
• Update Kubernetes documentation
• Add a changelog entry

Testing

Added Helm unit tests covering:

• Default disabled rendering
• Enabled rendering
• Policy and binding generation
• failurePolicy
• validationActions
• CEL expression
• ServiceAccount interpolation

Note: The Helm unit tests were added as part of this change. I was unable to execute them locally because the required Helm tooling was unavailable in my development environment. They will be executed as part of the repository's CI checks.

[copy-pr-bot]

This pull request requires additional validation before any workflows can run on NVIDIA's runners.

Pull request vetters can view their responsibilities here.

Contributors can view more details about this message here.

[greptile-apps]

Greptile Summary

This PR adds an opt-in ValidatingAdmissionPolicy and ValidatingAdmissionPolicyBinding that restrict the node-data-broker ServiceAccount to updating only the Node resource it runs on, using the authentication.kubernetes.io/node-name claim from projected ServiceAccount tokens. Both issues flagged in earlier review rounds (empty-list CEL index access, contains assertions in tests) have been addressed in the current revision.

• New Helm template renders the policy + binding behind a nodeDataBroker.validatingAdmissionPolicy.enabled guard (default false); the CEL expression is correct and includes a size > 0 guard before indexing the node-name list.
• Helm unit tests cover default-disabled, component-disabled, enabled (document count), API kinds, and the full CEL expression via equal assertions.
• Both failurePolicy and validationActions are hardcoded (Fail / [Deny]) with no corresponding values.yaml knobs, so operators cannot enable an audit-only (Warn) mode before switching to full enforcement.

Confidence Score: 5/5

Safe to merge; the feature is entirely opt-in and the CEL expression logic is correct.

All changed code is behind an opt-in flag defaulting to false, so existing deployments are unaffected. The CEL expression correctly guards against the empty-list edge case, and the helm-unittest assertions use the right assertion type. The only open concerns are about hardcoded policy knobs that reduce operator flexibility but do not introduce a functional defect.

charts/topograph/templates/nodeDataBroker/validatingadmissionpolicy.yaml — the hardcoded failurePolicy and validationActions are worth revisiting before the feature graduates from opt-in.

Important Files Changed

Filename	Overview
charts/topograph/templates/nodeDataBroker/validatingadmissionpolicy.yaml	New template rendering ValidatingAdmissionPolicy + Binding; CEL expression is correct with size guard; failurePolicy and validationActions are both hardcoded with no values.yaml knobs.
charts/topograph/tests/node-data-broker_validatingadmissionpolicy_test.yaml	New helm-unittest suite; previous contains assertion issue resolved by using equal throughout; tests cover enabled/disabled, document count, kinds, and CEL expressions.
charts/topograph/values.schema.json	Schema extended with validatingAdmissionPolicy object; only exposes enabled boolean, consistent with the rest of the chart's loose schema style.
charts/topograph/values.yaml	New validatingAdmissionPolicy block added under nodeDataBroker with enabled:false default; comment correctly cites Kubernetes 1.30+ and ServiceAccountTokenPodNodeInfo.
docs/engines/k8s.md	New section documenting the ValidatingAdmissionPolicy feature, prerequisites, and usage snippet; clear and accurate.
CHANGELOG.md	Changelog entry added to the Security section; accurately describes the new opt-in policy.

_{Reviews (3): Last reviewed commit: "fix: address review feedback for node-da..." | Re-trigger Greptile}

[ArangoGutierrez]

Nice defense-in-depth addition. A few issues keep it from working as intended as written — most importantly the CEL doesn't compile, and the version and feature-gate prerequisites are inaccurate. Details inline.

1. This has() call takes an indexed map key, which CEL rejects at compile time (invalid argument to has() macro — has() only accepts a field selection). The policy fails to compile, so the API server refuses it on apply and the restriction never takes effect. Use 'authentication.kubernetes.io/node-name' in request.userInfo.extra for the presence check. The helm-unittest suite only regex-matches the rendered string, so it doesn't catch this. (charts/topograph/templates/nodeDataBroker/validatingadmissionpolicy.yaml:20)
2. The template emits admissionregistration.k8s.io/v1, which is only served at GA in 1.30 (v1alpha1 in 1.26, v1beta1 in 1.28-1.29). With Chart.yaml kubeVersion >=1.27.0-0, enabling this on a 1.27-1.29 cluster fails to apply. Worth stating the real floor (>= 1.30) here and in the docs prerequisites. (charts/topograph/values.yaml:316)
3. For the broker's default pod-bound token, the authentication.kubernetes.io/node-name extra is populated by the ServiceAccountTokenPodNodeInfo feature gate, not ServiceAccountTokenNodeBinding (which mints node-bound tokens the chart doesn't request). Enabling only the named gate can leave the claim absent, and since the policy is fail-closed that denies every broker node update. (docs/engines/k8s.md:319)
4. failurePolicy: Fail with a matchConstraints that selects every node UPDATE from any user means all node updates (including kubelet heartbeats) run through this CEL; a fault would deny them cluster-wide. Moving the SA check into spec.matchConditions (username == the broker SA) limits both the evaluation and the fail-closed blast radius to just the broker's requests. (charts/topograph/templates/nodeDataBroker/validatingadmissionpolicy.yaml:9)
5. These substring matchRegex checks stay green even if the name-binding equality were dropped or inverted, and helm-unittest never compiles the CEL — which is how the invalid has() above passes CI. Consider asserting the full spec.validations[0].expression (the same contains->equal tightening as #385) and validating the policy on a live cluster as #392 did. (charts/topograph/tests/node-data-broker_validatingadmissionpolicy_test.yaml:90)

[ArangoGutierrez]

This has() call takes an indexed map key, which CEL rejects at compile time (invalid argument to has() macro — has() only accepts a field selection). The policy fails to compile, so the API server refuses it on apply and the restriction never takes effect. Use 'authentication.kubernetes.io/node-name' in request.userInfo.extra for the presence check. The helm-unittest suite only regex-matches the rendered string, so it doesn't catch this.

[ArangoGutierrez]

The template emits admissionregistration.k8s.io/v1, which is only served at GA in 1.30 (v1alpha1 in 1.26, v1beta1 in 1.28-1.29). With Chart.yaml kubeVersion >=1.27.0-0, enabling this on a 1.27-1.29 cluster fails to apply. Worth stating the real floor (>= 1.30) here and in the docs prerequisites.

[ArangoGutierrez]

For the broker's default pod-bound token, the authentication.kubernetes.io/node-name extra is populated by the ServiceAccountTokenPodNodeInfo feature gate, not ServiceAccountTokenNodeBinding (which mints node-bound tokens the chart doesn't request). Enabling only the named gate can leave the claim absent, and since the policy is fail-closed that denies every broker node update.

[ArangoGutierrez]

failurePolicy: Fail with a matchConstraints that selects every node UPDATE from any user means all node updates (including kubelet heartbeats) run through this CEL; a fault would deny them cluster-wide. Moving the SA check into spec.matchConditions (username == the broker SA) limits both the evaluation and the fail-closed blast radius to just the broker's requests.

[ArangoGutierrez]

These substring matchRegex checks stay green even if the name-binding equality were dropped or inverted, and helm-unittest never compiles the CEL — which is how the invalid has() above passes CI. Consider asserting the full spec.validations[0].expression (the same contains->equal tightening as #385) and validating the policy on a live cluster as #392 did.

[Shankar-v27]

Thanks for the detailed review and for taking the time to go through the implementation so thoroughly.

These are all valuable points. I'll go through each of them carefully:

• verify and correct the CEL expression if needed,
• update the documented Kubernetes version requirements and feature-gate prerequisites,
• evaluate moving the ServiceAccount check into matchConditions,
• and strengthen the Helm tests to better validate the generated policy.

I'll put together a follow-up commit addressing the applicable items after validating them against the Kubernetes API behavior and documentation.