MCO-2142: OSImageStream.spec.defaultStream validation#2736
MCO-2142: OSImageStream.spec.defaultStream validation#2736pablintino wants to merge 1 commit intoopenshift:masterfrom
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
openshift-ci-robot
added
the
jira/valid-reference
|
@pablintino: This pull request references MCO-2142 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 "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. |
|
Hello @pablintino! Some important instructions when contributing to openshift/api: |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (3)
📒 Files selected for processing (4)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds cross-field validation for OSImageStream so spec.defaultStream must reference a name present in status.availableStreams when status is populated. Implements Kubebuilder XValidation annotations in the Go type and the CRD x-kubernetes-validations rule, updates Swagger docs for defaultStream, and expands tests with numerous onUpdate cases covering positive and negative scenarios: cross-field consistency, stream-name RFC 1123 and length checks, image URL/digest formatting, and required-field validations. 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.5.0)Error: build linters: unable to load custom analyzer "kubeapilinter": tools/_output/bin/kube-api-linter.so, plugin: not implemented Comment |
openshift-ci
bot
added
the
size/L
Review Summary by QodoAdd OSImageStream.spec.defaultStream validation rule WalkthroughsDescription• Add CEL validation rule to OSImageStream.spec.defaultStream field - Ensures spec.defaultStream references existing stream in status.availableStreams - Allows validation to pass when status is not yet populated (bootstrap scenario) • Add comprehensive test cases covering validation scenarios - Valid stream updates, invalid stream references, and stream removal cases • Update generated CRD manifests with new validation rule Diagramflowchart LR
A["OSImageStream Type Definition"] -->|adds CEL validation| B["spec.defaultStream Validation"]
B -->|checks if exists in| C["status.availableStreams"]
B -->|allows missing| D["Bootstrap Scenario"]
E["Test Cases"] -->|validates| B
F["CRD Manifests"] -->|generated from| A
File Changes1. machineconfiguration/v1alpha1/types_osimagestream.go
|
Code Review by Qodo
1. spec.defaultStream rule undocumented
|
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: 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 |
| // +openshift:enable:FeatureGate=OSStreams | ||
| // +kubebuilder:metadata:labels=openshift.io/operator-managed= | ||
| // +kubebuilder:validation:XValidation:rule="self.metadata.name == 'cluster'",message="osimagestream is a singleton, .metadata.name must be 'cluster'" | ||
| // +kubebuilder:validation:XValidation:rule="!has(self.spec) || !has(self.spec.defaultStream) || !has(self.status) || self.spec.defaultStream in self.status.availableStreams.map(s, s.name)",message="spec.defaultStream must reference an existing stream name from status.availableStreams" |
There was a problem hiding this comment.
Note for reviewers: The .status.availableStreams is only refreshed during an update (and at install time of course). It may happen that the stream the user has selected is no longer available. The idea we have is to signal the user in a previous release that a stream is EoL and force them to remove all usages of that stream before allowing them to update with upgradable=False. That way, the source version is warrantied to point to streams available in the target version of the update.
There was a problem hiding this comment.
We discussed this a bit as a team. Essentially this saying that this validation would be different payload-to-payload (so breaking changes can theoretically occur as the validation "tightens", but in practice we'll always make sure that the user is on a default stream that is available on the payload it would upgrade to (and we'd never remove a stream in a z-stream, only y-stream). Therefore this would only ever check for user-action changes to this field.
With that in mind, we also debated whether this would be better just as a controller level validation instead, since it is a bit of an odd case of availableStreams that change from version to version.
(also as a note, I think !has(self.spec) is redundant since spec is a required field.)
There was a problem hiding this comment.
Updated to remove the not needed spec check.
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
| // +openshift:enable:FeatureGate=OSStreams | ||
| // +kubebuilder:metadata:labels=openshift.io/operator-managed= | ||
| // +kubebuilder:validation:XValidation:rule="self.metadata.name == 'cluster'",message="osimagestream is a singleton, .metadata.name must be 'cluster'" | ||
| // +kubebuilder:validation:XValidation:rule="!has(self.spec) || !has(self.spec.defaultStream) || !has(self.status) || self.spec.defaultStream in self.status.availableStreams.map(s, s.name)",message="spec.defaultStream must reference an existing stream name from status.availableStreams" |
There was a problem hiding this comment.
1. spec.defaultstream rule undocumented 📘 Rule violation ✓ Correctness
A new XValidation enforces that spec.defaultStream must match a name in status.availableStreams when status is present, but the spec.defaultStream field comment does not document this constraint (or the bootstrapping exception when status is absent). This can surprise API consumers because updates may be rejected by validation without the behavior being described in-field documentation.
Agent Prompt
## Issue description
A new type-level XValidation enforces that `spec.defaultStream` must be one of `status.availableStreams[].name` when `status` exists, but the `spec.defaultStream` field comment does not document this constraint (nor the bootstrapping exception when `status` is not present).
## Issue Context
Compliance requires that field comments fully describe all validation/optionality constraints that apply to the field, including constraints enforced via XValidation.
## Fix Focus Areas
- machineconfiguration/v1alpha1/types_osimagestream.go[26-26]
- machineconfiguration/v1alpha1/types_osimagestream.go[76-96]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
|
@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. |
Add a validation to the OSImageStream resource to ensure that the .spec.defaultStream fiel, when set, points to a value given in the .status.availableStreams. If the resourece has no status yet the validation should just pass and the MCO operator should take care of handling such spec. This only happens at cluster bootstrapping. Signed-off-by: Pablo Rodriguez Nava <git@amail.pablintino.com>
| // +openshift:enable:FeatureGate=OSStreams | ||
| // +kubebuilder:metadata:labels=openshift.io/operator-managed= | ||
| // +kubebuilder:validation:XValidation:rule="self.metadata.name == 'cluster'",message="osimagestream is a singleton, .metadata.name must be 'cluster'" | ||
| // +kubebuilder:validation:XValidation:rule="!has(self.spec.defaultStream) || !has(self.status) || self.spec.defaultStream in self.status.availableStreams.map(s, s.name)",message="spec.defaultStream must reference an existing stream name from status.availableStreams" |
There was a problem hiding this comment.
You need to test this, integration ratcheting tests in particular.
You'll also want to make sure that this only rejects spec writes.
Imagine a case where the user forces through any upgrade checks, currently a status update could fail if their spec value is no longer present in the spec list after update.
Adjust this to accept any time self.spec == oldSelf.spec would be a useful guard here
4 participants
Add a validation to the OSImageStream resource to ensure that the .spec.defaultStream fiel, when set, points to a value given in the .status.availableStreams.
If the resourece has no status yet the validation should just pass and the MCO operator should take care of handling such spec. This only happens at cluster bootstrapping.