test api review #41

richabanker · 2025-02-20T01:37:46Z

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.:

github-actions · 2025-02-20T01:38:16Z

pkg/credentialprovider/plugin/config.go

@@ -70,7 +72,7 @@ func decode(data []byte) (*kubeletconfig.CredentialProviderConfig, error) {
 }


Suggestion: It would be helpful to add a comment explaining what saTokenForCredentialProviders represents.

github-actions · 2025-02-20T01:38:17Z

pkg/credentialprovider/plugin/config.go

+
+		if provider.TokenAttributes != nil {
+			fldPath := fieldPath.Child("tokenAttributes")
+			if !saTokenForCredentialProviders {


Suggestion: Consider validating the length/format of ServiceAccountTokenAudience to prevent excessively long or invalid values. Use validation.IsDNS1123Subdomain or similar validation functions from k8s.io/apimachinery/pkg/util/validation if appropriate.

github-actions · 2025-02-20T01:38:17Z

pkg/credentialprovider/plugin/config.go

@@ -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"))
 		}


Consider changing len(provider.TokenAttributes.ServiceAccountTokenAudience) == 0 to len(strings.TrimSpace(provider.TokenAttributes.ServiceAccountTokenAudience)) == 0 to prevent the usage of empty strings consisting of spaces.

github-actions · 2025-02-20T01:38:19Z

pkg/credentialprovider/plugin/config.go

+			if !saTokenForCredentialProviders {
+				allErrs = append(allErrs, field.Forbidden(fldPath, "tokenAttributes is not supported when KubeletServiceAccountTokenForCredentialProviders feature gate is disabled"))
+			}
+			if len(provider.TokenAttributes.ServiceAccountTokenAudience) == 0 {


Suggestion: RequireServiceAccount is a pointer, is it intentional to mark this required? Should the intention actually be distinguishing unset from false? If so, it shouldn't be required. Consider adding a comment clarifying this.

github-actions · 2025-02-20T01:38:19Z

pkg/credentialprovider/plugin/config.go

+			if !saTokenForCredentialProviders {
+				allErrs = append(allErrs, field.Forbidden(fldPath, "tokenAttributes is not supported when KubeletServiceAccountTokenForCredentialProviders feature gate is disabled"))
+			}
+			if len(provider.TokenAttributes.ServiceAccountTokenAudience) == 0 {


The error message tokenAttributes is only supported for %s API version would be better worded to indicate what versions are not supported.

github-actions · 2025-02-20T01:38:21Z

pkg/credentialprovider/plugin/config.go

+			}
+			if provider.TokenAttributes.RequireServiceAccount == nil {
+				allErrs = append(allErrs, field.Required(fldPath.Child("requireServiceAccount"), "requireServiceAccount is required"))
+			}


Suggestion: It may be worthwhile to extract the string representation of credentialproviderv1.SchemeGroupVersion into a constant for reuse and clarity. This would also make it easier to update if the API version changes.

github-actions · 2025-02-20T01:38:21Z

pkg/credentialprovider/plugin/config.go

+				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())))


It's better to use apimachinery/pkg/util/validation/validation.IsQualifiedName directly to validate the annotation key rather than converting it to lowercase first and then calling IsQualifiedName. The function is case-insensitive, so explicitly converting to lowercase is unnecessary and reduces readability.

github-actions · 2025-02-20T01:38:22Z

pkg/credentialprovider/plugin/config.go

+			}
+
+			seenAnnotationKeys := sets.NewString()
+			// 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


Suggestion: Consider moving the key validation logic to a separate function for better readability and testability.

github-actions · 2025-02-20T01:38:24Z

pkg/credentialprovider/plugin/config.go

+			seenAnnotationKeys := sets.NewString()
+			// 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.


Suggestion: The comment refers to a specific commit hash in objectmeta.go. Consider updating this comment to refer to a stable location, such as a specific function or a more general description of the validation logic. Also, verify that the linked validation is still functionally the same, and update it if it is not.

github-actions · 2025-02-20T01:38:26Z

pkg/credentialprovider/plugin/config.go

+			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))


Suggestion: Rather than converting the annotation key to lowercase, consider using validation.IsQualifiedName directly, which is case-insensitive. This removes the potential for unintended behavior with mixed-case keys.

github-actions · 2025-02-20T01:38:27Z

pkg/credentialprovider/plugin/config.go

+					allErrs = append(allErrs, field.Invalid(fldPath.Child("serviceAccountAnnotationKeys"), k, msg))
+				}
+				if seenAnnotationKeys.Has(k) {
+					allErrs = append(allErrs, field.Duplicate(fldPath.Child("serviceAccountAnnotationKeys"), k))


Suggestion: Add a unit test specifically for the annotation key validation to ensure it correctly handles valid and invalid keys.

github-actions · 2025-02-20T01:38:29Z

pkg/credentialprovider/plugin/plugin.go

+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 {


Suggestion: It may be useful to emit an event, log a warning, or return a non-nil error from the RegisterCredentialProviderPlugins function when the feature gate is disabled. This would provide better visibility into the configuration state and help users understand why service account tokens are not being used.

github-actions · 2025-02-20T01:38:33Z

pkg/credentialprovider/plugin/plugin.go

+	if !featureGateEnabled || !serviceAccountTokenAudienceSet {
+		return nil
+	}
+


Suggestion: Add a comment explaining why the default value of expirationSeconds in the TokenRequestSpec is acceptable.

github-actions · 2025-02-20T01:38:34Z

pkg/credentialprovider/plugin/plugin.go

@@ -28,12 +28,18 @@ import (
 	"sync"
 	"time"



Suggestion: Add a comment describing the purpose of importing golang.org/x/crypto/cryptobyte.

github-actions · 2025-02-20T01:38:35Z

pkg/credentialprovider/plugin/plugin.go

+		audience:                     provider.TokenAttributes.ServiceAccountTokenAudience,
+		requireServiceAccount:        *provider.TokenAttributes.RequireServiceAccount,
+		getServiceAccountFunc:        getServiceAccount,
+		getServiceAccountTokenFunc:   getServiceAccountToken,


Suggestion: APIVersion and Kind should be defined as constants to avoid magic strings.

github-actions · 2025-02-20T01:38:37Z

pkg/credentialprovider/plugin/plugin.go

@@ -75,7 +82,10 @@ func init() {



Suggestion: Instead of passing saTokenForCredentialProvidersFeatureEnabled as an argument, directly invoke utilfeature.DefaultFeatureGate.Enabled within the validateCredentialProviderConfig function. This simplifies the function signature and reduces the chance of passing an incorrect value.

github-actions · 2025-02-20T01:38:38Z

pkg/credentialprovider/plugin/plugin.go

+
+	if p.serviceAccountProvider != nil {
+		if len(serviceAccountName) == 0 && p.serviceAccountProvider.requireServiceAccount {
+			klog.Errorf("Service account name is empty for pod %s/%s", podNamespace, podName)


Suggestion: It might be beneficial to add a comment explaining the purpose of using both image and registry as cache keys. Providing context on why two keys are utilized for caching would enhance comprehension.

github-actions · 2025-02-20T01:38:39Z

pkg/credentialprovider/plugin/plugin.go

@@ -89,8 +99,8 @@ func RegisterCredentialProviderPlugins(pluginConfigFile, pluginBinDir string) er
 		return err
 	}

-	errs := validateCredentialProviderConfig(credentialProviderConfig)
-	if len(errs) > 0 {
+	saTokenForCredentialProvidersFeatureEnabled := utilfeature.DefaultFeatureGate.Enabled(features.KubeletServiceAccountTokenForCredentialProviders)


Suggestion: The getServiceAccountToken and getServiceAccount functions should not be passed as parameters to newPluginProvider.

github-actions · 2025-02-20T01:38:39Z

pkg/credentialprovider/plugin/plugin.go

+			return credentialprovider.DockerConfig{}
+		}
+	}
+	res, err, _ := p.group.Do(singleFlightKey, func() (interface{}, error) {


Suggestion: Consider changing the input parameter to the ExecPlugin interface to a struct type instead of having multiple separate parameters.

github-actions · 2025-02-20T01:38:41Z

pkg/credentialprovider/plugin/plugin.go

@@ -89,8 +99,8 @@ func RegisterCredentialProviderPlugins(pluginConfigFile, pluginBinDir string) er
 		return err
 	}

-	errs := validateCredentialProviderConfig(credentialProviderConfig)
-	if len(errs) > 0 {
+	saTokenForCredentialProvidersFeatureEnabled := utilfeature.DefaultFeatureGate.Enabled(features.KubeletServiceAccountTokenForCredentialProviders)


Suggestion: Consider using dependency injection for getServiceAccountToken and getServiceAccount to improve testability and reduce dependencies. This could involve creating an interface for fetching service account tokens and having different implementations for testing and production.

github-actions · 2025-02-20T01:38:41Z

pkg/credentialprovider/plugin/plugin.go

+		}
+	}
+	res, err, _ := p.group.Do(singleFlightKey, func() (interface{}, error) {
+		return p.plugin.ExecPlugin(context.Background(), image, serviceAccountToken, requiredAnnotations)
 	})



Suggestion: It would be beneficial to add more details in the comment regarding the purpose and usage of serviceAccountAnnotations in the ExecPlugin function.

github-actions · 2025-02-20T01:38:43Z

pkg/credentialprovider/plugin/plugin.go

@@ -150,6 +162,7 @@ func newPluginProvider(pluginBinDir string, provider kubeletconfig.CredentialPro
 			envVars:      provider.Env,
 			environ:      os.Environ,
 		},


Suggestion: The type for serviceAccountAnnotationKeys should be []string type instead of the string type.

github-actions · 2025-02-20T01:38:44Z

pkg/credentialprovider/plugin/plugin.go

+		return nil, false, fmt.Errorf("error generating cache key: %w", err)
+	}
+
+	obj, found, err = p.cache.GetByKey(cacheKey)


Suggestion: It might be worthwhile to add a comment explaining the rationale behind using cryptobyte for generating the cache key. Consider using a standard hashing library.

github-actions · 2025-02-20T01:38:45Z

pkg/credentialprovider/plugin/plugin.go

+// 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.


Suggestion: Consider passing the pod information perPodPluginProvider during plugin initialization, rather than on every Provide call. This could reduce overhead if the pod information doesn't change frequently.

github-actions · 2025-02-20T01:38:45Z

pkg/credentialprovider/plugin/plugin.go

@@ -340,7 +514,12 @@ func (p *pluginProvider) getCachedCredentials(image string) (credentialprovider.
 		return obj.(*cacheEntry).credentials, true, nil
 	}

-	obj, found, err = p.cache.GetByKey(globalCacheKey)
+	cacheKey, err = p.generateCacheKey(globalCacheKey, baseCacheKey)
+	if err != nil {


Suggestion: The magic number 16 should be extracted to a named constant.

github-actions · 2025-02-20T01:38:47Z

pkg/credentialprovider/plugin/plugin.go

+// 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)
+	if err != nil {


Suggestion: It is required to check feature-gate before calling function p.provider.provide to avoid the case that new introduced functions get invoked by the old version api-server without enabling the feature-gate.

github-actions · 2025-02-20T01:38:47Z

pkg/credentialprovider/plugin/plugin.go

+	if err != nil {
+		return nil, false, fmt.Errorf("error generating cache key: %w", err)
+	}
+


Suggestion: The magic number 32 should be extracted to a named constant.

github-actions · 2025-02-20T01:38:49Z

pkg/credentialprovider/plugin/plugin.go

@@ -355,7 +534,7 @@ func (p *pluginProvider) getCachedCredentials(image string) (credentialprovider.
 // Plugin is the interface calling ExecPlugin. This is mainly for testability
 // so tests don't have to actually exec any processes.
 type Plugin interface {
-	ExecPlugin(ctx context.Context, image string) (*credentialproviderapi.CredentialProviderResponse, error)
+	ExecPlugin(ctx context.Context, image, serviceAccountToken string, serviceAccountAnnotations map[string]string) (*credentialproviderapi.CredentialProviderResponse, error)


Suggestion: Add a comment explaining why the StringKeySet is sorted.

github-actions · 2025-02-20T01:38:49Z

pkg/credentialprovider/plugin/plugin.go

+			return credentialprovider.DockerConfig{}
+		}
+	}
+	res, err, _ := p.group.Do(singleFlightKey, func() (interface{}, error) {


Suggestion: Suggest adding a comment explaining that ServiceAccountAnnotations are passed to the plugin via stdin.

github-actions · 2025-02-20T01:39:58Z

pkg/features/versioned_kube_features.go

@@ -370,7 +368,6 @@ var defaultVersionedKubernetesFeatureGates = map[featuregate.Feature]featuregate

 	genericfeatures.WatchFromStorageWithoutResourceVersion: {
 		{Version: version.MustParse("1.27"), Default: false, PreRelease: featuregate.Beta},
-		{Version: version.MustParse("1.33"), Default: false, PreRelease: featuregate.Deprecated, LockToDefault: true},
 	},

 	genericfeatures.WatchList: {


Similar to line 272, consider adding a comment explaining the deprecation of WatchFromStorageWithoutResourceVersion and why it is deprecated in 1.33.

github-actions · 2025-02-20T01:39:59Z

pkg/features/versioned_kube_features.go

+	KubeletServiceAccountTokenForCredentialProviders: {
+		{Version: version.MustParse("1.33"), Default: false, PreRelease: featuregate.Alpha},
+	},
+


It is good that KubeletServiceAccountTokenForCredentialProviders is introduced as an alpha feature gated on 1.33. Please also ensure you've added a // +featureGate=KubeletServiceAccountTokenForCredentialProviders to the corresponding field in the API definition. Also add the KEP link here for reviewers.

github-actions · 2025-02-20T01:40:01Z

pkg/kubelet/container/testing/fake_runtime.go

@@ -308,7 +308,7 @@ func (f *FakeRuntime) GetContainerLogs(_ context.Context, pod *v1.Pod, container
 	return f.Err
 }

-func (f *FakeRuntime) PullImage(ctx context.Context, image kubecontainer.ImageSpec, pullSecrets []v1.Secret, podSandboxConfig *runtimeapi.PodSandboxConfig) (string, error) {
+func (f *FakeRuntime) PullImage(ctx context.Context, image kubecontainer.ImageSpec, pullSecrets []v1.Secret, podSandboxConfig *runtimeapi.PodSandboxConfig, serviceAccountName string) (string, error) {


Suggestion: The serviceAccountName parameter added to the PullImage function should be documented with a comment explaining its purpose and how it's used (or not used) within the FakeRuntime. This will improve readability and maintainability.

github-actions · 2025-02-20T01:40:01Z

pkg/features/versioned_kube_features.go

@@ -678,12 +679,10 @@ var defaultVersionedKubernetesFeatureGates = map[featuregate.Feature]featuregate

 	StorageNamespaceIndex: {
 		{Version: version.MustParse("1.30"), Default: true, PreRelease: featuregate.Beta},


Similar to line 272, consider adding a comment explaining the deprecation of StorageNamespaceIndex and why it is deprecated in 1.33.

github-actions · 2025-02-20T01:40:03Z

pkg/features/versioned_kube_features.go

 	},

 	ServiceAccountNodeAudienceRestriction: {
-		{Version: version.MustParse("1.32"), Default: false, PreRelease: featuregate.Beta},
-		{Version: version.MustParse("1.33"), Default: true, PreRelease: featuregate.Beta},
+		{Version: version.MustParse("1.32"), Default: true, PreRelease: featuregate.Beta},


It seems like the feature ServiceAccountNodeAudienceRestriction is already in Beta in 1.32 and enabled by default, so the entry for 1.33 seems redundant. Consider removing it or adding a comment explaining why it's there.

github-actions · 2025-02-20T01:40:05Z

pkg/kubelet/container/testing/runtime_mock.go

-// PullImage provides a mock function with given fields: ctx, image, pullSecrets, podSandboxConfig
-func (_m *MockRuntime) PullImage(ctx context.Context, image container.ImageSpec, pullSecrets []corev1.Secret, podSandboxConfig *v1.PodSandboxConfig) (string, error) {
-	ret := _m.Called(ctx, image, pullSecrets, podSandboxConfig)
+// PullImage provides a mock function with given fields: ctx, image, pullSecrets, podSandboxConfig, serviceAccountName


Suggestion: The comment for the PullImage method should be updated to reflect the purpose of the new serviceAccountName parameter. Also, consider including the valid format for the parameter.

github-actions · 2025-02-20T01:40:05Z

pkg/features/versioned_kube_features.go

@@ -718,7 +717,6 @@ var defaultVersionedKubernetesFeatureGates = map[featuregate.Feature]featuregate
 	SidecarContainers: {
 		{Version: version.MustParse("1.28"), Default: false, PreRelease: featuregate.Alpha},
 		{Version: version.MustParse("1.29"), Default: true, PreRelease: featuregate.Beta},
-		{Version: version.MustParse("1.33"), Default: true, LockToDefault: true, PreRelease: featuregate.GA}, // GA in 1.33 remove in 1.36
 	},



It is good that you're marking SidecarContainers as GA in 1.33, but please ensure you remove the feature gate altogether in 1.36. Also, consider providing a KEP link.

github-actions · 2025-02-20T01:46:31Z

pkg/credentialprovider/plugin/config.go

@@ -70,7 +72,7 @@ func decode(data []byte) (*kubeletconfig.CredentialProviderConfig, error) {
 }


It's better to pass *field.ErrorList instead of field.ErrorList to avoid unnecessary copying of the list.

github-actions · 2025-02-20T01:46:34Z

pkg/credentialprovider/plugin/config.go

@@ -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"))


You could include the error message: "The TokenAttributes field is only supported when the KubeletServiceAccountToken feature gate is enabled."

github-actions · 2025-02-20T01:46:36Z

pkg/credentialprovider/plugin/config.go

@@ -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 {


ServiceAccountTokenAudience is Required, suggest to include that this ServiceAccountTokenAudience is a list and add length validation for that list. For example:

github-actions · 2025-02-20T01:46:37Z

pkg/credentialprovider/plugin/config.go

+		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"))


RequireServiceAccount is Required, suggest to include that this RequireServiceAccount is a bool value, and must not be nil.

github-actions · 2025-02-20T01:46:40Z

pkg/credentialprovider/plugin/config.go

+				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"))


This check should compare the API group name and API version, using constants. This makes the code more maintainable.

github-actions · 2025-02-20T01:46:42Z

pkg/credentialprovider/plugin/config.go

+				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"))


It is good practice to include a comment that refers to the Kubernetes validation logic, to provide context as to why it was included in the code, as well as a way to more easily locate the logic being referred to.

github-actions · 2025-02-20T01:46:44Z

pkg/credentialprovider/plugin/config.go

+			if provider.TokenAttributes.RequireServiceAccount == nil {
+				allErrs = append(allErrs, field.Required(fldPath.Child("requireServiceAccount"), "requireServiceAccount is required"))
+			}
+			if provider.APIVersion != credentialproviderv1.SchemeGroupVersion.String() {


The strings.ToLower(k) conversion could be done once and reused to avoid redundant work.

github-actions · 2025-02-20T01:46:45Z

pkg/credentialprovider/plugin/config.go

+				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())))


Instead of inlining the loop, you could move it into a separate function, with an expressive function name.

github-actions · 2025-02-20T01:46:49Z

pkg/credentialprovider/plugin/config.go

+			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())))
+			}
+


Consider to add max length validation for serviceAccountAnnotationKeys.

github-actions · 2025-02-20T01:46:57Z

pkg/credentialprovider/plugin/plugin.go

@@ -28,12 +28,18 @@ import (
 	"sync"
 	"time"

+	"golang.org/x/crypto/cryptobyte"


Consider adding an explicit import alias for clock to avoid potential naming conflicts. For example, kclock "k8s.io/utils/clock".

github-actions · 2025-02-20T01:46:59Z

pkg/credentialprovider/plugin/plugin.go

@@ -75,7 +82,10 @@ func init() {

 // RegisterCredentialProviderPlugins is called from kubelet to register external credential provider
 // plugins according to the CredentialProviderConfig config file.
-func RegisterCredentialProviderPlugins(pluginConfigFile, pluginBinDir string) error {
+func RegisterCredentialProviderPlugins(pluginConfigFile, pluginBinDir string,


It would be better to pass the saTokenForCredentialProvidersFeatureEnabled flag to newServiceAccountProvider rather than passing it to validateCredentialProviderConfig function.

github-actions · 2025-02-20T01:47:01Z

pkg/credentialprovider/plugin/plugin.go

+func newPluginProvider(pluginBinDir string, provider kubeletconfig.CredentialProvider,
+	getServiceAccountToken func(namespace, name string, tr *authenticationv1.TokenRequest) (*authenticationv1.TokenRequest, error),
+	getServiceAccount func(namespace, name string) (*v1.ServiceAccount, error),
+) (*pluginProvider, error) {
 	mediaType := "application/json"
 	info, ok := runtime.SerializerInfoForMediaType(codecs.SupportedMediaTypes(), mediaType)
 	if !ok {


It would be better to construct the serviceAccountProvider here and let the newPluginProvider function return error if needed.

github-actions · 2025-02-20T01:47:02Z

pkg/credentialprovider/plugin/plugin.go

@@ -150,6 +162,7 @@ func newPluginProvider(pluginBinDir string, provider kubeletconfig.CredentialPro
 			envVars:      provider.Env,
 			environ:      os.Environ,
 		},
+		serviceAccountProvider: newServiceAccountProvider(provider, getServiceAccount, getServiceAccountToken),
 	}, nil


Add comments to describe the purpose and usage of each field in the serviceAccountProvider struct.

github-actions · 2025-02-20T01:47:04Z

pkg/credentialprovider/plugin/plugin.go

+}
+
+type serviceAccountProvider struct {
+	audience                     string


Add comments to describe the purpose and usage of each field in the perPodPluginProvider struct.

github-actions · 2025-02-20T01:47:06Z

pkg/credentialprovider/plugin/plugin.go

+
+// getServiceAccountToken returns a service account token for the service account.
+func (s *serviceAccountProvider) getServiceAccountToken(podName, podNamespace, serviceAccountName string, podUID types.UID) (string, error) {
+	tr, err := s.getServiceAccountTokenFunc(podNamespace, serviceAccountName, &authenticationv1.TokenRequest{


The err variable is shadowed here. Use a different variable name for the error returned by p.generateCacheKey.

github-actions · 2025-02-20T01:47:08Z

pkg/credentialprovider/plugin/plugin.go

 // 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) {


The comment does not accurately describe the behavior of NewExpirationCache.

github-actions · 2025-02-20T01:47:10Z

pkg/credentialprovider/plugin/plugin.go

+	if err != nil {
+		klog.Errorf("Error generating cache key: %v", err)
+		return credentialprovider.DockerConfig{}
+	}


It seems odd to use uint32 for the length of annotations. Are you expecting more than 65535 annotations in total? Also, limiting the length of the entire cache key might be a good idea.

github-actions · 2025-02-20T01:47:11Z

pkg/credentialprovider/plugin/plugin.go

+	cacheKey, err = p.generateCacheKey(registry, baseCacheKey)
+	if err != nil {
+		return nil, false, fmt.Errorf("error generating cache key: %w", err)
+	}


consider adding a unit test.

github-actions · 2025-02-20T01:47:16Z

pkg/credentialprovider/plugin/plugins.go

+
+func registerCredentialProviderPlugin(name string, p *pluginProvider) {
+	providersMutex.Lock()
+	defer providersMutex.Unlock()


Consider using a more descriptive name than p for the loop variable in for _, p := range providers. A name like providerInfo might improve readability.

github-actions · 2025-02-20T01:47:18Z

pkg/credentialprovider/plugin/plugins.go

+	providersMutex.RLock()
+	defer providersMutex.RUnlock()
+
+	keyring := &externalCredentialProviderKeyring{


It would be beneficial to include a comment explaining why a zero-length slice is being pre-allocated with make([]credentialprovider.DockerConfigProvider, 0, len(providers)). Is it for performance reasons, or to avoid nil slices?

richabanker added the api-review label Feb 20, 2025

github-actions bot reviewed Feb 20, 2025

View reviewed changes

test api review

0dcfd19

richabanker force-pushed the hackathon-test-api-review-1 branch from 6683b3e to 0dcfd19 Compare February 20, 2025 01:45

github-actions bot reviewed Feb 20, 2025

View reviewed changes

richabanker closed this Feb 20, 2025

		@@ -70,7 +72,7 @@ func decode(data []byte) (*kubeletconfig.CredentialProviderConfig, error) {
		}

		@@ -678,12 +679,10 @@ var defaultVersionedKubernetesFeatureGates = map[featuregate.Feature]featuregate

		StorageNamespaceIndex: {
		{Version: version.MustParse("1.30"), Default: true, PreRelease: featuregate.Beta},

		@@ -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"))

test api review #41

test api review #41

Uh oh!

Conversation

richabanker commented Feb 20, 2025

What type of PR is this?

What this PR does / why we need it:

Which issue(s) this PR 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.:

Uh oh!

github-actions bot Feb 20, 2025

Choose a reason for hiding this comment

Uh oh!

github-actions bot Feb 20, 2025

Choose a reason for hiding this comment

Uh oh!

github-actions bot Feb 20, 2025

Choose a reason for hiding this comment

Uh oh!

github-actions bot Feb 20, 2025

Choose a reason for hiding this comment

Uh oh!

github-actions bot Feb 20, 2025

Choose a reason for hiding this comment

Uh oh!

github-actions bot Feb 20, 2025

Choose a reason for hiding this comment

Uh oh!

github-actions bot Feb 20, 2025

Choose a reason for hiding this comment

Uh oh!

github-actions bot Feb 20, 2025

Choose a reason for hiding this comment

Uh oh!

github-actions bot Feb 20, 2025

Choose a reason for hiding this comment

Uh oh!

github-actions bot Feb 20, 2025

Choose a reason for hiding this comment

Uh oh!

github-actions bot Feb 20, 2025

Choose a reason for hiding this comment

Uh oh!

github-actions bot Feb 20, 2025

Choose a reason for hiding this comment

Uh oh!

github-actions bot Feb 20, 2025

Choose a reason for hiding this comment

Uh oh!

github-actions bot Feb 20, 2025

Choose a reason for hiding this comment

Uh oh!

github-actions bot Feb 20, 2025

Choose a reason for hiding this comment

Uh oh!

github-actions bot Feb 20, 2025

Choose a reason for hiding this comment

Uh oh!

github-actions bot Feb 20, 2025

Choose a reason for hiding this comment

Uh oh!

github-actions bot Feb 20, 2025

Choose a reason for hiding this comment

Uh oh!

github-actions bot Feb 20, 2025

Choose a reason for hiding this comment

Uh oh!

github-actions bot Feb 20, 2025

Choose a reason for hiding this comment

Uh oh!

github-actions bot Feb 20, 2025

Choose a reason for hiding this comment

Uh oh!

github-actions bot Feb 20, 2025

Choose a reason for hiding this comment

Uh oh!

github-actions bot Feb 20, 2025

Choose a reason for hiding this comment