OCPCLOUD-3214: Add ClusterAPI config #2564

mdbooth · 2025-11-04T16:39:14Z

No description provided.

openshift-ci-robot · 2025-11-04T16:39:16Z

Pipeline controller notification
This repository is configured to use the pipeline controller. Second-stage tests will be triggered either automatically or after lgtm label is added, depending on the repository configuration. The pipeline controller will automatically detect which contexts are required and will utilize /test Prow commands to trigger the second stage.

For optional jobs, comment /test ? to see a list of all defined jobs. Review these jobs and use /test <job> to manually trigger optional jobs most likely to be impacted by the proposed changes.

openshift-ci-robot · 2025-11-04T16:39:19Z

@mdbooth: This pull request references OCPCLOUD-3214 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.21.0" version, but no target version was set.

In 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 · 2025-11-04T16:39:29Z

Hello @mdbooth! Some important instructions when contributing to openshift/api:
API design plays an important part in the user experience of OpenShift and as such API PRs are subject to a high level of scrutiny to ensure they follow our best practices. If you haven't already done so, please review the OpenShift API Conventions and ensure that your proposed changes are compliant. Following these conventions will help expedite the api review process for your PR.

openshift-ci · 2025-11-04T16:41:33Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign everettraven for approval. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

chrischdi · 2025-11-05T13:34:25Z

operator/v1alpha1/types_clusterapi.go

+	// The maximum number of unmanagedCustomResourceDefinitions is 128.
+	//
+	// +optional
+	// +listType=set


Q: is this resource created by each actor or will there be one instance with co-ownership from different actors?

Example 1:

1 ClusterAPI object created by HyperShift

1 ClusterAPI object created by ACM

Example 2:

1 ClusterAPI object, some entries in .spec.unmanagedCustomResourceDefinitions are from HyperShift, others are from ACM.

You can't have duplicates in listType=set, so multiple actors would own the same entry if that happened.

Each actor simply needs to submit an SSA transaction with their own requirements without reference to any other actors.

chrischdi · 2025-11-05T13:34:52Z

operator/v1alpha1/types_clusterapi.go

+}
+
+// ClusterAPISpec defines the desired configuration of the capi-operator.
+// +kubebuilder:validation:XValidation:rule="!has(oldSelf.unmanagedCustomResourceDefinitions) || has(self.unmanagedCustomResourceDefinitions)",message="unmanagedCustomResourceDefinitions cannot be unset once set"


We validate removal to not happen, do we also validate deletion of the object to not happen?

Interesting 🤔

I'm not convinced we can validate this here. I suspect we don't want to validate this at all, though: if the user is deleting ClusterOperators then they're already having a bad day. They've chosen this course of action and they're going to do it regardless. Whatever barriers we raise are just another in a succession of irritations that will be circumvented: disabling CVO, deleting VAPs, webhooks, etc. We can discourage the use of footguns, but ultimately we can't uninvent them.

chrischdi · 2025-11-05T13:54:34Z

operator/v1alpha1/tests/clusterapis.operator.openshift.io/ClusterAPIMachineManagement.yaml

+    # TODO: Cannot test the XValidation rule "unmanagedCustomResourceDefinitions cannot be unset once set"
+    # because MinProperties=1 on ClusterAPISpec prevents creating spec: {} in the first place.
+    # The validation is still enforced at the API level via the XValidation rule.


Shouldn't this work? (I tried it)

Suggested change

# TODO: Cannot test the XValidation rule "unmanagedCustomResourceDefinitions cannot be unset once set"

# because MinProperties=1 on ClusterAPISpec prevents creating spec: {} in the first place.

# The validation is still enforced at the API level via the XValidation rule.

- name: Should not allow unsetting unmanagedCustomResourceDefinitions

initial: |

apiVersion: operator.openshift.io/v1alpha1

kind: ClusterAPI

metadata:

name: cluster

spec:

unmanagedCustomResourceDefinitions:

- clusters.cluster.x-k8s.io

- machines.cluster.x-k8s.io

updated: |

apiVersion: operator.openshift.io/v1alpha1

kind: ClusterAPI

metadata:

name: cluster

spec:

unmanagedCustomResourceDefinitions: null

expectedError: "unmanagedCustomResourceDefinitions cannot be unset once set"

I don't think set to null is the same as unset.

However, I decided to go a different direction with this: I made spec optional. I think this should be the default for a ClusterOperator, tbh.

openshift-ci · 2025-11-10T17:59:19Z

@mdbooth: all tests passed!

Full PR test history. Your PR dashboard.

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. I understand the commands that are listed here.

yuqi-zhang

General structure looks fine, some questions inline

Also, based on Claude suggestion, all optional fields (spec, status, UnmanagedCustomResourceDefinitions, TargetConfigMaps, ActiveConfigMaps) should have a godoc line explaining what happens if the value is empty. (example: When omitted, the capi-operator will use default configuration with all CRDs managed by the operator. in the spec, assuming we keep spec optional)

yuqi-zhang · 2025-11-11T23:30:54Z

operator/v1alpha1/types_clusterapi.go

+//
+// Compatibility level 4: No compatibility is provided, the API can change at any point for any reason. These capabilities should not be used by applications needing long term support.
+// +openshift:compatibility-gen:level=4
+// +kubebuilder:validation:XValidation:rule="!has(oldSelf.spec) || !has(oldSelf.spec.unmanagedCustomResourceDefinitions) || has(self.spec.unmanagedCustomResourceDefinitions)",message="unmanagedCustomResourceDefinitions cannot be unset once set"


I think for validations, it's best to have it explicitly in the godoc as well if possible (so in this case, maybe just unmanagedCustomResourceDefinitions cannot be unset once set as a godoc line is fine?).

Side note, I thought that if we don't edit unmanagedCustomResourceDefinitions but instead delete the whole spec field after setting it at some point, self.spec.unmanagedCustomResourceDefinitions would maybe not evaluate correctly, but it seems that CEL can handle that null exception (and you have a test case above covering it)

yuqi-zhang · 2025-11-11T23:36:05Z

operator/v1alpha1/types_clusterapi.go

+// Compatibility level 4: No compatibility is provided, the API can change at any point for any reason. These capabilities should not be used by applications needing long term support.
+// +openshift:compatibility-gen:level=4
+// +kubebuilder:validation:XValidation:rule="!has(oldSelf.spec) || !has(oldSelf.spec.unmanagedCustomResourceDefinitions) || has(self.spec.unmanagedCustomResourceDefinitions)",message="unmanagedCustomResourceDefinitions cannot be unset once set"
+type ClusterAPI struct {


So, like the other operator types, I assume this will be a singleton. Interestingly enough the only one to enforce it is the olm operator: https://github.com/openshift/api/blob/master/operator/v1alpha1/types_olm.go#L25

I think generally speaking it's a good pattern to have, so probably worth adding that? Unless I'm misunderstanding how this can be configured.

yuqi-zhang · 2025-11-11T23:50:16Z

operator/v1alpha1/types_clusterapi.go

+
+	// spec is the specification of the desired behavior of the capi-operator.
+	// +optional
+	Spec *ClusterAPISpec `json:"spec,omitempty"`


Based on updated guidance, I think now we only pointers if there's a difference between the unset and empty struct, otherwise having it same as the status field below (not a pointer, omitzero) is the best option.

In this case, since "" and empty should both mean "no CRDs are unmanaged", I think this falls under that umbrella?

The other way we could maybe do this (and I don't know what is "better" conventions-wise) is to instead make this required, keep unmanagedCustomResourceDefinitions optional, move the // +kubebuilder:validation:XValidation:rule="!has(oldSelf.spec) || !has(oldSelf.spec.unmanagedCustomResourceDefinitions) || has(self.spec.unmanagedCustomResourceDefinitions)",message="unmanagedCustomResourceDefinitions cannot be unset once set" validation here to the spec, and remove the need for nil checking generally. This might be more future proof in case we want to add more spec fields in the future.

yuqi-zhang · 2025-11-11T23:56:58Z

operator/v1alpha1/types_clusterapi.go

+
+// ClusterAPISpec defines the desired configuration of the capi-operator.
+type ClusterAPISpec struct {
+	// unmanagedCustomResourceDefinitions is a list of ClusterResourceDefinition (CRD)


Presumably, this is only a set of CRDs that CAPI operator would use, so e.g. if I set "MachineConfigs" here, nothing would happen since CAPI doesn't manage MachineConfigs, nor can it tell CVO/MCO to not deploy/use some other api version.

Would there be a way for the user to discover what CRDs is able to be unmanaged here? Is that a big list or something we can add to the godoc/validator?

Reading this and openshift/enhancements#1845 I had initially thought that this is a generic list you can apply to any CRD but that shouldn't be the case right?

yuqi-zhang · 2025-11-12T00:03:02Z

operator/v1alpha1/types_clusterapi.go

+}
+
+// ClusterAPIStatus describes the current state of the capi-operator.
+// +kubebuilder:validation:MinProperties=1


Question: I assume only CAPI-operator would be updating the status of this object. Does the validation here help prevent a specific scenario?

(should be fine to have, just curious why this validation was added)

yuqi-zhang · 2025-11-12T00:07:52Z

operator/v1alpha1/types_clusterapi.go

+// ClusterAPIStatus describes the current state of the capi-operator.
+// +kubebuilder:validation:MinProperties=1
+type ClusterAPIStatus struct {
+	// targetConfigMaps is a list of ConfigMap names that the staging controller


I'm probably missing some context but I'm not 100% sure what these two configmap status field entails. I assume these are here for an admin to see? Are these reflecting the status of some other action the admin took elsewhere? Or something that has to do with the CRDs they just defined in the spec? (Just from an ELI5 perspective the status fields are confusing, if there's some other way to add some more context I think that would be helpful)

openshift-ci-robot added the jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. label Nov 4, 2025

openshift-ci bot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Nov 4, 2025

mdbooth marked this pull request as draft November 4, 2025 16:40

openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Nov 4, 2025

openshift-ci bot requested review from JoelSpeed and everettraven November 4, 2025 16:41

mdbooth force-pushed the ccapio-staged branch from 1eb67fc to 5e7a826 Compare November 4, 2025 16:43

mdbooth marked this pull request as ready for review November 4, 2025 16:44

openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Nov 4, 2025

chrischdi reviewed Nov 5, 2025

View reviewed changes

mdbooth force-pushed the ccapio-staged branch from 5e7a826 to c496532 Compare November 10, 2025 16:02

Add ClusterAPI config

465771a

mdbooth force-pushed the ccapio-staged branch from c496532 to 465771a Compare November 10, 2025 17:21

yuqi-zhang reviewed Nov 12, 2025

View reviewed changes

-    # TODO: Cannot test the XValidation rule "unmanagedCustomResourceDefinitions cannot be unset once set"
-    # because MinProperties=1 on ClusterAPISpec prevents creating spec: {} in the first place.
-    # The validation is still enforced at the API level via the XValidation rule.
+    - name: Should not allow unsetting unmanagedCustomResourceDefinitions
+      initial: |
+        apiVersion: operator.openshift.io/v1alpha1
+        kind: ClusterAPI
+        metadata:
+          name: cluster
+        spec:
+          unmanagedCustomResourceDefinitions:
+            - clusters.cluster.x-k8s.io
+            - machines.cluster.x-k8s.io
+      updated: |
+        apiVersion: operator.openshift.io/v1alpha1
+        kind: ClusterAPI
+        metadata:
+          name: cluster
+        spec:
+          unmanagedCustomResourceDefinitions: null
+      expectedError: "unmanagedCustomResourceDefinitions cannot be unset once set"

OCPCLOUD-3214: Add ClusterAPI config #2564

Are you sure you want to change the base?

OCPCLOUD-3214: Add ClusterAPI config #2564

Uh oh!

Conversation

mdbooth commented Nov 4, 2025

Uh oh!

openshift-ci-robot commented Nov 4, 2025

Uh oh!

openshift-ci-robot commented Nov 4, 2025 • edited by openshift-ci bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

openshift-ci bot commented Nov 4, 2025

Uh oh!

openshift-ci bot commented Nov 4, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

openshift-ci bot commented Nov 10, 2025

Uh oh!

yuqi-zhang left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

openshift-ci-robot commented Nov 4, 2025 •

edited by openshift-ci bot

Loading