OCPCLOUD-3359: Add component names, manifestSubstitutions, and observedGeneration to CAPI revisions#2786
OCPCLOUD-3359: Add component names, manifestSubstitutions, and observedGeneration to CAPI revisions#2786mdbooth wants to merge 2 commits intoopenshift:masterfrom
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. |
|
@mdbooth: This pull request references OCPCLOUD-3359 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 @mdbooth! Some important instructions when contributing to openshift/api: |
|
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:
📝 WalkthroughWalkthroughAdds manifest substitution support, component naming, and observed revision generation tracking to ClusterAPI types and CRD schemas. Introduces ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
openshift-ci
bot
added
the
size/XL
|
@mdbooth: This pull request references OCPCLOUD-3359 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. |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
/test verify-hypershift-integration |
|
PR-Agent: could not fine a component named |
|
/test verify-hypershift-integration |
1 similar comment
|
/test verify-hypershift-integration |
Generating tests for 'verify-hypershift-integration' ✏️️Work in progress ... |
|
/test verify-hypershift-integration |
|
PR-Agent: could not fine a component named |
openshift-ci
bot
removed
the
do-not-merge/work-in-progress
ⓘ You are approaching your monthly quota for Qodo. Upgrade your plan Review Summary by QodoAdd manifest substitutions, component names, and revision generation tracking
WalkthroughsDescription• Add manifestSubstitutions field to enable envsubst-style variable substitution in manifests • Add name field to components for improved readability and debugging • Add observedRevisionGeneration field to track ClusterAPI object generation observed by controller • Comprehensive validation rules for new fields with detailed error messages Diagramflowchart LR
A["ClusterAPIInstallerRevision"] -->|adds| B["manifestSubstitutions"]
C["ClusterAPIInstallerComponent"] -->|adds| D["name field"]
E["ClusterAPIStatus"] -->|adds| F["observedRevisionGeneration"]
B -->|contains| G["ClusterAPIInstallerRevisionManifestSubstitution"]
G -->|has| H["key and value"]
File Changes1. operator/v1alpha1/types_clusterapi.go
|
Code Review by Qodo
|
mdbooth
changed the title
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
| // name is the human-readable name of the component. It is only used to make | ||
| // api revisions easier to understand. It must consist of alphanumeric | ||
| // characters, or '-'. | ||
| // +optional | ||
| // +kubebuilder:validation:MinLength=1 | ||
| // +kubebuilder:validation:MaxLength=255 | ||
| // +kubebuilder:validation:XValidation:rule="self.matches('^[A-Za-z0-9-]+$')",message="name must consist of alphanumeric characters or '-'" | ||
| Name string `json:"name,omitempty"` |
There was a problem hiding this comment.
2. Component name length undocumented 📘 Rule violation ⚙ Maintainability
ClusterAPIInstallerComponent.Name adds MinLength/MaxLength validation but its comment does not document the length constraints or what happens when the optional field is omitted. This violates the requirements for documenting validation markers and omitted behavior.
Agent Prompt
## Issue description
The `ClusterAPIInstallerComponent.Name` field comment does not document all validation constraints (min/max length) and does not explicitly state behavior when the optional field is omitted.
## Issue Context
Compliance requires field comments to include all constraints expressed via kubebuilder markers and to describe omitted/default behavior for optional fields.
## Fix Focus Areas
- operator/v1alpha1/types_clusterapi.go[214-221]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| // ClusterAPIStatus describes the current state of the capi-operator. | ||
| // +kubebuilder:validation:XValidation:rule="self.revisions.exists(r, r.name == self.desiredRevision && self.revisions.all(s, s.revision <= r.revision))",message="desiredRevision must be the name of the revision with the highest revision number" | ||
| // +kubebuilder:validation:XValidation:rule="!has(self.currentRevision) || self.revisions.exists(r, r.name == self.currentRevision)",message="currentRevision must correspond to an entry in the revisions list" | ||
| // +kubebuilder:validation:XValidation:rule="!has(oldSelf.observedRevisionGeneration) || has(self.observedRevisionGeneration)",message="observedRevisionGeneration may not be unset once set" | ||
| type ClusterAPIStatus struct { | ||
| // currentRevision is the name of the most recently fully applied revision. | ||
| // It is written by the installer controller. If it is absent, it indicates |
There was a problem hiding this comment.
3. Unset-forbidden status field 🐞 Bug ☼ Reliability
The new status-level XValidation forbidding unsetting observedRevisionGeneration can make status updates fail during controller version skew, because multiple controllers write ClusterAPIStatus and an older writer will drop the new field on marshal. This can block unrelated status updates (e.g., installer controller updating currentRevision) once the revision controller has set observedRevisionGeneration.
Agent Prompt
### Issue description
A new XValidation rule forbids unsetting `status.observedRevisionGeneration` once set. Because `ClusterAPIStatus` is written by multiple controllers, and because older controller versions (or any writer using stale types) will drop unknown fields when marshalling status, this rule can cause status update failures and block progress.
### Issue Context
- `currentRevision` is written by the installer controller.
- `desiredRevision`/`revisions` are written by the revision controller.
Once the revision controller starts setting `observedRevisionGeneration`, any older installer controller still running (or any status writer not preserving the field) will have its `status` updates rejected.
### Fix Focus Areas
- operator/v1alpha1/types_clusterapi.go[78-123]
- operator/v1alpha1/zz_generated.crd-manifests/0000_30_cluster-api_01_clusterapis.crd.yaml[314-325]
- operator/v1alpha1/tests/clusterapis.operator.openshift.io/ClusterAPIMachineManagement.yaml[1074-1290]
### Suggested fix
Prefer backward-compatible validation:
- Drop the `may not be unset once set` XValidation, keeping only:
- `Minimum=1`
- `self >= oldSelf` (monotonic, when present)
This avoids hard failures during version skew; the revision controller can re-populate the field if an older writer clears it during rollout.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
There was a problem hiding this comment.
The API is v1alpha1 and the controller is in TPNU. I don't think we need to worry about this. I prefer the strictness.
|
@mdbooth: This pull request references OCPCLOUD-3359 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. |
![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 the current code and only fix it if needed.
Inline comments:
In `@operator/v1alpha1/types_clusterapi.go`:
- Around line 191-197: The manifestSubstitutions entry declares Value as a
pointer which prevents kubebuilder from marking it required; change the Value
field from a pointer (*string) to a non-pointer string type (string), keep the
existing kubebuilder comment markers (e.g., // +required and
MinLength/MaxLength), and remove the `omitempty` from the
`json:"value,omitempty"` tag (use `json:"value"`) so the generated CRD requires
the field while still allowing an empty string as a valid value; update the
struct field named `Value` in the same type that contains
`manifestSubstitutions` accordingly.
🪄 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), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 1ca73141-9640-4909-bd58-075370a70025
⛔ Files ignored due to path filters (3)
openapi/generated_openapi/zz_generated.openapi.gois excluded by!openapi/**operator/v1alpha1/zz_generated.crd-manifests/0000_30_cluster-api_01_clusterapis.crd.yamlis excluded by!**/zz_generated.crd-manifests/*operator/v1alpha1/zz_generated.featuregated-crd-manifests/clusterapis.operator.openshift.io/ClusterAPIMachineManagement.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**
📒 Files selected for processing (5)
operator/v1alpha1/tests/clusterapis.operator.openshift.io/ClusterAPIMachineManagement.yamloperator/v1alpha1/types_clusterapi.gooperator/v1alpha1/zz_generated.deepcopy.gooperator/v1alpha1/zz_generated.swagger_doc_generated.gopayload-manifests/crds/0000_30_cluster-api_01_clusterapis.crd.yaml
🚧 Files skipped from review as they are similar to previous changes (2)
- payload-manifests/crds/0000_30_cluster-api_01_clusterapis.crd.yaml
- operator/v1alpha1/zz_generated.swagger_doc_generated.go
There was a problem hiding this comment.
🧹 Nitpick comments (1)
operator/v1alpha1/tests/clusterapis.operator.openshift.io/ClusterAPIMachineManagement.yaml (1)
857-904: Add one negative test for omittedmanifestSubstitutions[].value.You already validate empty-string values; adding a case where
valueis entirely missing would lock in required-field behavior and prevent schema regressions.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@operator/v1alpha1/tests/clusterapis.operator.openshift.io/ClusterAPIMachineManagement.yaml` around lines 857 - 904, Add a new negative test case alongside "Should accept empty value in manifestSubstitutions" that attempts to apply a status revision whose revisions[].manifestSubstitutions contains an item with key: MY_VAR but no value field; name it something like "Should reject missing manifestSubstitutions value" and assert the update is rejected (i.e., the updated status should not be accepted / validation fails) to ensure manifestSubstitutions[].value is required. Locate the existing test block for manifestSubstitutions to copy the initial/updated/expected structure, change the updated to omit the value field under manifestSubstitutions and set the expected outcome to a rejection/validation failure for the status update.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In
`@operator/v1alpha1/tests/clusterapis.operator.openshift.io/ClusterAPIMachineManagement.yaml`:
- Around line 857-904: Add a new negative test case alongside "Should accept
empty value in manifestSubstitutions" that attempts to apply a status revision
whose revisions[].manifestSubstitutions contains an item with key: MY_VAR but no
value field; name it something like "Should reject missing manifestSubstitutions
value" and assert the update is rejected (i.e., the updated status should not be
accepted / validation fails) to ensure manifestSubstitutions[].value is
required. Locate the existing test block for manifestSubstitutions to copy the
initial/updated/expected structure, change the updated to omit the value field
under manifestSubstitutions and set the expected outcome to a
rejection/validation failure for the status update.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: a067e912-1eea-4a62-bed6-3e5f6a86a92b
⛔ Files ignored due to path filters (3)
openapi/generated_openapi/zz_generated.openapi.gois excluded by!openapi/**operator/v1alpha1/zz_generated.crd-manifests/0000_30_cluster-api_01_clusterapis.crd.yamlis excluded by!**/zz_generated.crd-manifests/*operator/v1alpha1/zz_generated.featuregated-crd-manifests/clusterapis.operator.openshift.io/ClusterAPIMachineManagement.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**
📒 Files selected for processing (5)
operator/v1alpha1/tests/clusterapis.operator.openshift.io/ClusterAPIMachineManagement.yamloperator/v1alpha1/types_clusterapi.gooperator/v1alpha1/zz_generated.deepcopy.gooperator/v1alpha1/zz_generated.swagger_doc_generated.gopayload-manifests/crds/0000_30_cluster-api_01_clusterapis.crd.yaml
🚧 Files skipped from review as they are similar to previous changes (3)
- payload-manifests/crds/0000_30_cluster-api_01_clusterapis.crd.yaml
- operator/v1alpha1/zz_generated.swagger_doc_generated.go
- operator/v1alpha1/zz_generated.deepcopy.go
openshift-ci
bot
added
size/XXL
There was a problem hiding this comment.
🧹 Nitpick comments (2)
operator/v1alpha1/types_clusterapi.go (1)
156-199: Enforce unique substitution keys.The schema currently permits the same envsubst variable name more than once, which makes the effective rendered value depend on downstream precedence. Adding a list-level uniqueness rule on
keywould keep revision content unambiguous.Suggested schema tweak
// +optional // +listType=atomic // +kubebuilder:validation:MinItems=1 // +kubebuilder:validation:MaxItems=32 + // +kubebuilder:validation:XValidation:rule="self.all(x, self.exists_one(y, x.key == y.key))",message="each manifest substitution must have a unique key" ManifestSubstitutions []ClusterAPIInstallerRevisionManifestSubstitution `json:"manifestSubstitutions,omitempty"`🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@operator/v1alpha1/types_clusterapi.go` around lines 156 - 199, The ManifestSubstitutions list allows duplicate keys; change its list metadata to enforce uniqueness by replacing the current +listType=atomic with +listType=map and add a list map key annotation (+listMapKey=key or the equivalent x-kubernetes list-map-keys) on the ManifestSubstitutions field so the CRD treats items as a map keyed by ClusterAPIInstallerRevisionManifestSubstitution.Key (the Key field already has the necessary validation), ensuring no two items can share the same key.operator/v1alpha1/tests/clusterapis.operator.openshift.io/ClusterAPIMachineManagement.yaml (1)
655-904: Good coverage here—please add one negative case for omittedmanifestSubstitutions[].value.These cases prove empty string is allowed, but they do not lock in the equally important invariant that
valuemust still be present. Given the*string++requiredcombination, a regression test here would be valuable.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@operator/v1alpha1/tests/clusterapis.operator.openshift.io/ClusterAPIMachineManagement.yaml` around lines 655 - 904, Add a negative test case under the existing manifestSubstitutions tests that submits a revision with a manifestSubstitutions entry that includes a key but omits the value field (e.g., manifestSubstitutions: - key: MISSING) and assert it fails by setting expectedStatusError to the required-field error for the missing value (use the same pattern as other negative tests and reference manifestSubstitutions, value, and expectedStatusError so the test ensures the value field is required).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In
`@operator/v1alpha1/tests/clusterapis.operator.openshift.io/ClusterAPIMachineManagement.yaml`:
- Around line 655-904: Add a negative test case under the existing
manifestSubstitutions tests that submits a revision with a manifestSubstitutions
entry that includes a key but omits the value field (e.g.,
manifestSubstitutions: - key: MISSING) and assert it fails by setting
expectedStatusError to the required-field error for the missing value (use the
same pattern as other negative tests and reference manifestSubstitutions, value,
and expectedStatusError so the test ensures the value field is required).
In `@operator/v1alpha1/types_clusterapi.go`:
- Around line 156-199: The ManifestSubstitutions list allows duplicate keys;
change its list metadata to enforce uniqueness by replacing the current
+listType=atomic with +listType=map and add a list map key annotation
(+listMapKey=key or the equivalent x-kubernetes list-map-keys) on the
ManifestSubstitutions field so the CRD treats items as a map keyed by
ClusterAPIInstallerRevisionManifestSubstitution.Key (the Key field already has
the necessary validation), ensuring no two items can share the same key.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 04e9d3fc-fa9a-4746-8729-0ba98e60a4e3
⛔ Files ignored due to path filters (4)
openapi/generated_openapi/zz_generated.openapi.gois excluded by!openapi/**openapi/openapi.jsonis excluded by!openapi/**operator/v1alpha1/zz_generated.crd-manifests/0000_30_cluster-api_01_clusterapis.crd.yamlis excluded by!**/zz_generated.crd-manifests/*operator/v1alpha1/zz_generated.featuregated-crd-manifests/clusterapis.operator.openshift.io/ClusterAPIMachineManagement.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**
📒 Files selected for processing (5)
operator/v1alpha1/tests/clusterapis.operator.openshift.io/ClusterAPIMachineManagement.yamloperator/v1alpha1/types_clusterapi.gooperator/v1alpha1/zz_generated.deepcopy.gooperator/v1alpha1/zz_generated.swagger_doc_generated.gopayload-manifests/crds/0000_30_cluster-api_01_clusterapis.crd.yaml
🚧 Files skipped from review as they are similar to previous changes (3)
- operator/v1alpha1/zz_generated.swagger_doc_generated.go
- payload-manifests/crds/0000_30_cluster-api_01_clusterapis.crd.yaml
- operator/v1alpha1/zz_generated.deepcopy.go
everettraven
left a comment
everettraven
left a comment
There was a problem hiding this comment.
Just a few comments. Aside from these, this looks pretty good to me.
|
/pipeline auto |
|
Pipeline controller notification The |
|
Scheduling tests matching the |
|
Tests from second stage were triggered manually. Pipeline can be controlled only manually, until HEAD changes. Use command to trigger second stage. |
|
/retest |
|
Tests from second stage were triggered manually. Pipeline can be controlled only manually, until HEAD changes. Use command to trigger second stage. |
|
/pipeline required |
|
Scheduling tests matching the |
|
Tests from second stage were triggered manually. Pipeline can be controlled only manually, until HEAD changes. Use command to trigger second stage. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: everettraven 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 |
openshift-ci
bot
added
the
approved
|
@mdbooth: The following tests 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. |
|
Tests from second stage were triggered manually. Pipeline can be controlled only manually, until HEAD changes. Use command to trigger second stage. |
|
/retest |
5 participants
component.name: This is a cosmetic change, but makes it easier for a human (e.g. dev/support) to understand what each component in a revision refers to.
component.manifestSubstitutions: These are values passed to 'envsubst' when rendering manifests. The initial use is to pass through cluster-wide TLS configuration in a controlled manner.
status.observedRevisionGeneration: Required by an API consumer to know that the revision controller has observed spec changes. Specifically, if a user adds an entry to spec.unmanagedCustomResourceDefinitions they need to know:
They are currently only able to observe the latter, making it impossible to use the API robustly.