OCPNODE-3863: Add CRIOCredentialProviderConfig API #2557
Conversation
|
Skipping CI for Draft Pull Request. |
openshift-ci
bot
added
the
do-not-merge/work-in-progress
|
Hello @QiWang19! Some important instructions when contributing to openshift/api: |
openshift-ci
bot
added
the
size/XL
QiWang19
changed the title
QiWang19
force-pushed
the
creds-provider
branch
from
c00c122 to
1db4267
Compare
|
/retest-required |
QiWang19
force-pushed
the
creds-provider
branch
2 times, most recently
from
8805169 to
108219f
Compare
|
/test integration |
QiWang19
force-pushed
the
creds-provider
branch
from
108219f to
066bf58
Compare
openshift-ci
bot
removed
the
do-not-merge/work-in-progress
|
PR has been reviewed by claude code /api-review. It helped run |
|
|
||
| // +kubebuilder:validation:MaxLength=512 | ||
| // +kubebuilder:validation:XValidation:rule=`self.matches('^((\\*|[a-zA-Z0-9]([a-zA-Z0-9-]*[a-zA-Z0-9])?)(\\.(\\*|[a-zA-Z0-9]([a-zA-Z0-9-]*[a-zA-Z0-9])?))*)(:[0-9]+)?(/[-a-zA-Z0-9_/]*)?$')`,message="invalid matchImages value, must be a valid fully qualified domain name with optional wildcard, port, and path" | ||
| type MatchImage string |
There was a problem hiding this comment.
@saschagrunert PTAL. as mentioned in the previsou enhancement discussion(openshift/enhancements#1861 (comment)), this is a stricter rule than the upstream matchImages, as it does not allow wildcard matching of partial subdomains like app*.k8s.io. Customers may raise concerns about this difference, but it simplifies the configuration.
What do you think?
There was a problem hiding this comment.
I mean, we can always make it looser later on while starting with a stricter regex. It's already fairly complex and we need extensive testing on that validation part.
The docs needs to be updated for this type, like:
// MatchImage is a string pattern used to match container image registry addresses.
// It must be a valid fully qualified domain name with optional wildcard, port, and path.
// The maximum length is 512 characters.
//
// Wildcards ('*') are supported for full subdomain labels and top-level domains.
// Each entry can optionally contain a port (e.g., :8080) and a path (e.g., /path).
// Wildcards are not allowed in the port or path portions.
//
// Examples:
// - "registry.io" - matches exactly registry.io
// - "*.azurecr.io" - matches any single subdomain of azurecr.io
// - "registry.io:8080/path" - matches with specific port and path prefix
//
// +kubebuilder:validation:MaxLength=512
// +kubebuilder:validation:XValidation:rule=`self.matches('^((\\*|[a-zA-Z0-9]([a-zA-Z0-9-]*[a-zA-Z0-9])?)(\\.(\\*|[a-zA-Z0-9]([a-zA-Z0-9-]*[a-zA-Z0-9])?))*)(:[0-9]+)?(/[-a-zA-Z0-9_/]*)?$')`,message="invalid matchImages value, must be a valid fully qualified domain name with optional wildcard, port, and path"
type MatchImage string|
|
||
| // +kubebuilder:validation:MaxLength=512 | ||
| // +kubebuilder:validation:XValidation:rule=`self.matches('^((\\*|[a-zA-Z0-9]([a-zA-Z0-9-]*[a-zA-Z0-9])?)(\\.(\\*|[a-zA-Z0-9]([a-zA-Z0-9-]*[a-zA-Z0-9])?))*)(:[0-9]+)?(/[-a-zA-Z0-9_/]*)?$')`,message="invalid matchImages value, must be a valid fully qualified domain name with optional wildcard, port, and path" | ||
| type MatchImage string |
There was a problem hiding this comment.
I mean, we can always make it looser later on while starting with a stricter regex. It's already fairly complex and we need extensive testing on that validation part.
The docs needs to be updated for this type, like:
// MatchImage is a string pattern used to match container image registry addresses.
// It must be a valid fully qualified domain name with optional wildcard, port, and path.
// The maximum length is 512 characters.
//
// Wildcards ('*') are supported for full subdomain labels and top-level domains.
// Each entry can optionally contain a port (e.g., :8080) and a path (e.g., /path).
// Wildcards are not allowed in the port or path portions.
//
// Examples:
// - "registry.io" - matches exactly registry.io
// - "*.azurecr.io" - matches any single subdomain of azurecr.io
// - "registry.io:8080/path" - matches with specific port and path prefix
//
// +kubebuilder:validation:MaxLength=512
// +kubebuilder:validation:XValidation:rule=`self.matches('^((\\*|[a-zA-Z0-9]([a-zA-Z0-9-]*[a-zA-Z0-9])?)(\\.(\\*|[a-zA-Z0-9]([a-zA-Z0-9-]*[a-zA-Z0-9])?))*)(:[0-9]+)?(/[-a-zA-Z0-9_/]*)?$')`,message="invalid matchImages value, must be a valid fully qualified domain name with optional wildcard, port, and path"
type MatchImage string
QiWang19
force-pushed
the
creds-provider
branch
2 times, most recently
from
d2a191b to
41bd600
Compare
openshift-ci
bot
added
size/XXL
QiWang19
changed the title
openshift-ci-robot
added
the
jira/valid-reference
|
@QiWang19: This pull request references OCPNODE-3863 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. 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. |
|
@QiWang19: This pull request references OCPNODE-3863 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. 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. |
QiWang19
force-pushed
the
creds-provider
branch
from
41bd600 to
e7df906
Compare
saschagrunert
left a comment
saschagrunert
left a comment
There was a problem hiding this comment.
Two nits, otherwise LGTM
|
/assign @everettraven |
|
/test integration |
QiWang19
force-pushed
the
creds-provider
branch
from
ad4cfb0 to
5086a52
Compare
|
/test integration |
| // For more details, see: | ||
| // - https://kubernetes.io/docs/tasks/administer-cluster/kubelet-credential-provider/ | ||
| // - https://github.com/cri-o/crio-credential-provider#architecture |
There was a problem hiding this comment.
You say there is no relation, yet they appear to me to be siblings in this feature?
The ECR provider exists already, and you are adding another credential provider alongside the ECR right?
My concern was about whether there might be overlap in the registries they claim to provide credentials for, WDYT? Is that likely to be possible?
| // | ||
| // +kubebuilder:validation:MaxLength=512 | ||
| // +kubebuilder:validation:XValidation:rule="self != '*'",message="global wildcard '*' is not allowed" | ||
| // +kubebuilder:validation:XValidation:rule=`self.matches('^((\\*|[a-zA-Z0-9]([a-zA-Z0-9-]*[a-zA-Z0-9])?)(\\.(\\*|[a-zA-Z0-9]([a-zA-Z0-9-]*[a-zA-Z0-9])?))*)(:[0-9]+)?(/[-a-zA-Z0-9_/]*)?$')`,message="invalid matchImages value, must be a valid fully qualified domain name with optional wildcard, port, and path" |
There was a problem hiding this comment.
Is it valid to allow uppercase characters? Could we reasonably restrict this only to lowercase?
There was a problem hiding this comment.
yes, we can restrict only to lowercases, since the hostname is not case sensitive.
|
|
||
| // +k8s:deepcopy-gen=true | ||
| // CRIOCredentialProviderConfigStatus defines the observed state of CRIOCredentialProviderConfig | ||
| type CRIOCredentialProviderConfigStatus struct { |
There was a problem hiding this comment.
Please add a MinProperties here
| type CRIOCredentialProviderConfigStatus struct { | |
| // +kubebuilder:validation:MinProperties:=1 | |
| type CRIOCredentialProviderConfigStatus struct { |
There should be no reason we need/want to persist status: {} as having a semantic meaning
| // Expected condition types include: | ||
| // "Validated": indicates whether the matchImages configuration is valid | ||
| // +optional | ||
| // +kubebuilder:validation:MaxItems=4 |
There was a problem hiding this comment.
Should also include a MinItems=1 please
machine/v1beta1/types_awsprovider.go
Outdated
| // +required | ||
| // +unionDiscriminator | ||
| Affinity *HostAffinity `json:"affinity,omitempty"` | ||
| Affinity HostAffinity `json:"affinity,omitempty"` |
There was a problem hiding this comment.
Why are you changing this?
QiWang19
force-pushed
the
creds-provider
branch
2 times, most recently
from
aae514b to
1ab2c1d
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
openapi/generated_openapi/zz_generated.openapi.go (2)
21642-21661: Array validation constraints still missing for matchImages.The schema is missing
MinItemsandMaxItemsconstraints even though line 21649 documents "must contain between 1 and 50 entries". This was flagged in previous reviews and marked as addressed, but the generated code still lacks the constraints.Since this is generated code, ensure the kubebuilder validation markers are present in the source type (likely
config/v1alpha1/types_crio_credential_provider_config.go):// +kubebuilder:validation:MinItems=1 // +kubebuilder:validation:MaxItems=50 MatchImages []string `json:"matchImages"`Then regenerate this file with
make update-codegen-crds.
21676-21697: MaxItems constraint still missing for conditions array.The schema is missing the
MaxItemsconstraint even though line 21686 documents "maximum number of conditions is 4". This was flagged in previous reviews and marked as addressed, but the generated code still lacks the constraint.Since this is generated code, ensure the kubebuilder validation marker is present in the source type (likely
config/v1alpha1/types_crio_credential_provider_config.go):// +kubebuilder:validation:MaxItems=4 Conditions []metav1.Condition `json:"conditions,omitempty"`Then regenerate this file with
make update-codegen-crds.openapi/openapi.json (1)
11792-11800: Missing array bounds validation on matchImages and conditions arrays (flagged in prior review).This is a continuation of the validation gap identified in the previous review. The OpenAPI schema still lacks
minItems/maxItemsconstraints despite PR requirements:
matchImages: should enforce "1–50 unique strings" → addminItems: 1, maxItems: 50status.conditions: should enforce "up to 4" → addmaxItems: 4Apply these changes to enforce cardinality at the schema level:
"matchImages": { "description": "...", "type": "array", + "minItems": 1, + "maxItems": 50, "items": { "type": "string", "default": "" }, "x-kubernetes-list-type": "set" }"conditions": { "description": "conditions represent the latest available observations of the configuration state", "type": "array", + "maxItems": 4, "items": { "default": {}, "$ref": "#/definitions/io.k8s.apimachinery.pkg.apis.meta.v1.Condition" }, "x-kubernetes-list-map-keys": [ "type" ], "x-kubernetes-list-type": "map" }Ensure the corresponding Go structs in
config/v1alpha1/include+kubebuilder:validation:MinItems=1,+kubebuilder:validation:MaxItems=50onmatchImages, and+kubebuilder:validation:MaxItems=4onconditions, so codegen produces the updated schema.Also applies to: 11807-11818
🧹 Nitpick comments (1)
config/v1alpha1/tests/criocredentialproviderconfigs.config.openshift.io/CRIOCredentialProviderConfig.yaml (1)
1-92: Add final newline to file.YAML convention requires a final newline character at the end of the file to ensure proper text file formatting.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (30)
config/v1alpha1/register.go(1 hunks)config/v1alpha1/tests/criocredentialproviderconfigs.config.openshift.io/CRIOCredentialProviderConfig.yaml(1 hunks)config/v1alpha1/types_crio_credential_provider_config.go(1 hunks)config/v1alpha1/zz_generated.crd-manifests/0000_10_config-operator_01_criocredentialproviderconfigs-CustomNoUpgrade.crd.yaml(1 hunks)config/v1alpha1/zz_generated.crd-manifests/0000_10_config-operator_01_criocredentialproviderconfigs-DevPreviewNoUpgrade.crd.yaml(1 hunks)config/v1alpha1/zz_generated.crd-manifests/0000_10_config-operator_01_criocredentialproviderconfigs-TechPreviewNoUpgrade.crd.yaml(1 hunks)config/v1alpha1/zz_generated.deepcopy.go(1 hunks)config/v1alpha1/zz_generated.featuregated-crd-manifests.yaml(1 hunks)config/v1alpha1/zz_generated.featuregated-crd-manifests/criocredentialproviderconfigs.config.openshift.io/CRIOCredentialProviderConfig.yaml(1 hunks)config/v1alpha1/zz_generated.swagger_doc_generated.go(1 hunks)features.md(1 hunks)features/features.go(1 hunks)hack/update-payload-crds.sh(1 hunks)openapi/generated_openapi/zz_generated.openapi.go(2 hunks)openapi/openapi.json(1 hunks)payload-manifests/crds/0000_10_config-operator_01_criocredentialproviderconfigs-CustomNoUpgrade.crd.yaml(1 hunks)payload-manifests/crds/0000_10_config-operator_01_criocredentialproviderconfigs-DevPreviewNoUpgrade.crd.yaml(1 hunks)payload-manifests/crds/0000_10_config-operator_01_criocredentialproviderconfigs-TechPreviewNoUpgrade.crd.yaml(1 hunks)payload-manifests/crds/0000_80_machine-config_01_internalreleaseimages-CustomNoUpgrade.crd.yaml(0 hunks)payload-manifests/crds/0000_80_machine-config_01_internalreleaseimages-DevPreviewNoUpgrade.crd.yaml(0 hunks)payload-manifests/crds/0000_80_machine-config_01_internalreleaseimages-TechPreviewNoUpgrade.crd.yaml(0 hunks)payload-manifests/crds/0000_80_machine-config_01_osimagestreams-CustomNoUpgrade.crd.yaml(0 hunks)payload-manifests/crds/0000_80_machine-config_01_osimagestreams-DevPreviewNoUpgrade.crd.yaml(0 hunks)payload-manifests/crds/0000_80_machine-config_01_osimagestreams-TechPreviewNoUpgrade.crd.yaml(0 hunks)payload-manifests/featuregates/featureGate-Hypershift-Default.yaml(1 hunks)payload-manifests/featuregates/featureGate-Hypershift-DevPreviewNoUpgrade.yaml(1 hunks)payload-manifests/featuregates/featureGate-Hypershift-TechPreviewNoUpgrade.yaml(1 hunks)payload-manifests/featuregates/featureGate-SelfManagedHA-Default.yaml(1 hunks)payload-manifests/featuregates/featureGate-SelfManagedHA-DevPreviewNoUpgrade.yaml(1 hunks)payload-manifests/featuregates/featureGate-SelfManagedHA-TechPreviewNoUpgrade.yaml(1 hunks)
💤 Files with no reviewable changes (6)
- payload-manifests/crds/0000_80_machine-config_01_osimagestreams-TechPreviewNoUpgrade.crd.yaml
- payload-manifests/crds/0000_80_machine-config_01_osimagestreams-CustomNoUpgrade.crd.yaml
- payload-manifests/crds/0000_80_machine-config_01_internalreleaseimages-CustomNoUpgrade.crd.yaml
- payload-manifests/crds/0000_80_machine-config_01_internalreleaseimages-DevPreviewNoUpgrade.crd.yaml
- payload-manifests/crds/0000_80_machine-config_01_osimagestreams-DevPreviewNoUpgrade.crd.yaml
- payload-manifests/crds/0000_80_machine-config_01_internalreleaseimages-TechPreviewNoUpgrade.crd.yaml
🚧 Files skipped from review as they are similar to previous changes (11)
- features/features.go
- config/v1alpha1/zz_generated.featuregated-crd-manifests.yaml
- config/v1alpha1/register.go
- hack/update-payload-crds.sh
- config/v1alpha1/zz_generated.swagger_doc_generated.go
- payload-manifests/featuregates/featureGate-SelfManagedHA-DevPreviewNoUpgrade.yaml
- payload-manifests/featuregates/featureGate-SelfManagedHA-Default.yaml
- payload-manifests/featuregates/featureGate-Hypershift-DevPreviewNoUpgrade.yaml
- payload-manifests/featuregates/featureGate-SelfManagedHA-TechPreviewNoUpgrade.yaml
- config/v1alpha1/zz_generated.crd-manifests/0000_10_config-operator_01_criocredentialproviderconfigs-CustomNoUpgrade.crd.yaml
- payload-manifests/featuregates/featureGate-Hypershift-TechPreviewNoUpgrade.yaml
🧰 Additional context used
📓 Path-based instructions (1)
**
⚙️ CodeRabbit configuration file
-Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity.
Files:
features.mdconfig/v1alpha1/zz_generated.crd-manifests/0000_10_config-operator_01_criocredentialproviderconfigs-DevPreviewNoUpgrade.crd.yamlconfig/v1alpha1/zz_generated.crd-manifests/0000_10_config-operator_01_criocredentialproviderconfigs-TechPreviewNoUpgrade.crd.yamlpayload-manifests/featuregates/featureGate-Hypershift-Default.yamlconfig/v1alpha1/tests/criocredentialproviderconfigs.config.openshift.io/CRIOCredentialProviderConfig.yamlconfig/v1alpha1/types_crio_credential_provider_config.goconfig/v1alpha1/zz_generated.deepcopy.goconfig/v1alpha1/zz_generated.featuregated-crd-manifests/criocredentialproviderconfigs.config.openshift.io/CRIOCredentialProviderConfig.yamlpayload-manifests/crds/0000_10_config-operator_01_criocredentialproviderconfigs-TechPreviewNoUpgrade.crd.yamlpayload-manifests/crds/0000_10_config-operator_01_criocredentialproviderconfigs-DevPreviewNoUpgrade.crd.yamlopenapi/generated_openapi/zz_generated.openapi.gopayload-manifests/crds/0000_10_config-operator_01_criocredentialproviderconfigs-CustomNoUpgrade.crd.yamlopenapi/openapi.json
🧬 Code graph analysis (2)
config/v1alpha1/types_crio_credential_provider_config.go (1)
config/v1alpha1/zz_generated.swagger_doc_generated.go (3)
CRIOCredentialProviderConfigSpec(254-256)CRIOCredentialProviderConfigStatus(263-265)CRIOCredentialProviderConfig(236-238)
config/v1alpha1/zz_generated.deepcopy.go (2)
config/v1alpha1/types_crio_credential_provider_config.go (5)
CRIOCredentialProviderConfig(25-43)CRIOCredentialProviderConfigStatus(109-123)CRIOCredentialProviderConfigList(131-139)CRIOCredentialProviderConfigSpec(46-86)MatchImage(104-104)config/v1alpha1/zz_generated.swagger_doc_generated.go (4)
CRIOCredentialProviderConfig(236-238)CRIOCredentialProviderConfigStatus(263-265)CRIOCredentialProviderConfigList(245-247)CRIOCredentialProviderConfigSpec(254-256)
🔇 Additional comments (16)
payload-manifests/featuregates/featureGate-Hypershift-Default.yaml (1)
57-59: ✅ Feature gate placement is correct.The CRIOCredentialProviderConfig gate is properly positioned in alphabetical order and disabled by default in the Hypershift-Default configuration, which aligns with preview feature behavior.
features.md (1)
36-36: ✅ Feature documentation is consistent and properly positioned.The feature table correctly shows CRIOCredentialProviderConfig as enabled in DevPreviewNoUpgrade and TechPreviewNoUpgrade environments, which aligns with the feature gate configuration.
config/v1alpha1/zz_generated.featuregated-crd-manifests/criocredentialproviderconfigs.config.openshift.io/CRIOCredentialProviderConfig.yaml (1)
1-206: ✅ CRD properly defines CRIOCredentialProviderConfig API surface.The featuregated CRD variant is well-structured with correct singleton enforcement, comprehensive matchImages validation including global wildcard rejection, and optional status field. The schema properly enforces all documented constraints.
config/v1alpha1/zz_generated.crd-manifests/0000_10_config-operator_01_criocredentialproviderconfigs-DevPreviewNoUpgrade.crd.yaml (1)
1-206: ✅ DevPreviewNoUpgrade variant is consistent with other CRD definitions.This generated CRD variant properly enforces all API constraints and is consistently structured with the featuregated version, with appropriate feature-set annotations for the DevPreviewNoUpgrade release gate.
payload-manifests/crds/0000_10_config-operator_01_criocredentialproviderconfigs-CustomNoUpgrade.crd.yaml (1)
106-111: ✅ CustomNoUpgrade payload CRD includes required global wildcard validation.The global wildcard rejection rule (
self != '*') is now present alongside the regex validation, ensuring consistent behavior across all CRD variants. This resolves the previous inconsistency flagged in earlier reviews.payload-manifests/crds/0000_10_config-operator_01_criocredentialproviderconfigs-DevPreviewNoUpgrade.crd.yaml (1)
106-111: ✅ DevPreviewNoUpgrade payload CRD is properly configured.The validation rules are correctly implemented with global wildcard rejection and regex validation both in place. The status field is properly optional, and the singleton enforcement is correct.
config/v1alpha1/zz_generated.crd-manifests/0000_10_config-operator_01_criocredentialproviderconfigs-TechPreviewNoUpgrade.crd.yaml (1)
106-111: ✅ TechPreviewNoUpgrade variant has correct validation implementation.The validation rules properly reject global wildcards and enforce lowercase domains with optional port/path constraints. The regex pattern is correctly structured with proper escaping for literal dot and asterisk matching.
config/v1alpha1/zz_generated.deepcopy.go (1)
195-302: Auto-generated deepcopy functions look correct.The generated deepcopy methods for
CRIOCredentialProviderConfigand related types follow the established patterns in this file. The pointer handling forStatus, slice copying forMatchImages(usingcopy()for the string alias), and the loop-basedDeepCopyIntoforConditionsare all consistent with similar types likeClusterImagePolicyandBackup.payload-manifests/crds/0000_10_config-operator_01_criocredentialproviderconfigs-TechPreviewNoUpgrade.crd.yaml (3)
119-194: Status schema is consistent with the optional-but-meaningful pattern.The schema correctly implements: status can be omitted (optional in Go type), but when present must have at least one property (
minProperties: 1), andconditionswhen present must have at least one item (minItems: 1). Sinceconditionsis the only field in status, this ensuresstatus: {}is rejected while allowing the controller to omit status entirely until it has something meaningful to report.This addresses the concern from the past review about the apparent inconsistency between optional status and required conditions—the
minPropertiesconstraint makes the semantics clear.
106-111: CEL validation rules correctly enforce constraints.The two-rule approach is good: first rejecting bare
*explicitly, then validating the pattern format. This makes error messages clearer for users who try to use a global wildcard.
199-202: Singleton enforcement via CEL validation is correctly implemented.The validation rule ensuring
.metadata.name == 'cluster'properly enforces the singleton pattern documented in the type description.config/v1alpha1/types_crio_credential_provider_config.go (5)
1-43: Well-documented type definition with proper singleton enforcement.The type definition is comprehensive with clear documentation about the singleton pattern, kubelet restart behavior, and compatibility level. The CEL validation for singleton naming and the feature gate annotation are correctly applied.
45-86: Spec definition with thorough documentation for matchImages patterns.The documentation clearly explains the wildcard rules, matching semantics, and provides helpful examples. The validation markers (
MaxItems=50,MinItems=1,listType=set) correctly enforce the constraints described in the documentation.
88-104: MatchImage type with robust validation rules.The dual CEL validation approach (explicit
self != '*'check plus regex pattern) provides clear error messages. The regex correctly enforces lowercase-only FQDNs with optional wildcards, ports, and paths as documented.
106-123: Status struct correctly implements min-properties constraint.The
MinProperties=1marker ensures that if status is present, it must contain meaningful data. Combined withMinItems=1on Conditions, this prevents persisting emptystatus: {}which has no semantic meaning.
141-176: Condition constants are well-documented.The constants for condition types and reasons provide clear semantic meaning for the API consumers. The documentation explains when each condition/reason applies, which will help both controller implementers and users interpreting the status.
QiWang19
force-pushed
the
creds-provider
branch
from
1ab2c1d to
51cf963
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
config/v1alpha1/types_crio_credential_provider_config.go (1)
88-104: MatchImage validation regex still disallows.in path segments, which is too restrictive.The
MatchImageCEL rule uses a path pattern of(/[-a-z0-9_/]*)?$, which forbids.in the path portion. This prevents expressing common valid image prefixes such asregistry.io/project.v2/pathor other repository paths containing.in segments, even though the documentation does not state that.is invalid for paths and typical OCI-style repository names allow it.Consider relaxing the path character class to include
.so valid repository prefixes are not rejected by schema validation. For example:-// +kubebuilder:validation:XValidation:rule=`self.matches('^((\\*|[a-z0-9]([a-z0-9-]*[a-z0-9])?)(\\.(\\*|[a-z0-9]([a-z0-9-]*[a-z0-9])?))*)(:[0-9]+)?(/[-a-z0-9_/]*)?$')`,message="invalid matchImages value, must be a valid fully qualified domain name in lowercase with optional wildcard, port, and path" +// +kubebuilder:validation:XValidation:rule=`self.matches('^((\\*|[a-z0-9]([a-z0-9-]*[a-z0-9])?)(\\.(\\*|[a-z0-9]([a-z0-9-]*[a-z0-9])?))*)(:[0-9]+)?(/[-a-z0-9._/]*)?$')`,message="invalid matchImages value, must be a valid fully qualified domain name in lowercase with optional wildcard, port, and path"After updating the regex here, regenerate the CRDs and OpenAPI artifacts so all manifests pick up the relaxed path validation.
openapi/openapi.json (2)
11792-11800: Missing array cardinality constraints on matchImages (duplicate issue, still unresolved).This concern was previously flagged and remains unaddressed. The
matchImagesarray schema lacksminItems: 1andmaxItems: 50constraints despite the PR requirements specifying "1–50 unique strings" for this field.Apply this fix to enforce the documented bounds:
"matchImages": { "description": "...", "type": "array", + "minItems": 1, + "maxItems": 50, "items": { "type": "string", "default": "" }, "x-kubernetes-list-type": "set" }Ensure the Go struct in
config/v1alpha1/types_crio_credential_provider_config.goincludes+kubebuilder:validation:MinItems=1and+kubebuilder:validation:MaxItems=50markers on theMatchImagesfield so code generation produces these constraints.
11807-11818: Missing maxItems constraint on conditions array (duplicate issue, still unresolved).The
conditionsarray schema lacks themaxItems: 4constraint despite PR requirements specifying "up to 4" for this field.Apply this fix to enforce the documented limit:
"conditions": { "description": "conditions represent the latest available observations of the configuration state", "type": "array", + "maxItems": 4, "items": { "default": {}, "$ref": "#/definitions/io.k8s.apimachinery.pkg.apis.meta.v1.Condition" }, "x-kubernetes-list-map-keys": [ "type" ], "x-kubernetes-list-type": "map" }Ensure the Go struct includes the
+kubebuilder:validation:MaxItems=4marker on theConditionsfield.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (24)
config/v1alpha1/register.go(1 hunks)config/v1alpha1/tests/criocredentialproviderconfigs.config.openshift.io/CRIOCredentialProviderConfig.yaml(1 hunks)config/v1alpha1/types_crio_credential_provider_config.go(1 hunks)config/v1alpha1/zz_generated.crd-manifests/0000_10_config-operator_01_criocredentialproviderconfigs-CustomNoUpgrade.crd.yaml(1 hunks)config/v1alpha1/zz_generated.crd-manifests/0000_10_config-operator_01_criocredentialproviderconfigs-DevPreviewNoUpgrade.crd.yaml(1 hunks)config/v1alpha1/zz_generated.crd-manifests/0000_10_config-operator_01_criocredentialproviderconfigs-TechPreviewNoUpgrade.crd.yaml(1 hunks)config/v1alpha1/zz_generated.deepcopy.go(1 hunks)config/v1alpha1/zz_generated.featuregated-crd-manifests.yaml(1 hunks)config/v1alpha1/zz_generated.featuregated-crd-manifests/criocredentialproviderconfigs.config.openshift.io/CRIOCredentialProviderConfig.yaml(1 hunks)config/v1alpha1/zz_generated.swagger_doc_generated.go(1 hunks)features.md(1 hunks)features/features.go(1 hunks)hack/update-payload-crds.sh(1 hunks)openapi/generated_openapi/zz_generated.openapi.go(2 hunks)openapi/openapi.json(1 hunks)payload-manifests/crds/0000_10_config-operator_01_criocredentialproviderconfigs-CustomNoUpgrade.crd.yaml(1 hunks)payload-manifests/crds/0000_10_config-operator_01_criocredentialproviderconfigs-DevPreviewNoUpgrade.crd.yaml(1 hunks)payload-manifests/crds/0000_10_config-operator_01_criocredentialproviderconfigs-TechPreviewNoUpgrade.crd.yaml(1 hunks)payload-manifests/featuregates/featureGate-Hypershift-Default.yaml(1 hunks)payload-manifests/featuregates/featureGate-Hypershift-DevPreviewNoUpgrade.yaml(1 hunks)payload-manifests/featuregates/featureGate-Hypershift-TechPreviewNoUpgrade.yaml(1 hunks)payload-manifests/featuregates/featureGate-SelfManagedHA-Default.yaml(1 hunks)payload-manifests/featuregates/featureGate-SelfManagedHA-DevPreviewNoUpgrade.yaml(1 hunks)payload-manifests/featuregates/featureGate-SelfManagedHA-TechPreviewNoUpgrade.yaml(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (10)
- config/v1alpha1/tests/criocredentialproviderconfigs.config.openshift.io/CRIOCredentialProviderConfig.yaml
- config/v1alpha1/zz_generated.deepcopy.go
- features.md
- payload-manifests/featuregates/featureGate-SelfManagedHA-Default.yaml
- payload-manifests/featuregates/featureGate-SelfManagedHA-DevPreviewNoUpgrade.yaml
- payload-manifests/featuregates/featureGate-Hypershift-Default.yaml
- payload-manifests/featuregates/featureGate-SelfManagedHA-TechPreviewNoUpgrade.yaml
- hack/update-payload-crds.sh
- payload-manifests/featuregates/featureGate-Hypershift-TechPreviewNoUpgrade.yaml
- config/v1alpha1/zz_generated.featuregated-crd-manifests.yaml
🧰 Additional context used
📓 Path-based instructions (1)
**
⚙️ CodeRabbit configuration file
-Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity.
Files:
config/v1alpha1/register.gofeatures/features.goconfig/v1alpha1/zz_generated.swagger_doc_generated.goopenapi/generated_openapi/zz_generated.openapi.gopayload-manifests/crds/0000_10_config-operator_01_criocredentialproviderconfigs-CustomNoUpgrade.crd.yamlpayload-manifests/crds/0000_10_config-operator_01_criocredentialproviderconfigs-DevPreviewNoUpgrade.crd.yamlconfig/v1alpha1/types_crio_credential_provider_config.gopayload-manifests/crds/0000_10_config-operator_01_criocredentialproviderconfigs-TechPreviewNoUpgrade.crd.yamlconfig/v1alpha1/zz_generated.crd-manifests/0000_10_config-operator_01_criocredentialproviderconfigs-DevPreviewNoUpgrade.crd.yamlpayload-manifests/featuregates/featureGate-Hypershift-DevPreviewNoUpgrade.yamlconfig/v1alpha1/zz_generated.crd-manifests/0000_10_config-operator_01_criocredentialproviderconfigs-TechPreviewNoUpgrade.crd.yamlconfig/v1alpha1/zz_generated.crd-manifests/0000_10_config-operator_01_criocredentialproviderconfigs-CustomNoUpgrade.crd.yamlconfig/v1alpha1/zz_generated.featuregated-crd-manifests/criocredentialproviderconfigs.config.openshift.io/CRIOCredentialProviderConfig.yamlopenapi/openapi.json
🧬 Code graph analysis (3)
config/v1alpha1/register.go (2)
config/v1alpha1/types_crio_credential_provider_config.go (2)
CRIOCredentialProviderConfig(25-43)CRIOCredentialProviderConfigList(131-139)config/v1alpha1/zz_generated.swagger_doc_generated.go (2)
CRIOCredentialProviderConfig(236-238)CRIOCredentialProviderConfigList(245-247)
features/features.go (1)
config/v1/types_feature.go (2)
DevPreviewNoUpgrade(49-49)TechPreviewNoUpgrade(45-45)
config/v1alpha1/zz_generated.swagger_doc_generated.go (1)
config/v1alpha1/types_crio_credential_provider_config.go (4)
CRIOCredentialProviderConfig(25-43)CRIOCredentialProviderConfigList(131-139)CRIOCredentialProviderConfigSpec(46-86)CRIOCredentialProviderConfigStatus(109-123)
🔇 Additional comments (16)
features/features.go (1)
134-140: LGTM! Feature gate declaration is well-structured.The feature gate follows the established pattern and correctly enables the CRIOCredentialProviderConfig API only in DevPreview and TechPreview feature sets, which is appropriate for a v1alpha1 API. The enhancement PR reference and ownership information are properly documented.
config/v1alpha1/zz_generated.crd-manifests/0000_10_config-operator_01_criocredentialproviderconfigs-TechPreviewNoUpgrade.crd.yaml (1)
1-206: LGTM! CRD manifest is well-defined.The CRD correctly implements:
- Singleton enforcement via CEL validation (line 202)
- Dual validation for matchImages: rejecting global wildcard
'*'and enforcing lowercase domain format- Optional status with map-keyed conditions
- Proper subresource configuration
Based on past review comments, previous issues with regex escaping and status requirements have been addressed.
payload-manifests/featuregates/featureGate-Hypershift-DevPreviewNoUpgrade.yaml (1)
118-120: LGTM! Feature gate entry is correctly positioned.The CRIOCredentialProviderConfig feature gate entry is properly formatted and alphabetically positioned in the enabled list for the DevPreviewNoUpgrade feature set.
config/v1alpha1/register.go (1)
43-44: LGTM! API type registration is correct.Both CRIOCredentialProviderConfig and CRIOCredentialProviderConfigList are properly registered following the established pattern for v1alpha1 API types.
config/v1alpha1/zz_generated.featuregated-crd-manifests/criocredentialproviderconfigs.config.openshift.io/CRIOCredentialProviderConfig.yaml (1)
1-206: LGTM! Feature-gated CRD manifest is consistent.This CRD variant maintains consistency with the other feature-set-specific manifests, implementing the same validation rules and structure. The singleton enforcement, dual matchImages validation, and optional status design are all correctly configured.
config/v1alpha1/zz_generated.crd-manifests/0000_10_config-operator_01_criocredentialproviderconfigs-DevPreviewNoUpgrade.crd.yaml (1)
1-206: LGTM! DevPreviewNoUpgrade CRD variant is consistent.This CRD manifest maintains structural and validation consistency with the TechPreviewNoUpgrade and other variants, ensuring uniform behavior across different feature sets.
config/v1alpha1/zz_generated.swagger_doc_generated.go (1)
229-265: LGTM! Swagger documentation is comprehensive.The Swagger documentation additions for CRIOCredentialProviderConfig types follow the established pattern and provide complete coverage of the new API surface. The documentation strings accurately reflect the CRD schema and type definitions.
payload-manifests/crds/0000_10_config-operator_01_criocredentialproviderconfigs-CustomNoUpgrade.crd.yaml (1)
107-111: LGTM! Validation rules are now consistent across all variants.Based on past review comments, the missing
self != '*'validation rule has been added. The CustomNoUpgrade CRD now correctly enforces both the global wildcard rejection and the regex pattern validation, maintaining consistency with the TechPreview and DevPreview variants.payload-manifests/crds/0000_10_config-operator_01_criocredentialproviderconfigs-DevPreviewNoUpgrade.crd.yaml (1)
1-206: DevPreviewNoUpgrade CRD schema and singleton validation look consistent.Cluster scope, the
metadata.name == 'cluster'singleton rule,spec.matchImagesstructure/constraints, andstatus.conditionsmap semantics are all wired correctly and align with the documented API behavior.payload-manifests/crds/0000_10_config-operator_01_criocredentialproviderconfigs-TechPreviewNoUpgrade.crd.yaml (1)
1-206: TechPreviewNoUpgrade CRD wiring matches the intended API and DevPreview variant.Names, scope, singleton CEL,
spec.matchImagesvalidations, andstatussubresource/conditions configuration are consistent with the v1alpha1 type and other CRD variants.config/v1alpha1/zz_generated.crd-manifests/0000_10_config-operator_01_criocredentialproviderconfigs-CustomNoUpgrade.crd.yaml (1)
1-206: Generated CustomNoUpgrade CRD appears in sync with the Go type and other variants.The OpenAPI schema, singleton
metadata.name == 'cluster'validation,spec.matchImagesstructure, andstatus.conditionsmap semantics all match the intended API surface and other gate variants.config/v1alpha1/types_crio_credential_provider_config.go (1)
9-43: Overall CRIOCredentialProviderConfig type shape and singleton semantics look good.The root type, singleton name validation, required
Spec, optionalStatuswithMinProperties=1, and the condition constants provide a clear and maintainable API surface for controllers.openapi/generated_openapi/zz_generated.openapi.go (4)
431-434: LGTM—Type registration follows established patterns.The four new CRIOCredentialProviderConfig types are correctly registered in the OpenAPI definitions map.
21535-21584: LGTM—Resource schema structure is correct.The main resource schema correctly requires
specwhile keepingstatusoptional, following Kubernetes API conventions.
21586-21634: LGTM—List schema follows Kubernetes conventions.The list resource schema is correctly structured with standard required fields.
21636-21668: Add missing validation constraints to matchImages schema.The
matchImagesfield schema (lines 21643-21661) lacksMinItemsandMaxItemsconstraints that are documented in the description ("must contain between 1 and 50 entries"). Since this is generated code, verify that the source type includes the required kubebuilder validation markers:// +kubebuilder:validation:MinItems=1 // +kubebuilder:validation:MaxItems=50 MatchImages []string `json:"matchImages"`If missing, add them to
config/v1alpha1/types_crio_credential_provider_config.goand regenerate withmake update-codegen-crds.
| // For more details, see: | ||
| // - https://kubernetes.io/docs/tasks/administer-cluster/kubelet-credential-provider/ | ||
| // - https://github.com/cri-o/crio-credential-provider#architecture |
There was a problem hiding this comment.
Can you explain in the godoc that this behaviour may happen? And the relationship between the existing providers and this provider
Something like
Depending on the platform, the CRI-O credential provider may be installed alongside an existing platform specific provider.
Conflicts between the existing platform specific provider image match configuration and this list will be handled by....
51cf963 to
cb5ad65
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
openapi/generated_openapi/zz_generated.openapi.go (1)
21535-21705: Align OpenAPI array constraints with documented limits formatchImagesandconditionsThe descriptive comments for these fields define hard limits that are not enforced in the generated schema:
- Line 21650+ (
matchImages): description says “must contain between 1 and 50 entries”, but the schema only setsType: arrayandItemswithx-kubernetes-list-type: set—there is noMinItemsorMaxItems. This allows empty or arbitrarily large lists, violating the documented contract and potentially increasing memory/validation cost.- Line 21687+ (
conditions): description says “The maximum number of conditions is 16”, but the schema again has noMaxItems. The list-map extensions are present, but there is no upper bound on length, contradicting the docs.Since this file is generated, the fix should be applied to the source types in
config/v1alpha1/types_crio_credential_provider_config.go, for example:// For Spec.matchImages: // // +kubebuilder:validation:MinItems=1 // +kubebuilder:validation:MaxItems=50 // +kubebuilder:validation:MaxLength=512 // optional but recommended if you want to enforce per-entry length as described MatchImages []string `json:"matchImages"` // For Status.Conditions: // // +kubebuilder:validation:MaxItems=16 Conditions []metav1.Condition `json:"conditions,omitempty"`Then regenerate OpenAPI/CRDs (e.g.
make update-codegen-crds) soMinItems/MaxItems(and optionallyMaxLength) appear in this schema and the API server enforces the documented limits.openapi/openapi.json (1)
11792-11800: Missing array bounds constraints (duplicate of prior review).This OpenAPI schema is auto-generated from Go type definitions. The previous review flagged that
matchImagesis missingminItems: 1andmaxItems: 50, andconditionsis missingmaxItems: 4, despite PR requirements specifying these bounds.Since this is a generated artifact, the fix must be applied to the source Go struct (
config/v1alpha1/types_crio_credential_provider_config.go) by adding+kubebuilder:validation:MinItems=1and+kubebuilder:validation:MaxItems=50markers to thematchImagesfield, and+kubebuilder:validation:MaxItems=4to theconditionsfield. Running code generation will then update this file correctly.Also applies to: 11807-11818
🧹 Nitpick comments (2)
config/v1alpha1/zz_generated.featuregated-crd-manifests/criocredentialproviderconfigs.config.openshift.io/CRIOCredentialProviderConfig.yaml (1)
56-70: Minor: fix incomplete sentence in matchImages documentation.Line 65 reads: "credentials from the CRIOCredentialProviderConfig in the." This sentence is incomplete and should be corrected for clarity.
Suggest completing the sentence; for example: "...credentials from the CRIOCredentialProviderConfig in the cluster take lower precedence."
payload-manifests/crds/0000_10_config-operator_01_criocredentialproviderconfigs-CustomNoUpgrade.crd.yaml (1)
56-70: Minor: same incomplete sentence in matchImages documentation.Line 65 has the same incomplete text as the featuregated variant: "...from the CRIOCredentialProviderConfig in the." Please apply the same fix here.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (24)
config/v1alpha1/register.go(1 hunks)config/v1alpha1/tests/criocredentialproviderconfigs.config.openshift.io/CRIOCredentialProviderConfig.yaml(1 hunks)config/v1alpha1/types_crio_credential_provider_config.go(1 hunks)config/v1alpha1/zz_generated.crd-manifests/0000_10_config-operator_01_criocredentialproviderconfigs-CustomNoUpgrade.crd.yaml(1 hunks)config/v1alpha1/zz_generated.crd-manifests/0000_10_config-operator_01_criocredentialproviderconfigs-DevPreviewNoUpgrade.crd.yaml(1 hunks)config/v1alpha1/zz_generated.crd-manifests/0000_10_config-operator_01_criocredentialproviderconfigs-TechPreviewNoUpgrade.crd.yaml(1 hunks)config/v1alpha1/zz_generated.deepcopy.go(1 hunks)config/v1alpha1/zz_generated.featuregated-crd-manifests.yaml(1 hunks)config/v1alpha1/zz_generated.featuregated-crd-manifests/criocredentialproviderconfigs.config.openshift.io/CRIOCredentialProviderConfig.yaml(1 hunks)config/v1alpha1/zz_generated.swagger_doc_generated.go(1 hunks)features.md(1 hunks)features/features.go(1 hunks)hack/update-payload-crds.sh(1 hunks)openapi/generated_openapi/zz_generated.openapi.go(2 hunks)openapi/openapi.json(1 hunks)payload-manifests/crds/0000_10_config-operator_01_criocredentialproviderconfigs-CustomNoUpgrade.crd.yaml(1 hunks)payload-manifests/crds/0000_10_config-operator_01_criocredentialproviderconfigs-DevPreviewNoUpgrade.crd.yaml(1 hunks)payload-manifests/crds/0000_10_config-operator_01_criocredentialproviderconfigs-TechPreviewNoUpgrade.crd.yaml(1 hunks)payload-manifests/featuregates/featureGate-Hypershift-Default.yaml(1 hunks)payload-manifests/featuregates/featureGate-Hypershift-DevPreviewNoUpgrade.yaml(1 hunks)payload-manifests/featuregates/featureGate-Hypershift-TechPreviewNoUpgrade.yaml(1 hunks)payload-manifests/featuregates/featureGate-SelfManagedHA-Default.yaml(1 hunks)payload-manifests/featuregates/featureGate-SelfManagedHA-DevPreviewNoUpgrade.yaml(1 hunks)payload-manifests/featuregates/featureGate-SelfManagedHA-TechPreviewNoUpgrade.yaml(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (6)
- payload-manifests/featuregates/featureGate-SelfManagedHA-TechPreviewNoUpgrade.yaml
- hack/update-payload-crds.sh
- config/v1alpha1/register.go
- payload-manifests/featuregates/featureGate-SelfManagedHA-Default.yaml
- features/features.go
- payload-manifests/featuregates/featureGate-Hypershift-Default.yaml
🧰 Additional context used
📓 Path-based instructions (1)
**
⚙️ CodeRabbit configuration file
-Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity.
Files:
payload-manifests/featuregates/featureGate-Hypershift-TechPreviewNoUpgrade.yamlconfig/v1alpha1/zz_generated.featuregated-crd-manifests/criocredentialproviderconfigs.config.openshift.io/CRIOCredentialProviderConfig.yamlpayload-manifests/crds/0000_10_config-operator_01_criocredentialproviderconfigs-TechPreviewNoUpgrade.crd.yamlconfig/v1alpha1/zz_generated.crd-manifests/0000_10_config-operator_01_criocredentialproviderconfigs-DevPreviewNoUpgrade.crd.yamlconfig/v1alpha1/zz_generated.swagger_doc_generated.goconfig/v1alpha1/zz_generated.crd-manifests/0000_10_config-operator_01_criocredentialproviderconfigs-TechPreviewNoUpgrade.crd.yamlpayload-manifests/crds/0000_10_config-operator_01_criocredentialproviderconfigs-DevPreviewNoUpgrade.crd.yamlconfig/v1alpha1/zz_generated.crd-manifests/0000_10_config-operator_01_criocredentialproviderconfigs-CustomNoUpgrade.crd.yamlconfig/v1alpha1/types_crio_credential_provider_config.goconfig/v1alpha1/tests/criocredentialproviderconfigs.config.openshift.io/CRIOCredentialProviderConfig.yamlconfig/v1alpha1/zz_generated.featuregated-crd-manifests.yamlopenapi/generated_openapi/zz_generated.openapi.gopayload-manifests/featuregates/featureGate-SelfManagedHA-DevPreviewNoUpgrade.yamlconfig/v1alpha1/zz_generated.deepcopy.gopayload-manifests/featuregates/featureGate-Hypershift-DevPreviewNoUpgrade.yamlfeatures.mdpayload-manifests/crds/0000_10_config-operator_01_criocredentialproviderconfigs-CustomNoUpgrade.crd.yamlopenapi/openapi.json
🧬 Code graph analysis (3)
config/v1alpha1/zz_generated.swagger_doc_generated.go (1)
config/v1alpha1/types_crio_credential_provider_config.go (4)
CRIOCredentialProviderConfig(25-43)CRIOCredentialProviderConfigList(139-147)CRIOCredentialProviderConfigSpec(46-94)CRIOCredentialProviderConfigStatus(117-131)
config/v1alpha1/types_crio_credential_provider_config.go (1)
config/v1alpha1/zz_generated.swagger_doc_generated.go (3)
CRIOCredentialProviderConfigSpec(254-256)CRIOCredentialProviderConfigStatus(263-265)CRIOCredentialProviderConfig(236-238)
config/v1alpha1/zz_generated.deepcopy.go (1)
config/v1alpha1/types_crio_credential_provider_config.go (5)
CRIOCredentialProviderConfig(25-43)CRIOCredentialProviderConfigList(139-147)CRIOCredentialProviderConfigSpec(46-94)MatchImage(112-112)CRIOCredentialProviderConfigStatus(117-131)
🔇 Additional comments (15)
config/v1alpha1/zz_generated.crd-manifests/0000_10_config-operator_01_criocredentialproviderconfigs-DevPreviewNoUpgrade.crd.yaml (1)
1-214: Well-structured CRD definition with proper validation and singleton enforcement.The manifest is comprehensive and correct:
- Dual validation (CEL wildcard check + regex pattern) prevents edge cases
- Singleton constraint (metadata.name == 'cluster') is properly enforced at line 207-210
- Status subresource enabled with appropriate conditions cardinality (1–16 items)
- Detailed matchImages documentation clearly explains scoping and matching semantics
payload-manifests/crds/0000_10_config-operator_01_criocredentialproviderconfigs-CustomNoUpgrade.crd.yaml (1)
106-112: Global wildcard validation is now properly included.The
self != '*'CEL rule at lines 107–108 correctly prevents bare wildcard matchImages, consistent with validation rules in other feature-set variants.payload-manifests/featuregates/featureGate-SelfManagedHA-DevPreviewNoUpgrade.yaml (1)
103-105: Feature gate entry correctly positioned and formatted.CRIOCredentialProviderConfig is properly inserted in alphabetical order between CRDCompatibilityRequirementOperator and ClientsPreferCBOR.
features.md (1)
36-36: Feature gate table entry is accurate and well-positioned.CRIOCredentialProviderConfig is correctly documented as enabled across all preview tiers (DevPreviewNoUpgrade and TechPreviewNoUpgrade) for both Hypershift and SelfManagedHA, with proper formatting and alphabetical ordering.
config/v1alpha1/zz_generated.featuregated-crd-manifests.yaml (1)
24-45: CRD manifest index entry is complete and properly structured.The criocredentialproviderconfigs.config.openshift.io entry includes all required metadata and is correctly positioned in alphabetical order.
payload-manifests/featuregates/featureGate-Hypershift-TechPreviewNoUpgrade.yaml (1)
133-135: Feature gate entry is correctly integrated.CRIOCredentialProviderConfig is properly positioned in alphabetical order and consistently enabled across TechPreviewNoUpgrade variants.
payload-manifests/featuregates/featureGate-Hypershift-DevPreviewNoUpgrade.yaml (1)
118-120: Feature gate entry is correctly positioned.CRIOCredentialProviderConfig is properly inserted in alphabetical order and consistently enabled across all preview tier manifests.
config/v1alpha1/tests/criocredentialproviderconfigs.config.openshift.io/CRIOCredentialProviderConfig.yaml (1)
6-92: Test coverage formatchImagespatterns looks solidThe onCreate tests exercise the key happy path and the main invalid cases (non-DNS characters, uppercase host/path, wildcard-in-path, partial-label wildcard, and global
*), and the expected error messages match the CRD validation text. This should give good confidence that the schema and the controller agree on semantics.config/v1alpha1/zz_generated.swagger_doc_generated.go (1)
229-265: Swagger docs correctly wired for new CRIOCredentialProviderConfig APIThe added SwaggerDoc maps for
CRIOCredentialProviderConfig, its list, spec, and status mirror the struct fields and comments fromtypes_crio_credential_provider_config.go, so OpenAPI/Swagger consumers will see a consistent description of the new API surface.config/v1alpha1/zz_generated.crd-manifests/0000_10_config-operator_01_criocredentialproviderconfigs-TechPreviewNoUpgrade.crd.yaml (1)
11-213: CRD schema for TechPreviewNoUpgrade is consistent with the API designThe CRD correctly models
CRIOCredentialProviderConfigas a cluster-scoped singleton (metadata.name == 'cluster'), enforcesspec.matchImagesas a 1–50 element set with per-item length and wildcard/FQDN validation, and wiresstatus.conditionsto the standardmetav1.Conditionshape. This matches the v1alpha1 type definitions and the test expectations for valid/invalidmatchImagesvalues.payload-manifests/crds/0000_10_config-operator_01_criocredentialproviderconfigs-TechPreviewNoUpgrade.crd.yaml (1)
11-205: TechPreview payload CRD matches the intended validation semanticsThe TechPreviewNoUpgrade payload CRD enforces the same singleton constraint,
matchImageslist shape, and lowercase/wildcard validation rules as the generated schema, so clusters using this feature-set will see consistent behavior for CRIOCredentialProviderConfig.payload-manifests/crds/0000_10_config-operator_01_criocredentialproviderconfigs-DevPreviewNoUpgrade.crd.yaml (1)
11-205: DevPreview payload CRD is consistent with TechPreview semanticsThis DevPreviewNoUpgrade CRD mirrors the TechPreview variant for
CRIOCredentialProviderConfig(singleton name,matchImagesconstraints, CEL validations, and status layout), which keeps behavior consistent across preview feature-sets.config/v1alpha1/zz_generated.crd-manifests/0000_10_config-operator_01_criocredentialproviderconfigs-CustomNoUpgrade.crd.yaml (1)
127-203: The file under review is consistent; payload-manifests have a different maxItems valueThe CustomNoUpgrade CRD in
config/v1alpha1/zz_generated.crd-manifests/correctly specifiesmaxItems: 16forstatus.conditions, matching the Go type definition (which documents a maximum of 16 conditions). All three variants in that directory (CustomNoUpgrade, DevPreviewNoUpgrade, TechPreviewNoUpgrade) consistently usemaxItems: 16.However, the corresponding CRDs in
payload-manifests/crds/specifymaxItems: 4for all three variants, which diverges from the Go type's documented limit. If these manifests are used in a deployment context, consider syncing them to usemaxItems: 16to align with the authoritative type definition and theconfig/manifests.openapi/generated_openapi/zz_generated.openapi.go (1)
431-434: New CRIOCredentialProviderConfig schemas correctly registered in OpenAPI mapThe four new CRIOCredentialProviderConfig-related schema functions are wired into
GetOpenAPIDefinitionsconsistently with existing config/v1alpha1 types. No issues here.config/v1alpha1/zz_generated.deepcopy.go (1)
195-298: DeepCopy implementations for CRIOCredentialProviderConfig family look consistentThe autogenerated DeepCopy/DeepCopyInto/DeepCopyObject implementations for:
CRIOCredentialProviderConfigCRIOCredentialProviderConfigListCRIOCredentialProviderConfigSpecCRIOCredentialProviderConfigStatusmatch existing patterns in this file (copy
TypeMeta/ObjectMeta/ListMeta, deep‑copy slices includingMatchImagesandConditions). I don’t see correctness or maintainability concerns here.
|
/test verify |
QiWang19
force-pushed
the
creds-provider
branch
from
cb5ad65 to
33614f2
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
openapi/openapi.json (1)
11792-11800: Missing array bounds validation on matchImages and conditions (duplicate issue).The previous review already flagged that the OpenAPI schema lacks
minItems/maxItemsconstraints. This issue persists in the current code:
- matchImages (lines 11792–11800): Missing
minItems: 1andmaxItems: 50- conditions (lines 11807–11818): Missing
maxItemsconstraintAdditionally, there is a discrepancy to clarify: the past review comment specified
maxItems: 4for conditions, but the AI summary states "up to 16 items." Confirm which bound is intended and apply the correct value.Ensure the Go struct in
config/v1alpha1/types_crio_credential_provider_config.goincludes the corresponding+kubebuilder:validation:MinItemsand+kubebuilder:validation:MaxItemsmarkers so codegen produces the updated OpenAPI schema.Also applies to: 11807-11818
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (24)
config/v1alpha1/register.go(1 hunks)config/v1alpha1/tests/criocredentialproviderconfigs.config.openshift.io/CRIOCredentialProviderConfig.yaml(1 hunks)config/v1alpha1/types_crio_credential_provider_config.go(1 hunks)config/v1alpha1/zz_generated.crd-manifests/0000_10_config-operator_01_criocredentialproviderconfigs-CustomNoUpgrade.crd.yaml(1 hunks)config/v1alpha1/zz_generated.crd-manifests/0000_10_config-operator_01_criocredentialproviderconfigs-DevPreviewNoUpgrade.crd.yaml(1 hunks)config/v1alpha1/zz_generated.crd-manifests/0000_10_config-operator_01_criocredentialproviderconfigs-TechPreviewNoUpgrade.crd.yaml(1 hunks)config/v1alpha1/zz_generated.deepcopy.go(1 hunks)config/v1alpha1/zz_generated.featuregated-crd-manifests.yaml(1 hunks)config/v1alpha1/zz_generated.featuregated-crd-manifests/criocredentialproviderconfigs.config.openshift.io/CRIOCredentialProviderConfig.yaml(1 hunks)config/v1alpha1/zz_generated.swagger_doc_generated.go(1 hunks)features.md(1 hunks)features/features.go(1 hunks)hack/update-payload-crds.sh(1 hunks)openapi/generated_openapi/zz_generated.openapi.go(2 hunks)openapi/openapi.json(1 hunks)payload-manifests/crds/0000_10_config-operator_01_criocredentialproviderconfigs-CustomNoUpgrade.crd.yaml(1 hunks)payload-manifests/crds/0000_10_config-operator_01_criocredentialproviderconfigs-DevPreviewNoUpgrade.crd.yaml(1 hunks)payload-manifests/crds/0000_10_config-operator_01_criocredentialproviderconfigs-TechPreviewNoUpgrade.crd.yaml(1 hunks)payload-manifests/featuregates/featureGate-Hypershift-Default.yaml(1 hunks)payload-manifests/featuregates/featureGate-Hypershift-DevPreviewNoUpgrade.yaml(1 hunks)payload-manifests/featuregates/featureGate-Hypershift-TechPreviewNoUpgrade.yaml(1 hunks)payload-manifests/featuregates/featureGate-SelfManagedHA-Default.yaml(1 hunks)payload-manifests/featuregates/featureGate-SelfManagedHA-DevPreviewNoUpgrade.yaml(1 hunks)payload-manifests/featuregates/featureGate-SelfManagedHA-TechPreviewNoUpgrade.yaml(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (7)
- config/v1alpha1/register.go
- payload-manifests/featuregates/featureGate-Hypershift-Default.yaml
- payload-manifests/featuregates/featureGate-SelfManagedHA-Default.yaml
- config/v1alpha1/zz_generated.featuregated-crd-manifests.yaml
- payload-manifests/featuregates/featureGate-SelfManagedHA-DevPreviewNoUpgrade.yaml
- features.md
- payload-manifests/featuregates/featureGate-Hypershift-TechPreviewNoUpgrade.yaml
🧰 Additional context used
📓 Path-based instructions (1)
**
⚙️ CodeRabbit configuration file
-Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity.
Files:
hack/update-payload-crds.shfeatures/features.goconfig/v1alpha1/zz_generated.crd-manifests/0000_10_config-operator_01_criocredentialproviderconfigs-DevPreviewNoUpgrade.crd.yamlpayload-manifests/crds/0000_10_config-operator_01_criocredentialproviderconfigs-TechPreviewNoUpgrade.crd.yamlpayload-manifests/crds/0000_10_config-operator_01_criocredentialproviderconfigs-DevPreviewNoUpgrade.crd.yamlconfig/v1alpha1/types_crio_credential_provider_config.gopayload-manifests/featuregates/featureGate-SelfManagedHA-TechPreviewNoUpgrade.yamlconfig/v1alpha1/zz_generated.swagger_doc_generated.goconfig/v1alpha1/zz_generated.crd-manifests/0000_10_config-operator_01_criocredentialproviderconfigs-TechPreviewNoUpgrade.crd.yamlconfig/v1alpha1/zz_generated.featuregated-crd-manifests/criocredentialproviderconfigs.config.openshift.io/CRIOCredentialProviderConfig.yamlconfig/v1alpha1/zz_generated.crd-manifests/0000_10_config-operator_01_criocredentialproviderconfigs-CustomNoUpgrade.crd.yamlconfig/v1alpha1/tests/criocredentialproviderconfigs.config.openshift.io/CRIOCredentialProviderConfig.yamlpayload-manifests/featuregates/featureGate-Hypershift-DevPreviewNoUpgrade.yamlpayload-manifests/crds/0000_10_config-operator_01_criocredentialproviderconfigs-CustomNoUpgrade.crd.yamlconfig/v1alpha1/zz_generated.deepcopy.goopenapi/generated_openapi/zz_generated.openapi.goopenapi/openapi.json
🧬 Code graph analysis (4)
features/features.go (1)
config/v1/types_feature.go (2)
DevPreviewNoUpgrade(49-49)TechPreviewNoUpgrade(45-45)
config/v1alpha1/types_crio_credential_provider_config.go (1)
config/v1alpha1/zz_generated.swagger_doc_generated.go (3)
CRIOCredentialProviderConfigSpec(254-256)CRIOCredentialProviderConfigStatus(263-265)CRIOCredentialProviderConfig(236-238)
config/v1alpha1/zz_generated.swagger_doc_generated.go (1)
config/v1alpha1/types_crio_credential_provider_config.go (4)
CRIOCredentialProviderConfig(25-43)CRIOCredentialProviderConfigList(139-147)CRIOCredentialProviderConfigSpec(46-94)CRIOCredentialProviderConfigStatus(117-131)
config/v1alpha1/zz_generated.deepcopy.go (2)
config/v1alpha1/types_crio_credential_provider_config.go (5)
CRIOCredentialProviderConfig(25-43)CRIOCredentialProviderConfigList(139-147)CRIOCredentialProviderConfigSpec(46-94)MatchImage(112-112)CRIOCredentialProviderConfigStatus(117-131)config/v1alpha1/zz_generated.swagger_doc_generated.go (4)
CRIOCredentialProviderConfig(236-238)CRIOCredentialProviderConfigList(245-247)CRIOCredentialProviderConfigSpec(254-256)CRIOCredentialProviderConfigStatus(263-265)
🔇 Additional comments (15)
hack/update-payload-crds.sh (1)
5-34: New CRIOCredentialProviderConfig CRD glob is wired correctly into the payload update script.The added
config/v1alpha1/...criocredentialproviderconfigs*.crd.yamlglob follows the existing patterns and ensures the new CRD is included inpayload-manifests/crds/during updates. No further changes needed.config/v1alpha1/zz_generated.crd-manifests/0000_10_config-operator_01_criocredentialproviderconfigs-TechPreviewNoUpgrade.crd.yaml (1)
1-214: TechPreview CRD schema and validations for CRIOCredentialProviderConfig look consistent and well-scoped.Singleton enforcement,
matchImagesconstraints (cardinality, length, wildcard semantics, lowercase requirement, and explicit rejection of global"*"), plus the status/conditions modeling all line up with the documented behavior and the dedicated tests. I don't see correctness or maintainability issues here.config/v1alpha1/zz_generated.featuregated-crd-manifests/criocredentialproviderconfigs.config.openshift.io/CRIOCredentialProviderConfig.yaml (1)
1-214: Feature-gated CRD manifest correctly mirrors the base schema and ties into the new feature gate.The schema matches the other CRIOCredentialProviderConfig CRDs, and the
feature-gate.release.openshift.io/CRIOCredentialProviderConfigannotation cleanly connects it to the gate. This is consistent with existing featuregated-CRD patterns.payload-manifests/featuregates/featureGate-SelfManagedHA-TechPreviewNoUpgrade.yaml (1)
116-120: Enabling CRIOCredentialProviderConfig for SelfManagedHA TechPreview is wired consistently.The new
"CRIOCredentialProviderConfig"entry in the enabled list aligns with the feature gate name infeatures.goand the CRD annotations. This keeps SelfManagedHA TechPreview in sync with the gate definition.payload-manifests/featuregates/featureGate-Hypershift-DevPreviewNoUpgrade.yaml (1)
116-120: Hypershift DevPreview feature gate manifest correctly enables CRIOCredentialProviderConfig.The added
"CRIOCredentialProviderConfig"gate matches the registered feature name and other payload manifests, keeping Hypershift DevPreview aligned with the new API rollout.features/features.go (1)
134-140: New FeatureGateCRIOCredentialProviderConfig definition follows existing patterns and matches payload manifests.Name, Jira component, contact, scope, enhancement PR, and enablement in
DevPreviewNoUpgradeandTechPreviewNoUpgradeall look consistent with how other preview API gates are introduced and with the CRD/FeatureGate YAML usage.config/v1alpha1/tests/criocredentialproviderconfigs.config.openshift.io/CRIOCredentialProviderConfig.yaml (1)
1-92: CRIOCredentialProviderConfig tests exercise key validation paths and align with CRD messages.The onCreate tests cover the happy path plus the main invalid
matchImagescases (illegal chars, uppercase host/path, wildcard misuse, and global"*"), and theexpectedErrorstrings match the CRD validation messages. This should catch regressions in the CEL/regex rules effectively.config/v1alpha1/zz_generated.crd-manifests/0000_10_config-operator_01_criocredentialproviderconfigs-DevPreviewNoUpgrade.crd.yaml (1)
1-214: DevPreview CRD variant for CRIOCredentialProviderConfig matches the TechPreview schema as expected.Only the feature-set annotation differs; the OpenAPI schema, singleton rule,
matchImagesconstraints, and status modeling are identical. This symmetry across feature sets is desirable and aligns with the associated tests.payload-manifests/crds/0000_10_config-operator_01_criocredentialproviderconfigs-TechPreviewNoUpgrade.crd.yaml (1)
1-214: LGTM! CRD is well-formed and consistent.The TechPreviewNoUpgrade CRD manifest correctly defines the CRIOCredentialProviderConfig API with:
- Proper singleton enforcement (metadata.name must be 'cluster')
- Comprehensive validation rules for matchImages (global wildcard rejection + regex for FQDN patterns)
- Consistent status semantics (optional, but when present requires at least one condition)
- Complete OpenAPI schema definitions
The structure aligns with OpenShift API conventions for cluster-scoped singleton resources.
payload-manifests/crds/0000_10_config-operator_01_criocredentialproviderconfigs-CustomNoUpgrade.crd.yaml (1)
1-214: LGTM! CustomNoUpgrade CRD variant is consistent.This CRD manifest is properly structured and consistent with the TechPreview variant, with the appropriate feature-set annotation for CustomNoUpgrade. All validation rules and schema definitions are correctly applied.
payload-manifests/crds/0000_10_config-operator_01_criocredentialproviderconfigs-DevPreviewNoUpgrade.crd.yaml (1)
1-214: LGTM! DevPreviewNoUpgrade CRD variant is correct.The DevPreviewNoUpgrade CRD manifest maintains consistency with other feature-gate variants while correctly applying the DevPreviewNoUpgrade feature-set annotation. Schema and validation rules are properly defined.
config/v1alpha1/zz_generated.swagger_doc_generated.go (1)
229-265: LGTM! Generated Swagger documentation is correct.The auto-generated Swagger documentation properly covers all CRIOCredentialProviderConfig types (Config, ConfigList, Spec, Status) with appropriate documentation strings that align with the Go type definitions.
config/v1alpha1/zz_generated.crd-manifests/0000_10_config-operator_01_criocredentialproviderconfigs-CustomNoUpgrade.crd.yaml (1)
1-214: LGTM! Generated CRD manifest is consistent.The generated CRD manifest correctly reflects the Go type definitions with proper validation rules, schema structure, and kubebuilder markers. It's consistent with the corresponding payload-manifests CRD variant.
config/v1alpha1/types_crio_credential_provider_config.go (1)
1-184: LGTM! API types are well-defined and properly validated.The CRIOCredentialProviderConfig API types are correctly structured with:
- Singleton enforcement via CEL validation (line 24)
- Comprehensive documentation for matchImages patterns with clear wildcard rules and examples
- Proper kubebuilder validation markers (MaxLength, XValidation for both global wildcard rejection and regex pattern)
- Well-documented condition types and reasons for status reporting
- Appropriate JSON tags (omitzero/omitempty) following project linting standards
All concerns from previous reviews have been addressed. The API provides a clear contract for CRI-O credential provider configuration.
config/v1alpha1/zz_generated.deepcopy.go (1)
195-298: LGTM! Generated deepcopy methods are correct.The auto-generated deepcopy methods for CRIOCredentialProviderConfig types follow standard Kubernetes patterns:
- Proper nil checks and allocation
- Correct deep-copying of ObjectMeta, Spec fields, and Status conditions
- Appropriate DeepCopyObject implementations for runtime.Object interface
The generated code correctly handles all field types including slices and nested structs.
QiWang19
force-pushed
the
creds-provider
branch
from
33614f2 to
c34aa43
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
openapi/openapi.json (2)
11792-11800: Missing array bounds validation on matchImages (duplicate of previous review).The
matchImagesarray lacksminItems: 1andmaxItems: 50constraints despite the PR spec explicitly requiring "1–50 unique strings". These bounds should be present in the OpenAPI schema alongside the existingx-kubernetes-list-type: "set".Add cardinality constraints:
"matchImages": { "description": "...", "type": "array", + "minItems": 1, + "maxItems": 50, "items": { "type": "string", "default": "" }, "x-kubernetes-list-type": "set" }Also verify that the corresponding Go struct in
config/v1alpha1/types_crio_credential_provider_config.goincludes the+kubebuilder:validation:MinItems=1and+kubebuilder:validation:MaxItems=50markers on theMatchImagesfield so that codegen produces these OpenAPI constraints.
11807-11818: Missing maxItems constraint on conditions array (duplicate of previous review).The
conditionsarray lacks themaxItems: 4constraint specified in the PR requirements. Add this bound to enforce the cardinality limit:"conditions": { "description": "conditions represent the latest available observations of the configuration state", "type": "array", + "maxItems": 4, "items": { "default": {}, "$ref": "#/definitions/io.k8s.apimachinery.pkg.apis.meta.v1.Condition" }, "x-kubernetes-list-map-keys": [ "type" ], "x-kubernetes-list-type": "map" }Also verify that the corresponding Go struct includes the
+kubebuilder:validation:MaxItems=4marker on theConditionsfield.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (24)
config/v1alpha1/register.go(1 hunks)config/v1alpha1/tests/criocredentialproviderconfigs.config.openshift.io/CRIOCredentialProviderConfig.yaml(1 hunks)config/v1alpha1/types_crio_credential_provider_config.go(1 hunks)config/v1alpha1/zz_generated.crd-manifests/0000_10_config-operator_01_criocredentialproviderconfigs-CustomNoUpgrade.crd.yaml(1 hunks)config/v1alpha1/zz_generated.crd-manifests/0000_10_config-operator_01_criocredentialproviderconfigs-DevPreviewNoUpgrade.crd.yaml(1 hunks)config/v1alpha1/zz_generated.crd-manifests/0000_10_config-operator_01_criocredentialproviderconfigs-TechPreviewNoUpgrade.crd.yaml(1 hunks)config/v1alpha1/zz_generated.deepcopy.go(1 hunks)config/v1alpha1/zz_generated.featuregated-crd-manifests.yaml(1 hunks)config/v1alpha1/zz_generated.featuregated-crd-manifests/criocredentialproviderconfigs.config.openshift.io/CRIOCredentialProviderConfig.yaml(1 hunks)config/v1alpha1/zz_generated.swagger_doc_generated.go(1 hunks)features.md(1 hunks)features/features.go(1 hunks)hack/update-payload-crds.sh(1 hunks)openapi/generated_openapi/zz_generated.openapi.go(2 hunks)openapi/openapi.json(1 hunks)payload-manifests/crds/0000_10_config-operator_01_criocredentialproviderconfigs-CustomNoUpgrade.crd.yaml(1 hunks)payload-manifests/crds/0000_10_config-operator_01_criocredentialproviderconfigs-DevPreviewNoUpgrade.crd.yaml(1 hunks)payload-manifests/crds/0000_10_config-operator_01_criocredentialproviderconfigs-TechPreviewNoUpgrade.crd.yaml(1 hunks)payload-manifests/featuregates/featureGate-Hypershift-Default.yaml(1 hunks)payload-manifests/featuregates/featureGate-Hypershift-DevPreviewNoUpgrade.yaml(1 hunks)payload-manifests/featuregates/featureGate-Hypershift-TechPreviewNoUpgrade.yaml(1 hunks)payload-manifests/featuregates/featureGate-SelfManagedHA-Default.yaml(1 hunks)payload-manifests/featuregates/featureGate-SelfManagedHA-DevPreviewNoUpgrade.yaml(1 hunks)payload-manifests/featuregates/featureGate-SelfManagedHA-TechPreviewNoUpgrade.yaml(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (7)
- payload-manifests/featuregates/featureGate-Hypershift-Default.yaml
- features/features.go
- payload-manifests/featuregates/featureGate-Hypershift-TechPreviewNoUpgrade.yaml
- payload-manifests/featuregates/featureGate-SelfManagedHA-Default.yaml
- payload-manifests/crds/0000_10_config-operator_01_criocredentialproviderconfigs-DevPreviewNoUpgrade.crd.yaml
- features.md
- payload-manifests/featuregates/featureGate-Hypershift-DevPreviewNoUpgrade.yaml
🧰 Additional context used
📓 Path-based instructions (1)
**
⚙️ CodeRabbit configuration file
-Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity.
Files:
config/v1alpha1/register.goconfig/v1alpha1/zz_generated.featuregated-crd-manifests/criocredentialproviderconfigs.config.openshift.io/CRIOCredentialProviderConfig.yamlconfig/v1alpha1/types_crio_credential_provider_config.goconfig/v1alpha1/zz_generated.crd-manifests/0000_10_config-operator_01_criocredentialproviderconfigs-TechPreviewNoUpgrade.crd.yamlconfig/v1alpha1/zz_generated.crd-manifests/0000_10_config-operator_01_criocredentialproviderconfigs-CustomNoUpgrade.crd.yamlconfig/v1alpha1/zz_generated.featuregated-crd-manifests.yamlpayload-manifests/featuregates/featureGate-SelfManagedHA-DevPreviewNoUpgrade.yamlconfig/v1alpha1/zz_generated.crd-manifests/0000_10_config-operator_01_criocredentialproviderconfigs-DevPreviewNoUpgrade.crd.yamlpayload-manifests/crds/0000_10_config-operator_01_criocredentialproviderconfigs-TechPreviewNoUpgrade.crd.yamlconfig/v1alpha1/zz_generated.swagger_doc_generated.gopayload-manifests/crds/0000_10_config-operator_01_criocredentialproviderconfigs-CustomNoUpgrade.crd.yamlpayload-manifests/featuregates/featureGate-SelfManagedHA-TechPreviewNoUpgrade.yamlopenapi/generated_openapi/zz_generated.openapi.goconfig/v1alpha1/tests/criocredentialproviderconfigs.config.openshift.io/CRIOCredentialProviderConfig.yamlconfig/v1alpha1/zz_generated.deepcopy.gohack/update-payload-crds.shopenapi/openapi.json
🧬 Code graph analysis (4)
config/v1alpha1/register.go (2)
config/v1alpha1/types_crio_credential_provider_config.go (2)
CRIOCredentialProviderConfig(25-43)CRIOCredentialProviderConfigList(139-147)config/v1alpha1/zz_generated.swagger_doc_generated.go (2)
CRIOCredentialProviderConfig(236-238)CRIOCredentialProviderConfigList(245-247)
config/v1alpha1/types_crio_credential_provider_config.go (1)
config/v1alpha1/zz_generated.swagger_doc_generated.go (3)
CRIOCredentialProviderConfigSpec(254-256)CRIOCredentialProviderConfigStatus(263-265)CRIOCredentialProviderConfig(236-238)
config/v1alpha1/zz_generated.swagger_doc_generated.go (1)
config/v1alpha1/types_crio_credential_provider_config.go (4)
CRIOCredentialProviderConfig(25-43)CRIOCredentialProviderConfigList(139-147)CRIOCredentialProviderConfigSpec(46-94)CRIOCredentialProviderConfigStatus(117-131)
config/v1alpha1/zz_generated.deepcopy.go (1)
config/v1alpha1/types_crio_credential_provider_config.go (5)
CRIOCredentialProviderConfig(25-43)CRIOCredentialProviderConfigList(139-147)CRIOCredentialProviderConfigSpec(46-94)MatchImage(112-112)CRIOCredentialProviderConfigStatus(117-131)
🔇 Additional comments (15)
hack/update-payload-crds.sh (1)
29-29: LGTM!The CRD glob pattern addition correctly follows the established convention and will properly match all CRIOCredentialProviderConfig manifest variants.
payload-manifests/featuregates/featureGate-SelfManagedHA-DevPreviewNoUpgrade.yaml (1)
103-105: LGTM!The CRIOCredentialProviderConfig feature gate is correctly added to the enabled list in alphabetical order, consistent with the DevPreviewNoUpgrade feature set.
payload-manifests/featuregates/featureGate-SelfManagedHA-TechPreviewNoUpgrade.yaml (1)
118-120: LGTM!The CRIOCredentialProviderConfig feature gate is correctly added to the enabled list in alphabetical order, consistent with the TechPreviewNoUpgrade feature set.
config/v1alpha1/zz_generated.featuregated-crd-manifests.yaml (1)
24-46: LGTM!The CRIOCredentialProviderConfig CRD metadata entry is complete and correctly structured, with appropriate singleton cluster scope and status subresource configuration.
config/v1alpha1/register.go (1)
43-44: LGTM!The registration of CRIOCredentialProviderConfig and CRIOCredentialProviderConfigList types follows the standard Kubernetes API registration pattern and is correctly placed alphabetically.
config/v1alpha1/tests/criocredentialproviderconfigs.config.openshift.io/CRIOCredentialProviderConfig.yaml (1)
1-92: LGTM!The test suite provides comprehensive coverage with seven test cases validating both valid configurations and various error scenarios (invalid characters, uppercase, wildcard placement, global wildcard). The structure is correct and aligns with the CRD validation rules.
config/v1alpha1/zz_generated.crd-manifests/0000_10_config-operator_01_criocredentialproviderconfigs-DevPreviewNoUpgrade.crd.yaml (1)
1-214: LGTM!The CRD manifest is well-structured with appropriate validations:
- Singleton enforcement via CEL validation
- Comprehensive matchImages validation (lowercase enforcement, wildcard rules, character constraints)
- Status is correctly optional while spec.matchImages is required
- Set semantics for matchImages prevent duplicates
config/v1alpha1/zz_generated.featuregated-crd-manifests/criocredentialproviderconfigs.config.openshift.io/CRIOCredentialProviderConfig.yaml (1)
1-214: LGTM!The feature-gated CRD manifest is correctly structured and consistent with the variant CRD manifests. The validations are appropriate, status is correctly optional, and the singleton enforcement is in place.
config/v1alpha1/zz_generated.crd-manifests/0000_10_config-operator_01_criocredentialproviderconfigs-TechPreviewNoUpgrade.crd.yaml (1)
55-124: CRD schema and validations look consistent with the Go type and design
matchImagesconstraints (min/max items, set semantics, per-item max length, CEL regex, and explicitself != '*'guard) and the singleton name validation on.metadata.name == 'cluster'line up with the documented behavior. Status shape and condition map semantics are also wired correctly.Also applies to: 127-210
config/v1alpha1/zz_generated.swagger_doc_generated.go (1)
229-265: Swagger documentation wiring for CRIOCredentialProviderConfig is coherentThe added SwaggerDoc maps for
CRIOCredentialProviderConfig, its List, Spec, and Status accurately reflect the Go doc comments and clearly describe singleton semantics andmatchImagesbehavior.payload-manifests/crds/0000_10_config-operator_01_criocredentialproviderconfigs-CustomNoUpgrade.crd.yaml (1)
55-124: CustomNoUpgrade CRD matches generated schema and enforces intended invariantsThe CustomNoUpgrade CRD mirrors the generated CRD: same
matchImagesvalidation (includingself != '*'and regex) and singleton.metadata.name == 'cluster'rule, so behavior should be consistent across feature sets.Also applies to: 127-210
config/v1alpha1/zz_generated.crd-manifests/0000_10_config-operator_01_criocredentialproviderconfigs-CustomNoUpgrade.crd.yaml (1)
55-124: Generated CustomNoUpgrade CRD aligns with the type definition and other variantsThe CustomNoUpgrade CRD under
config/v1alpha1/zz_generated.crd-manifestscorrectly reflects the Go type:matchImagesconstraints, theself != '*'guard, and the singleton.metadata.name == 'cluster'constraint all match the other variants.Also applies to: 127-210
config/v1alpha1/zz_generated.deepcopy.go (1)
195-298: Deepcopy implementations for CRIOCredentialProviderConfig types are correctThe generated DeepCopy/DeepCopyInto/DeepCopyObject methods for
CRIOCredentialProviderConfig, its List, Spec, and Status all correctly deep-copy slices and metadata, matching the patterns used for existing types in this package.payload-manifests/crds/0000_10_config-operator_01_criocredentialproviderconfigs-TechPreviewNoUpgrade.crd.yaml (1)
55-124: TechPreviewNoUpgrade payload CRD is consistent with the generated CRD and typesThis payload CRD faithfully mirrors the generated TechPreview schema:
matchImagesvalidation (including the global'*'rejection and regex), singleton.metadata.name == 'cluster'validation, and the status/conditions structure all line up with the Go API.Also applies to: 127-210
openapi/generated_openapi/zz_generated.openapi.go (1)
431-434: LGTM: Schema registration is correct.The four new CRIOCredentialProviderConfig type registrations follow the established pattern and correctly reference their schema generator functions.
QiWang19
force-pushed
the
creds-provider
branch
from
c34aa43 to
07c6dc5
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
openapi/generated_openapi/zz_generated.openapi.go (1)
21636-21668: Validation constraints still missing after previous fixes.Previous reviews flagged missing array validation constraints that remain absent:
- matchImages (lines 21650-21661): Description documents "between 1 and 50 entries" but
MinItems: 1andMaxItems: 50are not present in the schema.- conditions (lines 21687-21697): Description states "maximum number of conditions is 16" but
MaxItems: 16is not present in the schema.These constraints are essential to prevent empty arrays (matchImages) and unbounded growth (conditions).
Since these issues were previously marked as addressed, verify that the source types have the correct kubebuilder validation markers and regenerate:
make update-codegen-crdsIf regeneration still omits the constraints, the OpenAPI generator may not translate kubebuilder markers for these fields, requiring manual schema updates or generator enhancement.
Also applies to: 21677-21697
openapi/openapi.json (1)
11792-11800: Missing array bounds validation (duplicate of earlier comment; still unfixed).The
matchImagesarray (lines 11792–11800) lacksminItems: 1andmaxItems: 50, and theconditionsarray (lines 11807–11818) lacksmaxItems: 4, despite PR requirements explicitly stating these bounds. This validation gap was flagged in the previous review and remains unaddressed.The earlier comment provided the exact fix; please apply it or confirm the bounds should not be enforced at the schema level.
Also applies to: 11807-11818
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (24)
config/v1alpha1/register.go(1 hunks)config/v1alpha1/tests/criocredentialproviderconfigs.config.openshift.io/CRIOCredentialProviderConfig.yaml(1 hunks)config/v1alpha1/types_crio_credential_provider_config.go(1 hunks)config/v1alpha1/zz_generated.crd-manifests/0000_10_config-operator_01_criocredentialproviderconfigs-CustomNoUpgrade.crd.yaml(1 hunks)config/v1alpha1/zz_generated.crd-manifests/0000_10_config-operator_01_criocredentialproviderconfigs-DevPreviewNoUpgrade.crd.yaml(1 hunks)config/v1alpha1/zz_generated.crd-manifests/0000_10_config-operator_01_criocredentialproviderconfigs-TechPreviewNoUpgrade.crd.yaml(1 hunks)config/v1alpha1/zz_generated.deepcopy.go(1 hunks)config/v1alpha1/zz_generated.featuregated-crd-manifests.yaml(1 hunks)config/v1alpha1/zz_generated.featuregated-crd-manifests/criocredentialproviderconfigs.config.openshift.io/CRIOCredentialProviderConfig.yaml(1 hunks)config/v1alpha1/zz_generated.swagger_doc_generated.go(1 hunks)features.md(1 hunks)features/features.go(1 hunks)hack/update-payload-crds.sh(1 hunks)openapi/generated_openapi/zz_generated.openapi.go(2 hunks)openapi/openapi.json(1 hunks)payload-manifests/crds/0000_10_config-operator_01_criocredentialproviderconfigs-CustomNoUpgrade.crd.yaml(1 hunks)payload-manifests/crds/0000_10_config-operator_01_criocredentialproviderconfigs-DevPreviewNoUpgrade.crd.yaml(1 hunks)payload-manifests/crds/0000_10_config-operator_01_criocredentialproviderconfigs-TechPreviewNoUpgrade.crd.yaml(1 hunks)payload-manifests/featuregates/featureGate-Hypershift-Default.yaml(1 hunks)payload-manifests/featuregates/featureGate-Hypershift-DevPreviewNoUpgrade.yaml(1 hunks)payload-manifests/featuregates/featureGate-Hypershift-TechPreviewNoUpgrade.yaml(1 hunks)payload-manifests/featuregates/featureGate-SelfManagedHA-Default.yaml(1 hunks)payload-manifests/featuregates/featureGate-SelfManagedHA-DevPreviewNoUpgrade.yaml(1 hunks)payload-manifests/featuregates/featureGate-SelfManagedHA-TechPreviewNoUpgrade.yaml(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (10)
- features/features.go
- features.md
- payload-manifests/featuregates/featureGate-SelfManagedHA-DevPreviewNoUpgrade.yaml
- config/v1alpha1/zz_generated.featuregated-crd-manifests.yaml
- config/v1alpha1/tests/criocredentialproviderconfigs.config.openshift.io/CRIOCredentialProviderConfig.yaml
- payload-manifests/featuregates/featureGate-Hypershift-DevPreviewNoUpgrade.yaml
- payload-manifests/featuregates/featureGate-SelfManagedHA-TechPreviewNoUpgrade.yaml
- config/v1alpha1/zz_generated.deepcopy.go
- payload-manifests/featuregates/featureGate-SelfManagedHA-Default.yaml
- payload-manifests/featuregates/featureGate-Hypershift-TechPreviewNoUpgrade.yaml
🧰 Additional context used
📓 Path-based instructions (1)
**
⚙️ CodeRabbit configuration file
-Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity.
Files:
payload-manifests/featuregates/featureGate-Hypershift-Default.yamlpayload-manifests/crds/0000_10_config-operator_01_criocredentialproviderconfigs-DevPreviewNoUpgrade.crd.yamlhack/update-payload-crds.shconfig/v1alpha1/register.goconfig/v1alpha1/zz_generated.crd-manifests/0000_10_config-operator_01_criocredentialproviderconfigs-TechPreviewNoUpgrade.crd.yamlconfig/v1alpha1/types_crio_credential_provider_config.goconfig/v1alpha1/zz_generated.swagger_doc_generated.goopenapi/generated_openapi/zz_generated.openapi.gopayload-manifests/crds/0000_10_config-operator_01_criocredentialproviderconfigs-CustomNoUpgrade.crd.yamlconfig/v1alpha1/zz_generated.featuregated-crd-manifests/criocredentialproviderconfigs.config.openshift.io/CRIOCredentialProviderConfig.yamlpayload-manifests/crds/0000_10_config-operator_01_criocredentialproviderconfigs-TechPreviewNoUpgrade.crd.yamlconfig/v1alpha1/zz_generated.crd-manifests/0000_10_config-operator_01_criocredentialproviderconfigs-CustomNoUpgrade.crd.yamlconfig/v1alpha1/zz_generated.crd-manifests/0000_10_config-operator_01_criocredentialproviderconfigs-DevPreviewNoUpgrade.crd.yamlopenapi/openapi.json
🧬 Code graph analysis (2)
config/v1alpha1/register.go (2)
config/v1alpha1/types_crio_credential_provider_config.go (2)
CRIOCredentialProviderConfig(25-43)CRIOCredentialProviderConfigList(139-147)config/v1alpha1/zz_generated.swagger_doc_generated.go (2)
CRIOCredentialProviderConfig(236-238)CRIOCredentialProviderConfigList(245-247)
config/v1alpha1/zz_generated.swagger_doc_generated.go (1)
config/v1alpha1/types_crio_credential_provider_config.go (4)
CRIOCredentialProviderConfig(25-43)CRIOCredentialProviderConfigList(139-147)CRIOCredentialProviderConfigSpec(46-94)CRIOCredentialProviderConfigStatus(117-131)
🔇 Additional comments (14)
hack/update-payload-crds.sh (1)
5-34: New CRD glob inclusion looks correctAdding the
criocredentialproviderconfigsglob underconfig/v1alpha1/zz_generated.crd-manifestsshould properly pull the generated CRD variants intopayload-manifests/crds/.payload-manifests/featuregates/featureGate-Hypershift-Default.yaml (1)
55-62: Feature gate wired into Hypershift Default disabled setThe new
CRIOCredentialProviderConfigentry is correctly added to the disabled list for the Hypershift Default payload.config/v1alpha1/register.go (1)
30-47: Scheme registration for new v1alpha1 types is correctAdding
&CRIOCredentialProviderConfig{}and&CRIOCredentialProviderConfigList{}toscheme.AddKnownTypes(...)is the right wiring for API recognition/serialization.config/v1alpha1/zz_generated.crd-manifests/0000_10_config-operator_01_criocredentialproviderconfigs-DevPreviewNoUpgrade.crd.yaml (2)
55-126: Validation + singleton enforcement look consistent with documented contractGood to see:
- explicit rejection of global
"*"viaself != '*'- set semantics on
matchImages- singleton enforcement via
self.metadata.name == 'cluster'Also applies to: 207-214
4-10: This file is auto-generated and should not be manually edited. Theapi-approved.openshift.ioannotation is sourced from theopenshift:api-approved.openshift.iomarker inconfig/v1alpha1/types_crio_credential_provider_config.go. The reference to PR #1929 is correct and consistent across all CRD variants and generated manifests. Any annotation updates must be made to the Go source marker, not to the generated YAML files.Likely an incorrect or invalid review comment.
config/v1alpha1/zz_generated.featuregated-crd-manifests/criocredentialproviderconfigs.config.openshift.io/CRIOCredentialProviderConfig.yaml (2)
6-10: Feature-gate annotation on the CRD is correctly applied
feature-gate.release.openshift.io/CRIOCredentialProviderConfig: "true"is the right mechanism for the featuregated manifest.
4-10: No action required. Theapi-approved.openshift.ioannotation is correctly set to PR #1929 and is auto-generated from the source code annotation inconfig/v1alpha1/types_crio_credential_provider_config.go. This generated file should not be manually edited; if the PR reference needs to change, it must be updated in the source Go file's annotation comment.config/v1alpha1/zz_generated.crd-manifests/0000_10_config-operator_01_criocredentialproviderconfigs-TechPreviewNoUpgrade.crd.yaml (1)
4-10: No action needed. All criocredentialproviderconfigs CRD variants (CustomNoUpgrade, DevPreviewNoUpgrade, and TechPreviewNoUpgrade) correctly reference PR #1929 as defined in the source filetypes_crio_credential_provider_config.go. The variants are properly aligned and the generated manifests correctly reflect the canonical API approval annotation.Likely an incorrect or invalid review comment.
payload-manifests/crds/0000_10_config-operator_01_criocredentialproviderconfigs-DevPreviewNoUpgrade.crd.yaml (1)
4-10: Verify whether PR #1929 is the correct API approval reference for CRIOCredentialProviderConfig. Web search could not confirm PR #1929 exists or relates to this API. All CRD variants in the repository consistently reference this PR, but the approval link should be validated against the actual PR used when this API was approved.payload-manifests/crds/0000_10_config-operator_01_criocredentialproviderconfigs-CustomNoUpgrade.crd.yaml (1)
1-214: LGTM! CRD manifest properly configured.The CustomNoUpgrade CRD manifest is well-structured with:
- Proper singleton enforcement via CEL validation (lines 207-210)
- Correct wildcard rejection rule
self != '*'(lines 115-116) addressing the previous review concern- Consistent validation schema matching the Go type definitions
- Appropriate status subresource configuration
Previous review concerns about wildcard validation have been properly addressed.
config/v1alpha1/types_crio_credential_provider_config.go (1)
1-184: LGTM! Well-structured API type definitions.The Go type definitions are comprehensive and properly configured:
- Singleton enforcement via CEL validation (line 24)
- Correct use of project-specific json tags (
omitzero/omitempty) following kubeapilinter conventions- Dual validation rules for MatchImage preventing global wildcard and enforcing pattern (lines 110-111)
- Clear documentation throughout, including detailed condition constants (lines 149-184)
- Status semantics properly aligned with validation constraints
All previous review concerns have been addressed, and the implementation follows OpenShift API conventions.
config/v1alpha1/zz_generated.crd-manifests/0000_10_config-operator_01_criocredentialproviderconfigs-CustomNoUpgrade.crd.yaml (1)
1-214: LGTM! Generated CRD manifest consistent with API types.The generated CustomNoUpgrade CRD manifest correctly reflects the Go type definitions with:
- Proper validation rules including wildcard rejection (lines 115-116)
- Singleton enforcement (lines 207-210)
- Consistent schema across all generated variants
The generation process has correctly translated the Go type annotations into OpenAPI v3 schema.
payload-manifests/crds/0000_10_config-operator_01_criocredentialproviderconfigs-TechPreviewNoUpgrade.crd.yaml (1)
1-214: LGTM! TechPreviewNoUpgrade variant properly configured.The TechPreviewNoUpgrade CRD manifest is correctly configured with:
- Appropriate feature-set annotation (line 9)
- Consistent validation schema matching other variants
- Proper singleton and wildcard validation rules
The manifest maintains consistency across feature-set variants while correctly applying the TechPreviewNoUpgrade feature gate.
openapi/generated_openapi/zz_generated.openapi.go (1)
431-434: LGTM!The registry additions correctly wire the new CRIOCredentialProviderConfig schemas into the OpenAPI definitions map. The entries follow the established naming convention and are properly ordered.
| var map_CRIOCredentialProviderConfig = map[string]string{ | ||
| "": "CRIOCredentialProviderConfig holds cluster-wide singleton resource configurations for CRI-O credential provider, the name of this instance is \"cluster\". CRI-O credential provider is a binary shipped with CRI-O that provides a way to obtain container image pull credentials from external sources. For example, it can be used to fetch mirror registry credentials from secrets resources in the cluster within the same namespace the pod will be running in. CRIOCredentialProviderConfig configuration specifies the pod image sources registries that should trigger the CRI-O credential provider execution, which will resolve the CRI-O mirror configurations and obtain the necessary credentials for pod creation. Note: Configuration changes will only take effect after the kubelet restarts, which is automatically managed by the cluster during rollout.\n\nThe resource is a singleton named \"cluster\".\n\nCompatibility 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.", | ||
| "metadata": "metadata is the standard object's metadata. More info: https://git.k8s.io/community/contributors/devel/sig-architecture/api-conventions.md#metadata", | ||
| "spec": "spec defines the desired configuration of the CRI-O Credential Provider. This field is required and must be provided when creating the resource.", | ||
| "status": "status represents the current state of the CRIOCredentialProviderConfig. When omitted or nil, it indicates that the status has not yet been set by the controller. The controller will populate this field with validation conditions and operational state.", | ||
| } | ||
|
|
||
| func (CRIOCredentialProviderConfig) SwaggerDoc() map[string]string { | ||
| return map_CRIOCredentialProviderConfig | ||
| } | ||
|
|
||
| var map_CRIOCredentialProviderConfigList = map[string]string{ | ||
| "": "CRIOCredentialProviderConfigList contains a list of CRIOCredentialProviderConfig resources\n\nCompatibility 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.", | ||
| "metadata": "metadata is the standard list's metadata. More info: https://git.k8s.io/community/contributors/devel/sig-architecture/api-conventions.md#metadata", | ||
| } | ||
|
|
||
| func (CRIOCredentialProviderConfigList) SwaggerDoc() map[string]string { | ||
| return map_CRIOCredentialProviderConfigList | ||
| } | ||
|
|
||
| var map_CRIOCredentialProviderConfigSpec = map[string]string{ | ||
| "": "CRIOCredentialProviderConfigSpec defines the desired configuration of the CRI-O Credential Provider.", | ||
| "matchImages": "matchImages is a required list of string patterns used to determine whether the CRI-O credential provider should be invoked for a given image. This list is passed to the kubelet CredentialProviderConfig, and if any pattern matches the requested image, CRI-O credential provider will be invoked to obtain credentials for pulling that image or its mirrors. Depending on the platform, the CRI-O credential provider may be installed alongside an existing platform specific provider. Conflicts between the existing platform specific provider image match configuration and this list will be handled by the following precedence rule: credentials from built-in kubelet providers (e.g., ECR, GCR, ACR) take precedence over those from the CRIOCredentialProviderConfig when both match the same image. To avoid uncertainty, it is recommended to avoid configuring your private image patterns to overlap with existing platform specific provider config(e.g., the entries from https://github.com/openshift/machine-config-operator/blob/main/templates/common/aws/files/etc-kubernetes-credential-providers-ecr-credential-provider.yaml). You can check the resource's Status conditions to see if any entries were ignored due to exact matches with known built-in provider patterns.\n\nThis field is required and must contain between 1 and 50 entries. The list is treated as a set, so duplicate entries are not allowed.\n\nFor more details, see: https://kubernetes.io/docs/tasks/administer-cluster/kubelet-credential-provider/ https://github.com/cri-o/crio-credential-provider#architecture\n\nEach entry in matchImages is a pattern which can optionally contain a port and a path. Each entry must be no longer than 512 characters. Wildcards ('*') are supported for full subdomain labels, such as '*.k8s.io' or 'k8s.*.io', and for top-level domains, such as 'k8s.*' (which matches 'k8s.io' or 'k8s.net'). A global wildcard '*' (matching any domain) is not allowed. Wildcards may replace an entire hostname label (e.g., *.example.com), but they cannot appear within a label (e.g., f*oo.example.com) and are not allowed in the port or path. For example, 'example.*.com' is valid, but 'exa*mple.*.com' is not. Each wildcard matches only a single domain label, so '*.io' does **not** match '*.k8s.io'.\n\nA match exists between an image and a matchImage when all of the below are true: Both contain the same number of domain parts and each part matches. The URL path of an matchImages must be a prefix of the target image URL path. If the matchImages contains a port, then the port must match in the image as well.\n\nExample values of matchImages: - 123456789.dkr.ecr.us-east-1.amazonaws.com - *.azurecr.io - gcr.io - *.*.registry.io - registry.io:8080/path", | ||
| } | ||
|
|
||
| func (CRIOCredentialProviderConfigSpec) SwaggerDoc() map[string]string { | ||
| return map_CRIOCredentialProviderConfigSpec | ||
| } | ||
|
|
||
| var map_CRIOCredentialProviderConfigStatus = map[string]string{ | ||
| "": "CRIOCredentialProviderConfigStatus defines the observed state of CRIOCredentialProviderConfig", | ||
| "conditions": "conditions represent the latest available observations of the configuration state. When omitted, it indicates that no conditions have been reported yet. The maximum number of conditions is 16. Conditions are stored as a map keyed by condition type, ensuring uniqueness.\n\nExpected condition types include: \"Validated\": indicates whether the matchImages configuration is valid", | ||
| } | ||
|
|
||
| func (CRIOCredentialProviderConfigStatus) SwaggerDoc() map[string]string { | ||
| return map_CRIOCredentialProviderConfigStatus | ||
| } |
There was a problem hiding this comment.
Swagger docs addition is fine; List doc likely missing items
map_CRIOCredentialProviderConfigList should likely include an "items" entry like other *List docs in this file.
var map_CRIOCredentialProviderConfigList = map[string]string{
"": "CRIOCredentialProviderConfigList contains a list of CRIOCredentialProviderConfig resources\n\nCompatibility 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.",
"metadata": "metadata is the standard list's metadata. More info: https://git.k8s.io/community/contributors/devel/sig-architecture/api-conventions.md#metadata",
+ "items": "items is a list of CRIOCredentialProviderConfig",
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| var map_CRIOCredentialProviderConfig = map[string]string{ | |
| "": "CRIOCredentialProviderConfig holds cluster-wide singleton resource configurations for CRI-O credential provider, the name of this instance is \"cluster\". CRI-O credential provider is a binary shipped with CRI-O that provides a way to obtain container image pull credentials from external sources. For example, it can be used to fetch mirror registry credentials from secrets resources in the cluster within the same namespace the pod will be running in. CRIOCredentialProviderConfig configuration specifies the pod image sources registries that should trigger the CRI-O credential provider execution, which will resolve the CRI-O mirror configurations and obtain the necessary credentials for pod creation. Note: Configuration changes will only take effect after the kubelet restarts, which is automatically managed by the cluster during rollout.\n\nThe resource is a singleton named \"cluster\".\n\nCompatibility 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.", | |
| "metadata": "metadata is the standard object's metadata. More info: https://git.k8s.io/community/contributors/devel/sig-architecture/api-conventions.md#metadata", | |
| "spec": "spec defines the desired configuration of the CRI-O Credential Provider. This field is required and must be provided when creating the resource.", | |
| "status": "status represents the current state of the CRIOCredentialProviderConfig. When omitted or nil, it indicates that the status has not yet been set by the controller. The controller will populate this field with validation conditions and operational state.", | |
| } | |
| func (CRIOCredentialProviderConfig) SwaggerDoc() map[string]string { | |
| return map_CRIOCredentialProviderConfig | |
| } | |
| var map_CRIOCredentialProviderConfigList = map[string]string{ | |
| "": "CRIOCredentialProviderConfigList contains a list of CRIOCredentialProviderConfig resources\n\nCompatibility 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.", | |
| "metadata": "metadata is the standard list's metadata. More info: https://git.k8s.io/community/contributors/devel/sig-architecture/api-conventions.md#metadata", | |
| } | |
| func (CRIOCredentialProviderConfigList) SwaggerDoc() map[string]string { | |
| return map_CRIOCredentialProviderConfigList | |
| } | |
| var map_CRIOCredentialProviderConfigSpec = map[string]string{ | |
| "": "CRIOCredentialProviderConfigSpec defines the desired configuration of the CRI-O Credential Provider.", | |
| "matchImages": "matchImages is a required list of string patterns used to determine whether the CRI-O credential provider should be invoked for a given image. This list is passed to the kubelet CredentialProviderConfig, and if any pattern matches the requested image, CRI-O credential provider will be invoked to obtain credentials for pulling that image or its mirrors. Depending on the platform, the CRI-O credential provider may be installed alongside an existing platform specific provider. Conflicts between the existing platform specific provider image match configuration and this list will be handled by the following precedence rule: credentials from built-in kubelet providers (e.g., ECR, GCR, ACR) take precedence over those from the CRIOCredentialProviderConfig when both match the same image. To avoid uncertainty, it is recommended to avoid configuring your private image patterns to overlap with existing platform specific provider config(e.g., the entries from https://github.com/openshift/machine-config-operator/blob/main/templates/common/aws/files/etc-kubernetes-credential-providers-ecr-credential-provider.yaml). You can check the resource's Status conditions to see if any entries were ignored due to exact matches with known built-in provider patterns.\n\nThis field is required and must contain between 1 and 50 entries. The list is treated as a set, so duplicate entries are not allowed.\n\nFor more details, see: https://kubernetes.io/docs/tasks/administer-cluster/kubelet-credential-provider/ https://github.com/cri-o/crio-credential-provider#architecture\n\nEach entry in matchImages is a pattern which can optionally contain a port and a path. Each entry must be no longer than 512 characters. Wildcards ('*') are supported for full subdomain labels, such as '*.k8s.io' or 'k8s.*.io', and for top-level domains, such as 'k8s.*' (which matches 'k8s.io' or 'k8s.net'). A global wildcard '*' (matching any domain) is not allowed. Wildcards may replace an entire hostname label (e.g., *.example.com), but they cannot appear within a label (e.g., f*oo.example.com) and are not allowed in the port or path. For example, 'example.*.com' is valid, but 'exa*mple.*.com' is not. Each wildcard matches only a single domain label, so '*.io' does **not** match '*.k8s.io'.\n\nA match exists between an image and a matchImage when all of the below are true: Both contain the same number of domain parts and each part matches. The URL path of an matchImages must be a prefix of the target image URL path. If the matchImages contains a port, then the port must match in the image as well.\n\nExample values of matchImages: - 123456789.dkr.ecr.us-east-1.amazonaws.com - *.azurecr.io - gcr.io - *.*.registry.io - registry.io:8080/path", | |
| } | |
| func (CRIOCredentialProviderConfigSpec) SwaggerDoc() map[string]string { | |
| return map_CRIOCredentialProviderConfigSpec | |
| } | |
| var map_CRIOCredentialProviderConfigStatus = map[string]string{ | |
| "": "CRIOCredentialProviderConfigStatus defines the observed state of CRIOCredentialProviderConfig", | |
| "conditions": "conditions represent the latest available observations of the configuration state. When omitted, it indicates that no conditions have been reported yet. The maximum number of conditions is 16. Conditions are stored as a map keyed by condition type, ensuring uniqueness.\n\nExpected condition types include: \"Validated\": indicates whether the matchImages configuration is valid", | |
| } | |
| func (CRIOCredentialProviderConfigStatus) SwaggerDoc() map[string]string { | |
| return map_CRIOCredentialProviderConfigStatus | |
| } | |
| var map_CRIOCredentialProviderConfigList = map[string]string{ | |
| "": "CRIOCredentialProviderConfigList contains a list of CRIOCredentialProviderConfig resources\n\nCompatibility 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.", | |
| "metadata": "metadata is the standard list's metadata. More info: https://git.k8s.io/community/contributors/devel/sig-architecture/api-conventions.md#metadata", | |
| "items": "items is a list of CRIOCredentialProviderConfig", | |
| } | |
| func (CRIOCredentialProviderConfigList) SwaggerDoc() map[string]string { | |
| return map_CRIOCredentialProviderConfigList | |
| } | |
| var map_CRIOCredentialProviderConfigSpec = map[string]string{ | |
| "": "CRIOCredentialProviderConfigSpec defines the desired configuration of the CRI-O Credential Provider.", | |
| "matchImages": "matchImages is a required list of string patterns used to determine whether the CRI-O credential provider should be invoked for a given image. This list is passed to the kubelet CredentialProviderConfig, and if any pattern matches the requested image, CRI-O credential provider will be invoked to obtain credentials for pulling that image or its mirrors. Depending on the platform, the CRI-O credential provider may be installed alongside an existing platform specific provider. Conflicts between the existing platform specific provider image match configuration and this list will be handled by the following precedence rule: credentials from built-in kubelet providers (e.g., ECR, GCR, ACR) take precedence over those from the CRIOCredentialProviderConfig when both match the same image. To avoid uncertainty, it is recommended to avoid configuring your private image patterns to overlap with existing platform specific provider config(e.g., the entries from https://github.com/openshift/machine-config-operator/blob/main/templates/common/aws/files/etc-kubernetes-credential-providers-ecr-credential-provider.yaml). You can check the resource's Status conditions to see if any entries were ignored due to exact matches with known built-in provider patterns.\n\nThis field is required and must contain between 1 and 50 entries. The list is treated as a set, so duplicate entries are not allowed.\n\nFor more details, see: https://kubernetes.io/docs/tasks/administer-cluster/kubelet-credential-provider/ https://github.com/cri-o/crio-credential-provider#architecture\n\nEach entry in matchImages is a pattern which can optionally contain a port and a path. Each entry must be no longer than 512 characters. Wildcards ('*') are supported for full subdomain labels, such as '*.k8s.io' or 'k8s.*.io', and for top-level domains, such as 'k8s.*' (which matches 'k8s.io' or 'k8s.net'). A global wildcard '*' (matching any domain) is not allowed. Wildcards may replace an entire hostname label (e.g., *.example.com), but they cannot appear within a label (e.g., f*oo.example.com) and are not allowed in the port or path. For example, 'example.*.com' is valid, but 'exa*mple.*.com' is not. Each wildcard matches only a single domain label, so '*.io' does **not** match '*.k8s.io'.\n\nA match exists between an image and a matchImage when all of the below are true: Both contain the same number of domain parts and each part matches. The URL path of an matchImages must be a prefix of the target image URL path. If the matchImages contains a port, then the port must match in the image as well.\n\nExample values of matchImages: - 123456789.dkr.ecr.us-east-1.amazonaws.com - *.azurecr.io - gcr.io - *.*.registry.io - registry.io:8080/path", | |
| } | |
| func (CRIOCredentialProviderConfigSpec) SwaggerDoc() map[string]string { | |
| return map_CRIOCredentialProviderConfigSpec | |
| } | |
| var map_CRIOCredentialProviderConfigStatus = map[string]string{ | |
| "": "CRIOCredentialProviderConfigStatus defines the observed state of CRIOCredentialProviderConfig", | |
| "conditions": "conditions represent the latest available observations of the configuration state. When omitted, it indicates that no conditions have been reported yet. The maximum number of conditions is 16. Conditions are stored as a map keyed by condition type, ensuring uniqueness.\n\nExpected condition types include: \"Validated\": indicates whether the matchImages configuration is valid", | |
| } | |
| func (CRIOCredentialProviderConfigStatus) SwaggerDoc() map[string]string { | |
| return map_CRIOCredentialProviderConfigStatus | |
| } |
🤖 Prompt for AI Agents
In config/v1alpha1/zz_generated.swagger_doc_generated.go around lines 229 to
265, the CRIOCredentialProviderConfigList swagger map is missing the "items"
entry; add an "items" key to map_CRIOCredentialProviderConfigList with a short
description like: "items is the list of CRIOCredentialProviderConfig objects"
(matching the style of other *List docs), so the generated Swagger docs include
the list contents description.
...s/crds/0000_10_config-operator_01_criocredentialproviderconfigs-DevPreviewNoUpgrade.crd.yaml
Show resolved
Hide resolved
QiWang19
force-pushed
the
creds-provider
branch
from
07c6dc5 to
267555a
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (3)
config/v1alpha1/zz_generated.crd-manifests/0000_10_config-operator_01_criocredentialproviderconfigs-CustomNoUpgrade.crd.yaml (1)
114-120: RelaxmatchImagespath regex to allow.in repository paths
Current regex forbids dots in the optional path (/[-a-z0-9_/]*), which can block valid/common repo names. (This was raised previously.)- rule: self.matches('^((\\*|[a-z0-9]([a-z0-9-]*[a-z0-9])?)(\\.(\\*|[a-z0-9]([a-z0-9-]*[a-z0-9])?))*)(:[0-9]+)?(/[-a-z0-9_/]*)?$') + rule: self.matches('^((\\*|[a-z0-9]([a-z0-9-]*[a-z0-9])?)(\\.(\\*|[a-z0-9]([a-z0-9-]*[a-z0-9])?))*)(:[0-9]+)?(/[-a-z0-9._/]*)?$')openapi/generated_openapi/zz_generated.openapi.go (1)
21535-21705: Reintroduced:matchImages/conditionsarray limits described but not enforced in schema. The generated schema lacksMinItems/MaxItemsformatchImages(1..50) andMaxItemsforstatus.conditions(16), even though the descriptions require them—this is likely a source-marker or generator/regeneration issue (and was previously addressed).Please fix via the source Go types + regeneration (don’t patch this generated file). To verify quickly in-repo:
#!/bin/bash set -euo pipefail # Expect to see MinItems/MaxItems near matchImages and MaxItems near conditions. rg -n "matchImages|conditions" openapi/generated_openapi/zz_generated.openapi.go -n -C2 echo "---- matchImages constraints ----" rg -n "matchImages" openapi/generated_openapi/zz_generated.openapi.go -n -A40 | rg -n "MinItems|MaxItems|x-kubernetes-list-type" || true echo "---- conditions constraints ----" rg -n "\"conditions\"" openapi/generated_openapi/zz_generated.openapi.go -n -A40 | rg -n "MaxItems|x-kubernetes-list-type" || trueopenapi/openapi.json (1)
11785-11820: Add missing array bounds in generated schema (spec.matchImages,status.conditions)
This hunk still lacksminItems/maxItemsonmatchImagesandmaxItemsonstatus.conditions, so empty/unbounded lists remain valid at the OpenAPI layer. Please ensure codegen emitsminItems: 1,maxItems: 50formatchImagesandmaxItems: 4forconditions(likely via kubebuilder markers on the Go types).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (24)
config/v1alpha1/register.go(1 hunks)config/v1alpha1/tests/criocredentialproviderconfigs.config.openshift.io/CRIOCredentialProviderConfig.yaml(1 hunks)config/v1alpha1/types_crio_credential_provider_config.go(1 hunks)config/v1alpha1/zz_generated.crd-manifests/0000_10_config-operator_01_criocredentialproviderconfigs-CustomNoUpgrade.crd.yaml(1 hunks)config/v1alpha1/zz_generated.crd-manifests/0000_10_config-operator_01_criocredentialproviderconfigs-DevPreviewNoUpgrade.crd.yaml(1 hunks)config/v1alpha1/zz_generated.crd-manifests/0000_10_config-operator_01_criocredentialproviderconfigs-TechPreviewNoUpgrade.crd.yaml(1 hunks)config/v1alpha1/zz_generated.deepcopy.go(1 hunks)config/v1alpha1/zz_generated.featuregated-crd-manifests.yaml(1 hunks)config/v1alpha1/zz_generated.featuregated-crd-manifests/criocredentialproviderconfigs.config.openshift.io/CRIOCredentialProviderConfig.yaml(1 hunks)config/v1alpha1/zz_generated.swagger_doc_generated.go(1 hunks)features.md(1 hunks)features/features.go(1 hunks)hack/update-payload-crds.sh(1 hunks)openapi/generated_openapi/zz_generated.openapi.go(2 hunks)openapi/openapi.json(1 hunks)payload-manifests/crds/0000_10_config-operator_01_criocredentialproviderconfigs-CustomNoUpgrade.crd.yaml(1 hunks)payload-manifests/crds/0000_10_config-operator_01_criocredentialproviderconfigs-DevPreviewNoUpgrade.crd.yaml(1 hunks)payload-manifests/crds/0000_10_config-operator_01_criocredentialproviderconfigs-TechPreviewNoUpgrade.crd.yaml(1 hunks)payload-manifests/featuregates/featureGate-Hypershift-Default.yaml(1 hunks)payload-manifests/featuregates/featureGate-Hypershift-DevPreviewNoUpgrade.yaml(1 hunks)payload-manifests/featuregates/featureGate-Hypershift-TechPreviewNoUpgrade.yaml(1 hunks)payload-manifests/featuregates/featureGate-SelfManagedHA-Default.yaml(1 hunks)payload-manifests/featuregates/featureGate-SelfManagedHA-DevPreviewNoUpgrade.yaml(1 hunks)payload-manifests/featuregates/featureGate-SelfManagedHA-TechPreviewNoUpgrade.yaml(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (11)
- payload-manifests/featuregates/featureGate-SelfManagedHA-Default.yaml
- payload-manifests/featuregates/featureGate-Hypershift-TechPreviewNoUpgrade.yaml
- hack/update-payload-crds.sh
- config/v1alpha1/zz_generated.crd-manifests/0000_10_config-operator_01_criocredentialproviderconfigs-DevPreviewNoUpgrade.crd.yaml
- config/v1alpha1/zz_generated.featuregated-crd-manifests.yaml
- payload-manifests/featuregates/featureGate-Hypershift-DevPreviewNoUpgrade.yaml
- payload-manifests/featuregates/featureGate-SelfManagedHA-TechPreviewNoUpgrade.yaml
- features.md
- payload-manifests/crds/0000_10_config-operator_01_criocredentialproviderconfigs-CustomNoUpgrade.crd.yaml
- config/v1alpha1/tests/criocredentialproviderconfigs.config.openshift.io/CRIOCredentialProviderConfig.yaml
- config/v1alpha1/types_crio_credential_provider_config.go
🧰 Additional context used
📓 Path-based instructions (1)
**
⚙️ CodeRabbit configuration file
-Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity.
Files:
payload-manifests/featuregates/featureGate-Hypershift-Default.yamlpayload-manifests/featuregates/featureGate-SelfManagedHA-DevPreviewNoUpgrade.yamlconfig/v1alpha1/zz_generated.crd-manifests/0000_10_config-operator_01_criocredentialproviderconfigs-TechPreviewNoUpgrade.crd.yamlconfig/v1alpha1/zz_generated.featuregated-crd-manifests/criocredentialproviderconfigs.config.openshift.io/CRIOCredentialProviderConfig.yamlconfig/v1alpha1/register.goopenapi/generated_openapi/zz_generated.openapi.gofeatures/features.gopayload-manifests/crds/0000_10_config-operator_01_criocredentialproviderconfigs-TechPreviewNoUpgrade.crd.yamlpayload-manifests/crds/0000_10_config-operator_01_criocredentialproviderconfigs-DevPreviewNoUpgrade.crd.yamlconfig/v1alpha1/zz_generated.swagger_doc_generated.goconfig/v1alpha1/zz_generated.crd-manifests/0000_10_config-operator_01_criocredentialproviderconfigs-CustomNoUpgrade.crd.yamlopenapi/openapi.jsonconfig/v1alpha1/zz_generated.deepcopy.go
🧬 Code graph analysis (4)
config/v1alpha1/register.go (2)
config/v1alpha1/types_crio_credential_provider_config.go (2)
CRIOCredentialProviderConfig(25-43)CRIOCredentialProviderConfigList(139-147)config/v1alpha1/zz_generated.swagger_doc_generated.go (2)
CRIOCredentialProviderConfig(236-238)CRIOCredentialProviderConfigList(245-247)
features/features.go (1)
config/v1/types_feature.go (2)
DevPreviewNoUpgrade(49-49)TechPreviewNoUpgrade(45-45)
config/v1alpha1/zz_generated.swagger_doc_generated.go (1)
config/v1alpha1/types_crio_credential_provider_config.go (4)
CRIOCredentialProviderConfig(25-43)CRIOCredentialProviderConfigList(139-147)CRIOCredentialProviderConfigSpec(46-94)CRIOCredentialProviderConfigStatus(117-131)
config/v1alpha1/zz_generated.deepcopy.go (2)
config/v1alpha1/types_crio_credential_provider_config.go (5)
CRIOCredentialProviderConfig(25-43)CRIOCredentialProviderConfigList(139-147)CRIOCredentialProviderConfigSpec(46-94)MatchImage(112-112)CRIOCredentialProviderConfigStatus(117-131)config/v1alpha1/zz_generated.swagger_doc_generated.go (4)
CRIOCredentialProviderConfig(236-238)CRIOCredentialProviderConfigList(245-247)CRIOCredentialProviderConfigSpec(254-256)CRIOCredentialProviderConfigStatus(263-265)
🔇 Additional comments (11)
config/v1alpha1/zz_generated.deepcopy.go (1)
195-299: Generated DeepCopy implementations look correct for the new CRIOCredentialProviderConfig typespayload-manifests/featuregates/featureGate-SelfManagedHA-DevPreviewNoUpgrade.yaml (1)
101-108: FeatureGate enablement update is consistentpayload-manifests/featuregates/featureGate-Hypershift-Default.yaml (1)
52-65: Default Hypershift gating (disabled) matches expected rollout postureconfig/v1alpha1/register.go (1)
30-47: Scheme registration for CRIOCredentialProviderConfig types looks correctfeatures/features.go (1)
134-140: String "CRIOCredentialProviderConfig" is consistent across code and payload manifestsAll references to the feature gate string maintain consistent casing and spelling across type definitions, CRD manifests, feature gate configurations, and generated code. No typos or case drift detected.
config/v1alpha1/zz_generated.featuregated-crd-manifests/criocredentialproviderconfigs.config.openshift.io/CRIOCredentialProviderConfig.yaml (1)
1-214: LGTM! CRD validation design is sound.The CRD manifest is well-structured and addresses previous review concerns:
- The singleton enforcement via CEL validation (lines 208-210) correctly restricts the resource name to "cluster"
- The matchImages regex validation (line 119) has been verified through tests to correctly accept valid domain patterns with wildcards, ports, and paths
- The status constraints are semantically appropriate:
statusis optional and can be omitted for new resourcesminProperties: 1(line 132) ensures that if status is provided, it contains at least the conditions fieldminItems: 1(line 199) ensures that if conditions are provided, at least one condition is present- This prevents empty status objects while allowing controllers to properly initialize status with meaningful conditions
The validation rules align with the documented behavior that status can be omitted initially and will be populated by controllers with validation conditions.
config/v1alpha1/zz_generated.swagger_doc_generated.go (1)
229-265: LGTM! Swagger documentation is complete and follows established patterns.The generated Swagger documentation for CRIOCredentialProviderConfig and its related types is comprehensive and accurate:
- All four types (CRIOCredentialProviderConfig, CRIOCredentialProviderConfigList, CRIOCredentialProviderConfigSpec, CRIOCredentialProviderConfigStatus) have proper documentation maps
- Descriptions are detailed and match the CRD schema definitions
- The documentation clearly explains the singleton nature, matchImages validation requirements, and status conditions
The code follows the established pattern in this generated file. While the
map_CRIOCredentialProviderConfigListcould include an "items" entry for additional completeness (similar tomap_ClusterMonitoringListat line 169), this is consistent with most other *List types in this file which also omit that entry, and was noted in previous reviews.openapi/generated_openapi/zz_generated.openapi.go (1)
428-434: Registry wiring for new OpenAPI definitions looks consistent.payload-manifests/crds/0000_10_config-operator_01_criocredentialproviderconfigs-TechPreviewNoUpgrade.crd.yaml (3)
127-203: Clarify status constraints and conditions array handling.The status schema combines
minProperties: 1(line 132) withminItems: 1on the conditions array (line 198), which means status can only exist in one of two valid states: absent, or present with at least one condition. However, the description at lines 136–137 states "When omitted, it indicates that no conditions have been reported yet," which could be misunderstood as allowing an empty conditions array or empty status object{}.For clarity and to align with the intended controller behavior, consider:
- If this constraint is intentional (status must always contain conditions): update the description to explicitly state that status should only be present when there is at least one condition to report, or use a CEL validation rule to document this pattern.
- If empty status should be allowed: either remove
minProperties: 1or setminItems: 0on the conditions array, and update descriptions accordingly.The current schema is technically consistent, but the wording could mislead controller implementations into attempting invalid configurations.
50-126: matchImages validation and constraints are well-designed.The spec validation rules are comprehensive and appropriate:
- Regex pattern (line 119) correctly enforces lowercase FQDN format with optional wildcards, ports, and paths
- Wildcard constraints (line 116) prevent unsafe global matches
- Array bounds (minItems: 1, maxItems: 50) and string length limits (maxLength: 512) are reasonable
x-kubernetes-list-type: set(line 123) prevents duplicates
207-210: Singleton constraint enforcement is correct.The CEL validation rule properly enforces the singleton pattern by requiring
metadata.name == 'cluster', which aligns with the resource description and prevents accidental creation of multiple instances.
...fests/0000_10_config-operator_01_criocredentialproviderconfigs-TechPreviewNoUpgrade.crd.yaml
Show resolved
Hide resolved
...s/crds/0000_10_config-operator_01_criocredentialproviderconfigs-DevPreviewNoUpgrade.crd.yaml
Show resolved
Hide resolved
QiWang19
force-pushed
the
creds-provider
branch
from
267555a to
da5c4a2
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (9)
config/v1alpha1/zz_generated.crd-manifests/0000_10_config-operator_01_criocredentialproviderconfigs-TechPreviewNoUpgrade.crd.yaml (1)
114-119: Fix matchImages path validation to allow dots in repository paths.The regex pattern on line 119 restricts the path component to
[-a-z0-9_/]*, excluding dots. Repository paths commonly include dots (e.g.,.tar,.tgz). This restriction is unnecessarily strict.Update the path character class to include dots, matching the fix needed in the DevPreviewNoUpgrade variant:
- rule: self.matches('^((\\*|[a-z0-9]([a-z0-9-]*[a-z0-9])?)(\\.(\\*|[a-z0-9]([a-z0-9-]*[a-z0-9])?))*)(:[0-9]+)?(/[-a-z0-9_/]*)?$') + rule: self.matches('^((\\*|[a-z0-9]([a-z0-9-]*[a-z0-9])?)(\\.(\\*|[a-z0-9]([a-z0-9-]*[a-z0-9])?))*)(:[0-9]+)?(/[-a-z0-9._/]*)?$')Ensure both DevPreviewNoUpgrade and TechPreviewNoUpgrade CRD variants receive this fix for consistency.
payload-manifests/crds/0000_10_config-operator_01_criocredentialproviderconfigs-DevPreviewNoUpgrade.crd.yaml (1)
114-123: Allow dots inmatchImagespath component for repository paths.The regex pattern at line 119 disallows dots in the optional path segment:
(/[-a-z0-9_/]*)does not include\.. This rejects legitimate image references likeregistry.io/my.package/image, which commonly appear in container registries (e.g., Python packages, npm scopes with dots).Update the path character class to include dots:
- rule: self.matches('^((\\*|[a-z0-9]([a-z0-9-]*[a-z0-9])?)(\\.(\\*|[a-z0-9]([a-z0-9-]*[a-z0-9])?))*)(:[0-9]+)?(/[-a-z0-9_/]*)?$') + rule: self.matches('^((\\*|[a-z0-9]([a-z0-9-]*[a-z0-9])?)(\\.(\\*|[a-z0-9]([a-z0-9-]*[a-z0-9])?))*)(:[0-9]+)?(/[-a-z0-9._/]*)?$')payload-manifests/crds/0000_10_config-operator_01_criocredentialproviderconfigs-CustomNoUpgrade.crd.yaml (1)
114-123: Allow dots inmatchImagespath component for repository paths.The regex pattern at line 119 disallows dots in the optional path segment:
(/[-a-z0-9_/]*)does not include\.. This rejects legitimate image references likeregistry.io/my.package/image, which commonly appear in container registries (e.g., Python packages, npm scopes with dots).Update the path character class to include dots:
- rule: self.matches('^((\\*|[a-z0-9]([a-z0-9-]*[a-z0-9])?)(\\.(\\*|[a-z0-9]([a-z0-9-]*[a-z0-9])?))*)(:[0-9]+)?(/[-a-z0-9_/]*)?$') + rule: self.matches('^((\\*|[a-z0-9]([a-z0-9-]*[a-z0-9])?)(\\.(\\*|[a-z0-9]([a-z0-9-]*[a-z0-9])?))*)(:[0-9]+)?(/[-a-z0-9._/]*)?$')openapi/generated_openapi/zz_generated.openapi.go (2)
21636-21668:matchImagesschema missing MinItems/MaxItems vs documented 1..50 contract. This allows empty/unbounded arrays despite the description. Since this is generated output, fix via kubebuilder markers on the source type and regenerate; if generation still omits these, the openapi generation tooling/version likely isn’t honoring the markers."matchImages": { VendorExtensible: spec.VendorExtensible{ Extensions: spec.Extensions{ "x-kubernetes-list-type": "set", }, }, SchemaProps: spec.SchemaProps{ Description: "matchImages is a required list ... This field is required and must contain between 1 and 50 entries. ...", Type: []string{"array"}, + MinItems: 1, + MaxItems: 50, Items: &spec.SchemaOrArray{ Schema: &spec.Schema{ SchemaProps: spec.SchemaProps{ Default: "", Type: []string{"string"}, Format: "", }, }, }, }, },
21670-21705:conditionsschema missing MaxItems=16 vs documented limit. This makes the status array effectively unbounded. Fix at source + regenerate; if generation still omits it, treat as a tooling/generator gap."conditions": { VendorExtensible: spec.VendorExtensible{ Extensions: spec.Extensions{ "x-kubernetes-list-map-keys": []interface{}{ "type", }, "x-kubernetes-list-type": "map", }, }, SchemaProps: spec.SchemaProps{ Description: "conditions represent ... The maximum number of conditions is 16. ...", Type: []string{"array"}, + MaxItems: 16, Items: &spec.SchemaOrArray{ Schema: &spec.Schema{ SchemaProps: spec.SchemaProps{ Default: map[string]interface{}{}, Ref: ref("k8s.io/apimachinery/pkg/apis/meta/v1.Condition"), }, }, }, }, },config/v1alpha1/zz_generated.crd-manifests/0000_10_config-operator_01_criocredentialproviderconfigs-CustomNoUpgrade.crd.yaml (1)
114-119: Verify if path segment restriction is intentional.The
matchImagesvalidation regex at line 119 uses the character class[-a-z0-9_/]for the path portion, which does not include.(dot). This means patterns likeregistry.io/project.v2/subpathwould be rejected.A previous review flagged this same concern and was marked as "Addressed in commit a31eec1", but the regex still excludes dots from path segments. Please confirm whether:
- This restriction is intentional (paths with dots are not supported by design), or
- The fix was incomplete and the regex should be updated to
/[-a-z0-9._/]*to allow dots in pathsContainer image repository paths commonly include dots (per OCI naming conventions), so this limitation may surprise users and block valid configurations unless it's a deliberate design choice.
Based on past review comments.
payload-manifests/crds/0000_10_config-operator_01_criocredentialproviderconfigs-TechPreviewNoUpgrade.crd.yaml (1)
114-119: Verify if path segment restriction is intentional.The
matchImagesvalidation regex at line 119 uses the character class[-a-z0-9_/]for the path portion, which does not include.(dot). This means patterns likeregistry.io/project.v2/subpathwould be rejected.This is the same regex pattern as in the CustomNoUpgrade variant. Please confirm whether:
- This restriction is intentional (paths with dots are not supported by design), or
- The regex should be updated to
/[-a-z0-9._/]*to allow dots in pathsContainer image repository paths commonly include dots (per OCI naming conventions), so this limitation may surprise users and block valid configurations unless it's a deliberate design choice.
Based on past review comments.
openapi/openapi.json (2)
11792-11800: [Duplicate] Missing array bounds validation on matchImages.This issue was already flagged in a previous review. The
matchImagesarray schema lacksminItems: 1andmaxItems: 50constraints despite PR requirements specifying "1–50 unique strings". The array definition currently only includesx-kubernetes-list-type: "set"for uniqueness.Add the missing cardinality constraints:
"matchImages": { "description": "...", "type": "array", + "minItems": 1, + "maxItems": 50, "items": { "type": "string", "default": "" }, "x-kubernetes-list-type": "set" }Ensure the corresponding Go struct in
config/v1alpha1/types_crio_credential_provider_config.goincludes+kubebuilder:validation:MinItems=1and+kubebuilder:validation:MaxItems=50markers so codegen produces these OpenAPI constraints.
11807-11818: [Duplicate] Missing maxItems constraint on conditions array.This issue was already flagged in a previous review. The
conditionsarray schema in the status lacks themaxItems: 4constraint despite PR requirements specifying "up to 4" conditions. The array definition currently only includes list map configuration fortype-based keying.Add the missing cardinality constraint:
"conditions": { "description": "conditions represent the latest available observations of the configuration state", "type": "array", + "maxItems": 4, "items": { "default": {}, "$ref": "#/definitions/io.k8s.apimachinery.pkg.apis.meta.v1.Condition" }, "x-kubernetes-list-map-keys": [ "type" ], "x-kubernetes-list-type": "map" }Ensure the corresponding Go struct includes
+kubebuilder:validation:MaxItems=4marker so codegen produces this constraint in the OpenAPI schema.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (26)
config/v1alpha1/register.go(1 hunks)config/v1alpha1/tests/criocredentialproviderconfigs.config.openshift.io/CRIOCredentialProviderConfig.yaml(1 hunks)config/v1alpha1/types_crio_credential_provider_config.go(1 hunks)config/v1alpha1/zz_generated.crd-manifests/0000_10_config-operator_01_criocredentialproviderconfigs-CustomNoUpgrade.crd.yaml(1 hunks)config/v1alpha1/zz_generated.crd-manifests/0000_10_config-operator_01_criocredentialproviderconfigs-DevPreviewNoUpgrade.crd.yaml(1 hunks)config/v1alpha1/zz_generated.crd-manifests/0000_10_config-operator_01_criocredentialproviderconfigs-TechPreviewNoUpgrade.crd.yaml(1 hunks)config/v1alpha1/zz_generated.deepcopy.go(1 hunks)config/v1alpha1/zz_generated.featuregated-crd-manifests.yaml(1 hunks)config/v1alpha1/zz_generated.featuregated-crd-manifests/criocredentialproviderconfigs.config.openshift.io/CRIOCredentialProviderConfig.yaml(1 hunks)config/v1alpha1/zz_generated.swagger_doc_generated.go(1 hunks)features.md(1 hunks)features/features.go(1 hunks)hack/update-payload-crds.sh(1 hunks)openapi/generated_openapi/zz_generated.openapi.go(2 hunks)openapi/openapi.json(1 hunks)payload-manifests/crds/0000_10_config-operator_01_criocredentialproviderconfigs-CustomNoUpgrade.crd.yaml(1 hunks)payload-manifests/crds/0000_10_config-operator_01_criocredentialproviderconfigs-DevPreviewNoUpgrade.crd.yaml(1 hunks)payload-manifests/crds/0000_10_config-operator_01_criocredentialproviderconfigs-TechPreviewNoUpgrade.crd.yaml(1 hunks)payload-manifests/featuregates/featureGate-Hypershift-Default.yaml(1 hunks)payload-manifests/featuregates/featureGate-Hypershift-DevPreviewNoUpgrade.yaml(1 hunks)payload-manifests/featuregates/featureGate-Hypershift-OKD.yaml(1 hunks)payload-manifests/featuregates/featureGate-Hypershift-TechPreviewNoUpgrade.yaml(1 hunks)payload-manifests/featuregates/featureGate-SelfManagedHA-Default.yaml(1 hunks)payload-manifests/featuregates/featureGate-SelfManagedHA-DevPreviewNoUpgrade.yaml(1 hunks)payload-manifests/featuregates/featureGate-SelfManagedHA-OKD.yaml(1 hunks)payload-manifests/featuregates/featureGate-SelfManagedHA-TechPreviewNoUpgrade.yaml(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (10)
- config/v1alpha1/zz_generated.featuregated-crd-manifests.yaml
- payload-manifests/featuregates/featureGate-SelfManagedHA-Default.yaml
- features/features.go
- hack/update-payload-crds.sh
- config/v1alpha1/register.go
- payload-manifests/featuregates/featureGate-Hypershift-Default.yaml
- config/v1alpha1/types_crio_credential_provider_config.go
- features.md
- config/v1alpha1/zz_generated.swagger_doc_generated.go
- payload-manifests/featuregates/featureGate-SelfManagedHA-TechPreviewNoUpgrade.yaml
🧰 Additional context used
📓 Path-based instructions (1)
**
⚙️ CodeRabbit configuration file
-Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity.
Files:
config/v1alpha1/tests/criocredentialproviderconfigs.config.openshift.io/CRIOCredentialProviderConfig.yamlconfig/v1alpha1/zz_generated.crd-manifests/0000_10_config-operator_01_criocredentialproviderconfigs-TechPreviewNoUpgrade.crd.yamlpayload-manifests/featuregates/featureGate-Hypershift-TechPreviewNoUpgrade.yamlpayload-manifests/crds/0000_10_config-operator_01_criocredentialproviderconfigs-DevPreviewNoUpgrade.crd.yamlopenapi/generated_openapi/zz_generated.openapi.goconfig/v1alpha1/zz_generated.crd-manifests/0000_10_config-operator_01_criocredentialproviderconfigs-CustomNoUpgrade.crd.yamlpayload-manifests/featuregates/featureGate-Hypershift-OKD.yamlconfig/v1alpha1/zz_generated.crd-manifests/0000_10_config-operator_01_criocredentialproviderconfigs-DevPreviewNoUpgrade.crd.yamlconfig/v1alpha1/zz_generated.featuregated-crd-manifests/criocredentialproviderconfigs.config.openshift.io/CRIOCredentialProviderConfig.yamlpayload-manifests/featuregates/featureGate-Hypershift-DevPreviewNoUpgrade.yamlpayload-manifests/featuregates/featureGate-SelfManagedHA-DevPreviewNoUpgrade.yamlpayload-manifests/crds/0000_10_config-operator_01_criocredentialproviderconfigs-TechPreviewNoUpgrade.crd.yamlconfig/v1alpha1/zz_generated.deepcopy.gopayload-manifests/crds/0000_10_config-operator_01_criocredentialproviderconfigs-CustomNoUpgrade.crd.yamlopenapi/openapi.jsonpayload-manifests/featuregates/featureGate-SelfManagedHA-OKD.yaml
🧬 Code graph analysis (1)
config/v1alpha1/zz_generated.deepcopy.go (2)
config/v1alpha1/types_crio_credential_provider_config.go (5)
CRIOCredentialProviderConfig(25-43)CRIOCredentialProviderConfigList(139-147)CRIOCredentialProviderConfigSpec(46-94)MatchImage(112-112)CRIOCredentialProviderConfigStatus(117-131)config/v1alpha1/zz_generated.swagger_doc_generated.go (4)
CRIOCredentialProviderConfig(236-238)CRIOCredentialProviderConfigList(245-247)CRIOCredentialProviderConfigSpec(254-256)CRIOCredentialProviderConfigStatus(263-265)
🔇 Additional comments (11)
payload-manifests/featuregates/featureGate-Hypershift-OKD.yaml (1)
59-61: Feature gate entry correctly positioned in alphabetical order.Insertion into the disabled list is properly placed between
CRDCompatibilityRequirementOperatorandClientsAllowCBOR, maintaining alphabetical order.payload-manifests/featuregates/featureGate-SelfManagedHA-OKD.yaml (1)
59-61: Feature gate entry correctly positioned.Consistent with other OKD feature gate files. Entry is properly placed in the disabled list in alphabetical order.
payload-manifests/featuregates/featureGate-SelfManagedHA-DevPreviewNoUpgrade.yaml (1)
103-105: Feature gate correctly enabled for DevPreviewNoUpgrade.Entry is properly placed in the enabled list in alphabetical order, consistent with the DevPreviewNoUpgrade feature set semantics.
payload-manifests/featuregates/featureGate-Hypershift-TechPreviewNoUpgrade.yaml (1)
133-135: Feature gate correctly enabled for TechPreviewNoUpgrade.Entry properly positioned in the enabled list in alphabetical order, consistent with TechPreviewNoUpgrade feature gating.
payload-manifests/featuregates/featureGate-Hypershift-DevPreviewNoUpgrade.yaml (1)
118-120: Feature gate entry correctly positioned for DevPreviewNoUpgrade.Properly placed in alphabetical order within the enabled list for Hypershift DevPreviewNoUpgrade.
config/v1alpha1/tests/criocredentialproviderconfigs.config.openshift.io/CRIOCredentialProviderConfig.yaml (1)
1-92: Test cases properly structured with comprehensive validation coverage.The YAML structure is correct (no nesting issues as were fixed in commit 514adaf), and the seven test cases comprehensively cover the matchImages validation rules: valid registries, invalid characters, uppercase enforcement, wildcard constraints, and global wildcard rejection. Test case error messages align with the CRD validation rules.
config/v1alpha1/zz_generated.featuregated-crd-manifests/criocredentialproviderconfigs.config.openshift.io/CRIOCredentialProviderConfig.yaml (1)
127-203: LGTM: Status structure is properly optional and well-defined.The status field is correctly optional (not in the required array), with
minProperties: 1ensuring that when present it's not empty. The conditions array hasminItems: 1, preventing empty arrays, and the description correctly states "When omitted, it indicates that no conditions have been reported yet."—this aligns with the schema constraints.payload-manifests/crds/0000_10_config-operator_01_criocredentialproviderconfigs-DevPreviewNoUpgrade.crd.yaml (1)
127-203: LGTM: Status structure is properly optional and well-defined.The status field is correctly optional (not in the required array), with
minProperties: 1ensuring that when present it's not empty. The conditions array hasminItems: 1, preventing empty arrays, and the description correctly states "When omitted, it indicates that no conditions have been reported yet."—this aligns with the schema constraints.payload-manifests/crds/0000_10_config-operator_01_criocredentialproviderconfigs-CustomNoUpgrade.crd.yaml (1)
127-203: LGTM: Status structure is properly optional and well-defined.The status field is correctly optional (not in the required array), with
minProperties: 1ensuring that when present it's not empty. The conditions array hasminItems: 1, preventing empty arrays, and the description correctly states "When omitted, it indicates that no conditions have been reported yet."—this aligns with the schema constraints.openapi/generated_openapi/zz_generated.openapi.go (1)
428-434: OpenAPI registry wiring for new CRIOCredentialProviderConfig types looks correct.config/v1alpha1/zz_generated.deepcopy.go (1)
195-298: LGTM! Generated deepcopy implementations are correct.The auto-generated deepcopy methods for the new CRIOCredentialProviderConfig types follow the standard patterns and include proper nil checks, correct field copying, and appropriate runtime.Object implementations. All slice fields (MatchImages, Conditions, Items) are handled correctly with allocation and element-by-element deep copying.
...a1/tests/criocredentialproviderconfigs.config.openshift.io/CRIOCredentialProviderConfig.yaml
Show resolved
Hide resolved
| x-kubernetes-validations: | ||
| - message: global wildcard '*' is not allowed | ||
| rule: self != '*' | ||
| - message: invalid matchImages value, must be a valid fully qualified | ||
| domain name in lowercase with optional wildcard, port, and path | ||
| rule: self.matches('^((\\*|[a-z0-9]([a-z0-9-]*[a-z0-9])?)(\\.(\\*|[a-z0-9]([a-z0-9-]*[a-z0-9])?))*)(:[0-9]+)?(/[-a-z0-9_/]*)?$') |
There was a problem hiding this comment.
Fix matchImages path validation to allow dots in repository paths.
The regex pattern on line 119 restricts the path component to [-a-z0-9_/]*, which excludes dots (.). This is overly restrictive—repository paths commonly include dots (e.g., /my.repo/image.tar).
Update the path character class to include dots:
- rule: self.matches('^((\\*|[a-z0-9]([a-z0-9-]*[a-z0-9])?)(\\.(\\*|[a-z0-9]([a-z0-9-]*[a-z0-9])?))*)(:[0-9]+)?(/[-a-z0-9_/]*)?$')
+ rule: self.matches('^((\\*|[a-z0-9]([a-z0-9-]*[a-z0-9])?)(\\.(\\*|[a-z0-9]([a-z0-9-]*[a-z0-9])?))*)(:[0-9]+)?(/[-a-z0-9._/]*)?$')This aligns with the documented intent to support arbitrary repository paths. Based on learnings, this is a known issue flagged in previous reviews that should be corrected.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| x-kubernetes-validations: | |
| - message: global wildcard '*' is not allowed | |
| rule: self != '*' | |
| - message: invalid matchImages value, must be a valid fully qualified | |
| domain name in lowercase with optional wildcard, port, and path | |
| rule: self.matches('^((\\*|[a-z0-9]([a-z0-9-]*[a-z0-9])?)(\\.(\\*|[a-z0-9]([a-z0-9-]*[a-z0-9])?))*)(:[0-9]+)?(/[-a-z0-9_/]*)?$') | |
| x-kubernetes-validations: | |
| - message: global wildcard '*' is not allowed | |
| rule: self != '*' | |
| - message: invalid matchImages value, must be a valid fully qualified | |
| domain name in lowercase with optional wildcard, port, and path | |
| rule: self.matches('^((\\*|[a-z0-9]([a-z0-9-]*[a-z0-9])?)(\\.(\\*|[a-z0-9]([a-z0-9-]*[a-z0-9])?))*)(:[0-9]+)?(/[-a-z0-9._/]*)?$') |
...anifests/criocredentialproviderconfigs.config.openshift.io/CRIOCredentialProviderConfig.yaml
Show resolved
Hide resolved
| // +required | ||
| MatchImages []MatchImage `json:"matchImages,omitempty"` |
There was a problem hiding this comment.
@JoelSpeed Is it mandatory to ship a default singleton resource? If we have to ship a default singleton manifest, it will be an empty spec since we don't have meaningful default values needed for default.
I think if we have to ship this default resource, spec.matchImages should be optional to pass the validation?
There was a problem hiding this comment.
No, I think it can be off by default and have it created only when the user wants something to actually happen
There was a problem hiding this comment.
@JoelSpeed The openshift has implementaiton does not allow oc delete cluster resources. And the API cel validation does not allow "" spec.matchImages.
As a result, a user cannot delete the entire configuration they added via the CR.
Would it makes sense to let users clear their configuration, even if the CR itself is cluster and cannot be deleted? Probably we can allow spec.matchImages to be empty.
Although from the underlying implementation, once the secret obejct(s) of the matchImages entry is deleted, the effective result is equivalent to removing item(s) from spec.matchImages.
$ oc delete criocredentialproviderconfig/cluster
Error from server (Forbidden): criocredentialproviderconfigs.config.openshift.io "cluster" is forbidden: deleting required criocredentialproviderconfigs.config.openshift.io resource, named cluster, is not allowed
| // metadata is the standard object's metadata. | ||
| // More info: https://git.k8s.io/community/contributors/devel/sig-architecture/api-conventions.md#metadata | ||
| // +optional | ||
| metav1.ObjectMeta `json:"metadata"` |
There was a problem hiding this comment.
Needs omitzero here please
| metav1.ObjectMeta `json:"metadata"` | |
| metav1.ObjectMeta `json:"metadata,omitzero"` |
- Add API and feature gate for CRIOCredentialProviderConfig. Signed-off-by: Qi Wang <[email protected]>
QiWang19
force-pushed
the
creds-provider
branch
from
da5c4a2 to
e504b5e
Compare
|
/retest-required |
Hi @JoelSpeed, the enhancement PR got merged. openshift/enhancements#1861. we will need to followup on the enhancement to address the CRIOCredentialProvider resource a singleton. |
|
@QiWang19: 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. |
5 participants
enhancement: OCPNODE-3753: CRIOCredentialProviderConfig for Namespace-Scoped Mirror Authentication enhancements#1861
Add CRIOCredentialProviderConfig API setting configs https://github.com/cri-o/crio-credential-provider for retrieving mirror pull secrets
Add feature gate for CRIOCredentialProviderConfig feature gate.