feat(nodepool): trigger rollout on trust bundle ConfigMap content changes#9007
feat(nodepool): trigger rollout on trust bundle ConfigMap content changes#9007alokstech wants to merge 2 commits into
Conversation
…nges Hash additionalTrustBundle and proxy trustedCA ConfigMap contents in the NodePool config hash and watch those ConfigMaps so in-place CA updates roll out workers without renaming the reference. Refs: RFE-8751 Signed-off-by: alokstech <alok@redhat.com> Commit-Message-Assisted-by: Claude (via Claude Code) Co-authored-by: Cursor <cursoragent@cursor.com>
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
openshift-ci
Bot
added
do-not-merge/needs-area
needs-rebase
openshift-ci
Bot
added
needs-ok-to-test
|
Hi @alokstech. Thanks for your PR. I'm waiting for a openshift member to verify that this patch is reasonable to test. If it is, they should reply with Tip We noticed you've done this a few times! Consider joining the org to skip this step and gain Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. DetailsInstructions 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. |
openshift-ci
Bot
added
the
area/hypershift-operator
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: alokstech The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository YAML (base), Central YAML (inherited) Review profile: CHILL Plan: Enterprise Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (5)
🚧 Files skipped from review as they are similar to previous changes (5)
📝 WalkthroughWalkthroughNodePool rollout configuration now hashes Sequence Diagram(s)sequenceDiagram
participant ConfigMap
participant NodePoolController
participant HostedCluster
participant ConfigGenerator
participant NodePool
ConfigMap->>NodePoolController: update referenced trust bundle
NodePoolController->>HostedCluster: inspect trust-bundle references
HostedCluster-->>NodePoolController: identify matching NodePools
NodePoolController->>NodePool: enqueue reconciliation
NodePool->>ConfigGenerator: generate rollout configuration
ConfigGenerator->>ConfigMap: read ca-bundle.crt
ConfigMap-->>ConfigGenerator: return bundle content
ConfigGenerator->>NodePool: calculate rollout hash
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 11✅ Passed checks (11 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
I now have a complete picture. Here is the analysis: Test Failure Analysis CompleteJob Information
Test Failure AnalysisErrorSummaryNo CI tests ran. The PR branch ( Root CauseThe root cause is git merge conflicts in two files, not a product bug or test failure:
Conflicting commits on
The PR branch was last committed on 2026-07-12, so it missed commit Recommendations
Evidence
|
Combine trust-bundle content hashing with upstream rhelStream hash changes from main and update unit test expectations. Co-authored-by: Cursor <cursoragent@cursor.com>
openshift-ci
Bot
removed
the
needs-rebase
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
hypershift-operator/controllers/nodepool/token.go (1)
57-62: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winClarify the scope of these hashes These values are written to the token Secret for change detection, not to the NodePool rollout hash.
pullSecretHashstill tracks pull-secret content separately from the rollout path, so this wording implies rollout behavior that the code does not have.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@hypershift-operator/controllers/nodepool/token.go` around lines 57 - 62, Update the comment above pullSecretHash, additionalTrustBundleHash, and globalConfigHash to state that these hashes are written to the token Secret for change detection, not included in the NodePool rollout config hash. Clarify that pullSecretHash tracks pull-secret content separately, and remove the inaccurate claim that these values trigger NodePool rollouts.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@hypershift-operator/controllers/nodepool/config.go`:
- Around line 137-163: Update rolloutTrustBundleHashes and configMapCABundleHash
so missing or malformed trust-bundle ConfigMaps are represented as a degraded
HostedCluster condition rather than propagated as a hard NodePool reconciliation
error. Preserve successful hash calculation, and ensure NewConfigGenerator or
its caller records the degraded state while allowing reconciliation to continue
through transient delete/recreate windows.
---
Outside diff comments:
In `@hypershift-operator/controllers/nodepool/token.go`:
- Around line 57-62: Update the comment above pullSecretHash,
additionalTrustBundleHash, and globalConfigHash to state that these hashes are
written to the token Secret for change detection, not included in the NodePool
rollout config hash. Clarify that pullSecretHash tracks pull-secret content
separately, and remove the inaccurate claim that these values trigger NodePool
rollouts.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: b8ad4e64-1086-4660-99e7-3eb0dff885ce
⛔ Files ignored due to path filters (1)
docs/content/reference/aggregated-docs.mdis excluded by!docs/content/reference/aggregated-docs.md
📒 Files selected for processing (7)
docs/content/reference/nodepool-rollouts.mdhypershift-operator/controllers/nodepool/config.gohypershift-operator/controllers/nodepool/config_test.gohypershift-operator/controllers/nodepool/config_trustbundle_test.gohypershift-operator/controllers/nodepool/nodepool_controller.gohypershift-operator/controllers/nodepool/token.gohypershift-operator/controllers/nodepool/token_test.go
| func rolloutTrustBundleHashes(ctx context.Context, c client.Client, hc *hyperv1.HostedCluster) (additionalTrustBundleHash, proxyTrustedCAHash string, err error) { | ||
| if hc.Spec.AdditionalTrustBundle != nil { | ||
| additionalTrustBundleHash, err = configMapCABundleHash(ctx, c, hc.Namespace, hc.Spec.AdditionalTrustBundle.Name) | ||
| if err != nil { | ||
| return "", "", err | ||
| } | ||
| } | ||
| if hc.Spec.Configuration != nil && hc.Spec.Configuration.Proxy != nil && hc.Spec.Configuration.Proxy.TrustedCA.Name != "" { | ||
| proxyTrustedCAHash, err = configMapCABundleHash(ctx, c, hc.Namespace, hc.Spec.Configuration.Proxy.TrustedCA.Name) | ||
| if err != nil { | ||
| return "", "", err | ||
| } | ||
| } | ||
| return additionalTrustBundleHash, proxyTrustedCAHash, nil | ||
| } | ||
|
|
||
| func configMapCABundleHash(ctx context.Context, c client.Client, namespace, name string) (string, error) { | ||
| cm := &corev1.ConfigMap{} | ||
| if err := c.Get(ctx, client.ObjectKey{Namespace: namespace, Name: name}, cm); err != nil { | ||
| return "", fmt.Errorf("cannot get ConfigMap %s/%s: %w", namespace, name, err) | ||
| } | ||
| data, ok := cm.Data[certs.UserCABundleMapKey] | ||
| if !ok { | ||
| return "", fmt.Errorf("ConfigMap %s/%s missing %q key", namespace, name, certs.UserCABundleMapKey) | ||
| } | ||
| return supportutil.HashSimple(data), nil | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Map the relevant file and locate the call chain / tests.
ast-grep outline hypershift-operator/controllers/nodepool/config.go
rg -n "NewConfigGenerator|rolloutTrustBundleHashes|configMapCABundleHash|AdditionalTrustBundle|TrustedCA|config_trustbundle_test" hypershift-operator/controllers/nodepool -S
# Read the relevant section of the implementation.
sed -n '1,260p' hypershift-operator/controllers/nodepool/config.go
# Read the trust-bundle tests, if present.
sed -n '1,260p' hypershift-operator/controllers/nodepool/config_trustbundle_test.goRepository: openshift/hypershift
Length of output: 34581
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '320,520p' hypershift-operator/controllers/nodepool/nodepool_controller.go
sed -n '1088,1125p' hypershift-operator/controllers/nodepool/nodepool_controller.go
sed -n '340,410p' hypershift-operator/controllers/nodepool/conditions.goRepository: openshift/hypershift
Length of output: 13672
Trust-bundle lookup now blocks all NodePool reconciliation for the HostedCluster
In rolloutTrustBundleHashes/configMapCABundleHash, a missing or malformed trust-bundle ConfigMap becomes a hard error, and NewConfigGenerator propagates that on every NodePool reconcile path. If transient delete/recreate windows are expected, surface this as a degraded condition instead of failing the whole reconcile loop.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@hypershift-operator/controllers/nodepool/config.go` around lines 137 - 163,
Update rolloutTrustBundleHashes and configMapCABundleHash so missing or
malformed trust-bundle ConfigMaps are represented as a degraded HostedCluster
condition rather than propagated as a hard NodePool reconciliation error.
Preserve successful hash calculation, and ensure NewConfigGenerator or its
caller records the degraded state while allowing reconciliation to continue
through transient delete/recreate windows.
1 participant
Summary
Fixes RFE-8751 (Phase 1): NodePool rollouts now react to in-place changes of
ca-bundle.crtcontent in ConfigMaps referenced byHostedCluster.spec.additionalTrustBundleandHostedCluster.spec.configuration.proxy.trustedCA, not only reference name changes.Pull secret behavior is unchanged: in-place pull secret edits still do not trigger a NodePool rollout.
One-time migration on HyperShift Operator upgrade
Important operational note for reviewers and fleet operators:
This change replaces the rollout hash input for trust bundles from ConfigMap name to ConfigMap content hash. That means:
additionalTrustBundle/proxy.trustedCAca-bundle.crtafter migrationThe one-time migration rollout happens because the stored config hash was computed with the old formula (name) and the new operator computes a different hash (content) even when the PEM bytes are identical. After that rollout completes, later HO upgrades should not replace nodes unless trust-bundle content or another rollout input actually changes.
This is the same class of change called out in project guidance for NodePool config hash changes.
e2e-aws-upgrade-hypershift-operatorvalidates that HO upgrade does not spuriously roll nodes on default test clusters (no trust bundle configured); it does not exercise the one-time migration path for fleets that already reference trust bundles.Manual testing
Validated on AWS management cluster with custom HO image:
quay.io/rhn-support-alosingh/hypershift-operator:feat-rfe-8751-trust-bundle-rolloutrfe8751-testadditionalTrustBundleConfigMap with real self-signed CAca-bundle.crttriggeredUpdatingConfig=Trueand node replacement without renaming the ConfigMap or HostedCluster referenceTest plan
make verify(unit tests + lint)additionalTrustBundleConfigMap edit triggers NodePool rolloutpull-ci-openshift-hypershift-main-e2e-aws-upgrade-hypershift-operator(CI presubmit)Made with Cursor
Summary by CodeRabbit