OCPBUGS-80970: MCS fallback to latest v3#5816
OCPBUGS-80970: MCS fallback to latest v3#5816openshift-merge-bot[bot] merged 1 commit intoopenshift:mainfrom
Conversation
When an Ignition client requests a v3 minor version higher than the one MCS supports (e.g. 3.6.0), return the latest supported v3 version (currently 3.5.0) instead of erroring. This is safe because Ignition v3 spec versions are backward-compatible within the same major. Requests for a different major (e.g. v4) still error as before. Signed-off-by: Pablo Rodriguez Nava <git@amail.pablintino.eu>
|
@pablintino: This pull request references MCO-2182 which is a valid jira issue. 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. |
WalkthroughThe pull request exports three previously unexported error sentinel variables from the ignition converter module, updates all internal references to use these exported constants, and adds fallback logic in the API layer to return the maximum supported Ignition version when unsupported minor versions are requested on a supported major version. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
/test e2e-agent-compact-ipv4 |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
pkg/server/api.go (1)
295-302: Avoid a second source of truth for the max supported v3.
pkg/controller/common/ignition_converter.goalready owns the supported-version set. Hard-codingign3types.MaxVersionhere can drift on the next v3 bump and make negotiation return an older version than the converter actually supports. Consider deriving the fallback target from the converter instead.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/server/api.go` around lines 295 - 302, The code currently returns ign3types.MaxVersion as a fallback which duplicates the supported-version truth; instead, derive the fallback from the ignition converter owned in pkg/controller/common/ignition_converter.go—when detecting ctrlcommon.ErrIgnitionConverterUnknownVersion and newerMinorRequested, call the converter's exported API to obtain its max supported v3 version (rather than using ign3types.MaxVersion) and return that value so negotiation always reflects the converter's actual supported set.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@pkg/server/api.go`:
- Around line 295-302: The code currently returns ign3types.MaxVersion as a
fallback which duplicates the supported-version truth; instead, derive the
fallback from the ignition converter owned in
pkg/controller/common/ignition_converter.go—when detecting
ctrlcommon.ErrIgnitionConverterUnknownVersion and newerMinorRequested, call the
converter's exported API to obtain its max supported v3 version (rather than
using ign3types.MaxVersion) and return that value so negotiation always reflects
the converter's actual supported set.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 6f45236c-b330-4a44-abe0-0311e35f8278
📒 Files selected for processing (4)
pkg/controller/common/helpers_test.gopkg/controller/common/ignition_converter.gopkg/server/api.gopkg/server/api_test.go
| // Check if the error is because the client is asking for a minor version we still don't support | ||
| // We can assume it's safe to return our latest minor if the requested version is of the same | ||
| // major but higher minor version. | ||
| newerMinorRequested := ign3types.MaxVersion.Major == reqVersion.Major && ign3types.MaxVersion.Minor < reqVersion.Minor |
There was a problem hiding this comment.
Note for the reviewer: I'll probably change the behavior of GetSupportedMinorVersion to handle this and return the lastest supported minor for the given major version. Any version less than 2.2 will still be rejected here.
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: bfournie, pablintino 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 |
|
/verified by @bfournie |
|
@pablintino: This PR has been marked as verified by 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. |
|
/retitle OCPBUGS-80970: MCS fallback to latest v3 |
|
@pablintino: This pull request references Jira Issue OCPBUGS-80970, which is valid. 3 validation(s) were run on this bug
The bug has been updated to refer to the pull request using the external bug tracker. 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. |
|
/jira refresh |
|
@pablintino: This pull request references Jira Issue OCPBUGS-80970, which is valid. 3 validation(s) were run on this bug
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 e2e-gcp-op-ocl-part1 |
ac0f92c
into
openshift:main
|
@pablintino: Jira Issue OCPBUGS-80970: Some pull requests linked via external trackers have merged: The following pull request, linked via external tracker, has not merged: All associated pull requests must be merged or unlinked from the Jira bug in order for it to move to the next state. Once unlinked, request a bug refresh with Jira Issue OCPBUGS-80970 has not been moved to the MODIFIED state. This PR is marked as verified. If the remaining PRs listed above are marked as verified before merging, the issue will automatically be moved to VERIFIED after all of the changes from the PRs are available in an accepted nightly payload. 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. |
|
@pablintino: The following test failed, say
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. |
|
/jira refresh |
|
Fix included in accepted release 4.22.0-0.nightly-2026-03-29-055437 |
|
/cherry-pick release-4.21 |
|
@pablintino: new pull request created: #5883 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. |
- What I did
When an Ignition client requests a v3 minor version higher than the one MCS supports (e.g. 3.6.0), return the latest supported v3 version (currently 3.5.0) instead of erroring. This is safe because Ignition v3 spec versions are backward-compatible within the same major. Requests for a different major (e.g. v4) still error as before.
- How to verify it
E2Es should suffice
- Description for the changelog
MCS now returns the latest supported Ignition v3 config instead of erroring when a client requests an unsupported v3.x minor version.