CNTRLPLANE-3022: Add osImageStream to NodePool spec and status#8675
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
openshift-ci-robot
added
the
jira/valid-reference
|
Skipping CI for Draft Pull Request. |
|
@sdminonne: This pull request references CNTRLPLANE-3022 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. |
|
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 (19)
📒 Files selected for processing (5)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (4)
📝 WalkthroughWalkthroughThis pull request adds support for OS image stream selection in HyperShift node pools. The change introduces a new Sequence Diagram(s)sequenceDiagram
participant ClusterAdmin
participant FeatureGateConfig
participant NodePoolAPI
ClusterAdmin->>FeatureGateConfig: add/enable "OSStreams" entry (profiles)
ClusterAdmin->>NodePoolAPI: create/update NodePool with osImageStream
NodePoolAPI->>FeatureGateConfig: read "OSStreams" feature gate
Note right of NodePoolAPI: apply CEL validation (prevent rhel-10 -> rhel-9)
NodePoolAPI-->>ClusterAdmin: NodePoolStatus.osImageStream observed or defaulted
🚥 Pre-merge checks | ✅ 10 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (10 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
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: 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 |
![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 `@api/hypershift/v1beta1/nodepool_types.go`:
- Around line 242-261: The CEL XValidation currently allows removing
osImageStream (self.name unset) to bypass the rhel-10 -> rhel-9 downgrade block;
update the validation expression on the OSImageStream field (OSImageStream in
nodepool_types.go) to require that when oldSelf.name == 'rhel-10' the new
self.name must exist and not equal 'rhel-9' (e.g. replace the rule with:
"!has(oldSelf.name) || oldSelf.name != 'rhel-10' || (has(self.name) && self.name
!= 'rhel-9')") so an unset self.name no longer permits a downgrade escape hatch.
🪄 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: 38e6fd98-eec7-43ca-b03a-e9f00daa8428
⛔ Files ignored due to path filters (19)
api/hypershift/v1beta1/zz_generated.deepcopy.gois excluded by!**/zz_generated*.go,!**/zz_generated*api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests.yamlis excluded by!**/zz_generated*api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/nodepools.hypershift.openshift.io/OSStreams.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**client/applyconfiguration/hypershift/v1beta1/nodepoolspec.gois excluded by!client/**client/applyconfiguration/hypershift/v1beta1/nodepoolstatus.gois excluded by!client/**client/applyconfiguration/hypershift/v1beta1/osimagestreamreference.gois excluded by!client/**client/applyconfiguration/utils.gois excluded by!client/**cmd/install/assets/crds/hypershift-operator/payload-manifests/featuregates/featureGate-Hypershift-Default.yamlis excluded by!cmd/install/assets/**/*.yamlcmd/install/assets/crds/hypershift-operator/payload-manifests/featuregates/featureGate-Hypershift-TechPreviewNoUpgrade.yamlis excluded by!cmd/install/assets/**/*.yamlcmd/install/assets/crds/hypershift-operator/payload-manifests/featuregates/featureGate-SelfManagedHA-Default.yamlis excluded by!cmd/install/assets/**/*.yamlcmd/install/assets/crds/hypershift-operator/payload-manifests/featuregates/featureGate-SelfManagedHA-TechPreviewNoUpgrade.yamlis excluded by!cmd/install/assets/**/*.yamlcmd/install/assets/crds/hypershift-operator/tests/nodepools.hypershift.openshift.io/featuregated.nodepools.osimagestream.testsuite.yamlis excluded by!cmd/install/assets/**/*.yamlcmd/install/assets/crds/hypershift-operator/zz_generated.crd-manifests/nodepools-CustomNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/**,!cmd/install/assets/**/*.yamlcmd/install/assets/crds/hypershift-operator/zz_generated.crd-manifests/nodepools-TechPreviewNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/**,!cmd/install/assets/**/*.yamldocs/content/reference/aggregated-docs.mdis excluded by!docs/content/reference/aggregated-docs.mddocs/content/reference/api.mdis excluded by!docs/content/reference/api.mdvendor/github.com/openshift/hypershift/api/hypershift/v1beta1/nodepool_types.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/hypershift/api/hypershift/v1beta1/zz_generated.deepcopy.gois excluded by!vendor/**,!**/vendor/**,!**/zz_generated*.go,!**/zz_generated*vendor/github.com/openshift/hypershift/api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests.yamlis excluded by!vendor/**,!**/vendor/**,!**/zz_generated*
📒 Files selected for processing (5)
api/hypershift/v1beta1/featuregates/featureGate-Hypershift-Default.yamlapi/hypershift/v1beta1/featuregates/featureGate-Hypershift-TechPreviewNoUpgrade.yamlapi/hypershift/v1beta1/featuregates/featureGate-SelfManagedHA-Default.yamlapi/hypershift/v1beta1/featuregates/featureGate-SelfManagedHA-TechPreviewNoUpgrade.yamlapi/hypershift/v1beta1/nodepool_types.go
| // osImageStream specifies an OS stream to be used for nodes in this pool. | ||
| // | ||
| // This field can be optionally set to a known OSImageStream name to change | ||
| // the OS and Extension images with a well-known, tested, release-provided | ||
| // set of images. This enables a streamlined way of switching the pool's | ||
| // node OS to a different version than the cluster default, such as | ||
| // transitioning to a major RHEL version. | ||
| // | ||
| // When set, the referenced stream overrides the default OS images for the | ||
| // pool. When omitted, the pool uses the release version's default stream | ||
| // (rhel-9 for OCP < 5.0, rhel-10 for OCP >= 5.0). | ||
| // Changing this field triggers a rollout. Forward transitions | ||
| // (rhel-9 -> rhel-10) are allowed; backward transitions | ||
| // (rhel-10 -> rhel-9) are rejected by CEL validation because | ||
| // in-place OS downgrades are not supported. | ||
| // | ||
| // +openshift:enable:FeatureGate=OSStreams | ||
| // +kubebuilder:validation:XValidation:rule="!has(oldSelf.name) || oldSelf.name != 'rhel-10' || self.name != 'rhel-9'",message="OS stream downgrade from rhel-10 to rhel-9 is not allowed; create a new NodePool instead" | ||
| // +optional | ||
| OSImageStream OSImageStreamReference `json:"osImageStream,omitempty,omitzero"` |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify whether validation/tests currently cover unset transition from rhel-10.
rg -n -C3 'osImageStream|rhel-10|rhel-9|downgrade|unset|remove' \
api/hypershift/v1beta1 \
cmd/install/assets/crds/hypershift-operator/testsRepository: openshift/hypershift
Length of output: 50377
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Focused checks for OSStreams/CEL coverage in the ratcheting testsuite assets
rg -n --max-count 50 'osImageStream' cmd/install/assets/crds/hypershift-operator/tests || true
rg -n --max-count 50 'rhel-10' cmd/install/assets/crds/hypershift-operator/tests || true
rg -n --max-count 50 'rhel-9' cmd/install/assets/crds/hypershift-operator/tests || true
# Also check the full repo for any OSStreams transition tests (still capped)
rg -n --max-count 50 'OS stream downgrade from rhel-10 to rhel-9' . || true
rg -n --max-count 50 'OSStreams' cmd/install/assets/crds/hypershift-operator/tests || trueRepository: openshift/hypershift
Length of output: 7555
🏁 Script executed:
#!/bin/bash
set -euo pipefail
f="cmd/install/assets/crds/hypershift-operator/tests/nodepools.hypershift.openshift.io/featuregated.nodepools.osimagestream.testsuite.yaml"
# Show the whole test file with line numbers (it should be small)
wc -l "$f"
cat -n "$f" | sed -n '1,260p'
# Focused searches for omit/unset/delete-like test cases
rg -n 'unset|null|remove|omitted|delete|no osImageStream|without osImageStream|osImageStream:\s*$' "$f" || trueRepository: openshift/hypershift
Length of output: 8676
🏁 Script executed:
#!/bin/bash
set -euo pipefail
f="cmd/install/assets/crds/hypershift-operator/tests/nodepools.hypershift.openshift.io/featuregated.nodepools.osimagestream.testsuite.yaml"
# Tail the bottom part to see any additional onUpdate cases
tail -n 120 "$f" | nl -ba | sed -n '1,130p'
# Search for any removal/unset-style update tests
rg -n 'remove|unset|null|omitted|without osImageStream|no osImageStream|osImageStream:\s*$' "$f" || trueRepository: openshift/hypershift
Length of output: 107
🏁 Script executed:
#!/bin/bash
set -euo pipefail
f="cmd/install/assets/crds/hypershift-operator/tests/nodepools.hypershift.openshift.io/featuregated.nodepools.osimagestream.testsuite.yaml"
# Tail and add line numbers via awk
tail -n 120 "$f" | awk '{printf "%5d\t%s\n", NR, $0}'
# Search for removal/unset-style update tests (tests earlier showed only rhel-10<->rhel-9)
rg -n 'remove|unset|null|omitted|without osImageStream|no osImageStream|osImageStream:\s*$' "$f" || trueRepository: openshift/hypershift
Length of output: 4374
🏁 Script executed:
#!/bin/bash
set -euo pipefail
f="api/hypershift/v1beta1/nodepool_types.go"
sed -n '220,290p' "$f" | nl -ba | sed -n '1,110p'Repository: openshift/hypershift
Length of output: 107
🏁 Script executed:
#!/bin/bash
set -euo pipefail
f="api/hypershift/v1beta1/nodepool_types.go"
awk 'NR>=220 && NR<=290 {printf "%5d\t%s\n", NR, $0}' "$f"Repository: openshift/hypershift
Length of output: 3831
🌐 Web query:
Kubernetes CEL behavior when accessing missing field (self.name) evaluates to null; comparison != 'string'
💡 Result:
In Kubernetes’ CEL (x-kubernetes-validations / CEL admission rules), if self.name is a missing field, CEL field selection returns the default value for the field’s type; for message-typed fields that default is null (null_type) rather than throwing. The CEL language spec also defines that null has its own type (null_type) and that type checking/comparison behavior is sensitive to the operand types. So for the specific pattern “comparison!= 'string'”: - If self.name is missing and the schema says name is a string field (not message), field selection produces the string type’s primitive default (empty string), not null, so self.name!= 'string' evaluates based on empty-string semantics. - If self.name is missing but name is modeled as a message (or wrapper/nullable message) where the missing default is null, then self.name evaluates to null_type and the comparison self.name!= 'string' is a heterogeneous comparison against null_type; CEL generally doesn’t treat null as a normal string value. In K8s/CEL, the safe/expected way to handle “missing vs present” is to guard with has(self.name) (and/or use the CEL optional field-selection operator such as e.?f for optional presence). Practical takeaway: don’t rely on “!= 'string'” to distinguish missing from explicit values. Use has(self.name) and then compare, e.g. (has(self.name)? self.name!= 'string': false) Grounding: - CEL field selection semantics: if a message field is declared but not set, CEL produces the field’s default value; for message types this default is null [1][2]. - CEL null value semantics: null has its own type (null_type) [3][2]. - Kubernetes CEL guidance: use has to safely check field existence in validations (optional/absent fields otherwise cause errors or mis-evaluation) [4][5]. - Kubernetes/CEL discussion indicates that changing presence/default handling can affect comparisons involving null [6].
Citations:
- 1: https://github.com/google/cel-spec/blob/v0.7.0/doc/langdef.md
- 2: https://github.com/google/cel-spec/blob/v0.7.1/doc/langdef.md
- 3: https://github.com/google/cel-spec/blob/master/doc/langdef.md
- 4: https://linuxcent.com/kubernetes-crd-cel-validation/
- 5: https://stackoverflow.com/questions/79734436/how-to-check-if-an-optional-struct-field-is-set-using-x-kubernetes-validations
- 6: Consistent behavior on nullable struct used in cel expression with optonal kubernetes/kubernetes#126472
Close the osImageStream unset downgrade escape hatch in the OSStreams CEL rule.
The current rule blocks explicit rhel-10 -> rhel-9, but it doesn’t block rhel-10 -> unset (missing self.name), which can satisfy self.name != 'rhel-9' and effectively bypass the downgrade protection. The existing tests only cover rhel-10 -> rhel-9 failure and don’t include an osImageStream removal/unset update case.
💡 Proposed CEL fix
- // +kubebuilder:validation:XValidation:rule="!has(oldSelf.name) || oldSelf.name != 'rhel-10' || self.name != 'rhel-9'",message="OS stream downgrade from rhel-10 to rhel-9 is not allowed; create a new NodePool instead"
+ // +kubebuilder:validation:XValidation:rule="!has(oldSelf.name) || oldSelf.name != 'rhel-10' || (has(self.name) && self.name != 'rhel-9')",message="OS stream downgrade from rhel-10 to rhel-9 is not allowed; create a new NodePool instead"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // osImageStream specifies an OS stream to be used for nodes in this pool. | |
| // | |
| // This field can be optionally set to a known OSImageStream name to change | |
| // the OS and Extension images with a well-known, tested, release-provided | |
| // set of images. This enables a streamlined way of switching the pool's | |
| // node OS to a different version than the cluster default, such as | |
| // transitioning to a major RHEL version. | |
| // | |
| // When set, the referenced stream overrides the default OS images for the | |
| // pool. When omitted, the pool uses the release version's default stream | |
| // (rhel-9 for OCP < 5.0, rhel-10 for OCP >= 5.0). | |
| // Changing this field triggers a rollout. Forward transitions | |
| // (rhel-9 -> rhel-10) are allowed; backward transitions | |
| // (rhel-10 -> rhel-9) are rejected by CEL validation because | |
| // in-place OS downgrades are not supported. | |
| // | |
| // +openshift:enable:FeatureGate=OSStreams | |
| // +kubebuilder:validation:XValidation:rule="!has(oldSelf.name) || oldSelf.name != 'rhel-10' || self.name != 'rhel-9'",message="OS stream downgrade from rhel-10 to rhel-9 is not allowed; create a new NodePool instead" | |
| // +optional | |
| OSImageStream OSImageStreamReference `json:"osImageStream,omitempty,omitzero"` | |
| // osImageStream specifies an OS stream to be used for nodes in this pool. | |
| // | |
| // This field can be optionally set to a known OSImageStream name to change | |
| // the OS and Extension images with a well-known, tested, release-provided | |
| // set of images. This enables a streamlined way of switching the pool's | |
| // node OS to a different version than the cluster default, such as | |
| // transitioning to a major RHEL version. | |
| // | |
| // When set, the referenced stream overrides the default OS images for the | |
| // pool. When omitted, the pool uses the release version's default stream | |
| // (rhel-9 for OCP < 5.0, rhel-10 for OCP >= 5.0). | |
| // Changing this field triggers a rollout. Forward transitions | |
| // (rhel-9 -> rhel-10) are allowed; backward transitions | |
| // (rhel-10 -> rhel-9) are rejected by CEL validation because | |
| // in-place OS downgrades are not supported. | |
| // | |
| // +openshift:enable:FeatureGate=OSStreams | |
| // +kubebuilder:validation:XValidation:rule="!has(oldSelf.name) || oldSelf.name != 'rhel-10' || (has(self.name) && self.name != 'rhel-9')",message="OS stream downgrade from rhel-10 to rhel-9 is not allowed; create a new NodePool instead" | |
| // +optional | |
| OSImageStream OSImageStreamReference `json:"osImageStream,omitempty,omitzero"` |
🤖 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 `@api/hypershift/v1beta1/nodepool_types.go` around lines 242 - 261, The CEL
XValidation currently allows removing osImageStream (self.name unset) to bypass
the rhel-10 -> rhel-9 downgrade block; update the validation expression on the
OSImageStream field (OSImageStream in nodepool_types.go) to require that when
oldSelf.name == 'rhel-10' the new self.name must exist and not equal 'rhel-9'
(e.g. replace the rule with: "!has(oldSelf.name) || oldSelf.name != 'rhel-10' ||
(has(self.name) && self.name != 'rhel-9')") so an unset self.name no longer
permits a downgrade escape hatch.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #8675 +/- ##
=======================================
Coverage 41.43% 41.43%
=======================================
Files 756 756
Lines 93647 93647
=======================================
Hits 38802 38802
Misses 52124 52124
Partials 2721 2721
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
Test Failure Analysis CompleteJob Information
Test Failure AnalysisErrorSummaryThe Root CauseThe failure is a stale-base false positive in Codecov's project-level coverage gate. The specific chain:
Recommendations
Evidence
|
Add OSImageStreamReference type and osImageStream fields to NodePoolSpec and NodePoolStatus, gated behind the OSStreams feature gate (TechPreview). This is Phase 0 of the Dual-Stream RHEL 9/10 NodePool enhancement, enabling per-pool OS stream selection in HyperShift. The spec field allows users to override the default OS images for a NodePool by referencing a known OSImageStream (rhel-9 or rhel-10). A field-level CEL validation rule prevents downgrades from rhel-10 to rhel-9. The status field reports the observed OS stream on nodes. Signed-off-by: Salvatore Dario Minonne <sminonne@redhat.com> Commit-Message-Assisted-by: Claude (via Claude Code)
sdminonne
force-pushed
the
CNTRLPLANE-3022/Dual-Stream-NodePool-API-add-osImageStream-to-spec-status
branch
from
0b89acd to
ee12e05
Compare
openshift-ci
Bot
removed
the
do-not-merge/work-in-progress
|
@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. |
2 participants
What this PR does / why we need it:
Phase 0 of the Dual-Stream RHEL 9/10 NodePool enhancement (OCPSTRAT-3014 / CNTRLPLANE-3022). Adds
osImageStreamfields to NodePool spec and status, gated behind theOSStreamsfeature gate (TechPreview).OSImageStreamReferencewithnamefield restricted torhel-9orrhel-10via Enum validationNodePoolSpec.osImageStream— allows users to override the default OS images for a NodePool by referencing a known OSImageStream. A field-level CEL rule prevents downgrades fromrhel-10torhel-9NodePoolStatus.osImageStream— reports the observed OS stream on nodes in the poolOSStreamsfeature gate registered in all 4 feature gate files (disabled in Default, enabled in TechPreview)Which issue(s) this PR fixes:
Fixes CNTRLPLANE-3022
Special notes for your reviewer:
The CEL downgrade-prevention rule is placed at the field level (on
osImageStream) rather than the struct level (onNodePoolSpec). This ensures the rule is co-gated with the field itself and avoids CRD installation failures in the Default variant where the field doesn't exist in the schema.Checklist:
Test plan
make api— CRDs regenerated correctly (field present only in TechPreview/CustomNoUpgrade)make update— Full dependency sync passedmake verify— Linting and generation passedmake test-envtest-ocp— All envtests passed across K8s 1.30–1.35make test— All unit tests passedmake run-gitlint— Commit message validated🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Chores