From 1ef0c67fc21c6100efa38b10c0246d9dca519723 Mon Sep 17 00:00:00 2001 From: Tommy Murphy Date: Sun, 2 Apr 2023 15:39:40 -0400 Subject: [PATCH] chore: cleanup WritePayloads to be easier to use Makes WritePayloads() call Validate() internally so that it can be used safely on its own. Also cleans up some of the indentation on the provider client. --- pkg/secrets-store/provider_client.go | 40 +++++++++++------------ pkg/secrets-store/provider_client_test.go | 8 ++--- pkg/util/fileutil/writer.go | 4 +++ 3 files changed, 28 insertions(+), 24 deletions(-) diff --git a/pkg/secrets-store/provider_client.go b/pkg/secrets-store/provider_client.go index 6e8bcb92f..9a3a8c81d 100644 --- a/pkg/secrets-store/provider_client.go +++ b/pkg/secrets-store/provider_client.go @@ -66,10 +66,12 @@ const ServiceConfig = ` ` var ( - // PluginNameRe is the regular expression used to validate plugin names. - PluginNameRe = regexp.MustCompile(`^[a-zA-Z0-9_-]{0,30}$`) - ErrInvalidProvider = errors.New("invalid provider") - ErrProviderNotFound = errors.New("provider not found") + // pluginNameRe is the regular expression used to validate plugin names. + pluginNameRe = regexp.MustCompile(`^[a-zA-Z0-9_-]{0,30}$`) + + errInvalidProvider = errors.New("invalid provider") + errProviderNotFound = errors.New("provider not found") + errMissingObjectVersions = errors.New("missing object versions") ) // PluginClientBuilder builds and stores grpc clients for communicating with @@ -123,8 +125,8 @@ func (p *PluginClientBuilder) Get(ctx context.Context, provider string) (v1alpha } // client does not exist, create a new one - if !PluginNameRe.MatchString(provider) { - return nil, fmt.Errorf("%w: provider %q", ErrInvalidProvider, provider) + if !pluginNameRe.MatchString(provider) { + return nil, fmt.Errorf("%w: provider %q", errInvalidProvider, provider) } // check all paths @@ -147,7 +149,7 @@ func (p *PluginClientBuilder) Get(ctx context.Context, provider string) (v1alpha } if socketPath == "" { - return nil, fmt.Errorf("%w: provider %q", ErrProviderNotFound, provider) + return nil, fmt.Errorf("%w: provider %q", errProviderNotFound, provider) } conn, err := grpc.Dial( @@ -249,7 +251,7 @@ func MountContent(ctx context.Context, client v1alpha1.CSIDriverProviderClient, ov := resp.GetObjectVersion() if ov == nil { - return nil, internalerrors.GRPCProviderError, errors.New("missing object versions") + return nil, internalerrors.GRPCProviderError, errMissingObjectVersions } objectVersions := make(map[string]string) for _, v := range ov { @@ -263,19 +265,17 @@ func MountContent(ctx context.Context, client v1alpha1.CSIDriverProviderClient, klog.InfoS("proto above 1MiB, secret sync may fail", "size", size) } - if len(resp.GetFiles()) > 0 { - klog.V(5).Info("writing mount response files") - if err := fileutil.Validate(resp.GetFiles()); err != nil { - return nil, internalerrors.FileWriteError, err - } - if err := fileutil.WritePayloads(targetPath, resp.GetFiles()); err != nil { - return nil, internalerrors.FileWriteError, err - } - } else { - // when no files are returned we assume that the plugin has not migrated - // grpc responses for writing files yet. - klog.V(5).Info("mount response has no files") + if len(resp.GetFiles()) == 0 { + // The plugin mount response contains no files. Possible that the plugin + // is writing its own files instead of the driver (See Issue #551). + klog.V(5).Info("Empty files in mount response. It is possible that the plugin has not migrated to driver-written files (Issue #551).") + return objectVersions, "", nil + } + + if err := fileutil.WritePayloads(targetPath, resp.GetFiles()); err != nil { + return nil, internalerrors.FileWriteError, err } + klog.V(5).Info("mount response files written.") return objectVersions, "", nil } diff --git a/pkg/secrets-store/provider_client_test.go b/pkg/secrets-store/provider_client_test.go index cd9f39b2b..2e5eadb30 100644 --- a/pkg/secrets-store/provider_client_test.go +++ b/pkg/secrets-store/provider_client_test.go @@ -458,8 +458,8 @@ func TestPluginClientBuilderErrorNotFound(t *testing.T) { cb := NewPluginClientBuilder([]string{path}) ctx := context.Background() - if _, err := cb.Get(ctx, "notfoundprovider"); !errors.Is(err, ErrProviderNotFound) { - t.Errorf("Get(%s) = %v, want %v", "notfoundprovider", err, ErrProviderNotFound) + if _, err := cb.Get(ctx, "notfoundprovider"); !errors.Is(err, errProviderNotFound) { + t.Errorf("Get(%s) = %v, want %v", "notfoundprovider", err, errProviderNotFound) } // check that the provider is found once server is started @@ -480,8 +480,8 @@ func TestPluginClientBuilderErrorInvalid(t *testing.T) { cb := NewPluginClientBuilder([]string{path}) ctx := context.Background() - if _, err := cb.Get(ctx, "bad/provider/name"); !errors.Is(err, ErrInvalidProvider) { - t.Errorf("Get(%s) = %v, want %v", "bad/provider/name", err, ErrInvalidProvider) + if _, err := cb.Get(ctx, "bad/provider/name"); !errors.Is(err, errInvalidProvider) { + t.Errorf("Get(%s) = %v, want %v", "bad/provider/name", err, errInvalidProvider) } } diff --git a/pkg/util/fileutil/writer.go b/pkg/util/fileutil/writer.go index 847081d91..69c540cf9 100644 --- a/pkg/util/fileutil/writer.go +++ b/pkg/util/fileutil/writer.go @@ -43,6 +43,10 @@ func Validate(payloads []*v1alpha1.File) error { // atomic writer and converts the v1alpha1.File proto to the FileProjection type // used by the atomic writer. func WritePayloads(path string, payloads []*v1alpha1.File) error { + if err := Validate(payloads); err != nil { + return err + } + // cleanup any payload paths that may have been written by a previous // version of the driver/provider. if err := cleanupProviderFiles(path, payloads); err != nil {