K8OP-312 MC-1542 Fix reconciliation of Medusa configuration without c…

…redential files

CHANGELOG/CHANGELOG-1.22.md

@@ -14,3 +14,5 @@ Changelog for the K8ssandra Operator, new PRs should update the `unreleased` sec
 When cutting a new release, update the `unreleased` heading to the tag being generated and date, like `## vX.Y.Z - YYYY-MM-DD` and create a new placeholder section for  `unreleased` entries.
 
 ## unreleased
+
+* [BUGFIX] [#1489](https://github.com/k8ssandra/k8ssandra-operator/issues/1489) Fix reconciliation of Medusa with config without credentials file.

controllers/k8ssandra/k8ssandracluster_controller_test.go

@@ -102,6 +102,7 @@ func TestK8ssandraCluster(t *testing.T) {
 	t.Run("createMultiDcClusterWithControlPlaneReaper", testEnv.ControllerTest(ctx, createMultiDcClusterWithControlPlaneReaper))
 	t.Run("CreateMultiDcClusterWithMedusa", testEnv.ControllerTest(ctx, createMultiDcClusterWithMedusa))
 	t.Run("CreateSingleDcClusterWithMedusaConfigRef", testEnv.ControllerTest(ctx, createSingleDcClusterWithMedusaConfigRef))
+	t.Run("CreateSingleDcClusterWithoutStorageCredentials", testEnv.ControllerTest(ctx, createSingleDcClusterWithoutStorageCredentials))
 	t.Run("CreateSingleDcClusterWithManagementApiSecured", testEnv.ControllerTest(ctx, createSingleDcClusterWithManagementApiSecured))
 	t.Run("CreatingSingleDcClusterWithoutPrefixInClusterSpecFail", testEnv.ControllerTest(ctx, creatingSingleDcClusterWithoutPrefixInClusterSpecFails))
 	t.Run("CreateMultiDcClusterWithReplicatedSecrets", testEnv.ControllerTest(ctx, createMultiDcClusterWithReplicatedSecrets))

controllers/k8ssandra/medusa_reconciler.go

@@ -10,21 +10,16 @@ import (
 	"github.com/adutra/goalesce"
 	"github.com/go-logr/logr"
 	api "github.com/k8ssandra/k8ssandra-operator/apis/k8ssandra/v1alpha1"
-	k8ssandraapi "github.com/k8ssandra/k8ssandra-operator/apis/k8ssandra/v1alpha1"
 	medusaapi "github.com/k8ssandra/k8ssandra-operator/apis/medusa/v1alpha1"
-	replication "github.com/k8ssandra/k8ssandra-operator/apis/replication/v1alpha1"
 	cassandra "github.com/k8ssandra/k8ssandra-operator/pkg/cassandra"
 	"github.com/k8ssandra/k8ssandra-operator/pkg/labels"
 	medusa "github.com/k8ssandra/k8ssandra-operator/pkg/medusa"
 	"github.com/k8ssandra/k8ssandra-operator/pkg/reconciliation"
 	"github.com/k8ssandra/k8ssandra-operator/pkg/result"
 	"github.com/k8ssandra/k8ssandra-operator/pkg/secret"
 	"github.com/k8ssandra/k8ssandra-operator/pkg/utils"
-	corev1 "k8s.io/api/core/v1"
-	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/types"
 	"sigs.k8s.io/controller-runtime/pkg/client"
-	"sigs.k8s.io/controller-runtime/pkg/controller/controllerutil"
 )
 
 const (
@@ -60,15 +55,10 @@ func (r *K8ssandraClusterReconciler) reconcileMedusa(
 			}
 		}
 
-		if medusaSpec.StorageProperties.StorageSecretRef.Name == "" {
-			medusaSpec.StorageProperties.StorageSecretRef = corev1.LocalObjectReference{Name: ""}
-			if medusaSpec.StorageProperties.CredentialsType == "role-based" && medusaSpec.StorageProperties.StorageProvider == "s3" {
-				// It's okay for the secret ref to be unset
-				medusaSpec.StorageProperties.StorageSecretRef = corev1.LocalObjectReference{Name: ""}
-			} else {
-				return result.Error(fmt.Errorf("medusa storage secret is not defined for storage provider %s", medusaSpec.StorageProperties.StorageProvider))
-			}
+		if err := r.validateStorageCredentials(medusaSpec); err != nil {
+			return result.Error(err)
 		}
+
 		if res := r.reconcileMedusaConfigMap(ctx, remoteClient, kc, dcConfig, logger, dcNamespace); res.Completed() {
 			return res
 		}
@@ -168,12 +158,7 @@ func (r *K8ssandraClusterReconciler) reconcileMedusaSecrets(
 			logger.Error(err, "Failed to reconcile Medusa CQL user secret")
 			return result.Error(err)
 		}
-
-		if res := r.reconcileRemoteBucketSecretsDeprecated(ctx, r.ClientCache.GetLocalClient(), kc, logger); res.Completed() {
-			return res
-		}
 	}
-
 	logger.Info("Medusa user secrets successfully reconciled")
 	return result.Continue()
 }
@@ -234,102 +219,46 @@ func (r *K8ssandraClusterReconciler) mergeStorageProperties(
 		logger.Error(err, "failed to merge MedusaConfiguration StorageProperties")
 		return result.Error(err)
 	}
-	// medusaapi.MedusaConfiguration comes with a storage corev1.Secret containing the credentials to access the storage
-	// we make a copy of that secret for each cluster/dc, and then point to it with a corev1.LocalObjectReference
-	// when we do the copy, we name the secret as <cluster-name>-<original-secret-name>
-	// here we need to update the reference to point to that copied secret
-	if desiredKc.Namespace != medusaSpec.MedusaConfigurationRef.Name {
-		// Deprecated: when we remove the ability to reference a non-namespace-local MedusaConfig in v 1.17,
-		// this if statement should be eliminated.
-		mergedProperties.StorageSecretRef.Name = fmt.Sprintf("%s-%s", desiredKc.Name, mergedProperties.StorageSecretRef.Name)
-	} // imagine an else here which just contains `mergedProperties.StorageSecretRef.Name = mergedProperties.StorageSecretRef.Name`, since this is a no-op.
+
+	// only touch storage secret names when we actually need them (which we don't for role-based credentials)
+	if mergedProperties.CredentialsType != medusa.CredentialsTypeRoleBased {
+		// medusaapi.MedusaConfiguration comes with a storage corev1.Secret containing the credentials to access the storage
+		// we make a copy of that secret for each cluster/dc, and then point to it with a corev1.LocalObjectReference
+		// when we do the copy, we name the secret as <cluster-name>-<original-secret-name>
+		// here we need to update the reference to point to that copied secret
+		if desiredKc.Namespace != medusaSpec.MedusaConfigurationRef.Name {
+			// Deprecated: when we remove the ability to reference a non-namespace-local MedusaConfig in v 1.17,
+			// this if statement should be eliminated.
+			mergedProperties.StorageSecretRef.Name = fmt.Sprintf("%s-%s", desiredKc.Name, mergedProperties.StorageSecretRef.Name)
+		} // imagine an else here which just contains `mergedProperties.StorageSecretRef.Name = mergedProperties.StorageSecretRef.Name`, since this is a no-op.
+	}
 
 	// copy the merged properties back into the cluster
 	mergedProperties.DeepCopyInto(&desiredKc.Spec.Medusa.StorageProperties)
 	return result.Continue()
 }
 
-// Deprecated: This code path can be removed at version 1.17, as MedusaConfigs should now always be namespace-local to the K8ssandraCluster referencing them. At that point, we no longer
-// need this code, because it is mainly concerned with copying the bucket secrets into the K8ssandraCluster's namespace.
-func (r *K8ssandraClusterReconciler) reconcileRemoteBucketSecretsDeprecated(
-	ctx context.Context,
-	c client.Client,
-	kc *api.K8ssandraCluster,
-	logger logr.Logger,
-) result.ReconcileResult {
-	logger.Info("Reconciling Medusa bucket secrets")
-	medusaSpec := kc.Spec.Medusa
-
-	// there is nothing to reconcile if we're not using Medusa configuration reference
-	if medusaSpec == nil || medusaSpec.MedusaConfigurationRef.Name == "" {
-		logger.Info("MedusaConfigurationRef is not set, skipping bucket secret reconciliation")
-		return result.Continue()
-	}
-
-	// This is the deprecated code path. Moving forward we will use a replicated secret with a prefix, but we will remove this code path after v1.17.
-	// fetch the referenced configuration
-	medusaConfigName := medusaSpec.MedusaConfigurationRef.Name
-	medusaConfigNamespace := utils.FirstNonEmptyString(medusaSpec.MedusaConfigurationRef.Namespace, kc.Namespace)
-	medusaConfigKey := types.NamespacedName{Namespace: medusaConfigNamespace, Name: medusaConfigName}
-	medusaConfig := &medusaapi.MedusaConfiguration{}
-	if err := c.Get(ctx, medusaConfigKey, medusaConfig); err != nil {
-		logger.Error(err, fmt.Sprintf("could not get MedusaConfiguration %s/%s", medusaConfigNamespace, medusaConfigName))
-		return result.Error(err)
-	}
-
-	repSecret := replication.ReplicatedSecret{
-		ObjectMeta: metav1.ObjectMeta{
-			Name:      kc.GetClusterIdHash() + "-" + medusaConfig.Spec.StorageProperties.StorageSecretRef.Name,
-			Namespace: medusaConfigNamespace,
-			Labels: map[string]string{
-				k8ssandraapi.K8ssandraClusterNameLabel:      kc.Name,
-				k8ssandraapi.K8ssandraClusterNamespaceLabel: kc.Namespace,
-			},
-			Annotations: map[string]string{
-				k8ssandraapi.PurposeAnnotation: "This replicated secret is designed for the old codepath in the Medusa reconciler within the k8ssandra cluster controller. In this deprecated path, a MedusaConfig could reside in a different namespace to the K8ssandraCluster. This ReplicatedSecret ensures that the bucket secret is copied into the K8ssandraCluster's namespace. This codepath will be removed in v1.17.",
-			},
-		},
-		Spec: replication.ReplicatedSecretSpec{
-			Selector: &metav1.LabelSelector{
-				MatchLabels: map[string]string{
-					medusaapi.MedusaStorageSecretIdentifierLabel: utils.HashNameNamespace(
-						medusaConfig.Spec.StorageProperties.StorageSecretRef.Name,
-						medusaConfigNamespace),
-				},
-			},
-			ReplicationTargets: []replication.ReplicationTarget{
-				{
-					Namespace:    kc.Namespace,
-					TargetPrefix: kc.Name + "-",
-					DropLabels: []string{
-						medusaapi.MedusaStorageSecretIdentifierLabel,
-					},
-					AddLabels: map[string]string{
-						k8ssandraapi.K8ssandraClusterNameLabel:      kc.Name,
-						k8ssandraapi.K8ssandraClusterNamespaceLabel: kc.Namespace,
-						k8ssandraapi.ReplicatedByLabel:              k8ssandraapi.ReplicatedByLabelValue,
-					},
-				},
-			},
-		},
-	}
-	if err := controllerutil.SetControllerReference(kc, &repSecret, r.Scheme); err != nil {
-		return result.Error(err)
-	}
-	if err := controllerutil.SetOwnerReference(kc, &repSecret, r.Scheme); err != nil {
-		return result.Error(err)
-	}
-	// TODO: this should also have finalizer logic included in the k8ssandraCluster finalizer to remove the replicated secret if it is no longer being used.
-	// TODO: this should probably have a finalizer on it too so that the replicatedSecret cannot be deleted.
-
-	return reconciliation.ReconcileObject(ctx, c, r.DefaultDelay, repSecret)
-
-}
-
 func (r *K8ssandraClusterReconciler) getOperatorNamespace() string {
 	operatorNamespace, found := os.LookupEnv(operatorNamespaceEnvVar)
 	if !found {
 		return "default"
 	}
 	return operatorNamespace
 }
+
+func (r *K8ssandraClusterReconciler) validateStorageCredentials(medusaSpec *medusaapi.MedusaClusterTemplate) error {
+
+	// we must specify either storage secret or role-based credentials
+	if medusaSpec.StorageProperties.StorageSecretRef.Name == "" && medusaSpec.StorageProperties.CredentialsType != medusa.CredentialsTypeRoleBased {
+		return fmt.Errorf("must specify either a storge secret or use role-based credentials")
+	}
+
+	// if a storage secret is set, we error if role-based credentials are set too
+	if medusaSpec.StorageProperties.StorageSecretRef.Name != "" {
+		if medusaSpec.StorageProperties.CredentialsType == medusa.CredentialsTypeRoleBased {
+			return fmt.Errorf("cannot specify both a storage secret and role-based credentials: %s", medusaSpec.StorageProperties.StorageSecretRef.Name)
+		}
+	}
+
+	return nil
+}

controllers/k8ssandra/medusa_reconciler_test.go

@@ -3,6 +3,7 @@ package k8ssandra
 import (
 	"context"
 	"fmt"
+	"github.com/k8ssandra/k8ssandra-operator/pkg/medusa"
 	"strings"
 	"testing"
 
@@ -34,6 +35,8 @@ const (
 	prefixFromClusterSpec               = "prefix-from-cluster-spec"
 	defaultConcurrentTransfers          = 1
 	concurrentTransfersFromMedusaConfig = 2
+	keyFilePresent                      = true
+	keyFileAbsent                       = false
 )
 
 func dcTemplate(dcName string, dataPlaneContext string) api.CassandraDatacenterTemplate {
@@ -171,7 +174,7 @@ func createMultiDcClusterWithMedusa(t *testing.T, ctx context.Context, f *framew
 
 	t.Log("verify the config map exists and has the contents from the MedusaConfiguration object")
 	defaultPrefix := kc.Spec.Medusa.StorageProperties.Prefix
-	verifyConfigMap(require, ctx, f, namespace, defaultPrefix, defaultConcurrentTransfers)
+	verifyConfigMap(require, ctx, f, namespace, defaultPrefix, defaultConcurrentTransfers, keyFilePresent)
 
 	t.Log("update datacenter status to scaling up")
 	err = f.PatchDatacenterStatus(ctx, dc1Key, func(dc *cassdcapi.CassandraDatacenter) {
@@ -395,10 +398,51 @@ func createSingleDcClusterWithMedusaConfigRef(t *testing.T, ctx context.Context,
 	verifyReplicatedSecretReconciled(ctx, t, f, kc)
 
 	t.Log("verify the config map exists and has the contents from the MedusaConfiguration object")
-	verifyConfigMap(require, ctx, f, namespace, prefixFromClusterSpec, concurrentTransfersFromMedusaConfig)
+	verifyConfigMap(require, ctx, f, namespace, prefixFromClusterSpec, concurrentTransfersFromMedusaConfig, keyFilePresent)
 }
 
-func verifyConfigMap(r *require.Assertions, ctx context.Context, f *framework.Framework, namespace string, expectedPrefix string, expectedConcurrentTransfers int) {
+func createSingleDcClusterWithoutStorageCredentials(t *testing.T, ctx context.Context, f *framework.Framework, namespace string) {
+	require := require.New(t)
+
+	t.Log("Creating Medusa Configuration object")
+	medusaConfig := MedusaConfig(medusaConfigName, namespace)
+	medusaConfig.Spec.StorageProperties.CredentialsType = medusa.CredentialsTypeRoleBased
+	medusaConfigKey := controlPlaneContextKey(f, medusaConfig, f.ControlPlaneContext)
+	err := f.Create(ctx, medusaConfigKey, medusaConfig)
+
+	require.NoError(err, "failed to create Medusa Configuration")
+	require.Eventually(f.MedusaConfigExists(ctx, f.ControlPlaneContext, medusaConfigKey), timeout, interval)
+
+	kc := &api.K8ssandraCluster{
+		ObjectMeta: metav1.ObjectMeta{
+			Namespace: namespace,
+			Name:      k8ssandraClusterName,
+		},
+		Spec: api.K8ssandraClusterSpec{
+			Cassandra: &api.CassandraClusterTemplate{
+				Datacenters: []api.CassandraDatacenterTemplate{
+					dcTemplate("dc1", f.DataPlaneContexts[0]),
+				},
+			},
+			Medusa: medusaTemplateWithConfigRefWithPrefix(medusaConfigName, namespace, prefixFromClusterSpec),
+		},
+	}
+	kc.Spec.Medusa.StorageProperties.StorageSecretRef.Name = ""
+	require.NotNil(kc.Spec.Medusa.MedusaConfigurationRef)
+	require.Equal(medusaConfigName, kc.Spec.Medusa.MedusaConfigurationRef.Name)
+
+	t.Log("Creating k8ssandracluster with Medusa and a config ref")
+	err = f.Client.Create(ctx, kc)
+	require.NoError(err, "failed to create K8ssandraCluster")
+
+	// even though we're not creating a storage secret, we still need this to happen so that the Medusa config map gets created
+	verifyReplicatedSecretReconciled(ctx, t, f, kc)
+
+	t.Log("verify the config map exists and has the contents from the MedusaConfiguration object")
+	verifyConfigMap(require, ctx, f, namespace, prefixFromClusterSpec, concurrentTransfersFromMedusaConfig, keyFileAbsent)
+}
+
+func verifyConfigMap(r *require.Assertions, ctx context.Context, f *framework.Framework, namespace string, expectedPrefix string, expectedConcurrentTransfers int, keyFilePresent bool) {
 	configMapName := fmt.Sprintf("%s-medusa", k8ssandraClusterName)
 	configMapKey := framework.ClusterKey{NamespacedName: types.NamespacedName{Namespace: namespace, Name: configMapName}, K8sContext: f.DataPlaneContexts[0]}
 	configMap := &corev1.ConfigMap{}
@@ -409,7 +453,8 @@ func verifyConfigMap(r *require.Assertions, ctx context.Context, f *framework.Fr
 		}
 		prefixCorrect := strings.Contains(configMap.Data["medusa.ini"], fmt.Sprintf("prefix = %s", expectedPrefix))
 		concurrentTransfersCorrect := strings.Contains(configMap.Data["medusa.ini"], fmt.Sprintf("concurrent_transfers = %d", expectedConcurrentTransfers))
-		return prefixCorrect && concurrentTransfersCorrect
+		keyFileCorrect := strings.Contains(configMap.Data["medusa.ini"], "key_file = /etc/medusa-secrets/credentials") == keyFilePresent
+		return prefixCorrect && concurrentTransfersCorrect && keyFileCorrect
 	}, timeout, interval, "Medusa ConfigMap doesn't have the right content")
 }
 

pkg/medusa/reconcile.go

@@ -43,6 +43,8 @@ const (
 	MedusaBackupsVolumeName  = "medusa-backups"
 	MedusaBackupsMountPath   = "/mnt/backups"
 	serviceAccountNameEnvVar = "SERVICE_ACCOUNT_NAME"
+
+	CredentialsTypeRoleBased = "role-based"
 )
 
 var (