diff --git a/pkg/reconciler/managed/api.go b/pkg/reconciler/managed/api.go index 7bf6a1514..f7b64405a 100644 --- a/pkg/reconciler/managed/api.go +++ b/pkg/reconciler/managed/api.go @@ -106,25 +106,7 @@ func (a *APISecretPublisher) PublishConnection(ctx context.Context, o resource.C s := resource.ConnectionSecretFor(o, resource.MustGetKind(o, a.typer)) s.Data = c - err := a.secret.Apply(ctx, s, - resource.ConnectionSecretMustBeControllableBy(o.GetUID()), - resource.AllowUpdateIf(func(current, desired runtime.Object) bool { - // We consider the update to be a no-op and don't allow it if the - // current and existing secret data are identical. - //nolint:forcetypeassert // Will always be a secret. - return !cmp.Equal(current.(*corev1.Secret).Data, desired.(*corev1.Secret).Data, cmpopts.EquateEmpty()) - }), - ) - if resource.IsNotAllowed(err) { - // The update was not allowed because it was a no-op. - return false, nil - } - - if err != nil { - return false, errors.Wrap(err, errCreateOrUpdateSecret) - } - - return true, nil + return publishConnectionSecret(ctx, a.secret, s, o.GetUID(), false) } // UnpublishConnection is no-op since PublishConnection only creates resources @@ -164,35 +146,48 @@ func (a *APILocalSecretPublisher) PublishConnection(ctx context.Context, o resou s := resource.LocalConnectionSecretFor(o, resource.MustGetKind(o, a.typer)) s.Data = c - err := a.secret.Apply(ctx, s, - resource.ConnectionSecretMustBeControllableBy(o.GetUID()), + return publishConnectionSecret(ctx, a.secret, s, o.GetUID(), o.GetWriteConnectionSecretToReference().Metadata != nil) +} + +// UnpublishConnection is no-op since PublishConnection only creates resources +// that will be garbage collected by Kubernetes when the managed resource is +// deleted. +func (a *APILocalSecretPublisher) UnpublishConnection(_ context.Context, _ resource.LocalConnectionSecretOwner, _ ConnectionDetails) error { + return nil +} + +func connectionSecretNeedsUpdate(current, desired *corev1.Secret, manageMetadata bool) bool { + if !cmp.Equal(current.Data, desired.Data, cmpopts.EquateEmpty()) { + return true + } + + if !manageMetadata { + return false + } + + return !cmp.Equal(current.Labels, desired.Labels, cmpopts.EquateEmpty()) || + !cmp.Equal(current.Annotations, desired.Annotations, cmpopts.EquateEmpty()) +} + +// publishConnectionSecret applies the desired Secret via the given applicator, +// returning whether the secret was published (i.e. changed) and any error. +func publishConnectionSecret(ctx context.Context, applicator resource.Applicator, s *corev1.Secret, uid types.UID, manageMetadata bool) (bool, error) { + err := applicator.Apply(ctx, s, + resource.ConnectionSecretMustBeControllableBy(uid), resource.AllowUpdateIf(func(current, desired runtime.Object) bool { - // We consider the update to be a no-op and don't allow it if the - // current and existing secret data are identical. //nolint:forcetypeassert // Will always be a secret. - // NOTE(erhancagirici): cmp package is not recommended for production use - return !cmp.Equal(current.(*corev1.Secret).Data, desired.(*corev1.Secret).Data, cmpopts.EquateEmpty()) + return connectionSecretNeedsUpdate(current.(*corev1.Secret), desired.(*corev1.Secret), manageMetadata) }), ) if resource.IsNotAllowed(err) { - // The update was not allowed because it was a no-op. return false, nil } - if err != nil { return false, errors.Wrap(err, errCreateOrUpdateSecret) } - return true, nil } -// UnpublishConnection is no-op since PublishConnection only creates resources -// that will be garbage collected by Kubernetes when the managed resource is -// deleted. -func (a *APILocalSecretPublisher) UnpublishConnection(_ context.Context, _ resource.LocalConnectionSecretOwner, _ ConnectionDetails) error { - return nil -} - // An APISimpleReferenceResolver resolves references from one managed resource // to others by calling the referencing resource's ResolveReferences method, if // any. diff --git a/pkg/reconciler/managed/api_test.go b/pkg/reconciler/managed/api_test.go index c1d5952fd..7862d4751 100644 --- a/pkg/reconciler/managed/api_test.go +++ b/pkg/reconciler/managed/api_test.go @@ -253,6 +253,10 @@ func TestAPILocalSecretPublisher(t *testing.T) { mg := &fake.ModernManaged{ LocalConnectionSecretWriterTo: fake.LocalConnectionSecretWriterTo{Ref: &xpv2.LocalSecretReference{ Name: "coolsecret", + Metadata: &xpv2.SecretReferenceMetadata{ + Labels: map[string]string{"desired": "label"}, + Annotations: map[string]string{"desired": "annotation"}, + }, }}, } @@ -328,6 +332,105 @@ func TestAPILocalSecretPublisher(t *testing.T) { err: nil, }, }, + "MetadataChanged": { + reason: "Metadata-only changes should publish an updated local secret", + fields: fields{ + secret: resource.ApplyFn(func(ctx context.Context, o client.Object, ao ...resource.ApplyOption) error { + current := resource.LocalConnectionSecretFor(mg, fake.GVK(mg)) + current.Data = cd + current.Labels = map[string]string{"stale": "label"} + current.Annotations = map[string]string{"stale": "annotation"} + + for _, fn := range ao { + if err := fn(ctx, current, o); err != nil { + return err + } + } + + return nil + }), + typer: fake.SchemeWith(&fake.ModernManaged{}), + }, + args: args{ + ctx: context.Background(), + mg: mg, + c: cd, + }, + want: want{published: true}, + }, + "MetadataOmittedPreservesExisting": { + reason: "Omitting metadata should preserve existing local secret metadata and remain a no-op when data matches", + fields: fields{ + secret: resource.ApplyFn(func(ctx context.Context, o client.Object, ao ...resource.ApplyOption) error { + withoutMetadata := mg.DeepCopyObject().(*fake.ModernManaged) + withoutMetadata.LocalConnectionSecretWriterTo.Ref = &xpv2.LocalSecretReference{Name: "coolsecret"} + want := resource.LocalConnectionSecretFor(withoutMetadata, fake.GVK(withoutMetadata)) + want.Data = cd + want.Labels = map[string]string{"keep": "label"} + want.Annotations = map[string]string{"keep": "annotation"} + + for _, fn := range ao { + if err := fn(ctx, want, o); err != nil { + return err + } + } + + return nil + }), + typer: fake.SchemeWith(&fake.ModernManaged{}), + }, + args: args{ + ctx: context.Background(), + mg: &fake.ModernManaged{ + LocalConnectionSecretWriterTo: fake.LocalConnectionSecretWriterTo{Ref: &xpv2.LocalSecretReference{Name: "coolsecret"}}, + }, + c: cd, + }, + want: want{ + published: false, + err: nil, + }, + }, + "LabelsOnlyMetadataClearsAnnotations": { + reason: "Metadata with only labels specified should clear existing annotations", + fields: fields{ + secret: resource.ApplyFn(func(ctx context.Context, o client.Object, ao ...resource.ApplyOption) error { + labelsOnly := &fake.ModernManaged{ + LocalConnectionSecretWriterTo: fake.LocalConnectionSecretWriterTo{Ref: &xpv2.LocalSecretReference{ + Name: "coolsecret", + Metadata: &xpv2.SecretReferenceMetadata{ + Labels: map[string]string{"desired": "label"}, + }, + }}, + } + current := resource.LocalConnectionSecretFor(labelsOnly, fake.GVK(labelsOnly)) + current.Data = cd + current.Annotations = map[string]string{"stale": "annotation"} + + for _, fn := range ao { + if err := fn(ctx, current, o); err != nil { + return err + } + } + + return nil + }), + typer: fake.SchemeWith(&fake.ModernManaged{}), + }, + args: args{ + ctx: context.Background(), + mg: &fake.ModernManaged{ + LocalConnectionSecretWriterTo: fake.LocalConnectionSecretWriterTo{Ref: &xpv2.LocalSecretReference{ + Name: "coolsecret", + Metadata: &xpv2.SecretReferenceMetadata{ + Labels: map[string]string{"desired": "label"}, + }, + }}, + }, + c: cd, + }, + want: want{published: true}, + }, "Success": { reason: "A successful application of the connection secret should result in no error", fields: fields{ diff --git a/pkg/resource/resource.go b/pkg/resource/resource.go index 7375588bc..888cf0cef 100644 --- a/pkg/resource/resource.go +++ b/pkg/resource/resource.go @@ -19,6 +19,7 @@ package resource import ( "context" "fmt" + "maps" "sort" "strings" @@ -94,10 +95,28 @@ type LocalConnectionSecretOwner interface { // LocalConnectionSecretFor creates a connection secret in the namespace of the // supplied LocalConnectionSecretOwner, assumed to be of the supplied kind. func LocalConnectionSecretFor(o LocalConnectionSecretOwner, kind schema.GroupVersionKind) *corev1.Secret { + ref := o.GetWriteConnectionSecretToReference() + + var labels, annotations map[string]string + if ref.Metadata != nil { + if ref.Metadata.Labels != nil { + labels = maps.Clone(ref.Metadata.Labels) + } else { + labels = map[string]string{} + } + if ref.Metadata.Annotations != nil { + annotations = maps.Clone(ref.Metadata.Annotations) + } else { + annotations = map[string]string{} + } + } + return &corev1.Secret{ ObjectMeta: metav1.ObjectMeta{ Namespace: o.GetNamespace(), - Name: o.GetWriteConnectionSecretToReference().Name, + Name: ref.Name, + Labels: labels, + Annotations: annotations, OwnerReferences: []metav1.OwnerReference{meta.AsController(meta.TypedReferenceTo(o, kind))}, }, Type: SecretTypeConnection, @@ -110,10 +129,12 @@ func LocalConnectionSecretFor(o LocalConnectionSecretOwner, kind schema.GroupVer // written to 'default' namespace if the ConnectionSecretOwner does not specify // a namespace. func ConnectionSecretFor(o ConnectionSecretOwner, kind schema.GroupVersionKind) *corev1.Secret { + ref := o.GetWriteConnectionSecretToReference() + return &corev1.Secret{ ObjectMeta: metav1.ObjectMeta{ - Namespace: o.GetWriteConnectionSecretToReference().Namespace, - Name: o.GetWriteConnectionSecretToReference().Name, + Namespace: ref.Namespace, + Name: ref.Name, OwnerReferences: []metav1.OwnerReference{meta.AsController(meta.TypedReferenceTo(o, kind))}, }, Type: SecretTypeConnection, diff --git a/pkg/resource/resource_test.go b/pkg/resource/resource_test.go index 05c47ab29..726492d23 100644 --- a/pkg/resource/resource_test.go +++ b/pkg/resource/resource_test.go @@ -78,7 +78,13 @@ func TestLocalConnectionSecretFor(t *testing.T) { Name: name, UID: uid, }, - Ref: &xpv2.LocalSecretReference{Name: secretName}, + Ref: &xpv2.LocalSecretReference{ + Name: secretName, + Metadata: &xpv2.SecretReferenceMetadata{ + Labels: map[string]string{"l": "v"}, + Annotations: map[string]string{"a": "v"}, + }, + }, }, kind: MockOwnerGVK, }, @@ -86,6 +92,80 @@ func TestLocalConnectionSecretFor(t *testing.T) { ObjectMeta: metav1.ObjectMeta{ Namespace: namespace, Name: secretName, + Labels: map[string]string{"l": "v"}, + Annotations: map[string]string{ + "a": "v", + }, + OwnerReferences: []metav1.OwnerReference{{ + APIVersion: MockOwnerGVK.GroupVersion().String(), + Kind: MockOwnerGVK.Kind, + Name: name, + UID: uid, + Controller: &controller, + BlockOwnerDeletion: &controller, + }}, + }, + Type: SecretTypeConnection, + Data: map[string][]byte{}, + }, + }, + "LabelsOnlyCoercesAnnotationsToEmpty": { + args: args{ + o: &fake.MockLocalConnectionSecretOwner{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: namespace, + Name: name, + UID: uid, + }, + Ref: &xpv2.LocalSecretReference{ + Name: secretName, + Metadata: &xpv2.SecretReferenceMetadata{ + Labels: map[string]string{"l": "v"}, + }, + }, + }, + kind: MockOwnerGVK, + }, + want: &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: namespace, + Name: secretName, + Labels: map[string]string{"l": "v"}, + Annotations: map[string]string{}, + OwnerReferences: []metav1.OwnerReference{{ + APIVersion: MockOwnerGVK.GroupVersion().String(), + Kind: MockOwnerGVK.Kind, + Name: name, + UID: uid, + Controller: &controller, + BlockOwnerDeletion: &controller, + }}, + }, + Type: SecretTypeConnection, + Data: map[string][]byte{}, + }, + }, + "EmptyMetadataCoercesBothToEmpty": { + args: args{ + o: &fake.MockLocalConnectionSecretOwner{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: namespace, + Name: name, + UID: uid, + }, + Ref: &xpv2.LocalSecretReference{ + Name: secretName, + Metadata: &xpv2.SecretReferenceMetadata{}, + }, + }, + kind: MockOwnerGVK, + }, + want: &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: namespace, + Name: secretName, + Labels: map[string]string{}, + Annotations: map[string]string{}, OwnerReferences: []metav1.OwnerReference{{ APIVersion: MockOwnerGVK.GroupVersion().String(), Kind: MockOwnerGVK.Kind, @@ -132,7 +212,10 @@ func TestConnectionSecretFor(t *testing.T) { Name: name, UID: uid, }, - WriterTo: &xpv2.SecretReference{Namespace: namespace, Name: secretName}, + WriterTo: &xpv2.SecretReference{ + Namespace: namespace, + Name: secretName, + }, }, kind: MockOwnerGVK, }, diff --git a/pkg/xcrd/crd_test.go b/pkg/xcrd/crd_test.go index aa542f326..face8908f 100644 --- a/pkg/xcrd/crd_test.go +++ b/pkg/xcrd/crd_test.go @@ -2893,6 +2893,25 @@ func TestForCompositeResourceClaim(t *testing.T) { Required: []string{"name"}, Properties: map[string]extv1.JSONSchemaProps{ "name": {Type: "string"}, + "metadata": { + Type: "object", + Properties: map[string]extv1.JSONSchemaProps{ + "labels": { + Type: "object", + AdditionalProperties: &extv1.JSONSchemaPropsOrBool{ + Allows: true, + Schema: &extv1.JSONSchemaProps{Type: "string"}, + }, + }, + "annotations": { + Type: "object", + AdditionalProperties: &extv1.JSONSchemaPropsOrBool{ + Allows: true, + Schema: &extv1.JSONSchemaProps{Type: "string"}, + }, + }, + }, + }, }, }, }, @@ -3151,6 +3170,25 @@ func TestForCompositeResourceClaim(t *testing.T) { Required: []string{"name"}, Properties: map[string]extv1.JSONSchemaProps{ "name": {Type: "string"}, + "metadata": { + Type: "object", + Properties: map[string]extv1.JSONSchemaProps{ + "labels": { + Type: "object", + AdditionalProperties: &extv1.JSONSchemaPropsOrBool{ + Allows: true, + Schema: &extv1.JSONSchemaProps{Type: "string"}, + }, + }, + "annotations": { + Type: "object", + AdditionalProperties: &extv1.JSONSchemaPropsOrBool{ + Allows: true, + Schema: &extv1.JSONSchemaProps{Type: "string"}, + }, + }, + }, + }, }, }, }, @@ -3431,6 +3469,25 @@ func TestForCompositeResourceClaimEmptyXrd(t *testing.T) { Required: []string{"name"}, Properties: map[string]extv1.JSONSchemaProps{ "name": {Type: "string"}, + "metadata": { + Type: "object", + Properties: map[string]extv1.JSONSchemaProps{ + "labels": { + Type: "object", + AdditionalProperties: &extv1.JSONSchemaPropsOrBool{ + Allows: true, + Schema: &extv1.JSONSchemaProps{Type: "string"}, + }, + }, + "annotations": { + Type: "object", + AdditionalProperties: &extv1.JSONSchemaPropsOrBool{ + Allows: true, + Schema: &extv1.JSONSchemaProps{Type: "string"}, + }, + }, + }, + }, }, }, }, diff --git a/pkg/xcrd/schemas.go b/pkg/xcrd/schemas.go index faaf21629..230d22082 100644 --- a/pkg/xcrd/schemas.go +++ b/pkg/xcrd/schemas.go @@ -283,6 +283,25 @@ func CompositeResourceClaimSpecProps(defaultPol *xpv2.CompositeDeletePolicy) map Required: []string{"name"}, Properties: map[string]extv1.JSONSchemaProps{ "name": {Type: "string"}, + "metadata": { + Type: "object", + Properties: map[string]extv1.JSONSchemaProps{ + "labels": { + Type: "object", + AdditionalProperties: &extv1.JSONSchemaPropsOrBool{ + Allows: true, + Schema: &extv1.JSONSchemaProps{Type: "string"}, + }, + }, + "annotations": { + Type: "object", + AdditionalProperties: &extv1.JSONSchemaPropsOrBool{ + Allows: true, + Schema: &extv1.JSONSchemaProps{Type: "string"}, + }, + }, + }, + }, }, }, }