ARO-24037: gate cloud config on hash to prevent serving stale content#8946
ARO-24037: gate cloud config on hash to prevent serving stale content#8946twolff-gh wants to merge 2 commits into
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
|
Skipping CI for Draft Pull Request. |
openshift-ci
Bot
added
do-not-merge/work-in-progress
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis change adds deterministic hashing for cloud provider ConfigMap data, stores the hash in nodepool token secrets, and threads it through ignition payload generation. The ignition server now carries the hash in cache entries, invalidates cached payloads when the hash changes, and verifies Azure/OpenStack cloud provider ConfigMap data against the expected hash before writing cloud config. Tests were added and updated for hash generation, propagation, validation, and cache invalidation. Sequence Diagram(s)sequenceDiagram
participant ConfigGenerator
participant NodePoolToken
participant TokenSecretController
participant LocalIgnitionProvider
participant Cache
participant KubeAPI
ConfigGenerator->>KubeAPI: Read cloud provider ConfigMap
KubeAPI-->>ConfigGenerator: ConfigMap data
ConfigGenerator-->>NodePoolToken: cloudConfigHash
NodePoolToken->>TokenSecretController: store cloud-config-hash
TokenSecretController->>Cache: read cached payload
TokenSecretController->>TokenSecretController: compare CloudConfigHash
TokenSecretController->>LocalIgnitionProvider: GetPayload(..., cloudConfigHash)
LocalIgnitionProvider->>KubeAPI: Read cloud provider ConfigMap
KubeAPI-->>LocalIgnitionProvider: ConfigMap data
LocalIgnitionProvider->>LocalIgnitionProvider: hash and compare
LocalIgnitionProvider-->>TokenSecretController: payload or error
TokenSecretController->>Cache: store payload + CloudConfigHash
🚥 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 |
openshift-ci
Bot
removed
the
do-not-merge/work-in-progress
![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
🤖 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 359-392: The hash computation in
ConfigGenerator.GetCloudConfigHash is duplicated with
LocalIgnitionProvider.writeCloudProviderConfig, so extract the sort-and-hash
logic into a shared helper in support/util and have both call it. Make the
helper accept ConfigMap data as map[string]string and return the hash, keeping
the exact same behavior on both paths. While refactoring, address the
concatenation ambiguity by using a deterministic delimiter or encoding so
different key/value pairs cannot collide, and ensure both callers use the same
helper implementation.
🪄 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: 0594b6c0-9732-400b-b318-74d99bbd8b7a
📒 Files selected for processing (7)
hypershift-operator/controllers/nodepool/config.gohypershift-operator/controllers/nodepool/config_test.gohypershift-operator/controllers/nodepool/token.goignition-server/cmd/run_local_ignitionprovider.goignition-server/controllers/local_ignitionprovider.goignition-server/controllers/tokensecret_controller.goignition-server/controllers/tokensecret_controller_test.go
|
/test verify-deps |
b081c30 to
0760f42
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
ignition-server/controllers/local_ignitionprovider_test.go (1)
1154-1177: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider adding OpenStack hash match/mismatch cases for symmetry.
The hash validation logic in
writeCloudProviderConfigapplies identically to both Azure and OpenStack (perlocal_ignitionprovider.go), but only Azure gets match/mismatch coverage here. Adding equivalent OpenStack cases would close the coverage gap for the same code path.🤖 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 `@ignition-server/controllers/local_ignitionprovider_test.go` around lines 1154 - 1177, Add OpenStack hash validation coverage to the existing cloud config tests in local_ignitionprovider_test.go so it mirrors the Azure cases. Extend the table-driven cases around writeCloudProviderConfig to include both a matching and mismatching cloudConfigHash for hyperv1.OpenStackPlatform, using the same ConfigMap setup and expected write/error behavior, so the shared hash-checking path in writeCloudProviderConfig is exercised for both providers.ignition-server/controllers/tokensecret_controller.go (1)
278-278: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAvoid shadowing
errinside the payload-generation closure.Use distinct local names for the provider result/error to keep the control flow unambiguous.
Refactor to avoid shadowing
- payload, err := r.IgnitionProvider.GetPayload(ctx, releaseImage, config.String(), pullSecretHash, additionalTrustBundleHash, hcConfigurationHash, cloudConfigHash) - if err != nil { - return nil, fmt.Errorf("error getting ignition payload: %w", err) + generatedPayload, providerErr := r.IgnitionProvider.GetPayload(ctx, releaseImage, config.String(), pullSecretHash, additionalTrustBundleHash, hcConfigurationHash, cloudConfigHash) + if providerErr != nil { + return nil, fmt.Errorf("error getting ignition payload: %w", providerErr) } duration := time.Since(start).Round(time.Second).Seconds() log.Info("got ignition payload", "duration", duration) PayloadGenerationSeconds.Observe(duration) - return payload, err + return generatedPayload, nilAs per coding guidelines, "
**/*.go: Avoid variable shadowing."🤖 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 `@ignition-server/controllers/tokensecret_controller.go` at line 278, Avoid shadowing the outer err in the payload-generation closure by using distinct local names for the result and error from IgnitionProvider.GetPayload in tokensecret_controller.go. Update the closure that builds the payload so the provider call assigns to new variables, then handle the error using that local name before returning, keeping the control flow in the payload-generation path unambiguous.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/token.go`:
- Around line 365-371: The token Secret update in token.go can change the
cloud-config hash in place while keeping the same token ID, which lets the
ignition cache keep serving stale payloads. Update the token handling around the
tokenSecret.Data assignments in the token reconciliation path so cache entries
are keyed by the hash-bearing Secret data or are invalidated whenever
TokenSecretCloudConfigHashKey changes, and consider pairing the hash update with
token rotation to ensure fresh cloud.conf is served.
In `@ignition-server/controllers/tokensecret_controller.go`:
- Around line 275-278: The cached-token path in the token secret controller is
skipping the new cloud-config-hash validation, so update the cache-hit branch to
compare the current TokenSecretCloudConfigHashKey value before returning a
cached payload. In the token secret handling flow around cloudConfigHash and the
GetPayload call, make sure the same hash check runs for both cache misses and
cache hits so an in-place TokenSecret update cannot bypass validation.
---
Nitpick comments:
In `@ignition-server/controllers/local_ignitionprovider_test.go`:
- Around line 1154-1177: Add OpenStack hash validation coverage to the existing
cloud config tests in local_ignitionprovider_test.go so it mirrors the Azure
cases. Extend the table-driven cases around writeCloudProviderConfig to include
both a matching and mismatching cloudConfigHash for hyperv1.OpenStackPlatform,
using the same ConfigMap setup and expected write/error behavior, so the shared
hash-checking path in writeCloudProviderConfig is exercised for both providers.
In `@ignition-server/controllers/tokensecret_controller.go`:
- Line 278: Avoid shadowing the outer err in the payload-generation closure by
using distinct local names for the result and error from
IgnitionProvider.GetPayload in tokensecret_controller.go. Update the closure
that builds the payload so the provider call assigns to new variables, then
handle the error using that local name before returning, keeping the control
flow in the payload-generation path unambiguous.
🪄 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: f0a4ff14-07f4-4310-a358-a2fbfcbacd29
📒 Files selected for processing (8)
hypershift-operator/controllers/nodepool/config.gohypershift-operator/controllers/nodepool/config_test.gohypershift-operator/controllers/nodepool/token.goignition-server/cmd/run_local_ignitionprovider.goignition-server/controllers/local_ignitionprovider.goignition-server/controllers/local_ignitionprovider_test.goignition-server/controllers/tokensecret_controller.goignition-server/controllers/tokensecret_controller_test.go
🚧 Files skipped from review as they are similar to previous changes (5)
- ignition-server/controllers/tokensecret_controller_test.go
- ignition-server/cmd/run_local_ignitionprovider.go
- hypershift-operator/controllers/nodepool/config.go
- hypershift-operator/controllers/nodepool/config_test.go
- ignition-server/controllers/local_ignitionprovider.go
51be2af to
8d34581
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #8946 +/- ##
==========================================
+ Coverage 44.13% 44.15% +0.01%
==========================================
Files 772 779 +7
Lines 96260 98056 +1796
==========================================
+ Hits 42488 43294 +806
- Misses 50826 51729 +903
- Partials 2946 3033 +87
... and 9 files with indirect coverage changes
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: enxebre, twolff-gh The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
Bot
added
the
approved
|
/retitle: ARO-24037: gate cloud config on hash to prevent serving stale content |
openshift-ci
Bot
changed the title
openshift-ci-robot
added
the
jira/valid-reference
|
@twolff-gh: This pull request references ARO-24037 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the feature to target the "5.0.0" version, but no target version was set. DetailsIn response to this:
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. |
jparrill
left a comment
jparrill
left a comment
There was a problem hiding this comment.
Dropped some comments. Thanks!
I've retitled the issue in order to fill the bot expectations
86769f7 to
50e651b
Compare
openshift-ci
Bot
removed
the
needs-rebase
|
/lgtm |
|
Scheduling tests matching the |
|
/retitle ARO-24037: gate cloud config on hash to prevent serving stale content |
openshift-ci
Bot
changed the title
|
/label acknowledge-critical-fixes-only |
openshift-ci
Bot
added
the
acknowledge-critical-fixes-only
Test Failure Analysis CompleteJob Information
Test Failure AnalysisErrorSummaryAll 13 hosted clusters created during the test run initially experienced ExternalDNS resolution failures ( Root CauseThis failure is caused by transient ExternalDNS / Route53 DNS propagation delays in the shared CI management cluster infrastructure, not by the PR changes. Key evidence:
Recommendations
Evidence
|
|
/test e2e-aws |
|
/retest-required |
|
/test e2e-azure-v2-self-managed |
2 similar comments
|
/test e2e-azure-v2-self-managed |
|
/test e2e-azure-v2-self-managed |
…g stale content - Add cloud config hash computation in NodePool config reconciliation using ConfigMap content from the HCP namespace - Propagate cloud-config-hash through token secret data so the ignition server can validate content freshness before serving payloads - Watch cloud config ConfigMaps (cloud-config, openshift-config-user-ca) for convergence before generating ignition tokens - Add HashSimple utility in support/util for consistent content hashing Signed-off-by: Todd Wolff <twolff@redhat.com> Commit-Message-Assisted-by: Claude (via Claude Code)
…erver - Add cloudConfigHash parameter to IgnitionProvider.GetPayload interface (appended after osStream to preserve existing parameter positions) - Gate ignition payload generation on cloud config hash match in LocalIgnitionProvider, returning an error when the ConfigMap is stale - Invalidate cached payloads when cloud config hash changes by storing the hash in CacheValue and comparing on reconciliation - Report CloudConfigPending reason on token secret when hash validation fails so NodePool controller can observe convergence state Signed-off-by: Todd Wolff <twolff@redhat.com> Commit-Message-Assisted-by: Claude (via Claude Code)
6a66d36 to
077d180
Compare
|
|
/lgtm |
|
Scheduling tests matching the |
|
/retest |
|
/retest-required |
2 similar comments
|
/retest-required |
|
/retest-required |
|
@twolff-gh: all tests passed! Full PR test history. Your PR dashboard. 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. I understand the commands that are listed here. |
4 participants
Prevents stale cloud.conf from being served during the race between the CPO updating azure-cloud-config and the ignition server reading it.
Follows the existing pullSecretHash / additionalTrustBundleHash pattern: nodepool controller hashes the cloud config ConfigMap and stores it in the token secret; ignition server verifies the hash before writing cloud.conf, retrying on mismatch.
What this PR does / why we need it:
Which issue(s) this PR fixes:
Fixes https://redhat.atlassian.net/browse/ARO-24037
Related to #8865
Special notes for your reviewer:
Checklist:
Summary by CodeRabbit
New Features
cloud-config-hashvalue stored in token Secrets and propagated into ignition payload generation.Bug Fixes