OCPNODE-3008: Add v1 type ClusterImagePolicy and ImagePolicy #2310

QiWang19 · 2025-05-06T09:19:11Z

Upgrade ClusterImagePolicy and ImagePolicy under SigstoreVerification featuregate to v1. will workon upgrade controller code to use v1 API and promote the featuregate to default as next step.

openshift-ci-robot · 2025-05-06T09:19:16Z

@QiWang19: This pull request references OCPNODE-3008 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.20.0" version, but no target version was set.

In response to this:

Upgrade ClusterImagePolicy and ImagePolicy under SigstoreVerification featuregate to v1. will workon upgrade controller code to use v1 API and promote the featuregate to default as next step.

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-05-06T09:19:33Z

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

openshift-ci · 2025-05-06T09:19:44Z

Hello @QiWang19! 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-05-06T09:20:07Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: QiWang19
Once this PR has been reviewed and has the lgtm label, please assign deads2k 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

QiWang19 · 2025-05-06T10:28:16Z

/test all

QiWang19 · 2025-05-14T17:54:25Z

/test lint
/test verify

QiWang19 · 2025-05-15T13:41:25Z

/test all

QiWang19 · 2025-05-21T21:06:17Z

/test all

QiWang19 · 2025-05-22T15:37:49Z

/test all

QiWang19 · 2025-05-22T16:58:06Z

@JoelSpeed Could you review?

JoelSpeed · 2025-05-28T14:38:32Z

config/v1/types_cluster_image_policy.go

+	Spec ClusterImagePolicySpec `json:"spec"`
+	// status contains the observed state of the resource.
+	// +optional
+	Status ClusterImagePolicyStatus `json:"status,omitempty"`


Omitempty doesn't work on non-pointer structs, you can drop the omitempty tag

JoelSpeed · 2025-05-28T14:39:17Z

config/v1/types_cluster_image_policy.go

+
+// +k8s:deepcopy-gen=true
+type ClusterImagePolicyStatus struct {
+	// conditions provide details on the status of this API Resource.


Can we provide further detail on the types of conditions that should be expected to be set here? I suspect we have a complete list for the feature now?

For ImagePolicy, we have Pending conditions suggesting some configs from this object would be overwritten since they conflict with existing cluster-wide objects.
We don't have other conditions for ClusterImagePolicy/ImagePolicy since there is no mechanism for MCO to give rollout feedback of a machine config.

JoelSpeed · 2025-05-28T14:39:36Z

config/v1/types_cluster_image_policy.go

+// +k8s:deepcopy-gen=true
+type ClusterImagePolicyStatus struct {
+	// conditions provide details on the status of this API Resource.
+	// +kubebuilder:validation:MaxItems=100


100 seems high, do you know how many conditions the feature actually uses right now? Would say 8 or 16 be a more sensible limit?

JoelSpeed · 2025-05-28T14:40:41Z

config/v1/types_cluster_image_policy.go

+	// +listType=map
+	// +listMapKey=type
+	// +optional
+	Conditions []metav1.Condition `json:"conditions,omitempty"`


You should add a minimum length marker of 1 to this, so that [] is not a valid persisted object

Suggested change

Conditions []metav1.Condition `json:"conditions,omitempty"`

// +kubebuilder:validation:MinItems=1

Conditions []metav1.Condition `json:"conditions,omitempty"`

JoelSpeed · 2025-05-28T14:42:40Z

hack/update-payload-crds.sh

@@ -4,7 +4,6 @@ source "$(dirname "${BASH_SOURCE}")/lib/init.sh"

 crd_globs="\
    authorization/v1/zz_generated.crd-manifests/*_config-operator_*.crd*yaml\
-    config/v1/zz_generated.crd-manifests/*_config-operator_*.crd*yaml\


Why is this change required?

Add this change so the v1 manifests will not be included in the payload at this time https://github.com/openshift/api/tree/master/payload-manifests/crds. From our previous discussion, we will include the manifests later when MCO controller implementation is ready to use v1 APIs.

JoelSpeed · 2025-05-28T14:43:13Z

config/v1/types_image_policy.go

+	Spec ImagePolicySpec `json:"spec"`
+	// status contains the observed state of the resource.
+	// +optional
+	Status ImagePolicyStatus `json:"status,omitempty"`


Omitempty here is not doing anything

JoelSpeed · 2025-05-28T14:45:11Z

config/v1/types_image_policy.go

+
+// Policy defines the verification policy for the items in the scopes list.
+type Policy struct {
+	// rootOfTrust specifies the root of trust for the policy.


This godoc is a little light, can we expand at all on what a root of trust is, and/or why someone might want to configure this option

JoelSpeed · 2025-05-28T14:50:46Z

config/v1/types_image_policy.go

+}
+
+// PKI defines the root of trust based on Root CA(s) and corresponding intermediate certificates.
+type PKI struct {


Should add minimum lengths to each of the slices in this example

everettraven

An initial pass. Structure of the API looks good. Most of my comments are suggestions to improve the API documentation and a couple validations that appear to need some updating

everettraven · 2025-05-29T17:00:04Z

config/v1/types_cluster_image_policy.go

+	// +required
+	// +kubebuilder:validation:MaxItems=256


Explicitly state these constraints in the GoDoc for the field. Users will not be able to see the markers when looking at the generated documentation so stating these validations explicitly, in plain english, helps us convey constraints to user reading the generated documentation.

everettraven · 2025-05-29T17:00:51Z

config/v1/types_cluster_image_policy.go

+	Scopes []ImageScope `json:"scopes"`
+	// policy contains configuration to allow scopes to be verified, and defines how
+	// images not matching the verification policy will be treated.
+	// +required


Ditto on explicit inclusion in the GoDoc that this field is required.

everettraven · 2025-05-29T17:07:28Z

config/v1/types_image_policy.go

+	RootOfTrust PolicyRootOfTrust `json:"rootOfTrust"`
+	// signedIdentity specifies what image identity the signature claims about the image. The required matchPolicy field specifies the approach used in the verification process to verify the identity in the signature and the actual image identity, the default matchPolicy is "MatchRepoDigestOrExact".
+	// +optional
+	SignedIdentity PolicyIdentity `json:"signedIdentity,omitempty"`


As Joel has mentioned previously, omitempty doesn't do anything on a non-pointer struct. It can be dropped here.

everettraven · 2025-05-29T17:07:54Z

config/v1/types_image_policy.go

+// Policy defines the verification policy for the items in the scopes list.
+type Policy struct {
+	// rootOfTrust specifies the root of trust for the policy.
+	// +required


Explicitly mention in the GoDoc that this field is required.

everettraven · 2025-05-29T17:11:19Z

config/v1/types_image_policy.go

+	// +required
+	RootOfTrust PolicyRootOfTrust `json:"rootOfTrust"`
+	// signedIdentity specifies what image identity the signature claims about the image. The required matchPolicy field specifies the approach used in the verification process to verify the identity in the signature and the actual image identity, the default matchPolicy is "MatchRepoDigestOrExact".
+	// +optional


Explicitly mention that this field is optional in the GoDoc. It looks like you do appropriately state what the default behavior is if this field is omitted.

everettraven · 2025-05-29T18:34:07Z

config/v1/types_image_policy.go

+type PolicyMatchExactRepository struct {
+	// repository is the reference of the image identity to be matched.
+	// The value should be a repository name (by omitting the tag or digest) in a registry implementing the "Docker Registry HTTP API V2". For example, docker.io/library/busybox
+	// +required


Explicit mention of being required.

everettraven · 2025-05-29T18:36:56Z

config/v1/types_image_policy.go

+type PolicyMatchRemapIdentity struct {
+	// prefix is the prefix of the image identity to be matched.
+	// If the image identity matches the specified prefix, that prefix is replaced by the specified “signedPrefix” (otherwise it is used as unchanged and no remapping takes place).
+	// This useful when verifying signatures for a mirror of some other repository namespace that preserves the vendor’s repository structure.


Suggested change

// This useful when verifying signatures for a mirror of some other repository namespace that preserves the vendor’s repository structure.

// This is useful when verifying signatures for a mirror of some other repository namespace that preserves the vendor’s repository structure.

everettraven · 2025-05-29T18:38:44Z

config/v1/types_image_policy.go

+	// The prefix and signedPrefix values can be either host[:port] values (matching exactly the same host[:port], string), repository namespaces,
+	// or repositories (i.e. they must not contain tags/digests), and match as prefixes of the fully expanded form.
+	// For example, docker.io/library/busybox (not busybox) to specify that single repository, or docker.io/library (not an empty string) to specify the parent namespace of docker.io/library/busybox.
+	// +required


Explicitly mention that the field is required in the GoDoc.

everettraven · 2025-05-29T18:38:49Z

config/v1/types_image_policy.go

+	// signedPrefix is the prefix of the image identity to be matched in the signature. The format is the same as "prefix". The values can be either host[:port] values (matching exactly the same host[:port], string), repository namespaces,
+	// or repositories (i.e. they must not contain tags/digests), and match as prefixes of the fully expanded form.
+	// For example, docker.io/library/busybox (not busybox) to specify that single repository, or docker.io/library (not an empty string) to specify the parent namespace of docker.io/library/busybox.
+	// +required


Explicitly mention that the field is required in the GoDoc.

everettraven · 2025-05-29T18:40:11Z

config/v1/types_image_policy.go

+}
+
+// IdentityMatchPolicy defines the type of matching for "matchPolicy".
+// +kubebuilder:validation:Enum=MatchRepoDigestOrExact;MatchRepository;ExactRepository;RemapIdentity


From my understanding, this should allow the omitted value ("") - which is missing in this enum.

Should it? The enum is used in a required field context, I would expect in this case that not allowing the empty string is probably correct.

I see there is a "When omitted" description though, which does imply that "" is a valid value.

Looking at the shape here, I think we have a case of "optional struct that has a required field within, where the required fields do not accept the zero value"

In this case, the appropriate action is to make the struct field a pointer, so that it can be correctly omitted.

Unless we are deliberately going for discoverability, in which case we need to make sure that all of the fields within the struct accept their zero value

Unless we are deliberately going for discoverability, in which case we need to make sure that all of the fields within the struct accept their zero value

This is the case I was assuming we would have as this is a configuration API and our general stance is to have discoverability in these types of APIs.

I should have been a bit more clear as to why I understood this as needing to allow the omitted value, my apologies

Generally, when looking at discoverability comments we should start at the top as it were.

Given currently, https://github.com/openshift/api/pull/2310/files#diff-efbd815aeb2fd3f182852b1c6ec8317d546d44813792bfe7da5e253394f5ff18R72, the usage of the struct containing this is a DU that is optional, pointer, omitempty, we don't need to look at discoverability of fields within there until we have discussed if the signedIdentity field should be discoverable.

If the top level field isn't going to marshal down to {"matchPolicy":""} (which it won't currently) then we don't need "" to be valid here.

So, should signedIdentity be discoverable? Well, who is expected to create these objects? Are we ever likely to be producing these via a go client for the end user? Are we expecting 1 of these objects? Are we expecting many?

I don't think we originally aimed for discoverability on this API because there is no valid object for spec: {}, we need at least some config from an end user for this object to make sense (also, I expect there to be many of these rather than 1)

Upgrade ClusterImagePolicy and ImagePolicy under SigstoreVerification featuregate to v1. will workon upgrade controller code to use v1 API and promote the featuregate to default as next step. Signed-off-by: Qi Wang <[email protected]>

openshift-ci · 2025-06-02T22:37:14Z

@QiWang19: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
ci/prow/lint	`db83887`	link	true	`/test lint`

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.

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

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 May 6, 2025

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

QiWang19 force-pushed the sigstoreverification branch from bc22659 to c277fe2 Compare May 6, 2025 10:27

QiWang19 force-pushed the sigstoreverification branch 4 times, most recently from 28cc792 to c96787f Compare May 14, 2025 17:54

QiWang19 force-pushed the sigstoreverification branch from c96787f to a184848 Compare May 21, 2025 21:05

QiWang19 force-pushed the sigstoreverification branch from a184848 to 5886a58 Compare May 22, 2025 15:24

QiWang19 marked this pull request as ready for review May 22, 2025 16:25

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 May 22, 2025

openshift-ci bot requested review from deads2k and everettraven May 22, 2025 16:25

QiWang19 changed the title ~~OCPNODE-3008: Upgrade ClusterImagePolicy and ImagePolicy to v1~~ OCPNODE-3008: Add v1 type ClusterImagePolicy and ImagePolicy May 22, 2025

JoelSpeed reviewed May 28, 2025

View reviewed changes

everettraven reviewed May 29, 2025

View reviewed changes

QiWang19 force-pushed the sigstoreverification branch 2 times, most recently from c64bcf8 to e04c9af Compare June 2, 2025 21:09

Upgrade ClusterImagePolicy and ImagePolicy to v1

db83887

Upgrade ClusterImagePolicy and ImagePolicy under SigstoreVerification featuregate to v1. will workon upgrade controller code to use v1 API and promote the featuregate to default as next step. Signed-off-by: Qi Wang <[email protected]>

QiWang19 force-pushed the sigstoreverification branch from e04c9af to db83887 Compare June 2, 2025 22:12

	Conditions []metav1.Condition `json:"conditions,omitempty"`
	// +kubebuilder:validation:MinItems=1
	Conditions []metav1.Condition `json:"conditions,omitempty"`

	// This useful when verifying signatures for a mirror of some other repository namespace that preserves the vendor’s repository structure.
	// This is useful when verifying signatures for a mirror of some other repository namespace that preserves the vendor’s repository structure.

OCPNODE-3008: Add v1 type ClusterImagePolicy and ImagePolicy #2310

Are you sure you want to change the base?

OCPNODE-3008: Add v1 type ClusterImagePolicy and ImagePolicy #2310

Uh oh!

Conversation

QiWang19 commented May 6, 2025

Uh oh!

openshift-ci-robot commented May 6, 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 May 6, 2025

Uh oh!

openshift-ci bot commented May 6, 2025

Uh oh!

openshift-ci bot commented May 6, 2025

Uh oh!

QiWang19 commented May 6, 2025

Uh oh!

QiWang19 commented May 14, 2025

Uh oh!

QiWang19 commented May 15, 2025

Uh oh!

QiWang19 commented May 21, 2025

Uh oh!

QiWang19 commented May 22, 2025

Uh oh!

QiWang19 commented May 22, 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!

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!

everettraven 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!

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!

everettraven May 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

openshift-ci bot commented Jun 2, 2025

openshift-ci-robot commented May 6, 2025 •

edited by openshift-ci bot

Loading

everettraven May 30, 2025 •

edited

Loading