CNTRLPLANE-3030: ignition-server os-stream consumption and ValidOSImageStream condition#8792
CNTRLPLANE-3030: ignition-server os-stream consumption and ValidOSImageStream condition#8792sdminonne wants to merge 2 commits into
Conversation
…lidation Add a dedicated ValidOSImageStream condition to the NodePool status that validates the spec.osImageStream field independently. This provides granular feedback to users when an unsupported OS image stream name is specified (e.g. "rhel-8"), complementing the osImageStream reconciliation wiring in PR openshift#8730 (CNTRLPLANE-3553). The condition is: - Removed when osImageStream.Name is empty (no opinion needed) - True when the name is a supported stream (rhel-9, rhel-10) - False with InvalidOSImageStream reason for unsupported names Applies to both Replace and InPlace upgrade strategies since the condition validates the spec field regardless of how the stream is consumed downstream. Depends-on: openshift#8730 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
openshift-ci-robot
added
the
jira/valid-reference
|
@sdminonne: This pull request references CNTRLPLANE-3030 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 story 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. |
📝 WalkthroughWalkthroughThe PR adds OS image stream support across two subsystems. In the NodePool controller, two new API constants ( In the ignition server, a new 🚥 Pre-merge checks | ✅ 11✅ Passed checks (11 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
openshift-ci
Bot
added
area/api
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #8792 +/- ##
==========================================
+ Coverage 42.09% 42.12% +0.02%
==========================================
Files 766 767 +1
Lines 95047 95081 +34
==========================================
+ Hits 40012 40050 +38
+ Misses 52221 52218 -3
+ Partials 2814 2813 -1
... and 1 file 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:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
ignition-server/controllers/tokensecret_controller_test.go (1)
708-751: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winAdd a missing-key compatibility case for
TokenSecretOSStreamKey.Current cases only validate when the key exists (including empty). Add a case where the key is omitted entirely, so the test locks in behavior for pre-upgrade secrets.
Small test extension
testCases := []struct { name string osStream string expectedOSStream string + includeOSStreamKey bool }{ { name: "When os-stream is set to rhel-10 it should pass rhel-10 to GetPayload", osStream: "rhel-10", expectedOSStream: "rhel-10", + includeOSStreamKey: true, }, @@ { name: "When os-stream is empty it should pass empty string to GetPayload", osStream: "", expectedOSStream: "", + includeOSStreamKey: true, + }, + { + name: "When os-stream key is absent it should pass empty string to GetPayload", + expectedOSStream: "", + includeOSStreamKey: false, }, } @@ - secret := &corev1.Secret{ + data := map[string][]byte{ + TokenSecretTokenKey: []byte(uuid.New().String()), + TokenSecretReleaseKey: []byte("release"), + TokenSecretConfigKey: compressedConfig.Bytes(), + TokenSecretPullSecretHashKey: []byte("pull-hash"), + TokenSecretHCConfigurationHashKey: []byte("hc-hash"), + TokenSecretAdditionalTrustBundleHashKey: []byte("bundle-hash"), + } + if tc.includeOSStreamKey { + data[TokenSecretOSStreamKey] = []byte(tc.osStream) + } + secret := &corev1.Secret{ ObjectMeta: metav1.ObjectMeta{ @@ - Data: map[string][]byte{ - ... - TokenSecretOSStreamKey: []byte(tc.osStream), - }, + Data: data, }🤖 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_test.go` around lines 708 - 751, Add a new test case to the testCases slice that validates backward compatibility when the TokenSecretOSStreamKey is completely missing from the secret's Data map. Create a test case with a descriptive name indicating the key is missing, set expectedOSStream to an empty string as the default behavior, and then in the test setup for this case, ensure the TokenSecretOSStreamKey is not included in the Data map at all (unlike the other cases which all include this key). This will verify that the code correctly handles pre-upgrade secrets that don't have the os-stream key defined.
🤖 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 `@ignition-server/cmd/run_local_ignitionprovider.go`:
- Line 114: The GetPayload method call is hardcoding the osStream parameter to
an empty string instead of using the value from the token secret. Extract the
os-stream value from the token secret (similar to how other parameters like
config are extracted) and pass it as the osStream argument to the GetPayload
call instead of the empty string, so that stream-specific payload generation can
be properly reproduced in the debug path.
In `@ignition-server/controllers/tokensecret_controller.go`:
- Around line 275-278: The osStream variable extracted from the Secret data on
line 275 is passed directly to r.IgnitionProvider.GetPayload without validation,
creating a trust-boundary vulnerability where unsupported values could produce
invalid manifests. Add an allow-list validation check before the GetPayload call
to ensure osStream is one of the supported values: empty string, "rhel-9", or
"rhel-10". If the value does not match the allow-list, return an appropriate
error to prevent downstream processing with invalid osStream values.
---
Nitpick comments:
In `@ignition-server/controllers/tokensecret_controller_test.go`:
- Around line 708-751: Add a new test case to the testCases slice that validates
backward compatibility when the TokenSecretOSStreamKey is completely missing
from the secret's Data map. Create a test case with a descriptive name
indicating the key is missing, set expectedOSStream to an empty string as the
default behavior, and then in the test setup for this case, ensure the
TokenSecretOSStreamKey is not included in the Data map at all (unlike the other
cases which all include this key). This will verify that the code correctly
handles pre-upgrade secrets that don't have the os-stream key defined.
🪄 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: 1e670da7-7d19-442c-9153-6fcd9693989c
⛔ Files ignored due to path filters (1)
vendor/github.com/openshift/hypershift/api/hypershift/v1beta1/nodepool_conditions.gois excluded by!vendor/**,!**/vendor/**
📒 Files selected for processing (9)
api/hypershift/v1beta1/nodepool_conditions.gohypershift-operator/controllers/nodepool/nodepool_controller.gohypershift-operator/controllers/nodepool/osstream.gohypershift-operator/controllers/nodepool/osstream_test.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
|
/cc @enxebre |
|
/cc @jparrill |
…OSImageStream CR (InPlace) The NodePool controller writes an os-stream key to the token secret (see previous commit). This change implements the consumer side for the InPlace strategy: the ignition server reads that key, passes it through GetPayload(), and writes a 99_osimagestream.yaml CR into the MCC manifest directory so the MCO bootstrap pipeline resolves the correct osImageURL for the selected RHEL stream. For Replace strategy nodes this is a no-op — they boot directly from the AMI/image selected in the previous commit and never consume this ignition payload path. This implements enhancement Phase 2 Steps 7 and 8: - Add osStream parameter to IgnitionProvider.GetPayload() interface - Read os-stream from token secret in TokenSecretReconciler - Write OSImageStream CR between runMCO() and runMCC() when non-empty - Fix duplicate TestGetRHELStream compilation error Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
sdminonne
force-pushed
the
CNTRLPLANE-3030
branch
from
5062eb4 to
3a5a608
Compare
|
Addressed @coderabbitai review feedback:
|
|
Tip For best results, initiate chat on the files or code changes.
|
|
@sdminonne: 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. |
|
/approve for API |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: JoelSpeed, sdminonne 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 |
3 participants
Summary
This PR implements the consumer side of dual-stream RHEL NodePool support (enhancement), enabling the ignition server to inject an
OSImageStreamcustom resource into the ignition payload based on theos-streamkey written to the token secret by the NodePool controller.It also adds a dedicated
ValidOSImageStreamcondition to NodePool status for granular validation feedback onspec.osImageStream.Commit 1 —
ValidOSImageStreamconditionAdds a new NodePool condition (
ValidOSImageStream) that validatesspec.osImageStream.Nameindependently:osImageStream.Nameis empty (no opinion needed)rhel-9,rhel-10)InvalidOSImageStreamreason for unsupported namesThis complements PR #8730's validation inside
validMachineConfigConditionby providing a separate, user-facing signal specifically for osImageStream issues.Commit 2 — Ignition server
os-streamconsumption (InPlace)Reads the
os-streamkey from the token secret and injects anOSImageStreamCR manifest (99_osimagestream.yaml) into the MCC input directory betweenrunMCO()andrunMCC(). This is the mechanism by which InPlace nodes learn which RHEL stream to rebase to.Changes:
tokensecret_controller.go— readsos-streamfrom token secret, passes it toGetPayload()local_ignitionprovider.go— acceptsosStreamparam, writesOSImageStreamCR viawriteOSImageStreamManifest()run_local_ignitionprovider.go— readsos-streamfrom token secret for the debug pathUpgrade strategy coverage
InPlace
Fully functional. The ignition server reads the resolved stream from the token secret and injects the
OSImageStreamCR into the ignition payload. MCO/MCC on the node uses this to determine which RHEL stream to rebase to during in-place updates.Replace
Not directly addressed by this PR. Replace strategy relies on the NodePool controller selecting the correct boot image (AMI/GCP image) for the target stream — that wiring is fully handled by PR #8730 for both AWS (
StreamForNameindefaultNodePoolAMI) and GCP (StreamForNameindefaultNodePoolGCPImage). This PR'sValidOSImageStreamcondition applies to both strategies since it validates the spec field.What this PR does NOT cover
The following are handled by PR #8730 (CNTRLPLANE-3553) and deliberately excluded here:
resolveAWSAMI,defaultNodePoolAMI,resolveGCPImage,defaultNodePoolGCPImageStreamForNameintegration for both AWS and GCP boot image selectionresolvedRHELStreaminrolloutConfigand config hash computationos-streamwrite (TokenSecretOSStreamKey, writingresolvedRHELStream)setOSImageStreamStatus()for settingnodePool.Status.OSImageStreamgetRHELStream()wrapper inosstream.goRelationship with PR #8730
This PR is the consumer to #8730's producer:
getRHELStream()os-streamto token secretTokenSecretOSStreamKeyStreamForNamestatus.osImageStreamsetOSImageStreamStatus()os-streamfrom token secrettokensecret_controller.goOSImageStreamCR into ignitionlocal_ignitionprovider.goValidOSImageStreamconditionosstream.go+ API constantsThis PR depends on #8730 and should be rebased after it merges. The expected rebase conflicts are limited to
osstream.goandosstream_test.go(both PRs create these files) and are resolved by keeping all functions from both sides.Known limitations
ExternalTopologyModeguard: MCO currently blocksOSImageStreamprocessing for HyperShift topology. MCO-side changes are needed before the injected CR takes effect.Test plan
TestValidOSImageStreamCondition,TestWriteOSImageStreamManifest,TestReconcileOSStreamPropagationmake verifypasses cleanDepends-on: #8730
🤖 Generated with Claude Code