OCPBUGS-13553: Trigger NodePool rollout on in-place pull secret changes

[vsolanki12]

What this PR does / why we need it:

Previously, modifying the pull secret or additional trust bundle data in-place (without changing
the Secret/ConfigMap reference name) did not trigger a NodePool rollout because the rollout hash
only included the resource name, not a content hash.

This PR:

• Adds pullSecretHash and additionalTrustBundleHash fields to ConfigGenerator.rolloutConfig, computed from actual Secret/ConfigMap data
• Includes these content hashes in both Hash() and HashWithoutVersion() so in-place data changes produce a different rollout hash
• Removes the redundant hash computation from the Token struct (previously duplicated with TODO comments acknowledging the gap)
• Adds enqueueNodePoolsForSecret watcher so pull secret data changes trigger NodePool reconciliation (the pull secret lacks the nodePoolAnnotation, so the existing enqueueParentNodePool didn't match it)
• Updates the PullSecret field API doc to reflect the new behavior

Note: Deploying this fix will cause a one-time rollout for all existing NodePools on first reconciliation, because the hash now includes content hashes that were not previously part of the computation. This is expected and unavoidable.

Which issue(s) this PR fixes:

Fixes https://issues.redhat.com/browse/OCPBUGS-13553

Special notes for your reviewer:

• The additionalTrustBundleHash uses HashSimple(string) (not []byte) to match the ignition server's fetchAdditionalTrustBundle which returns string. The pullSecretHash uses HashSimple([]byte) to match the ignition server's fetchPullSecret which returns []byte.
• The AdditionalTrustBundle ConfigMap watcher is a pre-existing gap (no watcher triggers reconciliation when trust bundle data changes in-place). This is out of scope for this JIRA and can be a follow-up.
• Tested on a live KubeVirt HCP cluster (dhruvhcp-new500): confirmed that modifying pull secret in-place now triggers UpdatingConfig=True with a new target hash.

Checklist:

• Subject and description added to both, commit and PR.
• Relevant issues have been referenced.
• This change includes docs.
• This change includes unit tests.

Summary by CodeRabbit

• New Features

  • Rollout identity now reflects pull-secret and trust-bundle contents so updates to those secrets cause NodePool rollouts.
  • Controller now enqueues NodePool reconciliations when a HostedCluster pull Secret changes.

• Bug Fixes

  • Fixed detection so in-place updates to a referenced pull Secret or trust bundle trigger rollouts for existing NodePools.

• Documentation

  • Clarified that updating Secret data in place will trigger NodePool rollouts.

• Tests

  • Added and updated unit tests covering secret-change handling and new hashing behavior.

[openshift-merge-bot]

Pipeline controller notification
This repo is configured to use the pipeline controller. Second-stage tests will be triggered either automatically or after lgtm label is added, depending on the repository configuration. The pipeline controller will automatically detect which contexts are required and will utilize /test Prow commands to trigger the second stage.

For optional jobs, comment /test ? to see a list of all defined jobs. To trigger manually all jobs from second stage use /pipeline required command.

This repository is configured in: LGTM mode

[openshift-ci]

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

[openshift-ci-robot]

@vsolanki12: This pull request references Jira Issue OCPBUGS-13553, which is invalid:

• expected the bug to target the "5.0.0" version, but no target version was set

Comment /jira refresh to re-evaluate validity if changes to the Jira bug are made, or edit the title of this pull request to link to a different bug.

The bug has been updated to refer to the pull request using the external bug tracker.

Details

In response to this:

What this PR does / why we need it:

Previously, modifying the pull secret or additional trust bundle data in-place (without changing
the Secret/ConfigMap reference name) did not trigger a NodePool rollout because the rollout hash
only included the resource name, not a content hash.

This PR:

• Adds pullSecretHash and additionalTrustBundleHash fields to ConfigGenerator.rolloutConfig, computed from actual Secret/ConfigMap data
• Includes these content hashes in both Hash() and HashWithoutVersion() so in-place data changes produce a different rollout hash
• Removes the redundant hash computation from the Token struct (previously duplicated with TODO comments acknowledging the gap)
• Adds enqueueNodePoolsForSecret watcher so pull secret data changes trigger NodePool reconciliation (the pull secret lacks the nodePoolAnnotation, so the existing enqueueParentNodePool didn't match it)
• Updates the PullSecret field API doc to reflect the new behavior

Note: Deploying this fix will cause a one-time rollout for all existing NodePools on first reconciliation, because the hash now includes content hashes that were not previously part of the computation. This is expected and unavoidable.

Which issue(s) this PR fixes:

Fixes https://issues.redhat.com/browse/OCPBUGS-13553

Special notes for your reviewer:

• The additionalTrustBundleHash uses HashSimple(string) (not []byte) to match the ignition server's fetchAdditionalTrustBundle which returns string. The pullSecretHash uses HashSimple([]byte) to match the ignition server's fetchPullSecret which returns []byte.
• The AdditionalTrustBundle ConfigMap watcher is a pre-existing gap (no watcher triggers reconciliation when trust bundle data changes in-place). This is out of scope for this JIRA and can be a follow-up.
• Tested on a live KubeVirt HCP cluster (dhruvhcp-new500): confirmed that modifying pull secret in-place now triggers UpdatingConfig=True with a new target hash.

Checklist:

• Subject and description added to both, commit and PR.
• Relevant issues have been referenced.
• This change includes docs.
• This change includes unit tests.

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 openshift-eng/jira-lifecycle-plugin repository.

[openshift-ci]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: vsolanki12
Once this PR has been reviewed and has the lgtm label, please assign csrwng 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
• api/OWNERS

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

[vsolanki12]

/jira refresh

[openshift-ci-robot]

@vsolanki12: This pull request references Jira Issue OCPBUGS-13553, which is valid. The bug has been moved to the POST state.

3 validation(s) were run on this bug

• bug is open, matching expected state (open)
• bug target version (5.0.0) matches configured target version for branch (5.0.0)
• bug is in the state ASSIGNED, which is one of the valid states (NEW, ASSIGNED, POST)

No GitHub users were found matching the public email listed for the QA contact in Jira (jiezhao@redhat.com), skipping review request.

Details

In response to this:

/jira refresh

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 openshift-eng/jira-lifecycle-plugin repository.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Repository YAML (base), Central YAML (inherited)

Review profile: CHILL

Plan: Enterprise

Run ID: 66689459-a215-4a24-bdd1-44c9e1734717

📥 Commits

Reviewing files that changed from the base of the PR and between fe44067 and 481df5f.

⛔ Files ignored due to path filters (19)

• api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/AAA_ungated.yaml is excluded by !**/zz_generated.featuregated-crd-manifests/**
• api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/ClusterUpdateAcceptRisks.yaml is excluded by !**/zz_generated.featuregated-crd-manifests/**
• api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/ClusterVersionOperatorConfiguration.yaml is excluded by !**/zz_generated.featuregated-crd-manifests/**
• api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/ExternalOIDC.yaml is excluded by !**/zz_generated.featuregated-crd-manifests/**
• api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/ExternalOIDCWithUIDAndExtraClaimMappings.yaml is excluded by !**/zz_generated.featuregated-crd-manifests/**
• api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/ExternalOIDCWithUpstreamParity.yaml is excluded by !**/zz_generated.featuregated-crd-manifests/**
• api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/GCPPlatform.yaml is excluded by !**/zz_generated.featuregated-crd-manifests/**
• api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/HCPEtcdBackup.yaml is excluded by !**/zz_generated.featuregated-crd-manifests/**
• api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/HyperShiftOnlyDynamicResourceAllocation.yaml is excluded by !**/zz_generated.featuregated-crd-manifests/**
• api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/ImageStreamImportMode.yaml is excluded by !**/zz_generated.featuregated-crd-manifests/**
• api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/KMSEncryptionProvider.yaml is excluded by !**/zz_generated.featuregated-crd-manifests/**
• api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/OpenStack.yaml is excluded by !**/zz_generated.featuregated-crd-manifests/**
• api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/TLSAdherence.yaml is excluded by !**/zz_generated.featuregated-crd-manifests/**
• cmd/install/assets/crds/hypershift-operator/zz_generated.crd-manifests/hostedclusters-Hypershift-CustomNoUpgrade.crd.yaml is excluded by !**/zz_generated.crd-manifests/**, !cmd/install/assets/**/*.yaml
• cmd/install/assets/crds/hypershift-operator/zz_generated.crd-manifests/hostedclusters-Hypershift-Default.crd.yaml is excluded by !**/zz_generated.crd-manifests/**, !cmd/install/assets/**/*.yaml
• cmd/install/assets/crds/hypershift-operator/zz_generated.crd-manifests/hostedclusters-Hypershift-TechPreviewNoUpgrade.crd.yaml is excluded by !**/zz_generated.crd-manifests/**, !cmd/install/assets/**/*.yaml
• docs/content/reference/aggregated-docs.md is excluded by !docs/content/reference/aggregated-docs.md
• docs/content/reference/api.md is excluded by !docs/content/reference/api.md
• vendor/github.com/openshift/hypershift/api/hypershift/v1beta1/hostedcluster_types.go is excluded by !vendor/**, !**/vendor/**

📒 Files selected for processing (9)

• api/hypershift/v1beta1/hostedcluster_types.go
• hypershift-operator/controllers/nodepool/conditions_test.go
• hypershift-operator/controllers/nodepool/config.go
• hypershift-operator/controllers/nodepool/config_test.go
• hypershift-operator/controllers/nodepool/nodepool_controller.go
• hypershift-operator/controllers/nodepool/nodepool_controller_test.go
• hypershift-operator/controllers/nodepool/secret_janitor_test.go
• hypershift-operator/controllers/nodepool/token.go
• hypershift-operator/controllers/nodepool/token_test.go

✅ Files skipped from review due to trivial changes (3)

• hypershift-operator/controllers/nodepool/conditions_test.go
• api/hypershift/v1beta1/hostedcluster_types.go
• hypershift-operator/controllers/nodepool/secret_janitor_test.go

🚧 Files skipped from review as they are similar to previous changes (6)

• hypershift-operator/controllers/nodepool/nodepool_controller_test.go
• hypershift-operator/controllers/nodepool/nodepool_controller.go
• hypershift-operator/controllers/nodepool/token_test.go
• hypershift-operator/controllers/nodepool/config.go
• hypershift-operator/controllers/nodepool/token.go
• hypershift-operator/controllers/nodepool/config_test.go

---

📝 Walkthrough

Walkthrough

This PR modifies NodePool rollout identity to include content-derived hashes of the HostedCluster pull secret and optional additionalTrustBundle. ConfigGenerator now fetches and hashes those contents and incorporates them into Hash()/HashWithoutVersion(). The NodePool controller watches Secrets and enqueues NodePools when a referenced pull Secret changes. Token no longer computes/stores those hashes itself; reconcile reads hashes from ConfigGenerator. Tests and a small documentation comment were updated accordingly.

Sequence Diagram(s)

sequenceDiagram
  participant Secret
  participant NodePoolReconciler
  participant KubeAPI
  participant ConfigGenerator
  Secret->>NodePoolReconciler: Secret change event
  NodePoolReconciler->>KubeAPI: Get Secret annotation -> Get(NodePool) (if annotated)
  NodePoolReconciler->>KubeAPI: List HostedClusters(namespace)
  KubeAPI-->>NodePoolReconciler: HostedClusters
  NodePoolReconciler->>KubeAPI: List NodePools and match spec.clusterName
  NodePoolReconciler-->>NodePoolReconciler: Enqueue reconcile.Requests for matching NodePools
  ConfigGenerator->>KubeAPI: Get(Secret)/Get(ConfigMap) to read pullSecret/additionalTrustBundle
  KubeAPI-->>ConfigGenerator: Secret/ConfigMap data
  ConfigGenerator-->>ConfigGenerator: Compute pullSecretHash and additionalTrustBundleHash

Loading

Suggested reviewers

• muraee
• cblecker

🚥 Pre-merge checks | ✅ 10 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Test Structure And Quality	⚠️ Warning	TestEnqueueNodePoolsForSecret assertions lack failure messages, violating the test quality requirement. Similar tests in same file demonstrate messages should be included.	Add assertion messages like: g.Expect(reqs).To(HaveLen(tc.expectedCount), "unexpected request count") to improve test failure diagnostics.

✅ Passed checks (10 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The PR title clearly and specifically describes the main change: triggering NodePool rollout when pull secrets are modified in-place, which aligns with the core objective of the changeset.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Stable And Deterministic Test Names	✅ Passed	PR uses standard Go testing with table-driven tests, not Ginkgo. The custom check is specifically for Ginkgo test names (It(), Describe(), etc.), so it is not applicable to this PR.
Topology-Aware Scheduling Compatibility	✅ Passed	PR modifies only controller logic and API docs; no scheduling constraints, manifests, affinity rules, or Pod specs are introduced.
Ipv6 And Disconnected Network Test Compatibility	✅ Passed	PR adds no Ginkgo e2e tests, only standard Go unit tests. Custom check is not applicable to non-e2e test changes.
No-Weak-Crypto	✅ Passed	FNV-1a hash used for change detection in pull secrets/trust bundles is appropriate for non-cryptographic use. No weak crypto, custom implementations, or timing-sensitive secret comparisons found.
Container-Privileges	✅ Passed	PR modifies only Go source code files. No K8s container manifests or security context configurations are present, making the privileged container check not applicable.
No-Sensitive-Data-In-Logs	✅ Passed	PR implements proper security practices: pull secret and trust bundle contents are immediately hashed (never stored/logged), only hash values and resource names are exposed in logs and configuration.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[openshift-ci-robot]

@vsolanki12: This pull request references Jira Issue OCPBUGS-13553, which is valid.

3 validation(s) were run on this bug

• bug is open, matching expected state (open)
• bug target version (5.0.0) matches configured target version for branch (5.0.0)
• bug is in the state POST, which is one of the valid states (NEW, ASSIGNED, POST)

No GitHub users were found matching the public email listed for the QA contact in Jira (jiezhao@redhat.com), skipping review request.

Details

In response to this:

What this PR does / why we need it:

Previously, modifying the pull secret or additional trust bundle data in-place (without changing
the Secret/ConfigMap reference name) did not trigger a NodePool rollout because the rollout hash
only included the resource name, not a content hash.

This PR:

• Adds pullSecretHash and additionalTrustBundleHash fields to ConfigGenerator.rolloutConfig, computed from actual Secret/ConfigMap data
• Includes these content hashes in both Hash() and HashWithoutVersion() so in-place data changes produce a different rollout hash
• Removes the redundant hash computation from the Token struct (previously duplicated with TODO comments acknowledging the gap)
• Adds enqueueNodePoolsForSecret watcher so pull secret data changes trigger NodePool reconciliation (the pull secret lacks the nodePoolAnnotation, so the existing enqueueParentNodePool didn't match it)
• Updates the PullSecret field API doc to reflect the new behavior

Note: Deploying this fix will cause a one-time rollout for all existing NodePools on first reconciliation, because the hash now includes content hashes that were not previously part of the computation. This is expected and unavoidable.

Which issue(s) this PR fixes:

Fixes https://issues.redhat.com/browse/OCPBUGS-13553

Special notes for your reviewer:

• The additionalTrustBundleHash uses HashSimple(string) (not []byte) to match the ignition server's fetchAdditionalTrustBundle which returns string. The pullSecretHash uses HashSimple([]byte) to match the ignition server's fetchPullSecret which returns []byte.
• The AdditionalTrustBundle ConfigMap watcher is a pre-existing gap (no watcher triggers reconciliation when trust bundle data changes in-place). This is out of scope for this JIRA and can be a follow-up.
• Tested on a live KubeVirt HCP cluster (dhruvhcp-new500): confirmed that modifying pull secret in-place now triggers UpdatingConfig=True with a new target hash.

Checklist:

• Subject and description added to both, commit and PR.
• Relevant issues have been referenced.
• This change includes docs.
• This change includes unit tests.

Summary by CodeRabbit

• New Features

• Pull secret and trust bundle updates now properly trigger NodePool rollouts, ensuring configuration changes are consistently propagated across clusters.

• Bug Fixes

• Fixed secret change detection to correctly identify when pull secret data or trust bundle content is modified, triggering necessary rollouts for existing NodePools.

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 openshift-eng/jira-lifecycle-plugin repository.

[codecov]

Codecov Report

❌ Patch coverage is 60.00000% with 22 lines in your changes missing coverage. Please review.
✅ Project coverage is 41.48%. Comparing base (25817d4) to head (481df5f).
⚠️ Report is 33 commits behind head on main.

Files with missing lines	Patch %	Lines
hypershift-operator/controllers/nodepool/config.go	40.00%	8 Missing and 4 partials ⚠️
...erator/controllers/nodepool/nodepool_controller.go	65.51%	8 Missing and 2 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #8700      +/-   ##
==========================================
+ Coverage   41.43%   41.48%   +0.05%     
==========================================
  Files         756      756              
  Lines       93658    93675      +17     
==========================================
+ Hits        38807    38864      +57     
+ Misses      52128    52069      -59     
- Partials     2723     2742      +19     

Files with missing lines	Coverage Δ
hypershift-operator/controllers/nodepool/token.go	81.54% <100.00%> (-1.00%)	⬇️
...erator/controllers/nodepool/nodepool_controller.go	44.17% <65.51%> (+1.03%)	⬆️
hypershift-operator/controllers/nodepool/config.go	81.51% <40.00%> (-4.01%)	⬇️

... and 3 files with indirect coverage changes

Flag	Coverage Δ
cmd-support	34.87% <ø> (ø)
cpo-hostedcontrolplane	43.50% <ø> (ø)
cpo-other	43.17% <ø> (+0.42%)	⬆️
hypershift-operator	51.55% <60.00%> (-0.02%)	⬇️
other	31.64% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[coderabbitai]

Actionable comments posted: 2

🤖 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 106-113: The current code computes additionalTrustBundleHash from
only the ConfigMap data, but without tying it to the ConfigMap's metadata
changes it won't reliably trigger rollouts on in-place edits; update the logic
in the block that sets cg.rolloutConfig.additionalTrustBundleHash (and the
analogous block at the other occurrence) to incorporate the ConfigMap's
ResourceVersion (or UID+ResourceVersion) into the value hashed (e.g.,
concatenate additionalTrustBundle.Data["ca-bundle.crt"] with
additionalTrustBundle.ResourceVersion before calling supportutil.HashSimple) so
the hash changes on edits and becomes an effective rollout trigger.
- Around line 106-113: The code currently hashes
additionalTrustBundle.Data["ca-bundle.crt"] without verifying the key exists or
is non-empty; update the block that handles
hostedCluster.Spec.AdditionalTrustBundle (around the client.Get for
additionalTrustBundle and assignment to
cg.rolloutConfig.additionalTrustBundleHash) to validate that the "ca-bundle.crt"
key is present and non-empty in additionalTrustBundle.Data and return a
descriptive error if missing/empty, instead of calling supportutil.HashSimple on
a nil/empty value; ensure the error message references the
hostedCluster.Spec.AdditionalTrustBundle.Name and namespace for easier
debugging.

🪄 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: 2de1b436-b36f-4629-935c-d8e94db65bd3

📥 Commits

Reviewing files that changed from the base of the PR and between 9d4a78f and 2ad1771.

⛔ Files ignored due to path filters (19)

• api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/AAA_ungated.yaml is excluded by !**/zz_generated.featuregated-crd-manifests/**
• api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/ClusterUpdateAcceptRisks.yaml is excluded by !**/zz_generated.featuregated-crd-manifests/**
• api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/ClusterVersionOperatorConfiguration.yaml is excluded by !**/zz_generated.featuregated-crd-manifests/**
• api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/ExternalOIDC.yaml is excluded by !**/zz_generated.featuregated-crd-manifests/**
• api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/ExternalOIDCWithUIDAndExtraClaimMappings.yaml is excluded by !**/zz_generated.featuregated-crd-manifests/**
• api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/ExternalOIDCWithUpstreamParity.yaml is excluded by !**/zz_generated.featuregated-crd-manifests/**
• api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/GCPPlatform.yaml is excluded by !**/zz_generated.featuregated-crd-manifests/**
• api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/HCPEtcdBackup.yaml is excluded by !**/zz_generated.featuregated-crd-manifests/**
• api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/HyperShiftOnlyDynamicResourceAllocation.yaml is excluded by !**/zz_generated.featuregated-crd-manifests/**
• api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/ImageStreamImportMode.yaml is excluded by !**/zz_generated.featuregated-crd-manifests/**
• api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/KMSEncryptionProvider.yaml is excluded by !**/zz_generated.featuregated-crd-manifests/**
• api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/OpenStack.yaml is excluded by !**/zz_generated.featuregated-crd-manifests/**
• api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/TLSAdherence.yaml is excluded by !**/zz_generated.featuregated-crd-manifests/**
• cmd/install/assets/crds/hypershift-operator/zz_generated.crd-manifests/hostedclusters-Hypershift-CustomNoUpgrade.crd.yaml is excluded by !**/zz_generated.crd-manifests/**, !cmd/install/assets/**/*.yaml
• cmd/install/assets/crds/hypershift-operator/zz_generated.crd-manifests/hostedclusters-Hypershift-Default.crd.yaml is excluded by !**/zz_generated.crd-manifests/**, !cmd/install/assets/**/*.yaml
• cmd/install/assets/crds/hypershift-operator/zz_generated.crd-manifests/hostedclusters-Hypershift-TechPreviewNoUpgrade.crd.yaml is excluded by !**/zz_generated.crd-manifests/**, !cmd/install/assets/**/*.yaml
• docs/content/reference/aggregated-docs.md is excluded by !docs/content/reference/aggregated-docs.md
• docs/content/reference/api.md is excluded by !docs/content/reference/api.md
• vendor/github.com/openshift/hypershift/api/hypershift/v1beta1/hostedcluster_types.go is excluded by !vendor/**, !**/vendor/**

📒 Files selected for processing (8)

• api/hypershift/v1beta1/hostedcluster_types.go
• hypershift-operator/controllers/nodepool/conditions_test.go
• hypershift-operator/controllers/nodepool/config.go
• hypershift-operator/controllers/nodepool/config_test.go
• hypershift-operator/controllers/nodepool/nodepool_controller.go
• hypershift-operator/controllers/nodepool/secret_janitor_test.go
• hypershift-operator/controllers/nodepool/token.go
• hypershift-operator/controllers/nodepool/token_test.go

[coderabbitai]

Actionable comments posted: 1

🧹 Nitpick comments (1)

hypershift-operator/controllers/nodepool/config.go (1)

142-151: Run the rollout-upgrade e2e before merge.

These hash inputs now include referenced Secret/ConfigMap contents, and the PR notes this will trigger a one-time rollout for existing NodePools. Please make sure e2e-aws-upgrade-hypershift-operator is part of validation for this PR before merge. As per coding guidelines, "If it does, the PR must pass e2e-aws-upgrade-hypershift-operator to prove the rollout is safe."

🤖 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 142 - 151,
The change to ConfigGenerator.Hash (and the related HashWithoutVersion) now
includes referenced Secret/ConfigMap contents (cg.pullSecretHash,
cg.additionalTrustBundleHash, etc.), which will trigger a one-time rollout for
existing NodePools; before merging, run and ensure the
e2e-aws-upgrade-hypershift-operator job passes to validate the rollout is safe
and acceptable. Locate ConfigGenerator.Hash and
ConfigGenerator.HashWithoutVersion (and their use of supportutil.HashSimple and
cg.globalConfig) and run the full e2e-aws-upgrade-hypershift-operator test suite
against this PR, addressing any failures or flakiness found by the test prior to
merge. Ensure CI/job results are green and document the passing run in the PR.

Source: Coding guidelines

🤖 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/nodepool_controller_test.go`:
- Around line 3408-3410: In the TestEnqueueNodePoolsForSecret setup, don't
ignore errors from hyperv1.AddToScheme and corev1.AddToScheme; capture their
returned errors and fail the test if non-nil (e.g., use t.Fatalf or test
assertions like require.NoError) so scheme registration problems surface
immediately; update the calls that use the local variable scheme and replace the
ignored assignments with error checks for hyperv1.AddToScheme(scheme) and
corev1.AddToScheme(scheme).

---

Nitpick comments:
In `@hypershift-operator/controllers/nodepool/config.go`:
- Around line 142-151: The change to ConfigGenerator.Hash (and the related
HashWithoutVersion) now includes referenced Secret/ConfigMap contents
(cg.pullSecretHash, cg.additionalTrustBundleHash, etc.), which will trigger a
one-time rollout for existing NodePools; before merging, run and ensure the
e2e-aws-upgrade-hypershift-operator job passes to validate the rollout is safe
and acceptable. Locate ConfigGenerator.Hash and
ConfigGenerator.HashWithoutVersion (and their use of supportutil.HashSimple and
cg.globalConfig) and run the full e2e-aws-upgrade-hypershift-operator test suite
against this PR, addressing any failures or flakiness found by the test prior to
merge. Ensure CI/job results are green and document the passing run in the PR.

🪄 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: e05f9a1b-7e2f-4d8c-b871-3435bce6c467

📥 Commits

Reviewing files that changed from the base of the PR and between 2ad1771 and fe44067.

⛔ Files ignored due to path filters (19)

• api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/AAA_ungated.yaml is excluded by !**/zz_generated.featuregated-crd-manifests/**
• api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/ClusterUpdateAcceptRisks.yaml is excluded by !**/zz_generated.featuregated-crd-manifests/**
• api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/ClusterVersionOperatorConfiguration.yaml is excluded by !**/zz_generated.featuregated-crd-manifests/**
• api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/ExternalOIDC.yaml is excluded by !**/zz_generated.featuregated-crd-manifests/**
• api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/ExternalOIDCWithUIDAndExtraClaimMappings.yaml is excluded by !**/zz_generated.featuregated-crd-manifests/**
• api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/ExternalOIDCWithUpstreamParity.yaml is excluded by !**/zz_generated.featuregated-crd-manifests/**
• api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/GCPPlatform.yaml is excluded by !**/zz_generated.featuregated-crd-manifests/**
• api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/HCPEtcdBackup.yaml is excluded by !**/zz_generated.featuregated-crd-manifests/**
• api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/HyperShiftOnlyDynamicResourceAllocation.yaml is excluded by !**/zz_generated.featuregated-crd-manifests/**
• api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/ImageStreamImportMode.yaml is excluded by !**/zz_generated.featuregated-crd-manifests/**
• api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/KMSEncryptionProvider.yaml is excluded by !**/zz_generated.featuregated-crd-manifests/**
• api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/OpenStack.yaml is excluded by !**/zz_generated.featuregated-crd-manifests/**
• api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/TLSAdherence.yaml is excluded by !**/zz_generated.featuregated-crd-manifests/**
• cmd/install/assets/crds/hypershift-operator/zz_generated.crd-manifests/hostedclusters-Hypershift-CustomNoUpgrade.crd.yaml is excluded by !**/zz_generated.crd-manifests/**, !cmd/install/assets/**/*.yaml
• cmd/install/assets/crds/hypershift-operator/zz_generated.crd-manifests/hostedclusters-Hypershift-Default.crd.yaml is excluded by !**/zz_generated.crd-manifests/**, !cmd/install/assets/**/*.yaml
• cmd/install/assets/crds/hypershift-operator/zz_generated.crd-manifests/hostedclusters-Hypershift-TechPreviewNoUpgrade.crd.yaml is excluded by !**/zz_generated.crd-manifests/**, !cmd/install/assets/**/*.yaml
• docs/content/reference/aggregated-docs.md is excluded by !docs/content/reference/aggregated-docs.md
• docs/content/reference/api.md is excluded by !docs/content/reference/api.md
• vendor/github.com/openshift/hypershift/api/hypershift/v1beta1/hostedcluster_types.go is excluded by !vendor/**, !**/vendor/**

📒 Files selected for processing (9)

• api/hypershift/v1beta1/hostedcluster_types.go
• hypershift-operator/controllers/nodepool/conditions_test.go
• hypershift-operator/controllers/nodepool/config.go
• hypershift-operator/controllers/nodepool/config_test.go
• hypershift-operator/controllers/nodepool/nodepool_controller.go
• hypershift-operator/controllers/nodepool/nodepool_controller_test.go
• hypershift-operator/controllers/nodepool/secret_janitor_test.go
• hypershift-operator/controllers/nodepool/token.go
• hypershift-operator/controllers/nodepool/token_test.go

✅ Files skipped from review due to trivial changes (3)

• api/hypershift/v1beta1/hostedcluster_types.go
• hypershift-operator/controllers/nodepool/conditions_test.go
• hypershift-operator/controllers/nodepool/secret_janitor_test.go

🚧 Files skipped from review as they are similar to previous changes (4)

• hypershift-operator/controllers/nodepool/nodepool_controller.go
• hypershift-operator/controllers/nodepool/token.go
• hypershift-operator/controllers/nodepool/token_test.go
• hypershift-operator/controllers/nodepool/config_test.go

[hypershift-jira-solve-ci]

Now I have the complete picture. The workflow runs make generate update → make staticcheck → make fmt → make vet → then checks if the tree is clean. The make fmt step ran go fmt ./... which reformatted nodepool_controller_test.go, and then the final git update-index --refresh + git diff check detected the file was modified.

The log line hypershift-operator/controllers/nodepool/nodepool_controller_test.go: needs update comes from git update-index --refresh, confirming go fmt modified this file.

Test Failure Analysis Complete

Job Information

• Prow Job: verify / Verify (GitHub Actions workflow Verify)
• Build ID: 27200487133 (Job ID: 80302958412)
• PR: OCPBUGS-13553: Trigger NodePool rollout on in-place pull secret changes #8700 — OCPBUGS-13553: Trigger NodePool rollout on in-place pull secret changes
• Branch: OCPBUGS-13553-pull-secret-rollout
• Commit: fe4406752844f89ca7f560a50ac329ce09a64a41

Test Failure Analysis

Error

hypershift-operator/controllers/nodepool/nodepool_controller_test.go: needs update
Process completed with exit code 1.

Summary

The Verify workflow failed at the clean-tree check because the file hypershift-operator/controllers/nodepool/nodepool_controller_test.go was not properly formatted with go fmt before being pushed. The CI pipeline runs make fmt (which executes go fmt ./...), which reformatted the file. The subsequent git update-index --refresh detected the modification and exited with code 1, failing the job.

Root Cause

The newly added TestEnqueueNodePoolsForSecret test function in hypershift-operator/controllers/nodepool/nodepool_controller_test.go contains a struct literal with tab-aligned fields that do not conform to go fmt canonical formatting:

testCases := []struct {
    name           string
    secret         client.Object
    objects        []client.Object
    expectedCount  int
    expectedNames  []string
}{

The go fmt tool enforces consistent tab-based alignment for struct field declarations. The submitted code uses manual space/tab alignment that go fmt normalizes differently — specifically, the whitespace between field names and their types (e.g., expectedCount int with extra spacing) gets reformatted.

The CI workflow in .github/workflows/verify-reusable.yaml enforces this with a strict sequence:

1. make generate update — regenerate code
2. make staticcheck — static analysis
3. make fmt — run go fmt ./... (this reformats the test file)
4. make vet — vet checks
5. git update-index --refresh + git diff --exit-code HEAD — fail if any file was modified

Since go fmt modified nodepool_controller_test.go, the clean-tree check detected the diff and failed the job. This is purely a formatting issue — no compilation, logic, or test correctness problems exist.

Recommendations

1. Run go fmt locally before pushing: Execute go fmt ./hypershift-operator/controllers/nodepool/nodepool_controller_test.go (or make fmt) and commit the result.

2. Run the full verify locally: Execute the same sequence CI uses — make generate update && make fmt && make vet — then check git diff to ensure a clean tree before pushing.

3. Configure editor for auto-formatting: Set up your editor/IDE to run gofmt or goimports on save for .go files. This prevents formatting issues from ever reaching CI.

4. Use a pre-commit hook: Add a Git pre-commit hook that runs go fmt on staged .go files to catch formatting issues before push.

Evidence

Evidence	Detail
Failing step	git update-index --refresh in clean-tree check (last step of Verify job)
Modified file	hypershift-operator/controllers/nodepool/nodepool_controller_test.go
go fmt output	CI log shows go fmt ./... outputting hypershift-operator/controllers/nodepool/nodepool_controller_test.go (meaning it was reformatted)
git update-index output	hypershift-operator/controllers/nodepool/nodepool_controller_test.go: needs update
Exit code	1 from git update-index --refresh
Affected code	New TestEnqueueNodePoolsForSecret function added at line 3312+ with struct field alignment that go fmt normalizes
Workflow file	.github/workflows/verify-reusable.yaml — enforces make fmt → clean-tree check

---

[vsolanki12]

I have tried to reproduce and tested the fix in my test cluster as below:

Environment: KubeVirt HCP cluster dhruvhcp-new500

Custom operator image deployed:

  $ oc get pod -n hypershift -l name=operator \
      -o jsonpath='{range .items[*]}{.metadata.name}: {.spec.containers[0].image}{"\n"}{end}'
  operator-5c78c99-b5m4v: quay.io/vsolanki/hypershift-operator:fix-13553
  operator-5c78c99-srdl6: quay.io/vsolanki/hypershift-operator:fix-13553

Test scenario: Modified pull secret data in-place (without changing the Secret reference name) to verify that a NodePool rollout is now triggered.

Before fix: in-place pull secret change did NOT trigger rollout:

  $ oc patch secret dhruvhcp-new500-pull-secret -n clusters \
      --type merge -p '{"data":{".dockerconfigjson":"<modified-base64>"}}'
  secret/dhruvhcp-new500-pull-secret patched
  
  $ oc get nodepool -n clusters dhruvhcp-new500 \
      -o jsonpath='{.status.conditions[?(@.type=="UpdatingConfig")].status}'
  False

After fix: in-place pull secret change triggers rollout:

  $ oc patch secret dhruvhcp-new500-pull-secret -n clusters \
      --type merge -p '{"data":{".dockerconfigjson":"<modified-base64>"}}'
  secret/dhruvhcp-new500-pull-secret patched
  
  $ oc get nodepool -n clusters dhruvhcp-new500 \
      -o jsonpath='{.status.conditions[?(@.type=="UpdatingConfig")].status}'
  True
  
  Operator logs confirm hash change detected:
  {"level":"info","ts":"2026-06-08T...",
   "msg":"NodePool config is updating",
   "controller":"nodepool",
   "NodePool":{"name":"dhruvhcp-new500","namespace":"clusters"},
   "currentConfigHash":"<old-hash>",
   "targetConfigHash":"<new-hash>"}

Note: Deploying this fix causes a one time rollout for all existing NodePools on first reconciliation, because the hash now includes content hashes that were not previously part of the computation. This was observed and completed successfully.

[openshift-ci]

@vsolanki12: all tests passed!

Full PR test history. Your PR dashboard.

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. I understand the commands that are listed here.

[cblecker]

/hold
This is not what we want to do. Pull secrets shouldn't ever cause a rollout.

[bryan-cox]

Previously, modifying the pull secret or additional trust bundle data in-place (without changing
the Secret/ConfigMap reference name) did not trigger a NodePool rollout because the rollout hash
only included the resource name, not a content hash.

Agree with @cblecker. I don't think this bug ticket is valid anymore. I see it was created from 2023. It was a conscious decision NOT to cause a rollout when the pull secret content changed. You must make a new secret and change it in the ref to change the pull secret.

[vsolanki12]

Closing based on reviewer feedback. Both reviewers confirmed this is by design — pull secret changes should never cause a NodePool rollout. The supported workflow is to create a new Secret and update the reference name. The original bug (OCPBUGS-13553, filed 2023) is no longer valid.

[openshift-ci-robot]

@vsolanki12: This pull request references Jira Issue OCPBUGS-13553. The bug has been updated to no longer refer to the pull request using the external bug tracker. All external bug links have been closed. The bug has been moved to the NEW state.

Details

In response to this:

What this PR does / why we need it:

Previously, modifying the pull secret or additional trust bundle data in-place (without changing
the Secret/ConfigMap reference name) did not trigger a NodePool rollout because the rollout hash
only included the resource name, not a content hash.

This PR:

• Adds pullSecretHash and additionalTrustBundleHash fields to ConfigGenerator.rolloutConfig, computed from actual Secret/ConfigMap data
• Includes these content hashes in both Hash() and HashWithoutVersion() so in-place data changes produce a different rollout hash
• Removes the redundant hash computation from the Token struct (previously duplicated with TODO comments acknowledging the gap)
• Adds enqueueNodePoolsForSecret watcher so pull secret data changes trigger NodePool reconciliation (the pull secret lacks the nodePoolAnnotation, so the existing enqueueParentNodePool didn't match it)
• Updates the PullSecret field API doc to reflect the new behavior

Note: Deploying this fix will cause a one-time rollout for all existing NodePools on first reconciliation, because the hash now includes content hashes that were not previously part of the computation. This is expected and unavoidable.

Which issue(s) this PR fixes:

Fixes https://issues.redhat.com/browse/OCPBUGS-13553

Special notes for your reviewer:

• The additionalTrustBundleHash uses HashSimple(string) (not []byte) to match the ignition server's fetchAdditionalTrustBundle which returns string. The pullSecretHash uses HashSimple([]byte) to match the ignition server's fetchPullSecret which returns []byte.
• The AdditionalTrustBundle ConfigMap watcher is a pre-existing gap (no watcher triggers reconciliation when trust bundle data changes in-place). This is out of scope for this JIRA and can be a follow-up.
• Tested on a live KubeVirt HCP cluster (dhruvhcp-new500): confirmed that modifying pull secret in-place now triggers UpdatingConfig=True with a new target hash.

Checklist:

• Subject and description added to both, commit and PR.
• Relevant issues have been referenced.
• This change includes docs.
• This change includes unit tests.

Summary by CodeRabbit

• New Features

• Rollout identity now reflects pull-secret and trust-bundle contents so updates to those secrets cause NodePool rollouts.

• Controller now enqueues NodePool reconciliations when a HostedCluster pull Secret changes.

• Bug Fixes

• Fixed detection so in-place updates to a referenced pull Secret or trust bundle trigger rollouts for existing NodePools.

• Documentation

• Clarified that updating Secret data in place will trigger NodePool rollouts.

• Tests

• Added and updated unit tests covering secret-change handling and new hashing behavior.

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 openshift-eng/jira-lifecycle-plugin repository.