MGMT-23060: Make InfraEnv CpuArchitecture immutable after creation#9999
Conversation
openshift-ci-robot
added
the
jira/valid-reference
|
@bluesort: This pull request references MGMT-23060 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 bug to target the "4.22.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. |
|
Important Review skippedAuto reviews are limited based on label configuration. 🚫 Review skipped — only excluded labels are configured. (1)
Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
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:
WalkthroughAdds webhook validation that prevents changing an InfraEnv's Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
@bluesort: This pull request references MGMT-23060 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 bug to target the "4.22.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. |
openshift-ci
Bot
added
the
size/M
|
@bluesort: This pull request references MGMT-23060 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 bug to target the "4.22.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. |
openshift-ci
Bot
added
the
approved
|
@bluesort: This pull request references MGMT-23060 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 bug to target the "4.22.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. |
![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
🧹 Nitpick comments (1)
pkg/webhooks/agentinstall/v1beta1/infraenv_admission_hook_test.go (1)
218-239: Missing test coverage for empty-string edge cases.The tests cover the explicit change scenario (
x86_64→s390x) and the no-change scenario, but do not cover:
- Transition from
""(empty/unset) to"x86_64"— would this be blocked or allowed?- Transition from
""to a different architecture like"arm64"- Transition from
"x86_64"to""(clearing the field)These edge cases are important because existing InfraEnvs created without explicitly setting
CpuArchitecturewill have an empty string stored.📝 Suggested additional test cases
{ name: "Test InfraEnv.Spec.CpuArchitecture can't change from empty to non-default", newSpec: v1beta1.InfraEnvSpec{ CpuArchitecture: models.ClusterCPUArchitectureArm64, }, oldSpec: v1beta1.InfraEnvSpec{ CpuArchitecture: "", }, operation: admissionv1.Update, expectedAllowed: false, // or true, depending on desired behavior }, { name: "Test InfraEnv.Spec.CpuArchitecture transition from empty to default", newSpec: v1beta1.InfraEnvSpec{ CpuArchitecture: models.ClusterCPUArchitectureX8664, }, oldSpec: v1beta1.InfraEnvSpec{ CpuArchitecture: "", }, operation: admissionv1.Update, expectedAllowed: true, // semantically the same as default },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/webhooks/agentinstall/v1beta1/infraenv_admission_hook_test.go` around lines 218 - 239, Add table-driven test cases in infraenv_admission_hook_test.go to cover empty-string edge cases for CpuArchitecture: add cases where oldSpec.CpuArchitecture == "" and newSpec is models.ClusterCPUArchitectureX8664 (expectedAllowed true if treated as default) and where oldSpec.CpuArchitecture == "" and newSpec is models.ClusterCPUArchitectureArm64 (expectedAllowed false if disallowed), and a case where oldSpec has models.ClusterCPUArchitectureX8664 and newSpec.CpuArchitecture == "" (expectedAllowed false if clearing is disallowed); ensure these new entries are added to the same test cases slice used by the Test InfraEnv admission logic (the table that iterates over name, newSpec, oldSpec, operation, expectedAllowed) and use admissionv1.Update for operation so they run alongside the existing CpuArchitecture immutability tests.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/webhooks/agentinstall/v1beta1/infraenv_admission_hook.go`:
- Around line 257-268: The immutability check in the CpuArchitecture comparison
blocks changes from blocking empty->default transitions; update the check in
infraenv_admission_hook.go (the block that compares
oldObject.Spec.CpuArchitecture and newObject.Spec.CpuArchitecture) to normalize
empty string to the API default "x86_64" before comparing (either by small
helper normalizeCpuArch(value string) string or inline), so "" is treated as
"x86_64" and does not trigger a failed validation when new is explicitly
"x86_64"; also add a unit test covering the edge case oldSpec.CpuArchitecture ==
"" and newSpec.CpuArchitecture == "x86_64" (expected allowed) mirroring how
areClusterRefsEqual handles nils.
---
Nitpick comments:
In `@pkg/webhooks/agentinstall/v1beta1/infraenv_admission_hook_test.go`:
- Around line 218-239: Add table-driven test cases in
infraenv_admission_hook_test.go to cover empty-string edge cases for
CpuArchitecture: add cases where oldSpec.CpuArchitecture == "" and newSpec is
models.ClusterCPUArchitectureX8664 (expectedAllowed true if treated as default)
and where oldSpec.CpuArchitecture == "" and newSpec is
models.ClusterCPUArchitectureArm64 (expectedAllowed false if disallowed), and a
case where oldSpec has models.ClusterCPUArchitectureX8664 and
newSpec.CpuArchitecture == "" (expectedAllowed false if clearing is disallowed);
ensure these new entries are added to the same test cases slice used by the Test
InfraEnv admission logic (the table that iterates over name, newSpec, oldSpec,
operation, expectedAllowed) and use admissionv1.Update for operation so they run
alongside the existing CpuArchitecture immutability tests.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 6450f9f4-e4fb-458d-b6e6-ae2658c0552b
📒 Files selected for processing (3)
docs/user-guide/multiarch-and-heterogeneous-openshift.mdpkg/webhooks/agentinstall/v1beta1/infraenv_admission_hook.gopkg/webhooks/agentinstall/v1beta1/infraenv_admission_hook_test.go
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #9999 +/- ##
==========================================
+ Coverage 44.09% 45.37% +1.28%
==========================================
Files 415 416 +1
Lines 72258 74606 +2348
==========================================
+ Hits 31860 33853 +1993
- Misses 37518 37808 +290
- Partials 2880 2945 +65
🚀 New features to boost your workflow:
|
|
@bluesort: This pull request references MGMT-23060 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 bug to target the "4.22.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. |
|
/test edge-e2e-ai-operator-ztp |
|
/test edge-subsystem-kubeapi-aws |
openshift-ci
Bot
added
size/L
|
@bluesort: This pull request references MGMT-23060 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 bug to target the "4.22.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. |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
internal/controller/controllers/bmh_agent_controller.go (1)
1561-1579: Return a composite not-found error here.When every lookup misses, this bubbles up only the last
NotFound, which points operators at the Hypershift fallback even on standard clusters. Returning one error that lists the attempted sources would make this path much easier to debug.Possible adjustment
var err error + attempted := make([]string, 0, len(sources)) for _, src := range sources { + attempted = append(attempted, fmt.Sprintf("%s/%s", src.namespace, src.name)) key := types.NamespacedName{ Namespace: src.namespace, Name: src.name, } if err = spokeClient.Get(ctx, key, configMap); err == nil { certData = configMap.Data[src.key] break } // Only try the next source if the configmap doesn't exist. // fail fast on other errors if !k8serrors.IsNotFound(err) { return encodedMCSCrt, ignitionWithMCSCert, err } } if err != nil { - return encodedMCSCrt, ignitionWithMCSCert, err + return encodedMCSCrt, ignitionWithMCSCert, + errors.Errorf("failed to find CA bundle; tried %s: %v", strings.Join(attempted, ", "), err) }As per coding guidelines, "Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/controller/controllers/bmh_agent_controller.go` around lines 1561 - 1579, The loop over sources using spokeClient.Get (building types.NamespacedName from src.namespace/src.name) currently returns only the last NotFound error; instead, when no source yields certData, build and return a composite NotFound error that lists all attempted sources (namespace/name and key) to aid debugging. Modify the post-loop error handling around configMap and k8serrors.IsNotFound: collect attempted source identifiers during iteration, and if err is NotFound for all, construct a single descriptive error (e.g., "configmap not found in any of: ns1/name1:key, ns2/name2:key") and return that together with encodedMCSCrt and ignitionWithMCSCert; preserve the existing fast-fail behavior for non-NotFound errors from spokeClient.Get.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@internal/controller/controllers/bmh_agent_controller.go`:
- Around line 1561-1579: The loop over sources using spokeClient.Get (building
types.NamespacedName from src.namespace/src.name) currently returns only the
last NotFound error; instead, when no source yields certData, build and return a
composite NotFound error that lists all attempted sources (namespace/name and
key) to aid debugging. Modify the post-loop error handling around configMap and
k8serrors.IsNotFound: collect attempted source identifiers during iteration, and
if err is NotFound for all, construct a single descriptive error (e.g.,
"configmap not found in any of: ns1/name1:key, ns2/name2:key") and return that
together with encodedMCSCrt and ignitionWithMCSCert; preserve the existing
fast-fail behavior for non-NotFound errors from spokeClient.Get.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 717f927e-39fa-4445-9382-c90a0be5ae93
📒 Files selected for processing (7)
.dockerignore.gitignoredata/default_release_images.jsondeploy/podman/configmap.ymlinternal/controller/controllers/bmh_agent_controller.gointernal/controller/controllers/bmh_agent_controller_test.goopenshift/template.yaml
✅ Files skipped from review due to trivial changes (2)
- deploy/podman/configmap.yml
- openshift/template.yaml
openshift-ci
Bot
added
size/M
eliorerz
left a comment
eliorerz
left a comment
There was a problem hiding this comment.
Great work!
I would consider also merge your commits into single commit with proper details about the solution.
openshift-ci
Bot
added
size/L
openshift-ci
Bot
added
size/M
|
@gamli75: Overrode contexts on behalf of gamli75: ci/prow/e2e-agent-compact-ipv4-iso-no-registry 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 kubernetes-sigs/prow repository. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: bluesort, eliorerz 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 |
|
/override ci/prow/edge-e2e-ai-operator-ztp |
|
@eliorerz: Overrode contexts on behalf of eliorerz: ci/prow/edge-e2e-ai-operator-ztp 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 kubernetes-sigs/prow repository. |
|
/override ci/prow/e2e-agent-compact-ipv4-iso-no-registry |
|
@bluesort: Overrode contexts on behalf of bluesort: ci/prow/e2e-agent-compact-ipv4-iso-no-registry 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 kubernetes-sigs/prow repository. |
|
/override ci/prow/e2e-agent-compact-ipv4 |
|
@bluesort: Overrode contexts on behalf of bluesort: ci/prow/e2e-agent-compact-ipv4 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 kubernetes-sigs/prow repository. |
|
/hold Revision b0d0273 was retested 3 times: holding |
openshift-ci
Bot
added
the
do-not-merge/hold
|
/test edge-subsystem-kubeapi-aws |
|
/test edge-subsystem-aws |
|
/override ci/prow/e2e-agent-compact-ipv4 |
|
@bluesort: Overrode contexts on behalf of bluesort: ci/prow/e2e-agent-compact-ipv4 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 kubernetes-sigs/prow repository. |
|
/hold cancel |
openshift-ci
Bot
removed
the
do-not-merge/hold
openshift-merge-bot
Bot
merged commit 6446967
into
openshift:master
|
/cherry-pick release-ocm-2.16 |
|
/cherry-pick release-ocm-2.15 |
|
@bluesort: #9999 failed to apply on top of branch "release-ocm-2.16": 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 kubernetes-sigs/prow repository. |
|
@bluesort: #9999 failed to apply on top of branch "release-ocm-2.15": 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 kubernetes-sigs/prow repository. |
5 participants
Changing Spec.CpuArchitecture after InfraEnv creation didn't regenerate boot artifacts, causing download links to point to the wrong architecture (e.g., links still pointed to x86_64 after changing to s390x). This fix makes Spec.CpuArchitecture immutable by adding webhook validation that rejects any attempts to modify the field after creation, preventing the inconsistent state.
List all the issues related to this PR
What environments does this code impact?
How was this code tested?
Checklist
docs, README, etc)Reviewers Checklist