config/v1/types_cluster_version: Add Architecture to DesiredUpdate #1339

jottofar · 2022-11-01T20:59:26Z

field of ClusterVersion spec to request target architecture per OTA-817.

openshift-ci · 2022-11-01T20:59:35Z

Hello @jottofar! 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.

For merging purposes, this repository follows the no-Feature-Freeze process which means that in addition to the standard lgtm and approved labels this repository requires either:

bugzilla/valid-bug - applied if your PR references a valid bugzilla bug

OR

qe-approved, docs-approved, and px-approved - these labels can be applied by anyone in the openshift org via the /label <labelname> command.

Who should apply these qe/docs/px labels?

For a no-Feature-Freeze team who is merging a feature before code freeze, they need to get those labels applied to their api repo PR by the appropriate teams (i.e. qe, docs, px)
For a Feature Freeze (traditional) team who is merging a feature before FF, they can self-apply the labels (via /label commands), they are basically irrelevant for those teams
For a Feature Freeze team who is merging a feature after FF, the PR should be rejected barring an exception

config/v1/types_cluster_version.go

jottofar · 2022-11-08T13:19:41Z

/assign @deads2k

config/v1/types_cluster_version.go

LalatenduMohanty · 2022-11-10T15:45:20Z

/test verify

config/v1/types_cluster_version.go

config/v1/stable.clusterversion.testsuite.yaml

config/v1/types_cluster_version.go

JoelSpeed

Couple of questions

Also, I broke the verify job slightly so I think your CRD is out of date, verify job is fixed now

/test verify

JoelSpeed · 2022-12-05T10:54:17Z

config/v1/stable.clusterversion.testsuite.yaml

+      spec:
+        clusterID: foo
+        desiredUpdate:
+          architecture: Multi


Please make sure to add at least one test that sets architecture: "" explicitly please

Fixed in c9e0d99

JoelSpeed · 2022-12-05T10:57:33Z

config/v1/types_cluster_version.go

 type URL string

 // Update represents an administrator update request.
+// +kubebuilder:validation:XValidation:rule="has(self.architecture) && has(self.image) ? (self.architecture != '' || self.image != '') : true",message="cannot set both Architecture and Image"


Is this correct? If we have both architecture and image, you are checking if either of them is non empty. Do you not instead want to check if either of them is empty? You are trying to require that at least one of them is empty in this case right?

Yes, that's correct. Thats why we have message="cannot set both Architecture and Image"

Yes, but the validation itself is wrong, and you can see this if you look at the integration test failures which are reporting expected an error, but got nil.

Your current check passes if either value is non-empty. You want it to only pass when one value is empty.

I created 3165600 which demonstrates the diff you need, can you cherry pick that into this branch please

Suggested change

// +kubebuilder:validation:XValidation:rule="has(self.architecture) && has(self.image) ? (self.architecture != '' || self.image != '') : true",message="cannot set both Architecture and Image"

// +kubebuilder:validation:XValidation:rule="has(self.architecture) && has(self.image) ? (self.architecture == '' || self.image == '') : true",message="cannot set both Architecture and Image"

JoelSpeed · 2022-12-05T10:59:55Z

config/v1/stable.clusterversion.testsuite.yaml

+        desiredUpdate:
+          architecture: Multi
+    expectedError: "Version must be set if Architecture is set"
+  onUpdate:


Do any of the validations rely on a previously persisted state? If they don't, then we don't need separate update tests and the create validation should cover both cases

Ok, I am going to remove this test.

I had meant the entire set of onUpdate, none of them seem necessary since they all duplicate previous test cases IIUC. But I see now why you've got two different ones, it's the transitions from image to both and from architecture to both, makes sense, we should add an onCreate version though

Both onUpdate tests are covered by the following onCreate test, so unless I am missing something we do not need to add any new onCreate tests.

- name: Should not allow image and architecture to be set initial: | apiVersion: config.openshift.io/v1 kind: ClusterVersion spec: clusterID: foo desiredUpdate: architecture: Multi version: 4.11.1 image: bar

We have onCreate Should not allow image and architecture to be set which checks that both cannot be set so I'm not sure what can be added to onCreate to replicate the two onUpdates since each of them can start with only one of the values but the onCreate cannot (AFAIK).

And I originally added the onUpdate tests to cover a user doing an oc patch for example.

Also I can work on this even after the PR is merged. So lets not block the PR to get merged because of this.

LalatenduMohanty · 2022-12-05T19:38:38Z

/test integration

JoelSpeed · 2022-12-06T10:11:29Z

config/v1/stable.clusterversion.testsuite.yaml

+        desiredUpdate:
+          architecture: Multi
+    expectedError: "Version must be set if Architecture is set"
+  onUpdate:


I had meant the entire set of onUpdate, none of them seem necessary since they all duplicate previous test cases IIUC. But I see now why you've got two different ones, it's the transitions from image to both and from architecture to both, makes sense, we should add an onCreate version though

JoelSpeed · 2022-12-06T10:47:58Z

config/v1/types_cluster_version.go

 type URL string

 // Update represents an administrator update request.
+// +kubebuilder:validation:XValidation:rule="has(self.architecture) && has(self.image) ? (self.architecture != '' || self.image != '') : true",message="cannot set both Architecture and Image"


Yes, but the validation itself is wrong, and you can see this if you look at the integration test failures which are reporting expected an error, but got nil.

Your current check passes if either value is non-empty. You want it to only pass when one value is empty.

I created 3165600 which demonstrates the diff you need, can you cherry pick that into this branch please

Suggested change

// +kubebuilder:validation:XValidation:rule="has(self.architecture) && has(self.image) ? (self.architecture != '' || self.image != '') : true",message="cannot set both Architecture and Image"

// +kubebuilder:validation:XValidation:rule="has(self.architecture) && has(self.image) ? (self.architecture == '' || self.image == '') : true",message="cannot set both Architecture and Image"

JoelSpeed · 2022-12-06T10:48:27Z

config/v1/stable.clusterversion.testsuite.yaml

+      spec:
+        clusterID: foo
+        desiredUpdate:
+          architecture:


I would prefer to see this explicitly with the empty string

Suggested change

architecture:

architecture: ""

JoelSpeed · 2022-12-06T14:52:46Z

config/v1/types_cluster_version.go

+	// architecture. architecture can only be set to Multi thereby
+	// only allowing updates from single to multi architecture. If
+	// architecture is set image cannot be set and version must be
+	// set.


Nit: In other enums, we typically have a statement that says valid values are x,y,z, you have this in the blurb but it would be good to have that as an explicit statement as it gets a bit muddled in here IMO

Suggested change

// set.

// set.

// Valid values are 'Multi' and empty.

Agree, will fix it.

Fixed in 08f0ef0

JoelSpeed · 2022-12-06T14:53:16Z

config/v1/types_cluster_version.go

+	// architecture means either a single architecture or a multi
+	// architecture. architecture can only be set to Multi thereby
+	// only allowing updates from single to multi architecture. If
+	// architecture is set image cannot be set and version must be


I think you need a comma in here else this sentence is a bit hard to read

Suggested change

// architecture is set image cannot be set and version must be

// architecture is set, image cannot be set and version must be

Fixed in 08f0ef0

JoelSpeed · 2022-12-06T14:56:35Z

config/v1/types_cluster_version.go

+	// architecture is an optional field that indicates the desired
+	// value of the cluster architecture. In this context cluster
+	// architecture means either a single architecture or a multi
+	// architecture. architecture can only be set to Multi thereby


So there's no way to transition back to the a single arch cluster?

I wonder if we need to explicitly say, "on future updates, if the existing cluster image is multi architecture, then the downloaded image will also be multi architecture. There is no way to convert a cluster from multi to single architecture."

I think I missed this while reviewing before but it seems odd that on future updates, having the field omitted/empty relies on the existing architecture

There is no way to convert a cluster from multi to single architecture

Thinking about this, which I got from trying to understand this API, that's not actually true is it, if I forced an image into the update that was single arch I could force the cluster back to single arch, is that ok?

Thinking about this, which I got from trying to understand this API, that's not actually true is it, if I forced an image into the update that was single arch I could force the cluster back to single arch, is that ok?

That is right. Even if the current ask is only one time migration from single arch to multi arch , we do not want to add any more code to restrict the opposite. Officially we will only document that single to multi is supported. But there will be room for future change of scope.

JoelSpeed · 2022-12-06T15:58:34Z

config/v1/types_cluster_version.go

+// +kubebuilder:validation:XValidation:rule='has(self.architecture) && has(self.image) ? (self.architecture == "" || self.image == "") : true',message="cannot set both Architecture and Image"
+// +kubebuilder:validation:XValidation:rule='has(self.architecture) && self.architecture != "" ? self.version != "" : true',message="Version must be set if Architecture is set"


After spending a little time in the debugger, I found out you can't use single quotes to surround these, you have to use double quotes, can you switch the single for double and vice versa in this line

yeah, will fix it.

Fixed in 08f0ef0

JoelSpeed · 2022-12-07T09:33:06Z

config/v1/types_cluster_version.go

+// +kubebuilder:validation:XValidation:rule="has(self.architecture) && has(self.image) ? (self.architecture == "" || self.image == "") : true",message="cannot set both Architecture and Image"
+// +kubebuilder:validation:XValidation:rule="has(self.architecture) && self.architecture != "" ? self.version != "" : true",message="Version must be set if Architecture is set"


This still won't work as you're unquoting mid way through, either we move back to the length based or use single quotes, I don't mind either way tbh

Suggested change

// +kubebuilder:validation:XValidation:rule="has(self.architecture) && has(self.image) ? (self.architecture == "" || self.image == "") : true",message="cannot set both Architecture and Image"

// +kubebuilder:validation:XValidation:rule="has(self.architecture) && self.architecture != "" ? self.version != "" : true",message="Version must be set if Architecture is set"

// +kubebuilder:validation:XValidation:rule="has(self.architecture) && has(self.image) ? (self.architecture == '' || self.image == '') : true",message="cannot set both Architecture and Image"

// +kubebuilder:validation:XValidation:rule="has(self.architecture) && self.architecture != '' ? self.version != '' : true",message="Version must be set if Architecture is set"

or

Suggested change

// +kubebuilder:validation:XValidation:rule="has(self.architecture) && has(self.image) ? (self.architecture == "" || self.image == "") : true",message="cannot set both Architecture and Image"

// +kubebuilder:validation:XValidation:rule="has(self.architecture) && self.architecture != "" ? self.version != "" : true",message="Version must be set if Architecture is set"

// +kubebuilder:validation:XValidation:rule="has(self.architecture) && has(self.image) ? (size(self.architecture) == 0 || size(self.image) == 0) : true",message="cannot set both Architecture and Image"

// +kubebuilder:validation:XValidation:rule="has(self.architecture) && size(self.architecture) != 0 ? size(self.version) != 0 : true",message="Version must be set if Architecture is set"

Fixed in 354e2fb

openshift-ci · 2022-12-07T15:44:40Z

@jottofar: all tests passed!

Full PR test history. Your PR dashboard.

Details

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

deads2k · 2022-12-07T18:19:37Z

/lgtm

openshift-ci · 2022-12-07T18:20:01Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: deads2k, jottofar

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

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~OWNERS~~ [deads2k]

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

LalatenduMohanty · 2022-12-07T19:49:08Z

/label px-approved
/label qe-approved
/label docs-approved

Catching up with openshift/cluster-version-operator@9be6175c5f (pkg/cvo/sync_worker: Make expected/actual version mismatch fatal, 2020-08-09, openshift/cluster-version-operator#431), which uses the 'version' property as a sanity check for "is this pullspec the version I'm expecting?". This protects users from compromised or man-in-the-middled upstream update services who attempt downgrade and similar attacks by misrepresenting a recommended update. The text I'm adjusting landed in 354e2fb (config/v1/types_cluster_version: Add Architecture to DesiredUpdate, 2022-12-07, openshift#1339), but version-ignoring was never implemented, so nobody can be relying on that nominal behavior. And as the man-in-the-middle use case demonstrates, version-ignoring would be less safe than the version-match-enforcing behavior that the cluster-version operator has used since 2020.

Catching up with openshift/cluster-version-operator@9be6175c5f (pkg/cvo/sync_worker: Make expected/actual version mismatch fatal, 2020-08-09, openshift/cluster-version-operator#431), which uses the 'version' property as a sanity check for "is this pullspec the version I'm expecting?". This protects users from compromised or man-in-the-middled upstream update services who attempt downgrade and similar attacks by misrepresenting a recommended update. The text I'm adjusting landed in 354e2fb (config/v1/types_cluster_version: Add Architecture to DesiredUpdate, 2022-12-07, openshift#1339), but version-ignoring was never implemented, so nobody can be relying on that nominal behavior. And as the man-in-the-middle use case demonstrates, version-ignoring would be less safe than the version-match-enforcing behavior that the cluster-version operator has used since 2020. I edited types_cluster_version.go by hand, and then updated the other files with: $ hack/update-openapi.sh $ hack/update-codegen-crds.sh

Catching up with openshift/cluster-version-operator@9be6175c5f (pkg/cvo/sync_worker: Make expected/actual version mismatch fatal, 2020-08-09, openshift/cluster-version-operator#431), which uses the 'version' property as a sanity check for "is this pullspec the version I'm expecting?". This protects users from compromised or man-in-the-middled upstream update services who attempt downgrade and similar attacks by misrepresenting a recommended update. The text I'm adjusting landed in 354e2fb (config/v1/types_cluster_version: Add Architecture to DesiredUpdate, 2022-12-07, openshift#1339), but version-ignoring was never implemented, so nobody can be relying on that nominal behavior. And as the man-in-the-middle use case demonstrates, version-ignoring would be less safe than the version-match-enforcing behavior that the cluster-version operator has used since 2020. I edited types_cluster_version.go by hand, and then updated the other files with: $ hack/update-codegen-crds.sh $ hack/update-openapi.sh $ hack/update-swagger-docs.sh

openshift-ci bot requested review from bparees and knobunc November 1, 2022 20:59

wking reviewed Nov 7, 2022

View reviewed changes