From 0d9ec71da061676bab6f1c04c744266a4d4cb498 Mon Sep 17 00:00:00 2001 From: Jimmi Dyson Date: Fri, 18 Nov 2022 13:38:14 +0000 Subject: [PATCH] refactor: Satisfy linter - much improved structure --- .golangci.yml | 2 +- .../shim/plugin_executor.go | 57 +++--- pkg/credentialprovider/shim/shim.go | 180 +++++++++++------- pkg/credentialprovider/shim/shim_test.go | 28 ++- pkg/log/klog_levels.go | 9 + 5 files changed, 162 insertions(+), 114 deletions(-) create mode 100644 pkg/log/klog_levels.go diff --git a/.golangci.yml b/.golangci.yml index 3e153cf..da982bd 100644 --- a/.golangci.yml +++ b/.golangci.yml @@ -102,7 +102,7 @@ linters-settings: - name: cyclomatic arguments: [10] - name: function-result-limit - arguments: [3] + arguments: [4] issues: exclude-rules: diff --git a/pkg/credentialprovider/shim/plugin_executor.go b/pkg/credentialprovider/shim/plugin_executor.go index d1283eb..40e3010 100644 --- a/pkg/credentialprovider/shim/plugin_executor.go +++ b/pkg/credentialprovider/shim/plugin_executor.go @@ -11,25 +11,26 @@ import ( "os" "os/exec" "path/filepath" - "strings" "sync" "time" - "github.com/mesosphere/kubelet-image-credential-provider-shim/apis/config/v1alpha1" - "github.com/mesosphere/kubelet-image-credential-provider-shim/pkg/urlglobber" "golang.org/x/sync/singleflight" - + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/runtime/serializer/json" "k8s.io/klog/v2" "k8s.io/kubelet/config/v1beta1" credentialproviderapi "k8s.io/kubelet/pkg/apis/credentialprovider" + + "github.com/mesosphere/kubelet-image-credential-provider-shim/apis/config/v1alpha1" + "github.com/mesosphere/kubelet-image-credential-provider-shim/pkg/log" + "github.com/mesosphere/kubelet-image-credential-provider-shim/pkg/urlglobber" ) // newPluginProvider returns a new pluginProvider based on the credential provider config. func newPluginProvider( pluginBinDir string, - provider v1beta1.CredentialProvider, + provider *v1beta1.CredentialProvider, ) (*pluginProvider, error) { mediaType := "application/json" info, ok := runtime.SerializerInfoForMediaType(codecs.SupportedMediaTypes(), mediaType) @@ -43,7 +44,8 @@ func newPluginProvider( } return &pluginProvider{ - matchImages: provider.MatchImages, + matchImages: provider.MatchImages, + defaultCacheDuration: provider.DefaultCacheDuration, plugin: &execPlugin{ name: provider.Name, apiVersion: provider.APIVersion, @@ -58,6 +60,7 @@ func newPluginProvider( type Provider interface { Provide(image string) (*credentialproviderapi.CredentialProviderResponse, error) + DefaultCacheDuration() time.Duration } // pluginProvider is the plugin-based implementation of the DockerConfigProvider interface. @@ -71,6 +74,8 @@ type pluginProvider struct { // against this list of match URLs. matchImages []string + defaultCacheDuration *metav1.Duration + // plugin is the exec implementation of the credential providing plugin. plugin Plugin } @@ -79,6 +84,14 @@ var ErrInvalidCredentialProviderResponse = errors.New( "invalid response type returned by external credential provider", ) +func (p *pluginProvider) DefaultCacheDuration() time.Duration { + if p.defaultCacheDuration != nil { + return p.defaultCacheDuration.Duration + } + + return 0 +} + // Provide returns a *credentialproviderapi.CredentialProviderResponse based on the credentials returned // by executing the plugin. func (p *pluginProvider) Provide( @@ -153,7 +166,8 @@ func (e *execPlugin) ExecPlugin( ctx context.Context, image string, ) (*credentialproviderapi.CredentialProviderResponse, error) { - klog.V(5).Infof("Getting image %s credentials from external exec plugin %s", image, e.name) + klog.V(log.KLogDebug). + Infof("Getting image %s credentials from external exec plugin %s", image, e.name) authRequest := &credentialproviderapi.CredentialProviderRequest{Image: image} data, err := e.encodeRequest(authRequest) @@ -172,10 +186,13 @@ func (e *execPlugin) ExecPlugin( ctx, cancel := context.WithTimeout(ctx, 1*time.Minute) defer cancel() - cmd := exec.CommandContext(ctx, filepath.Join(e.pluginBinDir, e.name), e.args...) + cmd := exec.CommandContext( //nolint:gosec // Safe command args. + ctx, + filepath.Join(e.pluginBinDir, e.name), + e.args...) cmd.Stdout, cmd.Stderr, cmd.Stdin = stdout, stderr, stdin - var configEnvVars []string + configEnvVars := make([]string, 0, len(e.envVars)) for _, v := range e.envVars { configEnvVars = append(configEnvVars, fmt.Sprintf("%s=%s", v.Name, v.Value)) } @@ -186,7 +203,7 @@ func (e *execPlugin) ExecPlugin( // also, this behaviour is inline with Credential Provider Config spec cmd.Env = mergeEnvVars(e.environ(), configEnvVars) - if err = e.runPlugin(ctx, cmd, image); err != nil { + if err := e.runPlugin(ctx, cmd, image); err != nil { return nil, err } @@ -235,7 +252,7 @@ func (e *execPlugin) runPlugin(ctx context.Context, cmd *exec.Cmd, image string) return nil } -// encodeRequest encodes the internal CredentialProviderRequest type into the v1alpha1 version in json +// encodeRequest encodes the internal CredentialProviderRequest type into the v1alpha1 version in json. func (e *execPlugin) encodeRequest( request *credentialproviderapi.CredentialProviderRequest, ) ([]byte, error) { @@ -248,7 +265,7 @@ func (e *execPlugin) encodeRequest( } // decodeResponse decodes data into the internal CredentialProviderResponse type. -func (e *execPlugin) decodeResponse( +func (*execPlugin) decodeResponse( data []byte, ) (*credentialproviderapi.CredentialProviderResponse, error) { obj, gvk, err := codecs.UniversalDecoder().Decode(data, nil, nil) @@ -277,19 +294,11 @@ func (e *execPlugin) decodeResponse( return nil, fmt.Errorf("unable to convert %T to *CredentialProviderResponse", obj) } -// parseRegistry extracts the registry hostname of an image (including port if specified). -func parseRegistry(image string) string { - imageParts := strings.Split(image, "/") - return imageParts[0] -} - // mergedEnvVars overlays system defined env vars with credential provider env vars, // it gives priority to the credential provider vars allowing user to override system -// env vars +// env vars. func mergeEnvVars(sysEnvVars, credProviderVars []string) []string { - mergedEnvVars := sysEnvVars - for _, credProviderVar := range credProviderVars { - mergedEnvVars = append(mergedEnvVars, credProviderVar) - } - return mergedEnvVars + var mergedEnvVars []string + mergedEnvVars = append(mergedEnvVars, sysEnvVars...) + return append(mergedEnvVars, credProviderVars...) } diff --git a/pkg/credentialprovider/shim/shim.go b/pkg/credentialprovider/shim/shim.go index bf4054f..c70e6f8 100644 --- a/pkg/credentialprovider/shim/shim.go +++ b/pkg/credentialprovider/shim/shim.go @@ -22,6 +22,7 @@ import ( "github.com/mesosphere/kubelet-image-credential-provider-shim/apis/config/v1alpha1" "github.com/mesosphere/kubelet-image-credential-provider-shim/pkg/credentialprovider/plugin" + "github.com/mesosphere/kubelet-image-credential-provider-shim/pkg/log" "github.com/mesosphere/kubelet-image-credential-provider-shim/pkg/urlglobber" ) @@ -86,12 +87,14 @@ func (p *shimProvider) registerCredentialProviderPlugins() error { } for _, provider := range p.cfg.CredentialProviders.Providers { + provider := provider // Capture range variable. + pluginBin := filepath.Join(pluginBinDir, provider.Name) if _, err := os.Stat(pluginBin); err != nil { return fmt.Errorf("error inspecting binary executable %q: %w", pluginBin, err) } - pluginProvider, err := newPluginProvider(pluginBinDir, provider) + pluginProvider, err := newPluginProvider(pluginBinDir, &provider) if err != nil { return fmt.Errorf("error initializing plugin provider %q: %w", provider.Name, err) } @@ -110,7 +113,7 @@ func (p *shimProvider) registerCredentialProvider(name string, provider *pluginP if found { klog.Fatalf("Credential provider %q was registered twice", name) } - klog.V(4).Infof("Registered credential provider %q", name) + klog.V(log.KLogDebug).Infof("Registered credential provider %q", name) p.providers[name] = provider } @@ -121,86 +124,33 @@ func (p *shimProvider) GetCredentials( img string, _ []string, ) (*credentialproviderv1beta1.CredentialProviderResponse, error) { - globbedDomain, err := urlglobber.GlobbedDomainForImage(img) - if err != nil { - return nil, err - } - authMap := map[string]credentialproviderv1beta1.AuthConfig{} - mirrorAuth, mirrorAuthFound, err := p.getMirrorCredentialsForImage(img) + mirrorAuthConfig, cacheDuration, mirrorAuthFound, err := p.getMirrorCredentialsForImage(img) if err != nil { return nil, fmt.Errorf("failed to get mirror credentials: %w", err) } - cacheDuration := 0 * time.Second - - if mirrorAuthFound { - cacheDuration = mirrorAuth.CacheDuration.Duration - } - - originAuth, originAuthFound, err := p.getCredentialsForImage(img) + originAuthConfig, originCacheDuration, originAuthFound, err := p.getCredentialsForImage(img) if err != nil { return nil, fmt.Errorf("failed to get origin credentials: %w", err) } if originAuthFound { - authMap[img], err = singleAuthFromResponse(originAuth) - if err != nil { - return nil, err - } + authMap[img] = originAuthConfig - if !mirrorAuthFound || cacheDuration > originAuth.CacheDuration.Duration { - cacheDuration = originAuth.CacheDuration.Duration + if !mirrorAuthFound || cacheDuration > originCacheDuration { + cacheDuration = originCacheDuration } } if p.cfg.Mirror != nil && mirrorAuthFound { - mirrorAuthConfig, err := singleAuthFromResponse(mirrorAuth) + err := updateAuthConfigMapForMirror( + authMap, img, p.cfg.Mirror.MirrorCredentialsStrategy, mirrorAuthConfig, + ) if err != nil { return nil, err } - - switch p.cfg.Mirror.MirrorCredentialsStrategy { - case v1alpha1.MirrorCredentialsOnly: - // Only return mirror credentials by setting the image auth for the full image name whether it is already set or - // not. - authMap[img] = mirrorAuthConfig - case v1alpha1.MirrorCredentialsLast: - // Set mirror credentials for globbed domain to ensure that the mirror credentials are used last (glob matches - // have lowest precedence). - // - // This means that the kubelet will first try the mirror credentials, which containerd will try against both the - // configured mirror in containerd and the origin registry (which should fail as incorrect credentials for this - // registry) if the image is not found in the mirror. - // - // If containerd fails to pull using the mirror credentials, then the kubelet will try the origin credentials, - // which containerd will try first against the configured mirror (which should fail as incorrect credentials for - // this registry) and then against the origin registry. - authMap[globbedDomain] = mirrorAuthConfig - case v1alpha1.MirrorCredentialsFirst: - // Set mirror credentials for image to ensure that the mirror credentials are used first, and set any existing - // origin credentials for the globbed domain to ensure they are used last (glob matches have lowest precedence). - // - // This means that the kubelet will first try the origin credentials, which containerd will try against both the - // configured mirror in containerd (which should fail as incorrect credentials for this registry) and the origin - // registry. - // - // If containerd fails to pull using the origin credentials, then the kubelet will try the mirror credentials, - // which containerd will try first against the configured mirror and then against the origin registry (which - // should fail as incorrect credentials for this registry) if the image is not found in the mirror. - existing, found := authMap[img] - if found { - authMap[globbedDomain] = existing - } - authMap[img] = mirrorAuthConfig - default: - return nil, fmt.Errorf( - "%w: %q", - ErrUnsupportedMirrorCredentialStrategy, - p.cfg.Mirror.MirrorCredentialsStrategy, - ) - } } return &credentialproviderv1beta1.CredentialProviderResponse{ @@ -229,22 +179,85 @@ func singleAuthFromResponse(resp *credentialproviderv1beta1.CredentialProviderRe return credentialproviderv1beta1.AuthConfig{}, nil } +func updateAuthConfigMapForMirror( + authMap map[string]credentialproviderv1beta1.AuthConfig, + img string, + mirrorCredentialsStrategy v1alpha1.MirrorCredentialsStrategy, + mirrorAuthConfig credentialproviderv1beta1.AuthConfig, +) error { + globbedDomain, err := urlglobber.GlobbedDomainForImage(img) + if err != nil { + return err + } + + switch mirrorCredentialsStrategy { + case v1alpha1.MirrorCredentialsOnly: + // Only return mirror credentials by setting the image auth for the full image name whether it is already set or + // not. + authMap[img] = mirrorAuthConfig + case v1alpha1.MirrorCredentialsLast: + // Set mirror credentials for globbed domain to ensure that the mirror credentials are used last (glob matches + // have lowest precedence). + // + // This means that the kubelet will first try the mirror credentials, which containerd will try against both the + // configured mirror in containerd and the origin registry (which should fail as incorrect credentials for this + // registry) if the image is not found in the mirror. + // + // If containerd fails to pull using the mirror credentials, then the kubelet will try the origin credentials, + // which containerd will try first against the configured mirror (which should fail as incorrect credentials for + // this registry) and then against the origin registry. + authMap[globbedDomain] = mirrorAuthConfig + case v1alpha1.MirrorCredentialsFirst: + // Set mirror credentials for image to ensure that the mirror credentials are used first, and set any existing + // origin credentials for the globbed domain to ensure they are used last (glob matches have lowest precedence). + // + // This means that the kubelet will first try the origin credentials, which containerd will try against both the + // configured mirror in containerd (which should fail as incorrect credentials for this registry) and the origin + // registry. + // + // If containerd fails to pull using the origin credentials, then the kubelet will try the mirror credentials, + // which containerd will try first against the configured mirror and then against the origin registry (which + // should fail as incorrect credentials for this registry) if the image is not found in the mirror. + existing, found := authMap[img] + if found { + authMap[globbedDomain] = existing + } + authMap[img] = mirrorAuthConfig + default: + return fmt.Errorf( + "%w: %q", + ErrUnsupportedMirrorCredentialStrategy, + mirrorCredentialsStrategy, + ) + } + + return nil +} + func (p *shimProvider) getMirrorCredentialsForImage( img string, -) (*credentialproviderv1beta1.CredentialProviderResponse, bool, error) { +) (credentialproviderv1beta1.AuthConfig, time.Duration, bool, error) { // If mirror is not configured then return no credentials for the mirror. if p.cfg.Mirror == nil { - return nil, false, nil + return credentialproviderv1beta1.AuthConfig{}, 0, false, nil } imgURL, err := urlglobber.ParseSchemelessURL(img) if err != nil { - return nil, false, fmt.Errorf("failed to parse image %q to a URL: %w", img, err) + return credentialproviderv1beta1.AuthConfig{}, 0, false, fmt.Errorf( + "failed to parse image %q to a URL: %w", + img, + err, + ) } mirrorURL, err := urlglobber.ParseSchemelessURL(p.cfg.Mirror.Endpoint) if err != nil { - return nil, false, fmt.Errorf("failed to parse mirror %q to a URL: %w", img, err) + return credentialproviderv1beta1.AuthConfig{}, 0, false, fmt.Errorf( + "failed to parse mirror %q to a URL: %w", + img, + err, + ) } imgURL.Host = mirrorURL.Host @@ -255,16 +268,19 @@ func (p *shimProvider) getMirrorCredentialsForImage( func (p *shimProvider) getCredentialsForImage( img string, -) (*credentialproviderv1beta1.CredentialProviderResponse, bool, error) { +) (credentialproviderv1beta1.AuthConfig, time.Duration, bool, error) { // If only mirror credentials should be used then return no credentials for the origin. if isRegistryCredentialsOnly(p.cfg.Mirror) { - return nil, false, nil + return credentialproviderv1beta1.AuthConfig{}, 0, false, nil } for _, prov := range p.providers { resp, err := prov.Provide(img) if err != nil { - return nil, false, fmt.Errorf("failed to call plugin: %w", err) + return credentialproviderv1beta1.AuthConfig{}, 0, false, fmt.Errorf( + "failed to call plugin: %w", + err, + ) } if resp == nil || len(resp.Auth) == 0 { @@ -273,15 +289,31 @@ func (p *shimProvider) getCredentialsForImage( v1beta1Resp := &credentialproviderv1beta1.CredentialProviderResponse{} if err := scheme.Convert(resp, v1beta1Resp, nil); err != nil { - return nil, + return credentialproviderv1beta1.AuthConfig{}, + 0, false, - fmt.Errorf("failed to convert response from type %T to type %T: %w", resp, v1beta1Resp, err) + fmt.Errorf( + "failed to convert response from type %T to type %T: %w", + resp, + v1beta1Resp, + err, + ) + } + + cacheDuration := prov.DefaultCacheDuration() + if v1beta1Resp.CacheDuration != nil { + cacheDuration = v1beta1Resp.CacheDuration.Duration + } + + authConfig, err := singleAuthFromResponse(v1beta1Resp) + if err != nil { + return credentialproviderv1beta1.AuthConfig{}, 0, false, err } - return v1beta1Resp, true, nil + return authConfig, cacheDuration, true, nil } - return nil, false, nil + return credentialproviderv1beta1.AuthConfig{}, 0, false, nil } func isRegistryCredentialsOnly(cfg *v1alpha1.MirrorConfig) bool { diff --git a/pkg/credentialprovider/shim/shim_test.go b/pkg/credentialprovider/shim/shim_test.go index 211d34a..c18f1ba 100644 --- a/pkg/credentialprovider/shim/shim_test.go +++ b/pkg/credentialprovider/shim/shim_test.go @@ -1,7 +1,7 @@ // Copyright 2022 D2iQ, Inc. All rights reserved. // SPDX-License-Identifier: Apache-2.0 -package shim_test +package shim import ( "context" @@ -9,42 +9,40 @@ import ( "log" "os" "path/filepath" - - "github.com/mesosphere/kubelet-image-credential-provider-shim/pkg/credentialprovider/shim" ) -func ExampleGetCredentials() { - p, err := shim.NewProviderFromConfigFile(filepath.Join("testdata", "config.yaml")) +func ExampleCredentialProvider_GetCredentials() { + p, err := NewProviderFromConfigFile(filepath.Join("testdata", "config.yaml")) if err != nil { - log.Fatal(err) + log.Fatal(err) //nolint:revive // Allow fatal in examples. } creds, err := p.GetCredentials(context.Background(), "img.v1beta1/abc/def:v1.2.3", nil) - if err != nil { - log.Fatal(err) + log.Fatal(err) //nolint:revive // Allow fatal in examples. } if err := json.NewEncoder(os.Stdout).Encode(creds); err != nil { - log.Fatal(err) + log.Fatal(err) //nolint:revive // Allow fatal in examples. } + //nolint:lll // Long example output. // Output: {"kind":"CredentialProviderResponse","apiVersion":"credentialprovider.kubelet.k8s.io/v1beta1","cacheKeyType":"Image","cacheDuration":"5s","auth":{"img.v1beta1/abc/def:v1.2.3":{"username":"v1beta1user","password":"v1beta1password"}}} } -func ExampleGetCredentialsWithMirror() { - p, err := shim.NewProviderFromConfigFile(filepath.Join("testdata", "config-with-mirror.yaml")) +func ExampleCredentialProvider_GetCredentials_withMirror() { + p, err := NewProviderFromConfigFile(filepath.Join("testdata", "config-with-mirror.yaml")) if err != nil { - log.Fatal(err) + log.Fatal(err) //nolint:revive // Allow fatal in examples. } creds, err := p.GetCredentials(context.Background(), "img.v1beta1/abc/def:v1.2.3", nil) - if err != nil { - log.Fatal(err) + log.Fatal(err) //nolint:revive // Allow fatal in examples. } if err := json.NewEncoder(os.Stdout).Encode(creds); err != nil { - log.Fatal(err) + log.Fatal(err) //nolint:revive // Allow fatal in examples. } + //nolint:lll // Long example output. // Output: {"kind":"CredentialProviderResponse","apiVersion":"credentialprovider.kubelet.k8s.io/v1beta1","cacheKeyType":"Image","cacheDuration":"5s","auth":{"*.*":{"username":"v1beta1user","password":"v1beta1password"},"img.v1beta1/abc/def:v1.2.3":{"username":"mirroruser","password":"mirrorpassword"}}} } diff --git a/pkg/log/klog_levels.go b/pkg/log/klog_levels.go new file mode 100644 index 0000000..5b73c72 --- /dev/null +++ b/pkg/log/klog_levels.go @@ -0,0 +1,9 @@ +// Copyright 2022 D2iQ, Inc. All rights reserved. +// SPDX-License-Identifier: Apache-2.0 + +package log + +const ( + KLogDebug = 4 + KLogTrace = 5 +)