test api review #42
test api review #42
Conversation
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| @@ -125,6 +127,35 @@ func validateCredentialProviderConfig(config *kubeletconfig.CredentialProviderCo | |||
| if provider.DefaultCacheDuration != nil && provider.DefaultCacheDuration.Duration < 0 { | |||
| allErrs = append(allErrs, field.Invalid(fieldPath.Child("defaultCacheDuration"), provider.DefaultCacheDuration.Duration, "defaultCacheDuration must be greater than or equal to 0")) | |||
| } | |||
|
|
|||
| if provider.TokenAttributes != nil { | |||
| fldPath := fieldPath.Child("tokenAttributes") | |||
There was a problem hiding this comment.
For increased readability, it is better to define fldPath here rather than at line 133.
| if provider.TokenAttributes != nil { | ||
| fldPath := fieldPath.Child("tokenAttributes") | ||
| if !saTokenForCredentialProviders { | ||
| allErrs = append(allErrs, field.Forbidden(fldPath, "tokenAttributes is not supported when KubeletServiceAccountTokenForCredentialProviders feature gate is disabled")) |
There was a problem hiding this comment.
Suggest adding a Reason field to the error message, providing a one-word, CamelCase description of why tokenAttributes are forbidden.
| @@ -125,6 +127,35 @@ func validateCredentialProviderConfig(config *kubeletconfig.CredentialProviderCo | |||
| if provider.DefaultCacheDuration != nil && provider.DefaultCacheDuration.Duration < 0 { | |||
| allErrs = append(allErrs, field.Invalid(fieldPath.Child("defaultCacheDuration"), provider.DefaultCacheDuration.Duration, "defaultCacheDuration must be greater than or equal to 0")) | |||
There was a problem hiding this comment.
Suggest adding validation to ensure serviceAccountTokenAudience is a valid value (e.g., not too long, valid characters).
| if provider.TokenAttributes != nil { | ||
| fldPath := fieldPath.Child("tokenAttributes") | ||
| if !saTokenForCredentialProviders { | ||
| allErrs = append(allErrs, field.Forbidden(fldPath, "tokenAttributes is not supported when KubeletServiceAccountTokenForCredentialProviders feature gate is disabled")) |
There was a problem hiding this comment.
Suggest adding a link to documentation explaining why tokenAttributes is only supported with the feature gate enabled, and a message indicating next steps a user can take.
| @@ -125,6 +127,35 @@ func validateCredentialProviderConfig(config *kubeletconfig.CredentialProviderCo | |||
| if provider.DefaultCacheDuration != nil && provider.DefaultCacheDuration.Duration < 0 { | |||
| allErrs = append(allErrs, field.Invalid(fieldPath.Child("defaultCacheDuration"), provider.DefaultCacheDuration.Duration, "defaultCacheDuration must be greater than or equal to 0")) | |||
| } | |||
|
|
|||
| if provider.TokenAttributes != nil { | |||
There was a problem hiding this comment.
requireServiceAccount is a pointer, so allErrs = append(allErrs, field.Required(fldPath.Child("requireServiceAccount"), "requireServiceAccount is required")) will always fail. field.Required should be replaced to check nil pointer instead. It can be also simplified by not using pointer and assigning it with default value.
| allErrs = append(allErrs, field.Forbidden(fldPath, "tokenAttributes is not supported when KubeletServiceAccountTokenForCredentialProviders feature gate is disabled")) | ||
| } | ||
| if len(provider.TokenAttributes.ServiceAccountTokenAudience) == 0 { | ||
| allErrs = append(allErrs, field.Required(fldPath.Child("serviceAccountTokenAudience"), "serviceAccountTokenAudience is required")) |
There was a problem hiding this comment.
Suggest adding a Reason field to the error message, providing a one-word, CamelCase description of why serviceAccountTokenAudience is required.
| if provider.TokenAttributes != nil { | ||
| fldPath := fieldPath.Child("tokenAttributes") | ||
| if !saTokenForCredentialProviders { | ||
| allErrs = append(allErrs, field.Forbidden(fldPath, "tokenAttributes is not supported when KubeletServiceAccountTokenForCredentialProviders feature gate is disabled")) |
There was a problem hiding this comment.
Suggest checking provider.APIVersion before any field under TokenAttributes is checked.
| allErrs = append(allErrs, field.Required(fldPath.Child("serviceAccountTokenAudience"), "serviceAccountTokenAudience is required")) | ||
| } | ||
| if provider.TokenAttributes.RequireServiceAccount == nil { | ||
| allErrs = append(allErrs, field.Required(fldPath.Child("requireServiceAccount"), "requireServiceAccount is required")) |
There was a problem hiding this comment.
Suggest adding a Reason field to the error message, providing a one-word, CamelCase description of why requireServiceAccount is required.
| allErrs = append(allErrs, field.Required(fldPath.Child("serviceAccountTokenAudience"), "serviceAccountTokenAudience is required")) | ||
| } | ||
| if provider.TokenAttributes.RequireServiceAccount == nil { | ||
| allErrs = append(allErrs, field.Required(fldPath.Child("requireServiceAccount"), "requireServiceAccount is required")) |
There was a problem hiding this comment.
Consider moving the validation logic for serviceAccountAnnotationKeys keys to a separate helper function to improve readability and maintainability.
| allErrs = append(allErrs, field.Required(fldPath.Child("requireServiceAccount"), "requireServiceAccount is required")) | ||
| } | ||
| if provider.APIVersion != credentialproviderv1.SchemeGroupVersion.String() { | ||
| allErrs = append(allErrs, field.Forbidden(fldPath, fmt.Sprintf("tokenAttributes is only supported for %s API version", credentialproviderv1.SchemeGroupVersion.String()))) |
There was a problem hiding this comment.
Suggest adding a Reason field to the error message, providing a one-word, CamelCase description of why the APIVersion is forbidden when it is not equal to credentialproviderv1.SchemeGroupVersion.String().
| if provider.TokenAttributes.RequireServiceAccount == nil { | ||
| allErrs = append(allErrs, field.Required(fldPath.Child("requireServiceAccount"), "requireServiceAccount is required")) | ||
| } | ||
| if provider.APIVersion != credentialproviderv1.SchemeGroupVersion.String() { |
There was a problem hiding this comment.
It looks like the key is converted to lower case before validation. Should it be converted to lower case before inserting it to seenAnnotationKeys to ensure that two identical keys with different cases are treated as duplicates?
| // Using the validation logic for keys from https://github.com/kubernetes/kubernetes/blob/69dbc74417304328a9fd3c161643dc4f0a057f41/staging/src/k8s.io/apimachinery/pkg/api/validation/objectmeta.go#L46-L51 | ||
| for _, k := range provider.TokenAttributes.ServiceAccountAnnotationKeys { | ||
| // The rule is QualifiedName except that case doesn't matter, so convert to lowercase before checking. | ||
| for _, msg := range validation.IsQualifiedName(strings.ToLower(k)) { |
There was a problem hiding this comment.
It is advised to use a for loop or other looping construct to iterate through a collection instead of directly accessing each element in the collection. This improves code readability and maintainability.
| if provider.APIVersion != credentialproviderv1.SchemeGroupVersion.String() { | ||
| allErrs = append(allErrs, field.Forbidden(fldPath, fmt.Sprintf("tokenAttributes is only supported for %s API version", credentialproviderv1.SchemeGroupVersion.String()))) | ||
| } | ||
|
|
There was a problem hiding this comment.
It is recommended to provide documentation that suggests how these Keys are passed in by the user. Perhaps there's more information on the format that is required. Are there character limits? Are there suggested naming conventions?
| for _, k := range provider.TokenAttributes.ServiceAccountAnnotationKeys { | ||
| // The rule is QualifiedName except that case doesn't matter, so convert to lowercase before checking. | ||
| for _, msg := range validation.IsQualifiedName(strings.ToLower(k)) { | ||
| allErrs = append(allErrs, field.Invalid(fldPath.Child("serviceAccountAnnotationKeys"), k, msg)) |
There was a problem hiding this comment.
It's important to consider that k represents an annotation key, and as such, should be validated against the annotation key constraints rather than the qualified name constraints. Using IsQualifiedName is not semantically correct for annotation keys. Suggest using validation.IsAnnotationKey instead.
| for _, k := range provider.TokenAttributes.ServiceAccountAnnotationKeys { | ||
| // The rule is QualifiedName except that case doesn't matter, so convert to lowercase before checking. | ||
| for _, msg := range validation.IsQualifiedName(strings.ToLower(k)) { | ||
| allErrs = append(allErrs, field.Invalid(fldPath.Child("serviceAccountAnnotationKeys"), k, msg)) |
There was a problem hiding this comment.
Suggest adding a link to documentation explaining why each annotation key should be a valid Kubernetes annotation key, and what next steps to take to resolve the error.
| } | ||
|
|
||
| type serviceAccountProvider struct { | ||
| audience string |
There was a problem hiding this comment.
The comment mentions tokenAttributes.serviceAccountAnnotationKeys. It should be TokenAttributes.ServiceAccountAnnotationKeys to be consistent with the code.
| func newServiceAccountProvider( | ||
| provider kubeletconfig.CredentialProvider, | ||
| getServiceAccount func(namespace, name string) (*v1.ServiceAccount, error), | ||
| getServiceAccountToken func(namespace, name string, tr *authenticationv1.TokenRequest) (*authenticationv1.TokenRequest, error), |
There was a problem hiding this comment.
The parameter names namespace and name are ambiguous. Consider using podNamespace and serviceAccountName to make the intent clear.
| func RegisterCredentialProviderPlugins(pluginConfigFile, pluginBinDir string, | ||
| getServiceAccountToken func(namespace, name string, tr *authenticationv1.TokenRequest) (*authenticationv1.TokenRequest, error), | ||
| getServiceAccount func(namespace, name string) (*v1.ServiceAccount, error), | ||
| ) error { | ||
| if _, err := os.Stat(pluginBinDir); err != nil { | ||
| if os.IsNotExist(err) { | ||
| return fmt.Errorf("plugin binary directory %s did not exist", pluginBinDir) |
There was a problem hiding this comment.
It would be helpful to log the aggregated errors for debugging purposes.
| // requiredAnnotations is a map of annotation keys and values that the plugin requires to generate credentials | ||
| // that's defined in the tokenAttributes.serviceAccountAnnotationKeys field in the credential provider config. | ||
| func (s *serviceAccountProvider) getServiceAccountData(namespace, name string) (types.UID, map[string]string, error) { | ||
| sa, err := s.getServiceAccountFunc(namespace, name) |
There was a problem hiding this comment.
Consider adding a comment to explain why errors from these functions would not be returned, or why it is acceptable that the function would return immediately.
| } | ||
| } | ||
|
|
||
| // getServiceAccountData returns the service account UID and required annotations for the service account. |
There was a problem hiding this comment.
The parameter names podName and podNamespace are ambiguous. Consider using podNamespace and serviceAccountName to make the intent clear.
| func (s *serviceAccountProvider) getServiceAccountData(namespace, name string) (types.UID, map[string]string, error) { | ||
| sa, err := s.getServiceAccountFunc(namespace, name) | ||
| if err != nil { | ||
| return "", nil, err |
There was a problem hiding this comment.
Consider naming this variable keySingleFlight to reflect the nature of it.
| // getServiceAccountData returns the service account UID and required annotations for the service account. | ||
| // If the service account does not exist, an error is returned. | ||
| // requiredAnnotations is a map of annotation keys and values that the plugin requires to generate credentials | ||
| // that's defined in the tokenAttributes.serviceAccountAnnotationKeys field in the credential provider config. |
There was a problem hiding this comment.
The function name getServiceAccountFunc is ambiguous. Consider using GetServiceAccount to make the intent clear.
| if err != nil { | ||
| return "", nil, err | ||
| } | ||
|
|
There was a problem hiding this comment.
It might be cleaner to have the code extract this logic into a helper function for readability purposes.
| return "", nil, err | ||
| } | ||
|
|
||
| requiredAnnotations := make(map[string]string, len(s.serviceAccountAnnotationKeys)) |
There was a problem hiding this comment.
The function name getServiceAccountTokenFunc is ambiguous. Consider using GetServiceAccountToken to make the intent clear.
|
|
||
| requiredAnnotations := make(map[string]string, len(s.serviceAccountAnnotationKeys)) | ||
| for _, k := range s.serviceAccountAnnotationKeys { | ||
| val, ok := sa.Annotations[k] |
There was a problem hiding this comment.
Should the serviceAccountToken and requiredAnnotations be validated prior to being passed to the ExecPlugin?
|
|
||
| // Check if the credentials are cached and return them if found. | ||
| cachedConfig, found, errCache := p.getCachedCredentials(image, baseCacheKey) | ||
| if errCache != nil { | ||
| klog.Errorf("Failed to get cached docker config: %v", err) | ||
| return credentialprovider.DockerConfig{} |
There was a problem hiding this comment.
Cache keys, both for the key and the registry values, are constructed using the pod and service account metadata to improve isolation and security. However, in order to reduce memory footprint and improve performance of the cache, consider creating a fixed sized hash of this identifying information, rather than storing the strings directly. This will trade off the unlikely possibility of hash collisions against the memory savings from storing smaller data.
| } | ||
|
|
||
| return sa.UID, requiredAnnotations, nil | ||
| } |
There was a problem hiding this comment.
Consider adding a comment to explain the difference between the cacheKey and the singleFlightKey.
| } | ||
| } | ||
| res, err, _ := p.group.Do(singleFlightKey, func() (interface{}, error) { | ||
| return p.plugin.ExecPlugin(context.Background(), image, serviceAccountToken, requiredAnnotations) | ||
| }) |
There was a problem hiding this comment.
It would be beneficial to add a comment explaining the purpose of these parameters. This is to provide more context for developers using or maintaining this code in the future.
| func registerCredentialProviderPlugin(name string, p *pluginProvider) { | ||
| providersMutex.Lock() | ||
| defer providersMutex.Unlock() | ||
|
|
There was a problem hiding this comment.
Suggestion: Consider using a more descriptive name for the provider struct's field impl, like pluginImpl or credentialProviderImpl to improve readability.
| "k8s.io/kubernetes/pkg/credentialprovider" | ||
| "k8s.io/kubernetes/pkg/features" | ||
| ) | ||
|
|
There was a problem hiding this comment.
Consider renaming provider to registeredProvider for clarity. The current name is ambiguous, as it could refer to the interface credentialprovider.DockerConfigProvider being implemented. The new name would more accurately reflect that it represents a registered plugin.
| seenProviderNames.Insert(name) | ||
|
|
||
| providers = append(providers, provider{name, p}) | ||
| klog.V(4).Infof("Registered credential provider %q", name) |
There was a problem hiding this comment.
Suggestion: Add error handling if seenProviderNames.Insert(name) fails. Although sets.NewString() is unlikely to fail, adding a check would make the code more robust.
| type provider struct { | ||
| name string | ||
| impl *pluginProvider | ||
| } |
There was a problem hiding this comment.
Consider using sync.Map instead of sync.RWMutex and a slice for providers and seenProviderNames. sync.Map is optimized for concurrent read and write scenarios, which seems applicable here as providers are registered and then frequently looked up. Using sync.Map would also remove the need for the seenProviderNames set.
| var pp *perPodPluginProvider | ||
| if utilfeature.DefaultFeatureGate.Enabled(features.KubeletServiceAccountTokenForCredentialProviders) { | ||
| klog.V(4).InfoS("Generating per pod credential provider", "provider", p.name, "podName", podName, "podNamespace", podNamespace, "podUID", podUID, "serviceAccountName", serviceAccountName) | ||
| pp = &perPodPluginProvider{provider: p.impl, podName: podName, podNamespace: podNamespace, podUID: types.UID(podUID), serviceAccountName: serviceAccountName} |
There was a problem hiding this comment.
Suggestion: Rename podName, podNamespace, and podUID to name, namespace, and UID for less verbose logs.
| seenProviderNames.Insert(name) | ||
|
|
||
| providers = append(providers, provider{name, p}) | ||
| klog.V(4).Infof("Registered credential provider %q", name) |
There was a problem hiding this comment.
externalCredentialProviderKeyring is exposed, but it is not documented. Add a comment explaining its purpose.
| klog.V(4).Infof("Registered credential provider %q", name) | ||
| } | ||
|
|
||
| type externalCredentialProviderKeyring struct { |
There was a problem hiding this comment.
nit: Consider initializing the slice with providers[:0] instead of make([]credentialprovider.DockerConfigProvider, 0, len(providers)). While both achieve the same outcome, providers[:0] might be slightly more efficient, though the performance difference would be negligible.
| providers []credentialprovider.DockerConfigProvider | ||
| } | ||
|
|
||
| func NewExternalCredentialProviderDockerKeyring(podName, podNamespace, podUID, serviceAccountName string) credentialprovider.DockerKeyring { |
There was a problem hiding this comment.
It might be clearer to pull the feature gate check out into its own if statement before defining pp. This would make the logic a bit easier to read.
| } | ||
|
|
||
| func NewExternalCredentialProviderDockerKeyring(podName, podNamespace, podUID, serviceAccountName string) credentialprovider.DockerKeyring { | ||
| providersMutex.RLock() |
There was a problem hiding this comment.
Add a comment that the feature gate KubeletServiceAccountTokenForCredentialProviders enables the passing of pod information to the credential provider plugin. This will clarify the conditional logic.
| @@ -125,6 +127,35 @@ func validateCredentialProviderConfig(config *kubeletconfig.CredentialProviderCo | |||
| if provider.DefaultCacheDuration != nil && provider.DefaultCacheDuration.Duration < 0 { | |||
| allErrs = append(allErrs, field.Invalid(fieldPath.Child("defaultCacheDuration"), provider.DefaultCacheDuration.Duration, "defaultCacheDuration must be greater than or equal to 0")) | |||
| } | |||
|
|
|||
| if provider.TokenAttributes != nil { | |||
There was a problem hiding this comment.
The code states serviceAccountTokenAudience is required and requireServiceAccount is required. Is it possible to provide default values if these are not set? Are there valid use cases where these values are not defined and default values would work?
| if provider.TokenAttributes != nil { | ||
| fldPath := fieldPath.Child("tokenAttributes") | ||
| if !saTokenForCredentialProviders { | ||
| allErrs = append(allErrs, field.Forbidden(fldPath, "tokenAttributes is not supported when KubeletServiceAccountTokenForCredentialProviders feature gate is disabled")) |
There was a problem hiding this comment.
This validation logic requires provider.APIVersion == credentialproviderv1.SchemeGroupVersion.String(). This seems too restrictive. The code should ensure the the TokenAttributes are aligned with the APIVersion if set, or it should be set by default, but it should not simply reject other API versions if the KubeletServiceAccountTokenForCredentialProviders feature gate is enabled.
| if len(provider.TokenAttributes.ServiceAccountTokenAudience) == 0 { | ||
| allErrs = append(allErrs, field.Required(fldPath.Child("serviceAccountTokenAudience"), "serviceAccountTokenAudience is required")) | ||
| } | ||
| if provider.TokenAttributes.RequireServiceAccount == nil { |
There was a problem hiding this comment.
Consider creating a constant for the string "serviceAccountAnnotationKeys" to avoid duplication and potential typos. This makes code easier to maintain and change.
| } | ||
| if provider.TokenAttributes.RequireServiceAccount == nil { | ||
| allErrs = append(allErrs, field.Required(fldPath.Child("requireServiceAccount"), "requireServiceAccount is required")) | ||
| } |
There was a problem hiding this comment.
The comment refers to code from a specific commit. It would be better to explain the validation rule here directly, rather than referring to external code (which may change).
| if provider.TokenAttributes.RequireServiceAccount == nil { | ||
| allErrs = append(allErrs, field.Required(fldPath.Child("requireServiceAccount"), "requireServiceAccount is required")) | ||
| } | ||
| if provider.APIVersion != credentialproviderv1.SchemeGroupVersion.String() { |
There was a problem hiding this comment.
strings.ToLower(k) modifies the keys for validation purposes, but the original case is then added to the seenAnnotationKeys. This could lead to incorrect duplication detection if keys with different casing but identical lowercase representations are provided. Consider using the lowercase version for both validation and duplication checks for consistency.
| if provider.APIVersion != credentialproviderv1.SchemeGroupVersion.String() { | ||
| allErrs = append(allErrs, field.Forbidden(fldPath, fmt.Sprintf("tokenAttributes is only supported for %s API version", credentialproviderv1.SchemeGroupVersion.String()))) | ||
| } | ||
|
|
There was a problem hiding this comment.
This check seenAnnotationKeys.Has(k) will use the original case of the annotation key. This should be consistent with the validation that converts all keys to lowercase before evaluation. The logic should convert k to lowercase before calling Has(k) to ensure that the keys are not case-sensitive, consistent with the validation.
| @@ -28,12 +28,18 @@ import ( | |||
| "sync" | |||
| "time" | |||
|
|
|||
| "golang.org/x/crypto/cryptobyte" | |||
There was a problem hiding this comment.
Consider adding comments to explain the purpose of these imports.
| @@ -42,6 +48,7 @@ import ( | |||
| credentialproviderv1alpha1 "k8s.io/kubelet/pkg/apis/credentialprovider/v1alpha1" | |||
There was a problem hiding this comment.
The getServiceAccountToken and getServiceAccount functions are injected as dependencies. Add comments explaining their purpose, preconditions, and postconditions. For example, what happens if these functions return an error or nil? How should the caller handle these cases?
| @@ -150,6 +162,7 @@ func newPluginProvider(pluginBinDir string, provider kubeletconfig.CredentialPro | |||
| envVars: provider.Env, | |||
There was a problem hiding this comment.
It's not clear where the values for provider.TokenAttributes.RequireServiceAccount and provider.TokenAttributes.ServiceAccountTokenAudience come from. Add comments describing their purpose and relationship to the credential provider config. What happens if RequireServiceAccount is nil? It's being dereferenced without a nil check.
| // If the service account does not exist, an error is returned. | ||
| // requiredAnnotations is a map of annotation keys and values that the plugin requires to generate credentials | ||
| // that's defined in the tokenAttributes.serviceAccountAnnotationKeys field in the credential provider config. | ||
| func (s *serviceAccountProvider) getServiceAccountData(namespace, name string) (types.UID, map[string]string, error) { |
There was a problem hiding this comment.
The comment "unimplemented" is not helpful. Add a comment explaining the function of this struct.
| return "", nil, err | ||
| } | ||
|
|
||
| requiredAnnotations := make(map[string]string, len(s.serviceAccountAnnotationKeys)) |
There was a problem hiding this comment.
Since all types are Entry use a common type like credentialprovider.Type and explain what each type signifies.
| return "", err | ||
| } | ||
|
|
||
| return tr.Status.Token, nil | ||
| } |
There was a problem hiding this comment.
It would be more efficient to check !found rather than found == false.
| provider *pluginProvider | ||
|
|
||
| podName string | ||
| podNamespace string |
There was a problem hiding this comment.
What does it mean for a feature gate to be considered in token mode?
| // from cache or the exec plugin. | ||
| func (p *pluginProvider) Provide(image string) credentialprovider.DockerConfig { | ||
| func (p *pluginProvider) provide(image, podName, podNamespace string, podUID types.UID, serviceAccountName string) credentialprovider.DockerConfig { | ||
| if !p.isImageAllowed(image) { |
There was a problem hiding this comment.
Consider adding error handling in the caching logic. What does !found mean? Are there other actions the code should take?
| return credentialprovider.DockerConfig{} | ||
| } | ||
|
|
||
| if serviceAccountUID, requiredAnnotations, err = p.serviceAccountProvider.getServiceAccountData(podNamespace, serviceAccountName); err != nil { |
There was a problem hiding this comment.
Consider adding an error log if response.CacheDuration is invalid.
| } | ||
| } | ||
| res, err, _ := p.group.Do(singleFlightKey, func() (interface{}, error) { | ||
| return p.plugin.ExecPlugin(context.Background(), image, serviceAccountToken, requiredAnnotations) | ||
| }) | ||
|
|
||
| if err != nil { |
There was a problem hiding this comment.
It's possible that p.clock.Now() could change between the After and Add calls, leading to unexpected behavior. Consider caching the value of p.clock.Now() before the if statement and reusing it.
| cacheKey, err = p.generateCacheKey(cacheKey, baseCacheKey) | ||
| if err != nil { | ||
| klog.Errorf("Error generating cache key: %v", err) | ||
| return credentialprovider.DockerConfig{} |
There was a problem hiding this comment.
Add comments for the role each parameter is performing in the ExecPlugin function.
| @@ -377,10 +556,10 @@ type execPlugin struct { | |||
| // | |||
There was a problem hiding this comment.
Consider adding a unit test for this function with various inputs.
| klog.V(5).Infof("Getting image %s credentials from external exec plugin %s", image, e.name) | ||
|
|
||
| authRequest := &credentialproviderapi.CredentialProviderRequest{Image: image} | ||
| authRequest := &credentialproviderapi.CredentialProviderRequest{Image: image, ServiceAccountToken: serviceAccountToken, ServiceAccountAnnotations: serviceAccountAnnotations} | ||
| data, err := e.encodeRequest(authRequest) | ||
| if err != nil { |
There was a problem hiding this comment.
The use of uint32 for the length of annotations could be problematic if the number of annotations exceeds the maximum value of uint32, which is highly unlikely but should be handled.
| type provider struct { | ||
| name string | ||
| impl *pluginProvider | ||
| } |
There was a problem hiding this comment.
It's not obvious how the provider struct relates to the overall goal. Adding a comment explaining its role (e.g., "provider holds the plugin name and its implementation") would improve readability.
What type of PR is this?
What this PR does / why we need it:
Which issue(s) this PR fixes:
Fixes #
Special notes for your reviewer:
Does this PR introduce a user-facing change?
Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.: