Update CRIOCredentialProviderConfig doc to clarify actual design#1988
Update CRIOCredentialProviderConfig doc to clarify actual design#1988QiWang19 wants to merge 1 commit into
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
Signed-off-by: Qi Wang <qiwan@redhat.com>
QiWang19
force-pushed
the
crio-credential-config
branch
from
c5a27b5 to
166bb27
Compare
|
@QiWang19: all tests passed! 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. |
| - Generates and rolls out the `CRIOCredentialProviderConfig` to [CredentialProviderConfig](https://kubernetes.io/docs/reference/config-api/kubelet-config.v1/#kubelet-config-k8s-io-v1-CredentialProviderConfig) file to all cluster nodes | ||
| - Generates and rolls out the `CRIOCredentialProviderConfig` to [CredentialProviderConfig](https://kubernetes.io/docs/reference/config-api/kubelet-config.v1/#kubelet-config-k8s-io-v1-CredentialProviderConfig) file to all cluster nodes via `97-<pool>-generated-credentialproviderconfig` MachineConfig objects | ||
|
|
||
| - For non-cloud platforms, a daemon reload is required to reload the systemd service files before restarting kubelet. The node disruption policy includes a `DaemonReload` action for the drop-in file path, followed by a kubelet restart |
There was a problem hiding this comment.
non-cloud as in baremetal? shift on stack? vmware?
There was a problem hiding this comment.
oh I see, on platforms that already don't have a credential provider set. maybe word it that way
There was a problem hiding this comment.
Compared against the merged implementation and openshift-docs#111160. See inline comments.
Cross-cutting issues:
- Docs PR says cloud-only, but this enhancement adds non-cloud support. Needs coordination.
- RBAC verbs: docs PR has
["get", "list"], enhancement has["get"]. Which is correct? - Provider ordering: inconsistent between Step 1 (line 195, platform provider first) and Example Usage (line 462, crio first).
- RBAC subject (line 154):
kind: User+system:serviceaccount:...should bekind: ServiceAccountwithname/namespace.
| // +kubebuilder:validation:MinItems=1 | ||
| // +listType=set | ||
| // +optional | ||
| MatchImages []MatchImage `json:"matchImages,omitempty"` |
There was a problem hiding this comment.
The matchImages godoc here is much shorter than the merged implementation. Key omissions:
- Precedence rules (built-in providers like ECR/GCR/ACR take precedence)
- Overlap warning (avoid configuring patterns that overlap with platform providers)
- Status check guidance (check conditions for ignored entries)
- Detailed wildcard matching semantics
At minimum, the precedence rule and overlap warning should be included since they affect user configuration.
| // CRIOCredentialProviderConfig holds cluster-wide singleton resource configurations for CRI-O credential provider, | ||
| // the name of this instance is "cluster". | ||
| // | ||
| // The resource is a singleton named "cluster". |
There was a problem hiding this comment.
Lines 326 and 328 both say the resource is a singleton named "cluster". The implementation uses a more descriptive first paragraph instead. Consider matching it.
| Backward compatibility is maintained with existing mirror pulling process. | ||
|
|
||
| During a downgrade, the CRD and the `crio-credential-provider` support will be removed. The downgrade process should include deleting any `CRIOCredentialProviderConfig` CR instances and then allowing the CRD to be removed. | ||
| During a downgrade, the CRD and the `crio-credential-provider` support will be removed. The downgrade process should include removing the `matchImages` from the singleton "cluster" CR. |
There was a problem hiding this comment.
Incomplete: why must the admin clear matchImages before downgrading? Presumably to trigger cleanup of 97-<pool>-generated-credentialproviderconfig MachineConfig objects. State this explicitly. Also: what about non-cloud platform artifacts (systemd drop-in, generic config)? What happens if the admin skips this step?
| - Non-cloud platforms (bare-metal, vSphere, etc.): creates a new file at `/etc/kubernetes/credential-providers/generic-credential-provider.yaml` and generates a systemd drop-in unit at `/etc/systemd/system/kubelet.service.d/40-kubelet-crio-image-credential-provider.conf` | ||
|
|
||
| - trigger kubelet restarts to ensure new settings take effect on each configuration update | ||
| - create/update `97-<pool>-generated-credentialproviderconfig` MachineConfig objects for each MachineConfigPool |
There was a problem hiding this comment.
Missing: what does the controller do when matchImages is emptied (reverted to spec: {})? Delete the MachineConfig objects, update them with empty providers, or leave them? This is the "deactivation" path and ties into the downgrade strategy.
|
|
||
| - For cloud platforms (AWS, GCP, Azure): merges the `crio-credential-provider` entry into the existing platform-specific `CredentialProviderConfig` file `/etc/kubernetes/credential-providers/<platform>-credential-provider.yaml` | ||
|
|
||
| - For non-cloud platforms: creates a generic credential provider config file at `/etc/kubernetes/credential-providers/generic-credential-provider.yaml` and a systemd drop-in unit at `/etc/systemd/system/kubelet.service.d/40-kubelet-crio-image-credential-provider.conf` |
There was a problem hiding this comment.
The docs PR openshift-docs#111160 says this feature is cloud-only (AWS, GCP, Azure). Please coordinate: if non-cloud is implemented, the docs need updating; if not yet, note it here as planned.
There was a problem hiding this comment.
Done! Thanks @QiWang19 @saschagrunert
| // +listType=map | ||
| // +listMapKey=type | ||
| Conditions []metav1.Condition `json:"conditions,omitempty" patchStrategy:"merge" patchMergeKey:"type"` | ||
| Conditions []metav1.Condition `json:"conditions,omitempty"` |
There was a problem hiding this comment.
The implementation has richer godoc here: expected condition types and nil semantics. Consider matching it.
4 participants
Update the design doc to align with the merged implementations