Fix rotation enable check

Author: dargudear-google

controllers/secretproviderclasspodstatus_controller.go

@@ -419,13 +419,12 @@ func (r *SecretProviderClassPodStatusReconciler) createOrUpdateK8sSecret(ctx con
 		return err
 	}
 
-	klog.InfoS("Kubernetes secret is already created", "secret", klog.ObjectRef{Namespace: namespace, Name: name})
+	klog.V(5).InfoS("Kubernetes secret is already created", "secret", klog.ObjectRef{Namespace: namespace, Name: name})
 	err = r.writer.Update(ctx, secret)
 	if err != nil {
-		klog.ErrorS(err, "Unable to update kubernetes secret", "secret", klog.ObjectRef{Namespace: namespace, Name: name})
 		return err
 	}
-	klog.InfoS("successfully updated Kubernetes secret", "secret", klog.ObjectRef{Namespace: namespace, Name: name})
+	klog.V(5).InfoS("successfully updated Kubernetes secret", "secret", klog.ObjectRef{Namespace: namespace, Name: name})
 	return nil
 }
 

manifest_staging/charts/secrets-store-csi-driver/templates/csidriver.yaml

@@ -14,5 +14,4 @@ spec:
   requiresRepublish: true
   tokenRequests:
     {{- toYaml .Values.tokenRequests | nindent 2 }}
-  requiresRepublish: {{ .Values.requiresRepublish }}
   {{- end }}

manifest_staging/charts/secrets-store-csi-driver/values.yaml

@@ -232,16 +232,11 @@ providerHealthCheckInterval: 2m
 
 imagePullSecrets: []
 
-## This allows CSI drivers to impersonate the pods that they mount the volumes for.
-# refer to https://kubernetes-csi.github.io/docs/token-requests.html for more details.
-# Supported only for Kubernetes v1.20+
 tokenRequests: []
 # - audience: aud1
 # - audience: aud2
 
-# To set the requiresRepublish which can be used to refresh the mounted secret periodically
-# refer to https://kubernetes-csi.github.io/docs/token-requests.html for more details.
-# Supported only for Kubernetes v1.20+
+# this will be set to true even if enableSecretRotation is true
 requiresRepublish: true
 # -- Labels to apply to all resources
 commonLabels: {}

pkg/secrets-store/mocks/stats_reporter_mock.go

@@ -16,7 +16,9 @@ limitations under the License.
 
 package mocks // import sigs.k8s.io/secrets-store-csi-driver/pkg/secrets-store/mocks
 
-import "context"
+import (
+	"context"
+)
 
 type FakeReporter struct {
 	reportNodePublishCtMetricInvoked        int
@@ -73,3 +75,12 @@ func (f *FakeReporter) ReportSyncK8SecretCtMetricInvoked() int {
 func (f *FakeReporter) ReportSyncK8SecretDurationInvoked() int {
 	return f.reportSyncK8SecretDurationInvoked
 }
+
+func (f *FakeReporter) ReportRotationCtMetric(ctx context.Context, provider string, wasRotated bool) {
+}
+
+func (f *FakeReporter) ReportRotationErrorCtMetric(ctx context.Context, provider, errType string, wasRotated bool) {
+}
+
+func (f *FakeReporter) ReportRotationDuration(ctx context.Context, duration float64) {
+}

pkg/secrets-store/nodeserver.go

@@ -90,8 +90,15 @@ func (ns *nodeServer) NodePublishVolume(ctx context.Context, req *csi.NodePublis
 				}
 			}
 			ns.reporter.ReportNodePublishErrorCtMetric(ctx, providerName, errorReason)
+			if isRemountRequest {
+				ns.reporter.ReportRotationErrorCtMetric(ctx, providerName, errorReason, true)
+			}
 			return
 		}
+		if isRemountRequest {
+			ns.reporter.ReportRotationCtMetric(ctx, providerName, true)
+			ns.reporter.ReportRotationDuration(ctx, time.Since(startTime).Seconds())
+		}
 		ns.reporter.ReportNodePublishCtMetric(ctx, providerName)
 	}()
 
@@ -121,6 +128,16 @@ func (ns *nodeServer) NodePublishVolume(ctx context.Context, req *csi.NodePublis
 	podNamespace = attrib[CSIPodNamespace]
 	podUID = attrib[CSIPodUID]
 
+	if rotationEnabled {
+		lastModificationTime, err := ns.getLastUpdateTime(targetPath)
+		if err != nil {
+			klog.InfoS("could not find last modification time for targetpath", targetPath, "error", err)
+		} else if startTime.Before(lastModificationTime.Add(ns.rotationConfig.rotationCacheDuration)) {
+			// if next rotation is not yet due, then skip the mount operation
+			return &csi.NodePublishVolumeResponse{}, nil
+		}
+	}
+
 	mounted, err = ns.ensureMountPoint(targetPath)
 	if err != nil {
 		// kubelet will not create the CSI NodePublishVolume target directory in 1.20+, in accordance with the CSI specification.
@@ -143,16 +160,6 @@ func (ns *nodeServer) NodePublishVolume(ctx context.Context, req *csi.NodePublis
 		return &csi.NodePublishVolumeResponse{}, nil
 	}
 
-	if rotationEnabled {
-		lastModificationTime, err := ns.getLastUpdateTime(targetPath)
-		if err != nil {
-			klog.InfoS("could not find last modification time for targetpath", targetPath, "error", err)
-		} else if startTime.Before(lastModificationTime.Add(ns.rotationConfig.rotationPollInterval)) {
-			// if next rotation is not yet due, then skip the mount operation
-			return &csi.NodePublishVolumeResponse{}, nil
-		}
-	}
-
 	klog.V(2).InfoS("node publish volume", "target", targetPath, "volumeId", volumeID, "mount flags", mountFlags)
 
 	if isMockProvider(providerName) {
@@ -192,16 +199,12 @@ func (ns *nodeServer) NodePublishVolume(ctx context.Context, req *csi.NodePublis
 	for k, v := range attrib {
 		parameters[k] = v
 	}
-	// csi.storage.k8s.io/serviceAccount.tokens is empty for Kubernetes version < 1.20.
-	// For 1.20+, if tokenRequests is set in the CSI driver spec, kubelet will generate
-	// a token for the pod and send it to the CSI driver.
-	// This check is done for backward compatibility to support passing token from driver
-	// to provider irrespective of the Kubernetes version. If the token doesn't exist in the
-	// volume request context, the CSI driver will generate the token for the configured audience
-	// and send it to the provider in the parameters.
+	// If the token doesn't exist in the volume request context, the CSI driver
+	// will generate the token for the configured audience and send it to the
+	// provider in the parameters.
 	if parameters[CSIPodServiceAccountTokens] == "" {
 		// Inject pod service account token into volume attributes
-		klog.Info(err, "csi.storage.k8s.io/serviceAccount.tokens is not populated")
+		klog.Warning("csi.storage.k8s.io/serviceAccount.tokens is not populated")
 	}
 
 	// ensure it's read-only

pkg/secrets-store/nodeserver_test.go

@@ -297,8 +297,8 @@ func TestNodePublishVolume(t *testing.T) {
 				},
 			},
 			rotationConfig: &rotationConfig{
-				enabled:              false,
-				rotationPollInterval: time.Minute,
+				enabled:               false,
+				rotationCacheDuration: time.Minute,
 			},
 		},
 		{
@@ -331,8 +331,8 @@ func TestNodePublishVolume(t *testing.T) {
 				},
 			},
 			rotationConfig: &rotationConfig{
-				enabled:              true,
-				rotationPollInterval: -1 * time.Minute, // Using negative interval to pass the rotation interval check in unit tests
+				enabled:               true,
+				rotationCacheDuration: -1 * time.Minute, // Using negative interval to pass the rotation interval check in unit tests
 			},
 		},
 		{
@@ -362,8 +362,8 @@ func TestNodePublishVolume(t *testing.T) {
 				},
 			},
 			rotationConfig: &rotationConfig{
-				enabled:              true,
-				rotationPollInterval: time.Minute,
+				enabled:               true,
+				rotationCacheDuration: time.Minute,
 			},
 		},
 	}
@@ -407,7 +407,7 @@ func TestNodePublishVolume(t *testing.T) {
 					t.Fatalf("expected err to be nil, got: %v", err)
 				}
 				expectedMounts := 1
-				if ns.rotationConfig.enabled && ns.rotationConfig.rotationPollInterval > 0 {
+				if ns.rotationConfig.enabled && ns.rotationConfig.rotationCacheDuration > 0 {
 					// If due to rotation interval, NodePublishVolume has skipped, there should not be any mount operation
 					expectedMounts = 0
 				}

pkg/secrets-store/secrets-store.go

@@ -40,8 +40,8 @@ type SecretsStore struct {
 
 // rotationConfig stores the information required to rotate the secrets.
 type rotationConfig struct {
-	enabled              bool
-	rotationPollInterval time.Duration
+	enabled               bool
+	rotationCacheDuration time.Duration // After this much duration, NodePublishVolume will be acted.
 }
 
 func NewSecretsStoreDriver(driverName, nodeID, endpoint string,
@@ -91,8 +91,8 @@ func newNodeServer(nodeID string,
 
 func newRotationConfig(enabled bool, interval time.Duration) *rotationConfig {
 	return &rotationConfig{
-		enabled:              enabled,
-		rotationPollInterval: interval,
+		enabled:               enabled,
+		rotationCacheDuration: interval,
 	}
 }
 

pkg/secrets-store/stats_reporter.go

@@ -34,15 +34,19 @@ var (
 	errorKey    = "error_type"
 	osTypeKey   = "os_type"
 	runtimeOS   = runtime.GOOS
+	rotatedKey  = "rotated"
 )
 
 type reporter struct {
-	nodePublishTotal        metric.Int64Counter
-	nodeUnPublishTotal      metric.Int64Counter
-	nodePublishErrorTotal   metric.Int64Counter
-	nodeUnPublishErrorTotal metric.Int64Counter
-	syncK8sSecretTotal      metric.Int64Counter
-	syncK8sSecretDuration   metric.Float64Histogram
+	nodePublishTotal            metric.Int64Counter
+	nodeUnPublishTotal          metric.Int64Counter
+	nodePublishErrorTotal       metric.Int64Counter
+	nodeUnPublishErrorTotal     metric.Int64Counter
+	syncK8sSecretTotal          metric.Int64Counter
+	syncK8sSecretDuration       metric.Float64Histogram
+	rotationReconcileTotal      metric.Int64Counter
+	rotationReconcileErrorTotal metric.Int64Counter
+	rotationReconcileDuration   metric.Float64Histogram
 }
 
 type StatsReporter interface {
@@ -52,6 +56,9 @@ type StatsReporter interface {
 	ReportNodeUnPublishErrorCtMetric(ctx context.Context)
 	ReportSyncK8SecretCtMetric(ctx context.Context, provider string, count int)
 	ReportSyncK8SecretDuration(ctx context.Context, duration float64)
+	ReportRotationCtMetric(ctx context.Context, provider string, wasRotated bool)
+	ReportRotationErrorCtMetric(ctx context.Context, provider, errType string, wasRotated bool)
+	ReportRotationDuration(ctx context.Context, duration float64)
 }
 
 func NewStatsReporter() (StatsReporter, error) {
@@ -78,6 +85,16 @@ func NewStatsReporter() (StatsReporter, error) {
 	if r.syncK8sSecretDuration, err = meter.Float64Histogram("k8s_secret_duration_sec", metric.WithDescription("Distribution of how long it took to sync k8s secret")); err != nil {
 		return nil, err
 	}
+	if r.rotationReconcileTotal, err = meter.Int64Counter("rotation_reconcile", metric.WithDescription("Total number of rotation reconciles")); err != nil {
+		return nil, err
+	}
+	if r.rotationReconcileErrorTotal, err = meter.Int64Counter("rotation_reconcile_error", metric.WithDescription("Total number of rotation reconciles with error")); err != nil {
+		return nil, err
+	}
+	if r.rotationReconcileDuration, err = meter.Float64Histogram("rotation_reconcile_duration_sec", metric.WithDescription("Distribution of how long it took to rotate secrets-store content for pods")); err != nil {
+		return nil, err
+	}
+
 	return r, nil
 }
 
@@ -126,3 +143,29 @@ func (r *reporter) ReportSyncK8SecretDuration(ctx context.Context, duration floa
 	)
 	r.syncK8sSecretDuration.Record(ctx, duration, opt)
 }
+
+func (r *reporter) ReportRotationCtMetric(ctx context.Context, provider string, wasRotated bool) {
+	opt := metric.WithAttributes(
+		attribute.Key(providerKey).String(provider),
+		attribute.Key(osTypeKey).String(runtimeOS),
+		attribute.Key(rotatedKey).Bool(wasRotated),
+	)
+	r.rotationReconcileTotal.Add(ctx, 1, opt)
+}
+
+func (r *reporter) ReportRotationErrorCtMetric(ctx context.Context, provider, errType string, wasRotated bool) {
+	opt := metric.WithAttributes(
+		attribute.Key(providerKey).String(provider),
+		attribute.Key(errorKey).String(errType),
+		attribute.Key(osTypeKey).String(runtimeOS),
+		attribute.Key(rotatedKey).Bool(wasRotated),
+	)
+	r.rotationReconcileErrorTotal.Add(ctx, 1, opt)
+}
+
+func (r *reporter) ReportRotationDuration(ctx context.Context, duration float64) {
+	opt := metric.WithAttributes(
+		attribute.Key(osTypeKey).String(runtimeOS),
+	)
+	r.rotationReconcileDuration.Record(ctx, duration, opt)
+}

test/bats/e2e-provider.bats

@@ -418,6 +418,7 @@ export VALIDATE_TOKENS_AUDIENCE=$(get_token_requests_audience)
     run kubectl exec ${curl_pod_name} -n metrics -- curl http://${pod_ip}:8095/metrics
     assert_match "node_publish_total" "${output}"
     assert_match "node_unpublish_total" "${output}"
+    assert_match "rotation_reconcile_total" "${output}"
   done
   # keeping metrics ns in upgrade tests has no relevance
   # the namespace is only to run a curl pod to validate metrics