return an api error if existing values do not exist when un-redacting secrets

pkg/apis/management/v1/managementv1_suite_test.go

@@ -9,5 +9,5 @@ import (
 
 func TestV1(t *testing.T) {
 	RegisterFailHandler(Fail)
-	RunSpecs(t, "V1 Suite")
+	RunSpecs(t, "Management API Suite")
 }

pkg/apis/storage/v1/secrets.go

@@ -1,5 +1,10 @@
 package v1
 
+import (
+	"google.golang.org/grpc/codes"
+	"google.golang.org/grpc/status"
+)
+
 const Redacted = "***"
 
 func (cfg *StorageSpec) RedactSecrets() {
@@ -56,56 +61,90 @@ func (cfg *SSEConfig) RedactSecrets() {
 	}
 }
 
-func (cfg *StorageSpec) UnredactSecrets(unredacted *StorageSpec) {
-	if cfg == nil || unredacted == nil {
-		return
+func (cfg *StorageSpec) UnredactSecrets(unredacted *StorageSpec) error {
+	if cfg == nil {
+		return nil
 	}
-	if cfg.S3 != nil && unredacted.S3 != nil {
-		cfg.S3.UnredactSecrets(unredacted.S3)
+	if cfg.S3 != nil {
+		if err := cfg.S3.UnredactSecrets(unredacted.GetS3()); err != nil {
+			return err
+		}
 	}
-	if cfg.Gcs != nil && unredacted.Gcs != nil {
-		cfg.Gcs.UnredactSecrets(unredacted.Gcs)
+	if cfg.Gcs != nil {
+		if err := cfg.Gcs.UnredactSecrets(unredacted.GetGcs()); err != nil {
+			return err
+		}
 	}
-	if cfg.Azure != nil && unredacted.Azure != nil {
-		cfg.Azure.UnredactSecrets(unredacted.Azure)
+	if cfg.Azure != nil {
+		if err := cfg.Azure.UnredactSecrets(unredacted.GetAzure()); err != nil {
+			return err
+		}
 	}
-	if cfg.Swift != nil && unredacted.Swift != nil {
-		cfg.Swift.UnredactSecrets(unredacted.Swift)
+	if cfg.Swift != nil {
+		if err := cfg.Swift.UnredactSecrets(unredacted.GetSwift()); err != nil {
+			return err
+		}
 	}
+	return nil
 }
 
-func (cfg *S3StorageSpec) UnredactSecrets(unredacted *S3StorageSpec) {
+func (cfg *S3StorageSpec) UnredactSecrets(unredacted *S3StorageSpec) error {
 	if cfg.SecretAccessKey == Redacted {
+		if unredacted.GetSecretAccessKey() == "" {
+			return status.Error(codes.FailedPrecondition, "no secret access key provided")
+		}
 		cfg.SecretAccessKey = unredacted.SecretAccessKey
 	}
 	if cfg.Sse != nil {
-		cfg.Sse.UnredactSecrets(unredacted.Sse)
+		if err := cfg.Sse.UnredactSecrets(unredacted.GetSse()); err != nil {
+			return err
+		}
 	}
+	return nil
 }
 
-func (cfg *GCSStorageSpec) UnredactSecrets(unredacted *GCSStorageSpec) {
+func (cfg *GCSStorageSpec) UnredactSecrets(unredacted *GCSStorageSpec) error {
 	if cfg.ServiceAccount == Redacted {
+		if unredacted.GetServiceAccount() == "" {
+			return status.Error(codes.FailedPrecondition, "no service account provided")
+		}
 		cfg.ServiceAccount = unredacted.ServiceAccount
 	}
+	return nil
 }
 
-func (cfg *AzureStorageSpec) UnredactSecrets(unredacted *AzureStorageSpec) {
+func (cfg *AzureStorageSpec) UnredactSecrets(unredacted *AzureStorageSpec) error {
 	if cfg.StorageAccountKey == Redacted {
+		if unredacted.GetStorageAccountKey() == "" {
+			return status.Error(codes.FailedPrecondition, "no storage account key provided")
+		}
 		cfg.StorageAccountKey = unredacted.StorageAccountKey
 	}
 	if cfg.MsiResource == Redacted {
+		if unredacted.GetMsiResource() == "" {
+			return status.Error(codes.FailedPrecondition, "no MSI resource provided")
+		}
 		cfg.MsiResource = unredacted.MsiResource
 	}
+	return nil
 }
 
-func (cfg *SwiftStorageSpec) UnredactSecrets(unredacted *SwiftStorageSpec) {
+func (cfg *SwiftStorageSpec) UnredactSecrets(unredacted *SwiftStorageSpec) error {
 	if cfg.Password == Redacted {
+		if unredacted.GetPassword() == "" {
+			return status.Error(codes.FailedPrecondition, "no password provided")
+		}
 		cfg.Password = unredacted.Password
 	}
+	return nil
 }
 
-func (cfg *SSEConfig) UnredactSecrets(unredacted *SSEConfig) {
+func (cfg *SSEConfig) UnredactSecrets(unredacted *SSEConfig) error {
 	if cfg.KmsEncryptionContext == Redacted {
+		if unredacted.GetKmsEncryptionContext() == "" {
+			return status.Error(codes.FailedPrecondition, "no KMS encryption context provided")
+		}
 		cfg.KmsEncryptionContext = unredacted.KmsEncryptionContext
 	}
+	return nil
 }

pkg/apis/storage/v1/secrets_test.go

@@ -0,0 +1,178 @@
+package v1_test
+
+import (
+	. "github.com/onsi/ginkgo/v2"
+	. "github.com/onsi/gomega"
+
+	storagev1 "github.com/rancher/opni/pkg/apis/storage/v1"
+)
+
+var _ = Describe("Secrets Redaction", func() {
+	It("should redact secrets", func() {
+		cfg := &storagev1.StorageSpec{
+			S3: &storagev1.S3StorageSpec{
+				SecretAccessKey: "secret",
+				Sse: &storagev1.SSEConfig{
+					KmsEncryptionContext: "secret",
+				},
+			},
+			Gcs: &storagev1.GCSStorageSpec{
+				ServiceAccount: "secret",
+			},
+			Azure: &storagev1.AzureStorageSpec{
+				StorageAccountKey: "secret",
+				MsiResource:       "secret",
+			},
+			Swift: &storagev1.SwiftStorageSpec{
+				Password: "secret",
+			},
+		}
+		cfg.RedactSecrets()
+
+		Expect(cfg.S3.SecretAccessKey).To(Equal(storagev1.Redacted))
+		Expect(cfg.S3.Sse.KmsEncryptionContext).To(Equal(storagev1.Redacted))
+		Expect(cfg.Gcs.ServiceAccount).To(Equal(storagev1.Redacted))
+		Expect(cfg.Azure.StorageAccountKey).To(Equal(storagev1.Redacted))
+		Expect(cfg.Azure.MsiResource).To(Equal(storagev1.Redacted))
+		Expect(cfg.Swift.Password).To(Equal(storagev1.Redacted))
+
+		cfg2 := (*storagev1.StorageSpec)(nil)
+		Expect(cfg2.RedactSecrets).NotTo(Panic())
+	})
+	It("should un-redact secrets if previous values exist", func() {
+		cfg := &storagev1.StorageSpec{
+			S3: &storagev1.S3StorageSpec{
+				SecretAccessKey: "secret",
+				Sse: &storagev1.SSEConfig{
+					KmsEncryptionContext: "secret",
+				},
+			},
+			Gcs: &storagev1.GCSStorageSpec{
+				ServiceAccount: "secret",
+			},
+			Azure: &storagev1.AzureStorageSpec{
+				StorageAccountKey: "secret",
+				MsiResource:       "secret",
+			},
+			Swift: &storagev1.SwiftStorageSpec{
+				Password: "secret",
+			},
+		}
+		previous := cfg.DeepCopy()
+		cfg.RedactSecrets()
+		Expect(cfg.UnredactSecrets(previous)).To(Succeed())
+
+		Expect(cfg.S3.SecretAccessKey).To(Equal(previous.S3.SecretAccessKey))
+		Expect(cfg.S3.Sse.KmsEncryptionContext).To(Equal(previous.S3.Sse.KmsEncryptionContext))
+		Expect(cfg.Gcs.ServiceAccount).To(Equal(previous.Gcs.ServiceAccount))
+		Expect(cfg.Azure.StorageAccountKey).To(Equal(previous.Azure.StorageAccountKey))
+		Expect(cfg.Azure.MsiResource).To(Equal(previous.Azure.MsiResource))
+		Expect(cfg.Swift.Password).To(Equal(previous.Swift.Password))
+	})
+	It("should error when un-redacting if previous values do not exist", func() {
+		previous := &storagev1.StorageSpec{
+			S3: &storagev1.S3StorageSpec{
+				SecretAccessKey: "secret",
+				Sse: &storagev1.SSEConfig{
+					KmsEncryptionContext: "secret",
+				},
+			},
+			Gcs: &storagev1.GCSStorageSpec{
+				ServiceAccount: "secret",
+			},
+			Azure: &storagev1.AzureStorageSpec{
+				StorageAccountKey: "secret",
+				MsiResource:       "secret",
+			},
+			Swift: &storagev1.SwiftStorageSpec{
+				Password: "secret",
+			},
+		}
+		previous.RedactSecrets()
+		Expect(previous.UnredactSecrets(nil)).NotTo(Succeed())
+		Expect((*storagev1.StorageSpec)(nil).UnredactSecrets(previous)).To(Succeed())
+
+		updated := &storagev1.StorageSpec{
+			S3: &storagev1.S3StorageSpec{
+				SecretAccessKey: "",
+			},
+		}
+		Expect(previous.UnredactSecrets(updated)).NotTo(Succeed())
+
+		updated = &storagev1.StorageSpec{
+			S3: &storagev1.S3StorageSpec{
+				SecretAccessKey: "secret",
+				Sse: &storagev1.SSEConfig{
+					KmsEncryptionContext: "",
+				},
+			},
+		}
+		Expect(previous.UnredactSecrets(updated)).NotTo(Succeed())
+
+		updated = &storagev1.StorageSpec{
+			S3: &storagev1.S3StorageSpec{
+				SecretAccessKey: "secret",
+				Sse: &storagev1.SSEConfig{
+					KmsEncryptionContext: "secret",
+				},
+			},
+			Gcs: &storagev1.GCSStorageSpec{
+				ServiceAccount: "",
+			},
+		}
+		Expect(previous.UnredactSecrets(updated)).NotTo(Succeed())
+
+		updated = &storagev1.StorageSpec{
+			S3: &storagev1.S3StorageSpec{
+				SecretAccessKey: "secret",
+				Sse: &storagev1.SSEConfig{
+					KmsEncryptionContext: "secret",
+				},
+			},
+			Gcs: &storagev1.GCSStorageSpec{
+				ServiceAccount: "secret",
+			},
+			Azure: &storagev1.AzureStorageSpec{
+				StorageAccountKey: "",
+			},
+		}
+		Expect(previous.UnredactSecrets(updated)).NotTo(Succeed())
+
+		updated = &storagev1.StorageSpec{
+			S3: &storagev1.S3StorageSpec{
+				SecretAccessKey: "secret",
+				Sse: &storagev1.SSEConfig{
+					KmsEncryptionContext: "secret",
+				},
+			},
+			Gcs: &storagev1.GCSStorageSpec{
+				ServiceAccount: "secret",
+			},
+			Azure: &storagev1.AzureStorageSpec{
+				StorageAccountKey: "secret",
+				MsiResource:       "",
+			},
+		}
+		Expect(previous.UnredactSecrets(updated)).NotTo(Succeed())
+
+		updated = &storagev1.StorageSpec{
+			S3: &storagev1.S3StorageSpec{
+				SecretAccessKey: "secret",
+				Sse: &storagev1.SSEConfig{
+					KmsEncryptionContext: "secret",
+				},
+			},
+			Gcs: &storagev1.GCSStorageSpec{
+				ServiceAccount: "secret",
+			},
+			Azure: &storagev1.AzureStorageSpec{
+				StorageAccountKey: "secret",
+				MsiResource:       "secret",
+			},
+			Swift: &storagev1.SwiftStorageSpec{
+				Password: "",
+			},
+		}
+		Expect(previous.UnredactSecrets(updated)).NotTo(Succeed())
+	})
+})

pkg/apis/storage/v1/storagev1_suite_test.go

@@ -0,0 +1,13 @@
+package v1_test
+
+import (
+	"testing"
+
+	. "github.com/onsi/ginkgo/v2"
+	. "github.com/onsi/gomega"
+)
+
+func TestV1(t *testing.T) {
+	RegisterFailHandler(Fail)
+	RunSpecs(t, "Storage API Suite")
+}

plugins/metrics/pkg/gateway/drivers/opni_manager.go

@@ -235,10 +235,12 @@ func (k *OpniManager) ConfigureCluster(ctx context.Context, conf *cortexops.Clus
 		conf.Grafana.Hostname = defaultGrafanaHostname
 	}
 
-	mutator := func(cluster client.Object) {
+	mutator := func(cluster client.Object) error {
 		switch cluster := cluster.(type) {
 		case *v1beta2.MonitoringCluster:
-			conf.GetStorage().UnredactSecrets(cluster.Spec.Cortex.Storage)
+			if err := conf.GetStorage().UnredactSecrets(cluster.Spec.Cortex.Storage); err != nil {
+				return err
+			}
 			cluster.Spec.Cortex.Enabled = true
 			cluster.Spec.Cortex.Storage = conf.GetStorage()
 			if cluster.Spec.Cortex.Storage.Filesystem != nil &&
@@ -252,7 +254,9 @@ func (k *OpniManager) ConfigureCluster(ctx context.Context, conf *cortexops.Clus
 			}
 			cluster.Spec.Cortex.DeploymentMode = v1beta2.DeploymentMode(cortexops.DeploymentMode_name[int32(conf.GetMode())])
 		case *corev1beta1.MonitoringCluster:
-			conf.GetStorage().UnredactSecrets(cluster.Spec.Cortex.Storage)
+			if err := conf.GetStorage().UnredactSecrets(cluster.Spec.Cortex.Storage); err != nil {
+				return err
+			}
 			cluster.Spec.Cortex.Enabled = true
 			cluster.Spec.Cortex.Storage = conf.GetStorage()
 			if cluster.Spec.Cortex.Storage.Filesystem != nil &&
@@ -267,6 +271,7 @@ func (k *OpniManager) ConfigureCluster(ctx context.Context, conf *cortexops.Clus
 			cluster.Spec.Cortex.DeploymentMode = corev1beta1.DeploymentMode(cortexops.DeploymentMode_name[int32(conf.GetMode())])
 		}
 		controllerutil.SetOwnerReference(gatewayObject, cluster, k.k8sClient.Scheme())
+		return nil
 	}
 
 	if exists {
@@ -280,7 +285,9 @@ func (k *OpniManager) ConfigureCluster(ctx context.Context, conf *cortexops.Clus
 				return err
 			}
 			clone := existing.DeepCopyObject().(client.Object)
-			mutator(clone)
+			if err := mutator(clone); err != nil {
+				return err
+			}
 			cmp, err := patch.DefaultPatchMaker.Calculate(existing, clone,
 				patch.IgnoreStatusFields(),
 				patch.IgnoreVolumeClaimTemplateTypeMetaAndStatus(),
@@ -298,7 +305,9 @@ func (k *OpniManager) ConfigureCluster(ctx context.Context, conf *cortexops.Clus
 			return nil, fmt.Errorf("failed to update monitoring cluster: %w", err)
 		}
 	} else {
-		mutator(cluster)
+		if err := mutator(cluster); err != nil {
+			return nil, err
+		}
 		err := k.k8sClient.Create(ctx, cluster)
 		if err != nil {
 			return nil, fmt.Errorf("failed to create monitoring cluster: %w", err)