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.

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)

QiWang19 · 2025-06-03T14:54:20Z

@everettraven @JoelSpeed PTAL

JoelSpeed

Couple of small bits, but otherwise looks good

Do we have automated testing for this feature yet? We should make sure we are comfortable and somewhat near ready to promote before we merge the API, or at least, have confidence we will be in a position to pass the promotion requirements within 4.20 development

JoelSpeed · 2025-06-05T11:02:16Z

config/v1/types_image_policy.go

+	// +required
+	// +kubebuilder:validation:MaxLength=8192
+	// +kubebuilder:validation:MinLength=68
+	KeyData []byte `json:"keyData"`


Should these keydata fields have CEL similar to L173 to check the beginning and end of the value? Same for the second keydata field below

JoelSpeed · 2025-06-05T11:03:47Z

config/v1/types_image_policy.go

+	// caRootsData contains base64-encoded data of a certificate bundle PEM file, which contains one or more CA roots in the PEM format. The total length of the data must not exceed 8192 characters.
+	// +required
+	// +kubebuilder:validation:MaxLength=8192
+	// +kubebuilder:validation:MinLength=72


Why are these 72, when the other keys are 68?

I did not find offical document about minLength limit for certificate data or public key data so I set the minLength to the length of base64 encoding of -----BEGIN CERTIFICATE----- + -----END CERTIFICATE----- for certificate data, -----BEGIN PUBLIC KEY----- +-----END PUBLIC KEY----- for pubilc key data, the 78, 72 difference caused by different prefix, suffix.

QiWang19 · 2025-06-06T01:58:00Z

/test verify

QiWang19 · 2025-06-06T02:15:48Z

@JoelSpeed There are [sig-imagepolicy][OCPFeatureGate:SigstoreImageVerification][Serial] tests for this feature, I have this draft PR #2340 run the verify-feature-promotion test.
I am also working on fixing the test failure an was ipv4 and metal ipv6.

QiWang19 · 2025-06-10T14:53:36Z

@JoelSpeed PTAL

everettraven

/lgtm

JoelSpeed · 2025-06-12T10:46:44Z

PTAL at the linter issue, it's found a string without a max length, please set an appropriate maximum length on this one

QiWang19 · 2025-06-12T17:08:12Z

please set an appropriate maximum length on this one

@JoelSpeed There was a previous review about the MaxLength requirement on enums not being necessary #2310 (comment), I just wanted to double check if this is an upstream issue or if this validation is needed.

JoelSpeed · 2025-06-16T16:47:16Z

config/v1/types_image_policy.go

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


Make sure this matches the ClusterImagePolicy version, should have a Minitems as well

fixed. PTAL

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 <qiwan@redhat.com>

JoelSpeed · 2025-06-17T10:10:48Z

/override ci/prow/lint

/lgtm

openshift-ci · 2025-06-17T10:12:08Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: everettraven, JoelSpeed, QiWang19

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

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [JoelSpeed]

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

openshift-ci · 2025-06-17T10:12:19Z

@JoelSpeed: Overrode contexts on behalf of JoelSpeed: ci/prow/lint

In response to this:

/override ci/prow/lint

/lgtm

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.

openshift-ci · 2025-06-17T10:12:26Z

@QiWang19: 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.

openshift-bot · 2025-06-17T13:04:33Z

[ART PR BUILD NOTIFIER]

Distgit: ose-cluster-config-api
This PR has been included in build ose-cluster-config-api-container-v4.20.0-202506171245.p0.gcb94c55.assembly.stream.el9.
All builds following this will include this PR.

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 3 times, most recently from e04c9af to db83887 Compare June 2, 2025 22:12

JoelSpeed reviewed Jun 5, 2025

View reviewed changes

QiWang19 force-pushed the sigstoreverification branch 2 times, most recently from 834476d to fb6f080 Compare June 6, 2025 01:31

everettraven approved these changes Jun 11, 2025

View reviewed changes

openshift-ci bot assigned everettraven Jun 11, 2025

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Jun 11, 2025

JoelSpeed reviewed Jun 16, 2025

View reviewed changes

QiWang19 force-pushed the sigstoreverification branch from fb6f080 to 3c5d9a7 Compare June 16, 2025 16:53

openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Jun 16, 2025

Upgrade ClusterImagePolicy and ImagePolicy to v1

00034b1

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 <qiwan@redhat.com>

QiWang19 force-pushed the sigstoreverification branch from 3c5d9a7 to 00034b1 Compare June 16, 2025 17:35

openshift-ci bot assigned JoelSpeed Jun 17, 2025

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Jun 17, 2025

openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 17, 2025

openshift-merge-bot bot merged commit cb94c55 into openshift:master Jun 17, 2025
12 checks passed

QiWang19 mentioned this pull request Jul 7, 2025

OCPNODE-3006:Register v1 (cluster)imagepolicy #2394

Merged

	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

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!

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!

QiWang19 commented Jun 3, 2025

Uh oh!

JoelSpeed left a comment

Choose a reason for hiding this comment

openshift-ci-robot commented May 6, 2025 •

edited by openshift-ci bot

Loading

everettraven May 30, 2025 •

edited

Loading