Merge pull request #462 from tam7t/tam7t/reload-clients

k8s-ci-robot · web-flow · commit 7f21cd32e4ff · 2021-03-11T17:28:18.000-08:00
feat: connect to plugins at runtime instead of configuration
diff --git a/Makefile b/Makefile
@@ -282,7 +282,6 @@ ifdef TEST_WINDOWS
 			--set windows.image.tag=$(IMAGE_VERSION) \
 			--set windows.enabled=true \
 			--set linux.enabled=false \
-			--set grpcSupportedProviders=azure \
 			--set enableSecretRotation=true \
 			--set rotationPollInterval=30s
 else
@@ -291,7 +290,6 @@ else
 			--set linux.image.repository=$(REGISTRY)/$(IMAGE_NAME) \
 			--set linux.image.tag=$(IMAGE_VERSION) \
 			--set linux.image.pullPolicy="IfNotPresent" \
-			--set grpcSupportedProviders="azure;gcp;vault" \
 			--set enableSecretRotation=true \
 			--set rotationPollInterval=30s
 endif
diff --git a/cmd/secrets-store-csi-driver/main.go b/cmd/secrets-store-csi-driver/main.go
@@ -22,7 +22,6 @@ import (
 	"fmt"
 	"net/http"
 	_ "net/http/pprof" // #nosec
-	"strings"
 	"time"
 
 	"sigs.k8s.io/secrets-store-csi-driver/pkg/cache"
@@ -53,14 +52,12 @@ var (
 	logFormatJSON      = flag.Bool("log-format-json", false, "set log formatter to json")
 	providerVolumePath = flag.String("provider-volume", "/etc/kubernetes/secrets-store-csi-providers", "Volume path for provider")
 	// this will be removed in a future release
-	metricsAddr = flag.String("metrics-addr", ":8095", "The address the metric endpoint binds to")
-	// grpcSupportedProviders is a ; separated string that can contain a list of providers. The reason it's a string is to allow scenarios
-	// where the driver is being used with 2 providers, one which supports grpc and other using binary for provider.
-	grpcSupportedProviders = flag.String("grpc-supported-providers", "", "set list of providers that support grpc for driver-provider [alpha]")
-	enableSecretRotation   = flag.Bool("enable-secret-rotation", false, "Enable secret rotation feature [alpha]")
-	rotationPollInterval   = flag.Duration("rotation-poll-interval", 2*time.Minute, "Secret rotation poll interval duration")
-	enableProfile          = flag.Bool("enable-pprof", false, "enable pprof profiling")
-	profilePort            = flag.Int("pprof-port", 6065, "port for pprof profiling")
+	metricsAddr          = flag.String("metrics-addr", ":8095", "The address the metric endpoint binds to")
+	_                    = flag.String("grpc-supported-providers", "", "[DEPRECATED] set list of providers that support grpc for driver-provider [alpha]")
+	enableSecretRotation = flag.Bool("enable-secret-rotation", false, "Enable secret rotation feature [alpha]")
+	rotationPollInterval = flag.Duration("rotation-poll-interval", 2*time.Minute, "Secret rotation poll interval duration")
+	enableProfile        = flag.Bool("enable-pprof", false, "enable pprof profiling")
+	profilePort          = flag.Int("pprof-port", 6065, "port for pprof profiling")
 
 	// enable filtered watch for NodePublishSecretRef secrets. The filtering is done on the csi driver label: secrets-store.csi.k8s.io/used=true
 	// For Kubernetes secrets used to provide credentials for use with the CSI driver, set the label by running: kubectl label secret secrets-store-creds secrets-store.csi.k8s.io/used=true
@@ -147,24 +144,8 @@ func main() {
 	ctx := withShutdownSignal(context.Background())
 
 	// create provider clients
-	providerClients := make(map[string]*secretsstore.CSIProviderClient)
-	for _, provider := range strings.Split(*grpcSupportedProviders, ";") {
-		p := strings.TrimSpace(provider)
-		if len(p) != 0 {
-			// dialing clients is non-blocking and will be retried on errors
-			providerClients[provider], err = secretsstore.NewProviderClient(secretsstore.CSIProviderName(p), *providerVolumePath)
-			if err != nil {
-				klog.Fatalf("failed to create provider client, err: %+v", err)
-			}
-		}
-	}
-	defer func() {
-		for k, v := range providerClients {
-			if err := v.Close(); err != nil {
-				klog.ErrorS(err, "closing grpc client failed", "provider", k)
-			}
-		}
-	}()
+	providerClients := secretsstore.NewPluginClientBuilder(*providerVolumePath)
+	defer providerClients.Cleanup()
 
 	go func() {
 		klog.Infof("starting manager")
diff --git a/docs/book/src/providers.md b/docs/book/src/providers.md
@@ -22,7 +22,7 @@ This document highlights the implementation steps for adding a secrets-store-csi
 
 ### Implementation details
 
-The driver as of `v0.0.14` adds an option to use gRPC to communicate with the provider. This is an alpha feature and is introduced with a feature flag `--grpc-supported-providers`. The `--grpc-supported-providers` is a `;` delimited list of all providers that support gRPC for communication. The driver will communicate with the provider using gRPC only if the provider name is in the list of supported providers in `--grpc-supported-providers`.
+The driver as of `v0.0.14` adds an option to use gRPC to communicate with the provider. This is an alpha feature and is introduced with a feature flag `--grpc-supported-providers`. The `--grpc-supported-providers` is a `;` delimited list of all providers that support gRPC for communication. This flag will not be necessary after `v0.0.21` since this is the only supported communication mechanism.
 
 > Example usage: `--grpc-supported-providers=provider1;provider2`
 
@@ -33,6 +33,7 @@ To implement a secrets-store-csi-driver provider, you can develop a new provider
   - [fake server example](https://github.com/kubernetes-sigs/secrets-store-csi-driver/blob/master/provider/fake/fake_server.go)
 - Provider runs as a *daemonset* and is deployed on the same host(s) as the secrets-store-csi-driver pods
 - Provider Unix Domain Socket volume path. The default volume path for providers is [/etc/kubernetes/secrets-store-csi-driver-providers](https://github.com/kubernetes-sigs/secrets-store-csi-driver/blob/v0.0.14/deploy/secrets-store-csi-driver.yaml#L88-L89). Add the Unix Domain Socket to the dir in the format `/etc/kubernetes/secrets-store-csi-driver-providers/<provider name>.sock`
+- The `<provider name>` in `<provider name>.sock` must match the regular expression `^[a-zA-Z0-9_-]{0,30}$`
 - Provider mounts `<kubelet root dir>/pods` (default: [`/var/lib/kubelet/pods`](https://github.com/kubernetes-sigs/secrets-store-csi-driver/blob/v0.0.14/deploy/secrets-store-csi-driver.yaml#L86-L87)) with [`HostToContainer` mount propagation](https://kubernetes-csi.github.io/docs/deploying.html#driver-volume-mounts) to be able to write the external secrets store content to the volume target path
 
 See [design doc](https://docs.google.com/document/d/10-RHUJGM0oMN88AZNxjOmGz0NsWAvOYrWUEV-FbLWyw/edit?usp=sharing) for more details.
diff --git a/manifest_staging/charts/secrets-store-csi-driver/README.md b/manifest_staging/charts/secrets-store-csi-driver/README.md
@@ -80,6 +80,5 @@ The following table lists the configurable parameters of the csi-secrets-store-p
 | `rbac.install`                          | Install default rbac roles and bindings                                                                                           | true                                                |
 | `syncSecret.enabled`                    | Enable rbac roles and bindings required for syncing to Kubernetes native secrets (the default will change to false after v0.0.14) | true                                                |
 | `minimumProviderVersions`               | [**DEPRECATED**] A comma delimited list of key-value pairs of minimum provider versions with driver                               | `""`                                                |
-| `grpcSupportedProviders`                | A `;` delimited list of providers that support grpc for driver-provider                                                           | `"gcp;azure;vault;"`                                |
 | `enableSecretRotation`                  | Enable secret rotation feature [alpha]                                                                                            | `false`                                             |
 | `rotationPollInterval`                  | Secret rotation poll interval duration                                                                                            | `"120s"`                                            |
diff --git a/manifest_staging/charts/secrets-store-csi-driver/templates/secrets-store-csi-driver-windows.yaml b/manifest_staging/charts/secrets-store-csi-driver/templates/secrets-store-csi-driver-windows.yaml
@@ -66,9 +66,6 @@ spec:
             {{- if and (semverCompare ">= v0.0.9-0" .Values.windows.image.tag) .Values.minimumProviderVersions }}
             - "--min-provider-version={{ .Values.minimumProviderVersions }}"
             {{- end }}
-            {{- if and (semverCompare ">= v0.0.14-0" .Values.windows.image.tag) .Values.grpcSupportedProviders }}
-            - "--grpc-supported-providers={{ .Values.grpcSupportedProviders }}"
-            {{- end }}
             {{- if and (semverCompare ">= v0.0.15-0" .Values.windows.image.tag) .Values.enableSecretRotation }}
             - "--enable-secret-rotation={{ .Values.enableSecretRotation }}"
             {{- end }}
diff --git a/manifest_staging/charts/secrets-store-csi-driver/templates/secrets-store-csi-driver.yaml b/manifest_staging/charts/secrets-store-csi-driver/templates/secrets-store-csi-driver.yaml
@@ -66,9 +66,6 @@ spec:
             {{- if and (semverCompare ">= v0.0.8-0" .Values.linux.image.tag) .Values.minimumProviderVersions }}
             - "--min-provider-version={{ .Values.minimumProviderVersions }}"
             {{- end }}
-            {{- if and (semverCompare ">= v0.0.14-0" .Values.linux.image.tag) .Values.grpcSupportedProviders }}
-            - "--grpc-supported-providers={{ .Values.grpcSupportedProviders }}"
-            {{- end }}
             {{- if and (semverCompare ">= v0.0.15-0" .Values.linux.image.tag) .Values.enableSecretRotation }}
             - "--enable-secret-rotation={{ .Values.enableSecretRotation }}"
             {{- end }}
diff --git a/manifest_staging/charts/secrets-store-csi-driver/values.yaml b/manifest_staging/charts/secrets-store-csi-driver/values.yaml
@@ -144,9 +144,6 @@ syncSecret:
 ## e.g. provider1=0.0.2,provider2=0.0.3
 minimumProviderVersions:
 
-## ; delimited list of providers that support grpc for driver-provider [alpha]
-grpcSupportedProviders: gcp;azure;vault;
-
 ## Enable secret rotation feature [alpha]
 enableSecretRotation: false
 
diff --git a/manifest_staging/deploy/secrets-store-csi-driver-windows.yaml b/manifest_staging/deploy/secrets-store-csi-driver-windows.yaml
@@ -48,7 +48,6 @@ spec:
             - "--nodeid=$(KUBE_NODE_NAME)"
             - "--provider-volume=C:\\k\\secrets-store-csi-providers"
             - "--metrics-addr=:8095"
-            - "--grpc-supported-providers=azure;"
             - "--enable-secret-rotation=false"
             - "--rotation-poll-interval=2m"
           env:
diff --git a/manifest_staging/deploy/secrets-store-csi-driver.yaml b/manifest_staging/deploy/secrets-store-csi-driver.yaml
@@ -48,7 +48,6 @@ spec:
             - "--nodeid=$(KUBE_NODE_NAME)"
             - "--provider-volume=/etc/kubernetes/secrets-store-csi-providers"
             - "--metrics-addr=:8095"
-            - "--grpc-supported-providers=gcp;azure;vault;"
             - "--enable-secret-rotation=false"
             - "--rotation-poll-interval=2m"
           env:
diff --git a/pkg/rotation/reconciler.go b/pkg/rotation/reconciler.go
@@ -76,7 +76,7 @@ type Reconciler struct {
 	providerVolumePath   string
 	scheme               *runtime.Scheme
 	rotationPollInterval time.Duration
-	providerClients      map[string]*secretsstore.CSIProviderClient
+	providerClients      *secretsstore.PluginClientBuilder
 	queue                workqueue.RateLimitingInterface
 	reporter             StatsReporter
 	eventRecorder        record.EventRecorder
@@ -85,7 +85,7 @@ type Reconciler struct {
 }
 
 // NewReconciler returns a new reconciler for rotation
-func NewReconciler(s *runtime.Scheme, providerVolumePath, nodeName string, rotationPollInterval time.Duration, providerClients map[string]*secretsstore.CSIProviderClient, filteredWatchSecret bool) (*Reconciler, error) {
+func NewReconciler(s *runtime.Scheme, providerVolumePath, nodeName string, rotationPollInterval time.Duration, providerClients *secretsstore.PluginClientBuilder, filteredWatchSecret bool) (*Reconciler, error) {
 	config, err := buildConfig()
 	if err != nil {
 		return nil, err
@@ -273,13 +273,13 @@ func (r *Reconciler) reconcile(ctx context.Context, spcps *v1alpha1.SecretProvid
 	}
 
 	providerName = string(spc.Spec.Provider)
-	providerClient, exists := r.providerClients[providerName]
-	if !exists {
+	providerClient, err := r.providerClients.Get(ctx, providerName)
+	if err != nil {
 		errorReason = internalerrors.FailedToLookupProviderGRPCClient
 		r.generateEvent(pod, v1.EventTypeWarning, mountRotationFailedReason, fmt.Sprintf("failed to lookup provider client: %q", providerName))
 		return fmt.Errorf("failed to lookup provider client: %q", providerName)
 	}
-	newObjectVersions, errorReason, err := providerClient.MountContent(ctx, string(paramsJSON), string(secretsJSON), spcps.Status.TargetPath, string(permissionJSON), oldObjectVersions)
+	newObjectVersions, errorReason, err := secretsstore.MountContent(ctx, providerClient, string(paramsJSON), string(secretsJSON), spcps.Status.TargetPath, string(permissionJSON), oldObjectVersions)
 	if err != nil {
 		r.generateEvent(pod, v1.EventTypeWarning, mountRotationFailedReason, fmt.Sprintf("provider mount err: %+v", err))
 		return fmt.Errorf("failed to rotate objects for pod %s/%s, err: %+v", spcps.Namespace, spcps.Status.PodName, err)
diff --git a/pkg/rotation/reconciler_test.go b/pkg/rotation/reconciler_test.go
@@ -72,24 +72,17 @@ func newTestReconciler(s *runtime.Scheme, kubeClient kubernetes.Interface, crdCl
 		return nil, err
 	}
 
-	client, err := secretsstore.NewProviderClient("provider1", socketPath)
-	if err != nil {
-		return nil, err
-	}
-
 	return &Reconciler{
 		store:                store,
 		scheme:               s,
 		providerVolumePath:   socketPath,
 		rotationPollInterval: rotationPollInterval,
-		providerClients: map[string]*secretsstore.CSIProviderClient{
-			"provider1": client,
-		},
-		queue:         workqueue.NewRateLimitingQueue(workqueue.DefaultControllerRateLimiter()),
-		reporter:      newStatsReporter(),
-		eventRecorder: fakeRecorder,
-		kubeClient:    kubeClient,
-		crdClient:     crdClient,
+		providerClients:      secretsstore.NewPluginClientBuilder(socketPath),
+		queue:                workqueue.NewRateLimitingQueue(workqueue.DefaultControllerRateLimiter()),
+		reporter:             newStatsReporter(),
+		eventRecorder:        fakeRecorder,
+		kubeClient:           kubeClient,
+		crdClient:            crdClient,
 	}, nil
 }
 
diff --git a/pkg/secrets-store/nodeserver.go b/pkg/secrets-store/nodeserver.go
@@ -43,7 +43,7 @@ type nodeServer struct {
 	reporter           StatsReporter
 	nodeID             string
 	client             client.Client
-	providerClients    map[string]*CSIProviderClient
+	providerClients    *PluginClientBuilder
 }
 
 const (
@@ -289,13 +289,14 @@ func (ns *nodeServer) mountSecretsStoreObjectContent(ctx context.Context, provid
 		return nil, "", fmt.Errorf("providers volume path not found. Set PROVIDERS_VOLUME_PATH")
 	}
 
-	client, exists := ns.providerClients[providerName]
-	if !exists {
-		return nil, "", fmt.Errorf("provider not found in --grpc-supported-providers")
+	client, err := ns.providerClients.Get(ctx, providerName)
+	if err != nil {
+		return nil, "", fmt.Errorf("error connecting to provider %q: %w", providerName, err)
 	}
 
 	klog.InfoS("Using grpc client", "provider", providerName, "pod", podName)
-	return client.MountContent(ctx, attributes, secrets, targetPath, permission, nil)
+
+	return MountContent(ctx, client, attributes, secrets, targetPath, permission, nil)
 }
 
 func (ns *nodeServer) NodeExpandVolume(ctx context.Context, req *csi.NodeExpandVolumeRequest) (*csi.NodeExpandVolumeResponse, error) {
diff --git a/pkg/secrets-store/nodeserver_test.go b/pkg/secrets-store/nodeserver_test.go
@@ -38,19 +38,12 @@ import (
 	"sigs.k8s.io/controller-runtime/pkg/client/fake"
 )
 
-func testNodeServer(mountPoints []mount.MountPoint, client client.Client, grpcSupportProvider string, reporter StatsReporter) (*nodeServer, error) {
+func testNodeServer(mountPoints []mount.MountPoint, client client.Client, reporter StatsReporter) (*nodeServer, error) {
 	tmpDir, err := os.MkdirTemp("", "ut")
 	if err != nil {
 		return nil, err
 	}
-	providerClients := map[string]*CSIProviderClient{}
-	if grpcSupportProvider != "" {
-		client, err := NewProviderClient(CSIProviderName(grpcSupportProvider), tmpDir)
-		if err != nil {
-			return nil, err
-		}
-		providerClients[grpcSupportProvider] = client
-	}
+	providerClients := NewPluginClientBuilder(tmpDir)
 	return newNodeServer(NewFakeDriver(), tmpDir, "testnode", mount.NewFakeMounter(mountPoints), providerClients, client, reporter)
 }
 
@@ -64,15 +57,14 @@ func getTestTargetPath(pattern string, t *testing.T) string {
 
 func TestNodePublishVolume(t *testing.T) {
 	tests := []struct {
-		name                 string
-		nodePublishVolReq    csi.NodePublishVolumeRequest
-		mountPoints          []mount.MountPoint
-		initObjects          []runtime.Object
-		grpcSupportProviders string
-		RPCCode              codes.Code
-		wantsRPCCode         bool
-		expectedErr          bool
-		shouldRetryRemount   bool
+		name               string
+		nodePublishVolReq  csi.NodePublishVolumeRequest
+		mountPoints        []mount.MountPoint
+		initObjects        []runtime.Object
+		RPCCode            codes.Code
+		wantsRPCCode       bool
+		expectedErr        bool
+		shouldRetryRemount bool
 	}{
 		{
 			name:               "volume capabilities nil",
@@ -256,7 +248,7 @@ func TestNodePublishVolume(t *testing.T) {
 				test.mountPoints = append(test.mountPoints, mount.MountPoint{Path: absFile})
 			}
 			r := mocks.NewFakeReporter()
-			ns, err := testNodeServer(test.mountPoints, fake.NewFakeClientWithScheme(s, test.initObjects...), test.grpcSupportProviders, r)
+			ns, err := testNodeServer(test.mountPoints, fake.NewFakeClientWithScheme(s, test.initObjects...), r)
 			if err != nil {
 				t.Fatalf("expected error to be nil, got: %+v", err)
 			}
@@ -307,14 +299,13 @@ func TestNodePublishVolume(t *testing.T) {
 
 func TestMountSecretsStoreObjectContent(t *testing.T) {
 	tests := []struct {
-		name                 string
-		attributes           string
-		secrets              string
-		targetPath           string
-		permission           string
-		grpcSupportProviders string
-		expectedErrorReason  string
-		expectedErr          bool
+		name                string
+		attributes          string
+		secrets             string
+		targetPath          string
+		permission          string
+		expectedErrorReason string
+		expectedErr         bool
 	}{
 		{
 			name:        "attributes empty",
@@ -335,7 +326,7 @@ func TestMountSecretsStoreObjectContent(t *testing.T) {
 
 	for _, test := range tests {
 		t.Run(test.name, func(t *testing.T) {
-			ns, err := testNodeServer(nil, fake.NewFakeClientWithScheme(nil), test.grpcSupportProviders, mocks.NewFakeReporter())
+			ns, err := testNodeServer(nil, fake.NewFakeClientWithScheme(nil), mocks.NewFakeReporter())
 			if err != nil {
 				t.Fatalf("expected error to be nil, got: %+v", err)
 			}
@@ -352,14 +343,13 @@ func TestMountSecretsStoreObjectContent(t *testing.T) {
 
 func TestNodeUnpublishVolume(t *testing.T) {
 	tests := []struct {
-		name                 string
-		grpcSupportProviders string
-		nodeUnpublishVolReq  csi.NodeUnpublishVolumeRequest
-		mountPoints          []mount.MountPoint
-		RPCCode              codes.Code
-		wantsErr             bool
-		wantsRPCCode         bool
-		shouldRetryUnmount   bool
+		name                string
+		nodeUnpublishVolReq csi.NodeUnpublishVolumeRequest
+		mountPoints         []mount.MountPoint
+		RPCCode             codes.Code
+		wantsErr            bool
+		wantsRPCCode        bool
+		shouldRetryUnmount  bool
 	}{
 		{
 			name: "Failure: volume id is empty",
@@ -411,7 +401,7 @@ func TestNodeUnpublishVolume(t *testing.T) {
 			}
 
 			r := mocks.NewFakeReporter()
-			ns, err := testNodeServer(test.mountPoints, fake.NewFakeClientWithScheme(s), test.grpcSupportProviders, r)
+			ns, err := testNodeServer(test.mountPoints, fake.NewFakeClientWithScheme(s), r)
 			if err != nil {
 				t.Fatalf("expected error to be nil, got: %+v", err)
 			}
diff --git a/pkg/secrets-store/provider_client.go b/pkg/secrets-store/provider_client.go
diff --git a/pkg/secrets-store/provider_client_test.go b/pkg/secrets-store/provider_client_test.go
diff --git a/pkg/secrets-store/secrets-store.go b/pkg/secrets-store/secrets-store.go
diff --git a/provider/fake/fake_server.go b/provider/fake/fake_server.go
