rgw: watch referenced secrets

Signed-off-by: Artem Torubarov <artem.torubarov@sap.com>

pkg/operator/ceph/object/config.go

@@ -19,7 +19,6 @@ package object
 import (
 	"fmt"
 	"path"
-	"slices"
 	"strconv"
 	"strings"
 	"time"
@@ -39,24 +38,23 @@ caps mon = "allow rw"
 caps osd = "allow rwx"
 `
 
-	caBundleVolumeName                           = "rook-ceph-custom-ca-bundle"
-	caBundleUpdatedVolumeName                    = "rook-ceph-ca-bundle-updated"
-	caBundleTrustedDir                           = "/etc/pki/ca-trust/"
-	caBundleSourceCustomDir                      = caBundleTrustedDir + "source/anchors/"
-	caBundleExtractedDir                         = caBundleTrustedDir + "extracted/"
-	caBundleKeyName                              = "cabundle"
-	caBundleFileName                             = "custom-ca-bundle.crt"
-	certVolumeName                               = "rook-ceph-rgw-cert"
-	certDir                                      = "/etc/ceph/private"
-	certKeyName                                  = "cert"
-	certFilename                                 = "rgw-cert.pem"
-	certKeyFileName                              = "rgw-key.pem"
-	rgwPortInternalPort                    int32 = 8080
-	ServiceServingCertCAFile                     = "/var/run/secrets/kubernetes.io/serviceaccount/service-ca.crt"
-	HttpTimeOut                                  = time.Second * 15
-	rgwVaultVolumeName                           = "rgw-vault-volume"
-	rgwVaultDirName                              = "/etc/vault/rgw/"
-	objectStoreDependentResourceAnnotation       = "cephobjectstore.rook.io/is-used-by"
+	caBundleVolumeName              = "rook-ceph-custom-ca-bundle"
+	caBundleUpdatedVolumeName       = "rook-ceph-ca-bundle-updated"
+	caBundleTrustedDir              = "/etc/pki/ca-trust/"
+	caBundleSourceCustomDir         = caBundleTrustedDir + "source/anchors/"
+	caBundleExtractedDir            = caBundleTrustedDir + "extracted/"
+	caBundleKeyName                 = "cabundle"
+	caBundleFileName                = "custom-ca-bundle.crt"
+	certVolumeName                  = "rook-ceph-rgw-cert"
+	certDir                         = "/etc/ceph/private"
+	certKeyName                     = "cert"
+	certFilename                    = "rgw-cert.pem"
+	certKeyFileName                 = "rgw-key.pem"
+	rgwPortInternalPort       int32 = 8080
+	ServiceServingCertCAFile        = "/var/run/secrets/kubernetes.io/serviceaccount/service-ca.crt"
+	HttpTimeOut                     = time.Second * 15
+	rgwVaultVolumeName              = "rgw-vault-volume"
+	rgwVaultDirName                 = "/etc/vault/rgw/"
 )
 
 var (
@@ -196,13 +194,6 @@ func (c *clusterConfig) generateMonConfigOptions(rgwConfig *rgwConfig) (map[stri
 	if err != nil {
 		return configOptions, err
 	}
-	if rgwConfig.Auth.Keystone != nil && rgwConfig.KeystoneSecret != nil {
-		// set the owner reference on the secret to reconcile object store on secret changes
-		err = c.setObectStoreReferenceAnnotation(rgwConfig.KeystoneSecret)
-		if err != nil {
-			logger.Errorf("failed to set owner reference to keystone secret %q: %v", rgwConfig.KeystoneSecret.Name, err)
-		}
-	}
 
 	if s3 := rgwConfig.Protocols.S3; s3 != nil {
 		if s3.AuthUseKeystone != nil {
@@ -247,12 +238,6 @@ func (c *clusterConfig) generateMonConfigOptions(rgwConfig *rgwConfig) (map[stri
 				fmt.Sprintf("%s/%s", c.store.Namespace, c.store.Name), flag, val.Key, val.Name)
 		}
 
-		// set the owner reference on the secret to reconcile object store on secret changes
-		err = c.setObectStoreReferenceAnnotation(secret)
-		if err != nil {
-			logger.Errorf("failed to set owner reference to rgw config secret %q: %v", secret.Name, err)
-		}
-
 		if currVal, ok := configOptions[flag]; ok {
 			// RGW might break with some user-specified config overrides; log clearly to help triage
 			logger.Infof("overriding object store %q RGW config option %q with user-specified rgwConfigFromSecret",
@@ -266,28 +251,6 @@ func (c *clusterConfig) generateMonConfigOptions(rgwConfig *rgwConfig) (map[stri
 	return configOptions, nil
 }
 
-func (c *clusterConfig) setObectStoreReferenceAnnotation(secret *v1.Secret) error {
-	// add or update annotation with reference to objectstore to the secret
-	if secret.Annotations == nil {
-		secret.Annotations = map[string]string{}
-	}
-	var existing []string
-	if val := secret.Annotations[objectStoreDependentResourceAnnotation]; val != "" {
-		existing = strings.Split(val, ",")
-	}
-	if slices.Contains(existing, c.store.Name) {
-		return nil
-	}
-
-	existing = append(existing, c.store.Name)
-	secret.Annotations[objectStoreDependentResourceAnnotation] = strings.Join(existing, ",")
-	_, err := c.context.Clientset.CoreV1().Secrets(c.clusterInfo.Namespace).Update(c.clusterInfo.Context, secret, metav1.UpdateOptions{})
-	if err != nil {
-		return err
-	}
-	return nil
-}
-
 func configureKeystoneAuthentication(rgwConfig *rgwConfig, configOptions map[string]string) (map[string]string, error) {
 
 	keystone := rgwConfig.Auth.Keystone

pkg/operator/ceph/object/config_test.go

@@ -234,87 +234,3 @@ func TestRgwConfigFromSecret(t *testing.T) {
 	assert.NoError(t, err)
 	assert.EqualValues(t, "secVal", got["rgw_secret_conf_name"])
 }
-
-func Test_clusterConfig_setObectStoreReferenceAnnotation(t *testing.T) {
-	objectStore := simpleStore()
-	objStoreName := objectStore.Name
-	tests := []struct {
-		name              string
-		annotationsBefore map[string]string
-		wantAnnotations   map[string]string
-		wantErr           bool
-	}{
-		{
-			name:              "annotation set to secret without annotation",
-			annotationsBefore: nil,
-			wantAnnotations: map[string]string{
-				objectStoreDependentResourceAnnotation: objStoreName,
-			},
-		},
-		{
-			name:              "other annotations preserved",
-			annotationsBefore: map[string]string{"other": "annotation"},
-			wantAnnotations: map[string]string{
-				"other":                                "annotation",
-				objectStoreDependentResourceAnnotation: objStoreName,
-			},
-		},
-		{
-			name: "already exists",
-			annotationsBefore: map[string]string{
-				objectStoreDependentResourceAnnotation: objStoreName,
-			},
-			wantAnnotations: map[string]string{
-				objectStoreDependentResourceAnnotation: objStoreName,
-			},
-		},
-		{
-			name: "new obj store name appended to existing",
-			annotationsBefore: map[string]string{
-				objectStoreDependentResourceAnnotation: "some-store",
-			},
-			wantAnnotations: map[string]string{
-				objectStoreDependentResourceAnnotation: "some-store," + objStoreName,
-			},
-		},
-		{
-			name: "already exists in a list",
-			annotationsBefore: map[string]string{
-				objectStoreDependentResourceAnnotation: "some-store," + objStoreName,
-			},
-			wantAnnotations: map[string]string{
-				objectStoreDependentResourceAnnotation: "some-store," + objStoreName,
-			},
-		},
-	}
-	for _, tt := range tests {
-		t.Run(tt.name, func(t *testing.T) {
-			ctx := &clusterd.Context{
-				Clientset: test.New(t, 3),
-			}
-			info := cephclient.AdminTestClusterInfo("rook-ceph")
-			objectStore.Namespace = info.Namespace
-			c := &clusterConfig{
-				store:       objectStore,
-				context:     ctx,
-				clusterInfo: cephclient.AdminTestClusterInfo("rook-ceph"),
-				ownerInfo:   k8sutil.NewOwnerInfo(objectStore, runtime.NewScheme()),
-			}
-			secret := &v1.Secret{
-				ObjectMeta: metav1.ObjectMeta{
-					Name:        "my-secret",
-					Namespace:   objectStore.Namespace,
-					Annotations: tt.annotationsBefore,
-				},
-			}
-			_, err := ctx.Clientset.CoreV1().Secrets(c.clusterInfo.Namespace).Create(context.TODO(), secret, metav1.CreateOptions{})
-			assert.NoError(t, err, "create secret")
-			if err := c.setObectStoreReferenceAnnotation(secret); (err != nil) != tt.wantErr {
-				t.Errorf("clusterConfig.setObectStoreReferenceAnnotation() error = %v, wantErr %v", err, tt.wantErr)
-			}
-			got, err := ctx.Clientset.CoreV1().Secrets(c.clusterInfo.Namespace).Get(context.TODO(), secret.Name, metav1.GetOptions{})
-			assert.NoError(t, err, "get secret")
-			assert.EqualValues(t, tt.wantAnnotations, got.Annotations)
-		})
-	}
-}

pkg/operator/ceph/object/controller.go

@@ -20,7 +20,6 @@ import (
 	"context"
 	"fmt"
 	"reflect"
-	"slices"
 	"strings"
 	"syscall"
 	"time"
@@ -157,37 +156,59 @@ func add(mgr manager.Manager, r reconcile.Reconciler) error {
 	return nil
 }
 
-// Watch update and create secrets annotated with the object store name
+// Watch all secrets not owned by Rook
 func secretPredicate() predicate.Predicate {
+	rookGV := cephv1.SchemeGroupVersion.String()
 	return predicate.Funcs{
 		UpdateFunc: func(e event.UpdateEvent) bool {
 			secret, ok := e.ObjectNew.(*corev1.Secret)
 			if !ok {
+				// not a secret
 				return false
 			}
-			annotations := secret.GetAnnotations()
-			return annotations[objectStoreDependentResourceAnnotation] != ""
+			// check if secret already owned by Rook:
+			for _, owner := range secret.OwnerReferences {
+				if owner.APIVersion == rookGV {
+					// already owned by Rook CR
+					return false
+				}
+			}
+			return true
 		},
 		CreateFunc: func(e event.CreateEvent) bool {
 			secret, ok := e.Object.(*corev1.Secret)
 			if !ok {
+				// not a secret
 				return false
 			}
-			annotations := secret.GetAnnotations()
-			return annotations[objectStoreDependentResourceAnnotation] != ""
+			// check if secret already owned by Rook:
+			for _, owner := range secret.OwnerReferences {
+				if owner.APIVersion == rookGV {
+					// already owned by Rook CR
+					return false
+				}
+			}
+			return true
 		},
 		DeleteFunc: func(e event.DeleteEvent) bool {
 			secret, ok := e.Object.(*corev1.Secret)
 			if !ok {
+				// not a secret
 				return false
 			}
-			annotations := secret.GetAnnotations()
-			return annotations[objectStoreDependentResourceAnnotation] != ""
+			// check if secret already owned by Rook:
+			for _, owner := range secret.OwnerReferences {
+				if owner.APIVersion == rookGV {
+					// already owned by Rook CR
+					return false
+				}
+			}
+			return true
 		},
 	}
 }
 
-// Maps secret annotated with the object store name to the object store CR
+// Maps secret referenced by object store to the object store CR
 func mapSecretToCR(k8sClient client.Client) func(context.Context, client.Object) []reconcile.Request {
 	return func(ctx context.Context, obj client.Object) []reconcile.Request {
 		secret, ok := obj.(*corev1.Secret)
@@ -195,17 +216,6 @@ func mapSecretToCR(k8sClient client.Client) func(context.Context, client.Object)
 			return nil
 		}
 
-		// get object store names from secret annotation
-		value, exists := secret.GetAnnotations()[objectStoreDependentResourceAnnotation]
-		if !exists || value == "" {
-			return nil
-		}
-		objStoreNames := strings.Split(value, ",")
-		// trim whitespaces from object store names
-		for i, objStoreName := range objStoreNames {
-			objStoreNames[i] = strings.TrimSpace(objStoreName)
-		}
-
 		// lookup object store CRs by name
 		objStores := cephv1.CephObjectStoreList{}
 		err := k8sClient.List(ctx, &objStores, client.InNamespace(secret.Namespace))
@@ -221,18 +231,25 @@ func mapSecretToCR(k8sClient client.Client) func(context.Context, client.Object)
 
 		var requests []reconcile.Request
 		for _, objStore := range objStores.Items {
-			// filter object stores by name in secret annotation
-			if !slices.Contains(objStoreNames, objStore.Name) {
+			found := false
+			// check if secret is referred in object store rgwConfigFromSecret:
+			for _, sec := range objStore.Spec.Gateway.RgwConfigFromSecret {
+				if sec.Name == secret.Name {
+					requests = append(requests, reconcile.Request{NamespacedName: types.NamespacedName{Name: objStore.Name, Namespace: objStore.Namespace}})
+					found = true
+					break
+				}
+			}
+			if found {
+				// secret reference found, ObjectStore added to reconcile list.
+				// check next object store
+				continue
+			}
+			if objStore.Spec.Auth.Keystone != nil && objStore.Spec.Auth.Keystone.ServiceUserSecretName == secret.Name {
+				requests = append(requests, reconcile.Request{NamespacedName: types.NamespacedName{Name: objStore.Name, Namespace: objStore.Namespace}})
 				continue
 			}
-			requests = append(requests, reconcile.Request{
-				NamespacedName: types.NamespacedName{
-					Name:      objStore.Name,
-					Namespace: secret.Namespace,
-				},
-			})
 		}
-
 		return requests
 	}
 }

pkg/operator/ceph/object/controller_test.go

@@ -45,7 +45,6 @@ import (
 	"k8s.io/apimachinery/pkg/runtime"
 	"k8s.io/apimachinery/pkg/types"
 	"k8s.io/client-go/tools/record"
-	k8sclient "sigs.k8s.io/controller-runtime/pkg/client"
 	"sigs.k8s.io/controller-runtime/pkg/client/fake"
 	"sigs.k8s.io/controller-runtime/pkg/reconcile"
 )
@@ -1035,150 +1034,110 @@ func TestDiffVersions(t *testing.T) {
 }
 
 func Test_mapSecretToCR(t *testing.T) {
-	type args struct {
-		obj k8sclient.Object
-	}
-	tests := []struct {
-		name              string
-		existingObjStores []string
-		args              args
-		want              []reconcile.Request
-	}{
-		{
-			name: "secret without annotation",
-			args: args{
-				obj: &v1.Secret{
-					ObjectMeta: metav1.ObjectMeta{
-						Name:        "secret",
-						Namespace:   "ns",
-						Annotations: nil,
-					},
-				},
+	t.Run("secret not referenced", func(t *testing.T) {
+		// create fake k8s cliend and add CephObjectStore objects
+		var objects []runtime.Object
+		o := simpleStore()
+		o.Namespace = "ns"
+		o.Name = "store1"
+		objects = append(objects, o)
+		s := scheme.Scheme
+		s.AddKnownTypes(cephv1.SchemeGroupVersion, &cephv1.CephObjectStore{})
+		s.AddKnownTypes(cephv1.SchemeGroupVersion, &cephv1.CephObjectStoreList{})
+		cl := fake.NewClientBuilder().WithScheme(s).WithRuntimeObjects(objects...).Build()
+		mapFunc := mapSecretToCR(cl)
+		got := mapFunc(context.TODO(), &v1.Secret{
+			ObjectMeta: metav1.ObjectMeta{
+				Name:      "secret",
+				Namespace: "ns",
 			},
-			want: nil,
-		},
-		{
-			name: "secret without required annotation",
-			args: args{
-				obj: &v1.Secret{
-					ObjectMeta: metav1.ObjectMeta{
-						Name:        "secret",
-						Namespace:   "ns",
-						Annotations: map[string]string{"foo": "bar"},
-					},
-				},
-			},
-			want: nil,
-		},
-		{
-			name:              "object store does not exist",
-			existingObjStores: nil,
-			args: args{
-				obj: &v1.Secret{
-					ObjectMeta: metav1.ObjectMeta{
-						Name:      "secret",
-						Namespace: "ns",
-						Annotations: map[string]string{
-							objectStoreDependentResourceAnnotation: "store1",
-						},
-					},
-				},
-			},
-			want: nil,
-		},
-		{
-			name:              "secret with single obj store reference",
-			existingObjStores: []string{"store1", "store2", "store3"},
-			args: args{
-				obj: &v1.Secret{
-					ObjectMeta: metav1.ObjectMeta{
-						Name:      "secret",
-						Namespace: "ns",
-						Annotations: map[string]string{
-							objectStoreDependentResourceAnnotation: "store1",
-						},
-					},
-				},
-			},
-			want: []reconcile.Request{
-				{NamespacedName: types.NamespacedName{Name: "store1", Namespace: "ns"}},
-			},
-		},
-		{
-			name:              "secret with multiple obj store references",
-			existingObjStores: []string{"store1", "store2", "store3"},
-			args: args{
-				obj: &v1.Secret{
-					ObjectMeta: metav1.ObjectMeta{
-						Name:      "secret",
-						Namespace: "ns",
-						Annotations: map[string]string{
-							objectStoreDependentResourceAnnotation: "store1,store2",
-						},
-					},
-				},
-			},
-			want: []reconcile.Request{
-				{NamespacedName: types.NamespacedName{Name: "store1", Namespace: "ns"}},
-				{NamespacedName: types.NamespacedName{Name: "store2", Namespace: "ns"}},
-			},
-		},
-		{
-			name:              "whitespaces are trimmed",
-			existingObjStores: []string{"store1", "store2", "store3"},
-			args: args{
-				obj: &v1.Secret{
-					ObjectMeta: metav1.ObjectMeta{
-						Name:      "secret",
-						Namespace: "ns",
-						Annotations: map[string]string{
-							objectStoreDependentResourceAnnotation: " store1, store2 ",
-						},
-					},
-				},
-			},
-			want: []reconcile.Request{
-				{NamespacedName: types.NamespacedName{Name: "store1", Namespace: "ns"}},
-				{NamespacedName: types.NamespacedName{Name: "store2", Namespace: "ns"}},
-			},
-		},
-		{
-			name:              "should be a secret",
-			existingObjStores: []string{"store1", "store2", "store3"},
-			args: args{
-				obj: &v1.ConfigMap{
-					ObjectMeta: metav1.ObjectMeta{
-						Name:      "secret",
-						Namespace: "ns",
-						Annotations: map[string]string{
-							objectStoreDependentResourceAnnotation: "store1",
-						},
-					},
-				},
-			},
-			want: nil,
-		},
-	}
-	for _, tt := range tests {
-		t.Run(tt.name, func(t *testing.T) {
-			// create fake k8s cliend and add CephObjectStore objects
-			var objects []runtime.Object
-			for _, name := range tt.existingObjStores {
-				o := simpleStore()
-				o.Namespace = "ns"
-				o.Name = name
-				objects = append(objects, o)
-			}
-			s := scheme.Scheme
-			s.AddKnownTypes(cephv1.SchemeGroupVersion, &cephv1.CephObjectStore{})
-			s.AddKnownTypes(cephv1.SchemeGroupVersion, &cephv1.CephObjectStoreList{})
-			cl := fake.NewClientBuilder().WithScheme(s).WithRuntimeObjects(objects...).Build()
-
-			// test mapSecretToCR:
-			mapFunc := mapSecretToCR(cl)
-			if got := mapFunc(context.TODO(), tt.args.obj); !reflect.DeepEqual(got, tt.want) {
-				t.Errorf("mapSecretToCR() = %v, want %v", got, tt.want)
-			}
 		})
-	}
+		assert.Empty(t, got, "expected empty list")
+	})
+	t.Run("secret referenced in rgw config", func(t *testing.T) {
+		// create fake k8s cliend and add CephObjectStore objects
+		var objects []runtime.Object
+		otherStore := simpleStore()
+		otherStore.Namespace = "ns"
+		otherStore.Name = "store1"
+		inConf := simpleStore()
+		inConf.Namespace = "ns"
+		inConf.Name = "store2"
+		inConf.Spec.Gateway.RgwConfigFromSecret = map[string]v1.SecretKeySelector{
+			"rgw": {Key: "key", LocalObjectReference: v1.LocalObjectReference{Name: "secret"}},
+		}
+		objects = append(objects, otherStore, inConf)
+		s := scheme.Scheme
+		s.AddKnownTypes(cephv1.SchemeGroupVersion, &cephv1.CephObjectStore{})
+		s.AddKnownTypes(cephv1.SchemeGroupVersion, &cephv1.CephObjectStoreList{})
+		cl := fake.NewClientBuilder().WithScheme(s).WithRuntimeObjects(objects...).Build()
+		mapFunc := mapSecretToCR(cl)
+		got := mapFunc(context.TODO(), &v1.Secret{
+			ObjectMeta: metav1.ObjectMeta{
+				Name:      "secret",
+				Namespace: "ns",
+			},
+		})
+		assert.Len(t, got, 1, "expected 1 item")
+		assert.Equal(t, "store2", got[0].Name, "expected store2")
+
+		got = mapFunc(context.TODO(), &v1.Secret{
+			ObjectMeta: metav1.ObjectMeta{
+				Name:      "secret-other",
+				Namespace: "ns",
+			},
+		})
+		assert.Empty(t, got, "empty: wrong secret name")
+
+		got = mapFunc(context.TODO(), &v1.Secret{
+			ObjectMeta: metav1.ObjectMeta{
+				Name:      "secret",
+				Namespace: "ns-other",
+			},
+		})
+		assert.Empty(t, got, "empty: wrong secret ns")
+	})
+	t.Run("secret referenced in keystone config", func(t *testing.T) {
+		// create fake k8s cliend and add CephObjectStore objects
+		var objects []runtime.Object
+		otherStore := simpleStore()
+		otherStore.Namespace = "ns"
+		otherStore.Name = "store1"
+		inConf := simpleStore()
+		inConf.Namespace = "ns"
+		inConf.Name = "store2"
+		inConf.Spec.Auth.Keystone = &cephv1.KeystoneSpec{
+			ServiceUserSecretName: "secret",
+		}
+		objects = append(objects, otherStore, inConf)
+		s := scheme.Scheme
+		s.AddKnownTypes(cephv1.SchemeGroupVersion, &cephv1.CephObjectStore{})
+		s.AddKnownTypes(cephv1.SchemeGroupVersion, &cephv1.CephObjectStoreList{})
+		cl := fake.NewClientBuilder().WithScheme(s).WithRuntimeObjects(objects...).Build()
+		mapFunc := mapSecretToCR(cl)
+		got := mapFunc(context.TODO(), &v1.Secret{
+			ObjectMeta: metav1.ObjectMeta{
+				Name:      "secret",
+				Namespace: "ns",
+			},
+		})
+		assert.Len(t, got, 1, "expected 1 item")
+		assert.Equal(t, "store2", got[0].Name, "expected store2")
+
+		got = mapFunc(context.TODO(), &v1.Secret{
+			ObjectMeta: metav1.ObjectMeta{
+				Name:      "secret-other",
+				Namespace: "ns",
+			},
+		})
+		assert.Empty(t, got, "empty: wrong secret name")
+
+		got = mapFunc(context.TODO(), &v1.Secret{
+			ObjectMeta: metav1.ObjectMeta{
+				Name:      "secret",
+				Namespace: "ns-other",
+			},
+		})
+		assert.Empty(t, got, "empty: wrong secret ns")
+	})
 }