diff --git a/pkg/operator/ceph/object/config.go b/pkg/operator/ceph/object/config.go index 1b062654e..dad9879c6 100644 --- a/pkg/operator/ceph/object/config.go +++ b/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 diff --git a/pkg/operator/ceph/object/config_test.go b/pkg/operator/ceph/object/config_test.go index f03a4ab8f..929130bd4 100644 --- a/pkg/operator/ceph/object/config_test.go +++ b/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) - }) - } -} diff --git a/pkg/operator/ceph/object/controller.go b/pkg/operator/ceph/object/controller.go index cbe77230e..1d45b8ac9 100644 --- a/pkg/operator/ceph/object/controller.go +++ b/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 } } diff --git a/pkg/operator/ceph/object/controller_test.go b/pkg/operator/ceph/object/controller_test.go index a3ef8dc53..51a9818a2 100644 --- a/pkg/operator/ceph/object/controller_test.go +++ b/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") + }) }