Hackathon test api review for an open PR against k/k #128372 that has a huge diff #36
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")) | |||
There was a problem hiding this comment.
Consider adding a comment that indicates the reason why serviceAccountTokenAudience is required. This will help future developers understand the intent of the validation.
| @@ -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.
Consider adding a comment that indicates the reason why requireServiceAccount is required. This will help future developers understand the intent of the validation.
| 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 error message should include the API Group as well as the version, because without that information it can be hard to understand which API version it is referring to.
| 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.
Consider extracting this logic into its own function. This will help to keep the validateCredentialProviderConfig function small and easy to understand.
| } | ||
| if len(provider.TokenAttributes.ServiceAccountTokenAudience) == 0 { | ||
| allErrs = append(allErrs, field.Required(fldPath.Child("serviceAccountTokenAudience"), "serviceAccountTokenAudience is required")) | ||
| } |
There was a problem hiding this comment.
Add comments that state the source of this validation logic.
| 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.
Validation functions from k8s.io/apimachinery return a slice of strings, so there's no need to iterate over them. You can just append them to allErrs directly.
| 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.
Consider using sets.String.Insert(strings.ToLower(k)) for consistency with line 140.
| @@ -42,6 +48,7 @@ import ( | |||
| credentialproviderv1alpha1 "k8s.io/kubelet/pkg/apis/credentialprovider/v1alpha1" | |||
| credentialproviderv1beta1 "k8s.io/kubelet/pkg/apis/credentialprovider/v1beta1" | |||
| "k8s.io/kubernetes/pkg/credentialprovider" | |||
| "k8s.io/kubernetes/pkg/features" | |||
There was a problem hiding this comment.
Consider directly passing the boolean value of the feature gate instead of passing the entire utility function to minimize dependencies. This can improve testability and encapsulation.
| @@ -134,7 +147,6 @@ func newPluginProvider(pluginBinDir string, provider kubeletconfig.CredentialPro | |||
| } | |||
|
|
|||
| clock := clock.RealClock{} | |||
|
|
|||
| return &pluginProvider{ | |||
| clock: clock, | |||
| matchImages: provider.MatchImages, | |||
There was a problem hiding this comment.
The lastCachePurge field is updated inside a lock in getCachedCredentials but not in Provide. Consider adding a lock around the update in Provide to prevent race conditions.
| @@ -178,6 +191,82 @@ type pluginProvider struct { | |||
|
|
|||
| // lastCachePurge is the last time cache is cleaned for expired entries. | |||
There was a problem hiding this comment.
The featureGateEnabled variable is defined but not used. Remove the unused variable.
| if !ok { | ||
| continue | ||
| } | ||
| requiredAnnotations[k] = val |
There was a problem hiding this comment.
It is helpful to include documentation for each parameter in the getServiceAccountToken function, explaining its purpose and expected value.
| // that the plugin can use this information to get the pod's service account and generate bound service account tokens | ||
| // for plugins running in service account token mode. | ||
| type perPodPluginProvider struct { | ||
| provider *pluginProvider |
There was a problem hiding this comment.
The err variable is shadowed within the if p.serviceAccountProvider != nil block. Rename the err variable in the outer scope or use a different name in the inner scope to avoid shadowing.
| } | ||
|
|
||
| // Enabled always returns true since registration of the plugin via kubelet implies it should be enabled. | ||
| func (p *perPodPluginProvider) Enabled() bool { |
There was a problem hiding this comment.
It would be good to log what parameters are being passed to ExecPlugin, especially the image and if a service account token is being passed. This would be helpful for debugging.
| // Enabled always returns true since registration of the plugin via kubelet implies it should be enabled. | ||
| func (p *perPodPluginProvider) Enabled() bool { | ||
| return true | ||
| } |
There was a problem hiding this comment.
The err variable is shadowed within the res, err, _ := p.group.Do block. Rename the err variable in the outer scope or use a different name in the inner scope to avoid shadowing.
| var requiredAnnotations map[string]string | ||
| var err error | ||
| var baseCacheKey string | ||
|
|
There was a problem hiding this comment.
This block of code repeats itself. Consider creating a helper function to encapsulate this logic to reduce duplication.
| klog.Errorf("Failed to get service account %s/%s: %v", podNamespace, serviceAccountName, err) | ||
| return credentialprovider.DockerConfig{} | ||
| } | ||
| } |
There was a problem hiding this comment.
A singleflight key should be created if p.serviceAccountProvider != nil.
| @@ -280,6 +437,12 @@ func (p *pluginProvider) Provide(image string) credentialprovider.DockerConfig { | |||
| expiresAt = p.clock.Now().Add(response.CacheDuration.Duration) | |||
| } | |||
|
|
|||
| cacheKey, err = p.generateCacheKey(cacheKey, baseCacheKey) | |||
There was a problem hiding this comment.
It would be helpful to include documentation for each parameter in the ExecPlugin function, explaining its purpose and expected value. Also, add a brief description of what the ExecPlugin function does.
| return nil, false, fmt.Errorf("error generating cache key: %w", err) | ||
| } | ||
|
|
||
| obj, found, err = p.cache.GetByKey(cacheKey) |
There was a problem hiding this comment.
It would be helpful to include documentation for each parameter in the generateBaseCacheKey function, explaining its purpose and expected value. Also, add a brief description of what the generateBaseCacheKey function does.
| return nil, false, fmt.Errorf("error generating cache key: %w", err) | ||
| } | ||
|
|
||
| obj, found, err = p.cache.GetByKey(cacheKey) | ||
| if err != nil { | ||
| return nil, false, err | ||
| } |
There was a problem hiding this comment.
Consider adding validation to ensure that serviceAccountNamespace, serviceAccountName, and serviceAccountUID are not empty before adding them to the cache key. This prevents unexpected behavior if these values are missing.
| "k8s.io/kubernetes/pkg/credentialprovider" | ||
| "k8s.io/kubernetes/pkg/features" | ||
| ) | ||
|
|
There was a problem hiding this comment.
Consider using a more descriptive name than p for the loop variable in registerCredentialProviderPlugin. Something like pluginEntry would improve readability.
richabanker
force-pushed
the
hackathon
branch
from
039d256 to
99ea87f
Compare
richabanker
force-pushed
the
hackathon-test-api-review-1
branch
from
38d207a to
945dbd5
Compare
| @@ -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.
When KubeletServiceAccountTokenForCredentialProviders feature gate is disabled, the function should return immediately.
| @@ -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.
serviceAccountTokenAudience should have validation limits, and check that it has the correct format.
| 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.
It might be clearer to use provider.TokenAttributes.RequireServiceAccount == nil to check if RequireServiceAccount has a default value.
| 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.
Does tokenAttributes support other API versions? If it's going to support the new API, it would be better to have an if statement instead of direct comparison.
| 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.
The loop variable k should be a qualified name and not accept characters from other languages.
| 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.
saTokenForCredentialProvidersFeatureEnabled is calculated but not used directly within this function. It is only used within the validateCredentialProviderConfig function. Consider moving the calculation of this variable inside the validateCredentialProviderConfig function to reduce the scope of the variable. This would improve readability and reduce the chance of accidental misuse.
| } | ||
|
|
||
| type serviceAccountProvider struct { | ||
| audience string |
There was a problem hiding this comment.
featureGateEnabled and serviceAccountTokenAudienceSet are calculated and then used to determine if the serviceAccountProvider is initialized. If the serviceAccountProvider is nil in several places, then there will be a lot of if provider.serviceAccountProvider != nil { ... } checks. Consider returning an error from this function if the feature gate is not enabled, which would simplify the subsequent logic and make it easier to reason about.
| defer providersMutex.RUnlock() | ||
|
|
||
| keyring := &externalCredentialProviderKeyring{ | ||
| providers: make([]credentialprovider.DockerConfigProvider, 0, len(providers)), |
There was a problem hiding this comment.
It would be helpful to add a comment briefly explaining the purpose of checking if the feature gate is enabled. Specifically, indicate why the perPodPluginProvider is being initialized differently based on the feature gate.
|
|
||
| keyring := &externalCredentialProviderKeyring{ | ||
| providers: make([]credentialprovider.DockerConfigProvider, 0, len(providers)), | ||
| } |
There was a problem hiding this comment.
Consider adding more context on the klog message by including the feature gate being checked KubeletServiceAccountTokenForCredentialProviders.
| } | ||
|
|
||
| keyring.providers = append(keyring.providers, pp) | ||
| } |
There was a problem hiding this comment.
It would be helpful to add a comment explaining why a temporary BasicDockerKeyring is instantiated. Clarify the purpose of transferring provided auths to this temporary keyring.
| Lifetime: 5 * time.Minute, | ||
| }) | ||
| } | ||
|
|
||
| // CachingDockerConfigProvider implements DockerConfigProvider by composing |
There was a problem hiding this comment.
Consider removing the init() function and registering the default provider elsewhere. This makes the provider more configurable and testable.
| Provider: &defaultDockerConfigProvider{}, | ||
| Lifetime: 5 * time.Minute, | ||
| }, | ||
| }, |
There was a problem hiding this comment.
Instead of hardcoding the lifetime to 5 minutes, consider making it configurable via a flag or a configuration file. This would allow users to tune the caching behavior based on their specific needs and environment.
| @@ -37,17 +37,20 @@ const ( | |||
| // across the file. | |||
|
|
|||
| // owner: @aojea | |||
| // Deprecated: v1.31 | |||
There was a problem hiding this comment.
Consider adding a comment explaining what "Deprecated" means in this context (e.g., "Deprecated: This feature gate will be removed in a future release.")
| // | ||
| // Allow kubelet to request a certificate without any Node IP available, only | ||
| // with DNS names. | ||
| AllowDNSOnlyNodeCSR featuregate.Feature = "AllowDNSOnlyNodeCSR" | ||
|
|
||
| // owner: @HirazawaUi | ||
| // Deprecated: v1.32 | ||
| // | ||
| // Allow spec.terminationGracePeriodSeconds to be overridden by MaxPodGracePeriodSeconds in soft evictions. | ||
| AllowOverwriteTerminationGracePeriodSeconds featuregate.Feature = "AllowOverwriteTerminationGracePeriodSeconds" | ||
|
|
richabanker
force-pushed
the
hackathon-test-api-review-1
branch
from
945dbd5 to
6683b3e
Compare
| @@ -125,6 +127,35 @@ func validateCredentialProviderConfig(config *kubeletconfig.CredentialProviderCo | |||
| if provider.DefaultCacheDuration != nil && provider.DefaultCacheDuration.Duration < 0 { | |||
There was a problem hiding this comment.
Consider adding a feature gate check before accessing config.Providers[*].TokenAttributes. This mirrors the pattern used for fldPath and saTokenForCredentialProviders and ensures that the code accessing the TokenAttributes is only executed when the feature gate is enabled.
| @@ -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.
Change the error message to "The KubeletServiceAccountTokenForCredentialProviders feature gate must be enabled to use tokenAttributes". This provides a more user-friendly error message that clearly indicates how to resolve the issue.
| @@ -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.
Change the error message to "serviceAccountTokenAudience must be specified when tokenAttributes is provided". This clarifies the requirement and indicates that the field cannot be left empty when the parent field is used.
| 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.
Change the error message to "requireServiceAccount must be specified when tokenAttributes is provided". This clarifies the requirement and indicates that the field cannot be left empty when the parent field is used.
| } | ||
| if len(provider.TokenAttributes.ServiceAccountTokenAudience) == 0 { | ||
| allErrs = append(allErrs, field.Required(fldPath.Child("serviceAccountTokenAudience"), "serviceAccountTokenAudience is required")) | ||
| } |
There was a problem hiding this comment.
Change the error message to "tokenAttributes is only supported for the credentialprovider.kubelet.k8s.io/v1 API version". It's more conventional to display the full group/version string instead of relying on SchemeGroupVersion, making the error message self-contained.
| 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.
The comment // Using the validation logic for keys from... should be reworded to more concisely convey the intent, such as // Validate service account annotation keys.
| 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.
Add a check to see if credentialproviderv1.SchemeGroupVersion.String() is same as config.Providers[*].APIVersion. It doesn't make sense for saTokenForCredentialProviders to be enabled, but have an older APIVersion.
| 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.
Suggestion: It would be helpful to log the reason for validation failures to assist debugging.
| @@ -134,7 +147,6 @@ func newPluginProvider(pluginBinDir string, provider kubeletconfig.CredentialPro | |||
| } | |||
There was a problem hiding this comment.
Suggestion: Consider using a more descriptive name than serviceAccountProvider, such as tokenProvider or authProvider, to better reflect its purpose.
| @@ -178,6 +191,82 @@ type pluginProvider struct { | |||
|
|
|||
| // lastCachePurge is the last time cache is cleaned for expired entries. | |||
| lastCachePurge time.Time | |||
|
|
|||
| // serviceAccountProvider holds the logic for handling service account tokens when needed. | |||
There was a problem hiding this comment.
Suggestion: The comment says "...that's defined in the...". It should read, "...that are defined in...".
|
|
||
| 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.
Suggestion: Add a comment explaining the singleflight key.
| return sa.UID, requiredAnnotations, nil | ||
| } | ||
|
|
||
| // getServiceAccountToken returns a service account token for the service account. |
There was a problem hiding this comment.
Suggestion: The comments for types Plugin interface and execPlugin should both clarify what CredentialProviderResponse actually represents.
| if p.serviceAccountProvider != nil { | ||
| if singleFlightKey, err = p.generateCacheKey(singleFlightKey, baseCacheKey); err != nil { | ||
| klog.Errorf("Error generating singleflight key: %v", err) | ||
| return credentialprovider.DockerConfig{} |
There was a problem hiding this comment.
Suggestion: Given the added complexity of service account token mode, consider adding more structured logging, for example, logging the service account details or success/failure events related to token retrieval.
| @@ -331,7 +499,13 @@ func (p *pluginProvider) getCachedCredentials(image string) (credentialprovider. | |||
| } | |||
There was a problem hiding this comment.
Suggestion: Use backticks for 1., 2. to be consistent with the rest of the document.
| cacheKey, err = p.generateCacheKey(registry, baseCacheKey) | ||
| if err != nil { | ||
| return nil, false, fmt.Errorf("error generating cache key: %w", err) | ||
| } |
There was a problem hiding this comment.
Suggestion: The comment does not accurately describe all of the base cache key's components. It should include a reference to the service account UID and the annotations.
| return nil, false, fmt.Errorf("error generating cache key: %w", err) | ||
| } | ||
|
|
||
| obj, found, err = p.cache.GetByKey(cacheKey) | ||
| if err != nil { | ||
| return nil, false, err | ||
| } |
There was a problem hiding this comment.
Suggestion: It is not clear how cryptobyte ensures security of the keys.
| providersMutex.Lock() | ||
| defer providersMutex.Unlock() | ||
|
|
||
| if seenProviderNames.Has(name) { |
There was a problem hiding this comment.
Consider using a more descriptive name than p for the loop variable in registerCredentialProviderPlugin, such as providerEntry or currProvider, to improve readability.
| keyring := &externalCredentialProviderKeyring{ | ||
| providers: make([]credentialprovider.DockerConfigProvider, 0, len(providers)), | ||
| } | ||
|
|
There was a problem hiding this comment.
It would be helpful to add a comment explaining why externalCredentialProviderKeyring is needed, what problem does this struct solves, and how does it interacts with DockerConfigProvider and credentialprovider.DockerKeyring interface.
| 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.
The KubeletServiceAccountTokenForCredentialProviders feature gate name is quite long. Is there a shorter, more conventional name that could be used? Perhaps related to "PerPodCredentials" or "ScopedCredentials"?
| 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} | ||
| } else { |
There was a problem hiding this comment.
Consider using klog.InfoS's arguments to set the podUID as types.UID since you are already converting the type and will allow to print a more accurate value for debugging.
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.: