From a49f1ab43b17bd1eec52cfec98dbfb7f3add32ba Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?S=C3=A9bastien=20GLON?= Date: Tue, 1 Apr 2025 14:23:25 +0200 Subject: [PATCH] feat(secret): support multiple ownerRefs --- helpers/secrets.go | 40 +++++++++++++++++++++++++++------------- helpers/secrets_test.go | 4 +++- utils/utils.go | 2 ++ 3 files changed, 32 insertions(+), 14 deletions(-) diff --git a/helpers/secrets.go b/helpers/secrets.go index b578bb0b..ae479d52 100644 --- a/helpers/secrets.go +++ b/helpers/secrets.go @@ -16,9 +16,9 @@ import ( hvsclient "github.com/hashicorp/hcp-sdk-go/clients/cloud-vault-secrets/preview/2023-11-28/client/secret_service" "github.com/hashicorp/hcp-sdk-go/clients/cloud-vault-secrets/preview/2023-11-28/models" corev1 "k8s.io/api/core/v1" - "k8s.io/apimachinery/pkg/api/equality" apierrors "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/utils/ptr" "sigs.k8s.io/controller-runtime/pkg/client" ctrlclient "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/log" @@ -116,7 +116,7 @@ func FindSecretsOwnedByObj(ctx context.Context, client ctrlclient.Client, obj ct var result []corev1.Secret for _, s := range secrets.Items { - if err := checkSecretIsOwnedByObj(&s, []metav1.OwnerReference{ownerRef}); err == nil { + if err := checkSecretIsOwnedByObj(&s, ownerRef); err == nil { result = append(result, s) } } @@ -214,13 +214,12 @@ func SyncSecret(ctx context.Context, client ctrlclient.Client, obj ctrlclient.Ob // these are the OwnerReferences that should be included in any Secret that is created/owned by // the syncable-secret - references := []metav1.OwnerReference{ - { - APIVersion: meta.APIVersion, - Kind: meta.Kind, - Name: obj.GetName(), - UID: obj.GetUID(), - }, + reference := metav1.OwnerReference{ + APIVersion: meta.APIVersion, + Kind: meta.Kind, + Name: obj.GetName(), + UID: obj.GetUID(), + Controller: ptr.To(true), } if exists { logger.V(consts.LogLevelDebug).Info("Found pre-existing secret", @@ -232,7 +231,7 @@ func SyncSecret(ctx context.Context, client ctrlclient.Client, obj ctrlclient.Ob } if checkOwnerShip { - if err := checkSecretIsOwnedByObj(dest, references); err != nil { + if err := checkSecretIsOwnedByObj(dest, reference); err != nil { return err } } @@ -272,7 +271,7 @@ func SyncSecret(ctx context.Context, client ctrlclient.Client, obj ctrlclient.Ob dest.Type = secretType dest.SetAnnotations(meta.Destination.Annotations) dest.SetLabels(labels) - dest.SetOwnerReferences(references) + dest.SetOwnerReferences([]metav1.OwnerReference{reference}) logger.V(consts.LogLevelTrace).Info("ObjectMeta", "objectMeta", dest.ObjectMeta) if exists { // secret type is immutable, so we need to force recreate the secret when the @@ -419,7 +418,7 @@ func CheckOwnerLabels(o ctrlclient.Object) error { } // checkSecretIsOwnedByObj validates the Secret is owned by obj by checking its Labels and OwnerReferences. -func checkSecretIsOwnedByObj(dest *corev1.Secret, references []metav1.OwnerReference) error { +func checkSecretIsOwnedByObj(dest *corev1.Secret, reference metav1.OwnerReference) error { // checking for Secret ownership relies on first checking the Secret's labels, // then verifying that its OwnerReferences match the SyncableSecret. @@ -430,7 +429,22 @@ func checkSecretIsOwnedByObj(dest *corev1.Secret, references []metav1.OwnerRefer key := ctrlclient.ObjectKeyFromObject(dest) // check that obj is the Secret's true Owner if len(dest.OwnerReferences) > 0 { - if !equality.Semantic.DeepEqual(dest.OwnerReferences, references) { + found := false + // foundButNotEqual :=false + for _, existingOwnerRef := range dest.OwnerReferences { + if existingOwnerRef.APIVersion == reference.APIVersion && + existingOwnerRef.Kind == reference.Kind && + existingOwnerRef.Name == reference.Name && + existingOwnerRef.UID == reference.UID { + // if we're completely the same, there's nothing to do + // I can't use the deepEqual without BreakingChange due to controller bool value not present on old secrets + // foundButNotEqual = equality.Semantic.DeepEqual(existingOwnerRef, reference) + found = true + break + + } + } + if !found { // we are not the owner, perhaps another syncable-secret resource owns this secret? errs = errors.Join(errs, fmt.Errorf("invalid ownerReferences, refs=%#v", dest.OwnerReferences)) } diff --git a/helpers/secrets_test.go b/helpers/secrets_test.go index 2462f39f..9457ce39 100644 --- a/helpers/secrets_test.go +++ b/helpers/secrets_test.go @@ -21,6 +21,7 @@ import ( metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/types" + "k8s.io/utils/ptr" ctrlclient "sigs.k8s.io/controller-runtime/pkg/client" secretsv1beta1 "github.com/hashicorp/vault-secrets-operator/api/v1beta1" @@ -186,7 +187,7 @@ func TestFindSecretsOwnedByObj(t *testing.T) { } func TestSyncSecret(t *testing.T) { - t.Parallel() + // t.Parallel() ctx := context.Background() clientBuilder := testutils.NewFakeClientBuilder() @@ -506,6 +507,7 @@ func TestSyncSecret(t *testing.T) { Kind: tt.obj.Kind, Name: tt.obj.Name, UID: tt.obj.GetUID(), + Controller: ptr.To(true), }, }, }, diff --git a/utils/utils.go b/utils/utils.go index 26dc6322..20318712 100644 --- a/utils/utils.go +++ b/utils/utils.go @@ -19,6 +19,7 @@ import ( metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/util/json" + "k8s.io/utils/ptr" ctrlclient "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/client/apiutil" "sigs.k8s.io/controller-runtime/pkg/log/zap" @@ -61,6 +62,7 @@ func GetOwnerRefFromObj(owner ctrlclient.Object, scheme *runtime.Scheme) (metav1 apiVersion, kind := gvk.ToAPIVersionAndKind() ownerRef.APIVersion = apiVersion ownerRef.Kind = kind + ownerRef.Controller = ptr.To(true) return ownerRef, nil }