-
Notifications
You must be signed in to change notification settings - Fork 580
OCPNODE-3863: Add CRIOCredentialProviderConfig API #2557
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,52 @@ | ||
| apiVersion: apiextensions.k8s.io/v1 # Hack because controller-gen complains if we don't have this | ||
| name: "CRIOCredentialProviderConfig" | ||
| crdName: "criocredentialproviderconfigs.config.openshift.io" | ||
| featureGates: | ||
| - CRIOCredentialProviderConfig | ||
| tests: | ||
| onCreate: | ||
| - name: Should create a valid CRIOCredentialProviderConfig | ||
| initial: | | ||
| apiVersion: config.openshift.io/v1alpha1 | ||
| kind: CRIOCredentialProviderConfig | ||
| spec: | ||
| matchImages: | ||
| - 123456789.dkr.ecr.us-east-1.amazonaws.com | ||
| - "*.azurecr.io" | ||
| - gcr.io | ||
| - "*.*.registry.io" | ||
| - registry.io:8080/path | ||
| expected: | | ||
| apiVersion: config.openshift.io/v1alpha1 | ||
| kind: CRIOCredentialProviderConfig | ||
| spec: | ||
| matchImages: | ||
| - 123456789.dkr.ecr.us-east-1.amazonaws.com | ||
| - "*.azurecr.io" | ||
| - gcr.io | ||
| - "*.*.registry.io" | ||
| - registry.io:8080/path | ||
| - name: Should reject matchImages with invalid characters | ||
| initial: | | ||
| apiVersion: config.openshift.io/v1alpha1 | ||
| kind: CRIOCredentialProviderConfig | ||
| spec: | ||
| matchImages: | ||
| - "reg!stry.io" | ||
| expectedError: "spec.matchImages[0]: Invalid value: \"string\": invalid matchImages value, must be a valid fully qualified domain name with optional wildcard, port, and path" | ||
| - name: Should reject matchImages wildcard in the path | ||
| initial: | | ||
| apiVersion: config.openshift.io/v1alpha1 | ||
| kind: CRIOCredentialProviderConfig | ||
| spec: | ||
| matchImages: | ||
| - "registry.io:8080/pa*th" | ||
| expectedError: "spec.matchImages[0]: Invalid value: \"string\": invalid matchImages value, must be a valid fully qualified domain name with optional wildcard, port, and path" | ||
| - name: Should reject wildcard for partial subdomains | ||
| initial: | | ||
| apiVersion: config.openshift.io/v1alpha1 | ||
| kind: CRIOCredentialProviderConfig | ||
| spec: | ||
| matchImages: | ||
| - "example.app*.com" | ||
| expectedError: "spec.matchImages[0]: Invalid value: \"string\": invalid matchImages value, must be a valid fully qualified domain name with optional wildcard, port, and path" | ||
| Original file line number | Diff line number | Diff line change | ||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,153 @@ | ||||||||||||
| package v1alpha1 | ||||||||||||
|
|
||||||||||||
| import metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" | ||||||||||||
|
|
||||||||||||
| // +genclient | ||||||||||||
| // +genclient:nonNamespaced | ||||||||||||
| // +k8s:deepcopy-gen:interfaces=k8s.io/apimachinery/pkg/runtime.Object | ||||||||||||
|
|
||||||||||||
| // CRIOCredentialProviderConfig holds cluster-wide configurations for CRI-O credential provider. CRI-O credential provider is a binary shipped with CRI-O that provides a way to obtain container image pull credentials from external sources. | ||||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Out of interest, was it considered to add this config to any existing CRs, or is there nothing appropriate that this would fit into that exists today? |
||||||||||||
| // 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. | ||||||||||||
| // | ||||||||||||
| // Compatibility level 4: No compatibility is provided, the API can change at any point for any reason. These capabilities should not be used by applications needing long term support. | ||||||||||||
| // +kubebuilder:object:root=true | ||||||||||||
| // +kubebuilder:resource:path=criocredentialproviderconfigs,scope=Cluster | ||||||||||||
| // +kubebuilder:subresource:status | ||||||||||||
| // +openshift:api-approved.openshift.io=https://github.com/openshift/api/pull/1929 | ||||||||||||
| // +openshift:file-pattern=cvoRunLevel=0000_10,operatorName=config-operator,operatorOrdering=01 | ||||||||||||
| // +openshift:enable:FeatureGate=CRIOCredentialProviderConfig | ||||||||||||
| // +openshift:compatibility-gen:level=4 | ||||||||||||
| type CRIOCredentialProviderConfig struct { | ||||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I assume that this is a singleton in the cluster which should be called cluster? So can we add a CEL validation that ensures the name is only ever cluster? (Some other config APIs already have this if you want an example, look for an XValidation with |
||||||||||||
| metav1.TypeMeta `json:",inline"` | ||||||||||||
|
|
||||||||||||
| // 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"` | ||||||||||||
|
|
||||||||||||
| // spec defines the desired configuration of the CRI-O Credential Provider. | ||||||||||||
| // This field is required and must be provided when creating the resource. | ||||||||||||
| // +required | ||||||||||||
| Spec CRIOCredentialProviderConfigSpec `json:"spec,omitzero"` | ||||||||||||
|
Comment on lines
+29
to
+32
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Will this resource exist on every OpenShift cluster? Do we intend to create these for existing clusters? |
||||||||||||
|
|
||||||||||||
| // 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. | ||||||||||||
| // +optional | ||||||||||||
| Status *CRIOCredentialProviderConfigStatus `json:"status,omitempty"` | ||||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This being a pointer tells me that there is a semantic difference between omitted and |
||||||||||||
| } | ||||||||||||
|
|
||||||||||||
| // CRIOCredentialProviderConfigSpec defines the desired configuration of the CRI-O Credential Provider. | ||||||||||||
| type CRIOCredentialProviderConfigSpec struct { | ||||||||||||
| // 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. | ||||||||||||
| // | ||||||||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We're missing the docs for min and max items here, also that it refers to a set:
Suggested change
|
||||||||||||
| // This field is required and must contain between 1 and 50 entries. | ||||||||||||
| // The list is treated as a set, so duplicate entries are not allowed. | ||||||||||||
| // | ||||||||||||
| // For more details, see: | ||||||||||||
| // - https://kubernetes.io/docs/tasks/administer-cluster/kubelet-credential-provider/ | ||||||||||||
| // - https://github.com/cri-o/crio-credential-provider#architecture | ||||||||||||
|
Comment on lines
+52
to
+54
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What is the relationship between say the Kubelet ECR credential provider and the crio credential providers? |
||||||||||||
| // | ||||||||||||
| // Each entry in matchImages is a pattern which can optionally contain a port and a path. | ||||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The maximum length of each entry isn't documented here |
||||||||||||
| // 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'). | ||||||||||||
|
Comment on lines
+56
to
+58
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In the Kubelet credential provider, only leading |
||||||||||||
| // A global wildcard '*' (matching any domain) is not allowed. | ||||||||||||
| // Wildcards are not allowed in the port or path, nor may they appear in the middle of a hostname label. | ||||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Perhaps this can be clarified to say that a wildcard may be used in place, but not within a hostname label? I think that's right isn't it? |
||||||||||||
| // For example, '*.example.com' is valid, but 'example*.*.com' is not. | ||||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This example is a little confusing because you move from a leading |
||||||||||||
| // Each wildcard matches only a single domain label, | ||||||||||||
| // so '*.io' does **not** match '*.k8s.io'. | ||||||||||||
QiWang19 marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||||||||
| // | ||||||||||||
| // A 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. | ||||||||||||
|
Comment on lines
+65
to
+68
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Use full sentences, not bullets |
||||||||||||
| // | ||||||||||||
| // Example values of matchImages: | ||||||||||||
| // - 123456789.dkr.ecr.us-east-1.amazonaws.com | ||||||||||||
| // - *.azurecr.io | ||||||||||||
| // - gcr.io | ||||||||||||
| // - *.*.registry.io | ||||||||||||
| // - registry.io:8080/path | ||||||||||||
|
Comment on lines
+70
to
+75
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think this will render as you expect. Try an I think you can fix this by adding a blank new line between each list entry |
||||||||||||
| // | ||||||||||||
| // +kubebuilder:validation:MaxItems=50 | ||||||||||||
| // +kubebuilder:validation:MinItems=1 | ||||||||||||
| // +listType=set | ||||||||||||
| // +required | ||||||||||||
| MatchImages []MatchImage `json:"matchImages,omitempty"` | ||||||||||||
| } | ||||||||||||
QiWang19 marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||||||||
|
|
||||||||||||
| // 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 | ||||||||||||
|
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @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.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 |
||||||||||||
|
|
||||||||||||
| // +k8s:deepcopy-gen=true | ||||||||||||
| // CRIOCredentialProviderConfigStatus defines the observed state of CRIOCredentialProviderConfig | ||||||||||||
| type CRIOCredentialProviderConfigStatus struct { | ||||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No other status fields? How do I know when I make a change to the spec that the system has observed and effected that change for me? Worth explaining at all that this triggers a node reboot across all nodes in the cluster? This is a fairly disruptive change and we should document that |
||||||||||||
| // conditions represent the latest available observations of the configuration state. | ||||||||||||
| // When omitted or empty, it indicates that no conditions have been reported yet. | ||||||||||||
| // The maximum number of conditions is 4. | ||||||||||||
| // Conditions are stored as a map keyed by condition type, ensuring uniqueness. | ||||||||||||
| // | ||||||||||||
| // Expected condition types include: | ||||||||||||
| // - "Validated": indicates whether the matchImages configuration is valid | ||||||||||||
|
Comment on lines
+109
to
+110
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Use full sentences not bullets. Known conditions types are Validated and ... (Are there more?) |
||||||||||||
| // +optional | ||||||||||||
| // +kubebuilder:validation:MaxItems=4 | ||||||||||||
| // +listType=map | ||||||||||||
| // +listMapKey=type | ||||||||||||
| Conditions []metav1.Condition `json:"conditions,omitempty"` | ||||||||||||
| } | ||||||||||||
|
|
||||||||||||
| // +k8s:deepcopy-gen:interfaces=k8s.io/apimachinery/pkg/runtime.Object | ||||||||||||
|
|
||||||||||||
| // CRIOCredentialProviderConfigList contains a list of CRIOCredentialProviderConfig resources | ||||||||||||
| // | ||||||||||||
| // Compatibility level 4: No compatibility is provided, the API can change at any point for any reason. These capabilities should not be used by applications needing long term support. | ||||||||||||
| // +openshift:compatibility-gen:level=4 | ||||||||||||
| type CRIOCredentialProviderConfigList struct { | ||||||||||||
| metav1.TypeMeta `json:",inline"` | ||||||||||||
|
|
||||||||||||
| // metadata is the standard list's metadata. | ||||||||||||
| // More info: https://git.k8s.io/community/contributors/devel/sig-architecture/api-conventions.md#metadata | ||||||||||||
| metav1.ListMeta `json:"metadata"` | ||||||||||||
|
|
||||||||||||
| Items []CRIOCredentialProviderConfig `json:"items"` | ||||||||||||
| } | ||||||||||||
|
|
||||||||||||
| const ( | ||||||||||||
| // ConditionTypeValidated is a condition type that indicates whether the CRIOCredentialProviderConfig | ||||||||||||
| // matchImages configuration has been validated successfully. | ||||||||||||
| // When True, all matchImage patterns are valid and have been applied. | ||||||||||||
| // When False, the configuration contains errors (see Reason for details). | ||||||||||||
| // Possible reasons for False status: | ||||||||||||
| // - ValidationFailed: matchImages contains invalid patterns | ||||||||||||
| // - ConfigurationPartiallyApplied: some matchImage entries were ignored due to conflicts | ||||||||||||
| ConditionTypeValidated = "Validated" | ||||||||||||
|
|
||||||||||||
| // ReasonValidationFailed is a condition reason used with ConditionTypeValidated=False | ||||||||||||
| // to indicate that the matchImages configuration contains one or more invalid registry patterns | ||||||||||||
| // that do not conform to the required format (valid FQDN with optional wildcard, port, and path). | ||||||||||||
| ReasonValidationFailed = "ValidationFailed" | ||||||||||||
|
|
||||||||||||
| // ReasonConfigurationPartiallyApplied is a condition reason used with ConditionTypeValidated=False | ||||||||||||
| // to indicate that some matchImage entries were ignored due to conflicts or overlapping patterns. | ||||||||||||
| // The condition message will contain details about which entries were ignored and why. | ||||||||||||
| ReasonConfigurationPartiallyApplied = "ConfigurationPartiallyApplied" | ||||||||||||
| ) | ||||||||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: Missing final new line char