From 03cc37abe65a7ef69cfcf2a3dea20fbc57cfb851 Mon Sep 17 00:00:00 2001 From: Hasan Turken Date: Wed, 14 Feb 2024 16:35:27 +0300 Subject: [PATCH 1/4] Ability to replay deletion with Usage spec Signed-off-by: Hasan Turken (cherry picked from commit b0dad402cbb5f939e068d3fa49c6a2260c1ed57d) (cherry picked from commit b3a366f2036abade23e5af30cb2e3e9f4934c3e3) --- apis/apiextensions/v1alpha1/usage_types.go | 3 ++ .../v1alpha1/zz_generated.deepcopy.go | 5 ++ .../apiextensions.crossplane.io_usages.yaml | 5 ++ .../apiextensions/usage/reconciler.go | 14 +++++ .../apiextensions/usage/reconciler_test.go | 54 +++++++++++++++++++ internal/usage/handler.go | 27 ++++++++-- internal/usage/handler_test.go | 33 +++++++----- 7 files changed, 125 insertions(+), 16 deletions(-) diff --git a/apis/apiextensions/v1alpha1/usage_types.go b/apis/apiextensions/v1alpha1/usage_types.go index f590b4a10..1cb1ee0e8 100644 --- a/apis/apiextensions/v1alpha1/usage_types.go +++ b/apis/apiextensions/v1alpha1/usage_types.go @@ -68,6 +68,9 @@ type UsageSpec struct { // Reason is the reason for blocking deletion of the resource. // +optional Reason *string `json:"reason,omitempty"` + // ReplayDeletion will trigger a deletion on the used resource during the deletion of the usage itself, if it was attempted to be deleted at least once. + // +optional + ReplayDeletion *bool `json:"replayDeletion,omitempty"` } // UsageStatus defines the observed state of Usage. diff --git a/apis/apiextensions/v1alpha1/zz_generated.deepcopy.go b/apis/apiextensions/v1alpha1/zz_generated.deepcopy.go index c53d5a361..45148977d 100644 --- a/apis/apiextensions/v1alpha1/zz_generated.deepcopy.go +++ b/apis/apiextensions/v1alpha1/zz_generated.deepcopy.go @@ -229,6 +229,11 @@ func (in *UsageSpec) DeepCopyInto(out *UsageSpec) { *out = new(string) **out = **in } + if in.ReplayDeletion != nil { + in, out := &in.ReplayDeletion, &out.ReplayDeletion + *out = new(bool) + **out = **in + } } // DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new UsageSpec. diff --git a/cluster/crds/apiextensions.crossplane.io_usages.yaml b/cluster/crds/apiextensions.crossplane.io_usages.yaml index 4356bcff3..70a22152e 100644 --- a/cluster/crds/apiextensions.crossplane.io_usages.yaml +++ b/cluster/crds/apiextensions.crossplane.io_usages.yaml @@ -125,6 +125,11 @@ spec: reason: description: Reason is the reason for blocking deletion of the resource. type: string + replayDeletion: + description: ReplayDeletion will trigger a deletion on the used resource + during the deletion of the usage itself, if it was attempted to + be deleted at least once. + type: boolean required: - of type: object diff --git a/internal/controller/apiextensions/usage/reconciler.go b/internal/controller/apiextensions/usage/reconciler.go index 107f3d5b9..4dfd3416f 100644 --- a/internal/controller/apiextensions/usage/reconciler.go +++ b/internal/controller/apiextensions/usage/reconciler.go @@ -85,6 +85,7 @@ const ( reasonRemoveInUseLabel event.Reason = "RemoveInUseLabel" reasonAddFinalizer event.Reason = "AddFinalizer" reasonRemoveFinalizer event.Reason = "RemoveFinalizer" + reasonReplayDeletion event.Reason = "ReplayDeletion" reasonUsageConfigured event.Reason = "UsageConfigured" reasonWaitUsing event.Reason = "WaitingUsingDeleted" @@ -315,6 +316,19 @@ func (r *Reconciler) Reconcile(ctx context.Context, req reconcile.Request) (reco } } + if u.Spec.ReplayDeletion != nil && *u.Spec.ReplayDeletion && used.GetAnnotations() != nil && used.GetAnnotations()[usage.AnnotationKeyFirstDeletionAttempt] != "" { + // We have already recorded a deletion attempt and want to replay deletion, let's delete the used resource. + log.Debug("Replaying deletion of the used resource", "apiVersion", used.GetAPIVersion(), "kind", used.GetKind(), "name", used.GetName()) + go func() { + // We do the deletion async and after some delay to make sure the usage is deleted before the + // deletion attempt. We remove the finalizer on this Usage right below, so, we know it will disappear + // very soon. + time.Sleep(2 * time.Second) + // We cannot use the context from the reconcile function since it will be cancelled after the reconciliation. + _ = r.client.Delete(context.Background(), used) + }() + } + // Remove the finalizer from the usage if err = r.usage.RemoveFinalizer(ctx, u); err != nil { log.Debug(errRemoveFinalizer, "error", err) diff --git a/internal/controller/apiextensions/usage/reconciler_test.go b/internal/controller/apiextensions/usage/reconciler_test.go index 3b10e3d1f..7f69830cd 100644 --- a/internal/controller/apiextensions/usage/reconciler_test.go +++ b/internal/controller/apiextensions/usage/reconciler_test.go @@ -26,6 +26,7 @@ import ( kerrors "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime/schema" + "k8s.io/utils/ptr" "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/manager" "sigs.k8s.io/controller-runtime/pkg/reconcile" @@ -38,6 +39,7 @@ import ( "github.com/crossplane/crossplane-runtime/pkg/test" "github.com/crossplane/crossplane/apis/apiextensions/v1alpha1" + "github.com/crossplane/crossplane/internal/usage" "github.com/crossplane/crossplane/internal/xcrd" ) @@ -723,6 +725,58 @@ func TestReconcile(t *testing.T) { r: reconcile.Result{}, }, }, + "SuccessfulDeleteWithReplayDeletion": { + reason: "We should replay deletion after usage is gone and replayDeletion is true.", + args: args{ + mgr: &fake.Manager{}, + opts: []ReconcilerOption{ + WithClientApplicator(xpresource.ClientApplicator{ + Client: &test.MockClient{ + MockGet: test.NewMockGetFn(nil, func(obj client.Object) error { + if o, ok := obj.(*v1alpha1.Usage); ok { + o.SetDeletionTimestamp(&now) + o.Spec.ReplayDeletion = ptr.To(true) + o.Spec.Of.ResourceRef = &v1alpha1.ResourceRef{Name: "cool"} + return nil + } + if o, ok := obj.(*composed.Unstructured); ok { + o.SetAnnotations(map[string]string{usage.AnnotationKeyFirstDeletionAttempt: time.Now().String()}) + o.SetLabels(map[string]string{inUseLabelKey: "true"}) + return nil + } + return errors.New("unexpected object type") + }), + MockList: test.NewMockListFn(nil, func(_ client.ObjectList) error { + return nil + }), + MockUpdate: test.NewMockUpdateFn(nil, func(obj client.Object) error { + if o, ok := obj.(*composed.Unstructured); ok { + if o.GetLabels()[inUseLabelKey] != "" { + t.Errorf("expected in use label to be removed") + } + return nil + } + return errors.New("unexpected object type") + }), + MockDelete: func(_ context.Context, _ client.Object, _ ...client.DeleteOption) error { + return nil + }, + }, + }), + WithSelectorResolver(fakeSelectorResolver{ + resourceSelectorFn: func(_ context.Context, _ *v1alpha1.Usage) error { + return nil + }, + }), + WithFinalizer(xpresource.FinalizerFns{RemoveFinalizerFn: func(_ context.Context, _ xpresource.Object) error { + return nil + }}), + }, + }, + want: want{ + r: reconcile.Result{}, + }, + }, "SuccessfulWaitWhenUsingStillThere": { reason: "We should wait until the using resource is deleted.", args: args{ diff --git a/internal/usage/handler.go b/internal/usage/handler.go index 2865c4621..8ef0fb4c9 100644 --- a/internal/usage/handler.go +++ b/internal/usage/handler.go @@ -21,6 +21,7 @@ import ( "context" "fmt" "net/http" + "time" admissionv1 "k8s.io/api/admission/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" @@ -34,6 +35,7 @@ import ( "github.com/crossplane/crossplane-runtime/pkg/controller" "github.com/crossplane/crossplane-runtime/pkg/errors" "github.com/crossplane/crossplane-runtime/pkg/logging" + xpmeta "github.com/crossplane/crossplane-runtime/pkg/meta" xpunstructured "github.com/crossplane/crossplane-runtime/pkg/resource/unstructured" "github.com/crossplane/crossplane/apis/apiextensions/v1alpha1" @@ -44,6 +46,10 @@ const ( // indexing and retrieving needed CRDs InUseIndexKey = "inuse.apiversion.kind.name" + // AnnotationKeyFirstDeletionAttempt is the annotation key used to record the timestamp for first deletion attempt + // which was blocked due to usage. + AnnotationKeyFirstDeletionAttempt = "usage.crossplane.io/first-deletion-attempt" + // Error strings. errFmtUnexpectedOp = "unexpected operation %q, expected \"DELETE\"" ) @@ -87,7 +93,7 @@ func SetupWebhookWithManager(mgr ctrl.Manager, options controller.Options) error // Handler implements the admission Handler for Composition. type Handler struct { - reader client.Reader + client client.Client log logging.Logger } @@ -102,9 +108,9 @@ func WithLogger(l logging.Logger) HandlerOption { } // NewHandler returns a new Handler. -func NewHandler(reader client.Reader, opts ...HandlerOption) *Handler { +func NewHandler(client client.Client, opts ...HandlerOption) *Handler { h := &Handler{ - reader: reader, + client: client, log: logging.NewNopLogger(), } @@ -135,13 +141,26 @@ func (h *Handler) Handle(ctx context.Context, request admission.Request) admissi func (h *Handler) validateNoUsages(ctx context.Context, u *unstructured.Unstructured) admission.Response { h.log.Debug("Validating no usages", "apiVersion", u.GetAPIVersion(), "kind", u.GetKind(), "name", u.GetName()) usageList := &v1alpha1.UsageList{} - if err := h.reader.List(ctx, usageList, client.MatchingFields{InUseIndexKey: IndexValueForObject(u)}); err != nil { + if err := h.client.List(ctx, usageList, client.MatchingFields{InUseIndexKey: IndexValueForObject(u)}); err != nil { h.log.Debug("Error when getting Usages", "apiVersion", u.GetAPIVersion(), "kind", u.GetKind(), "name", u.GetName(), "err", err) return admission.Errored(http.StatusInternalServerError, err) } if len(usageList.Items) > 0 { msg := inUseMessage(usageList) h.log.Debug("Usage found, deletion not allowed", "apiVersion", u.GetAPIVersion(), "kind", u.GetKind(), "name", u.GetName(), "msg", msg) + + // If the resource is being deleted, we want to record the first deletion attempt + // so that we can track whether a deletion was attempted at least once. + if u.GetAnnotations() == nil || u.GetAnnotations()[AnnotationKeyFirstDeletionAttempt] == "" { + orig := u.DeepCopy() + xpmeta.AddAnnotations(u, map[string]string{AnnotationKeyFirstDeletionAttempt: metav1.Now().Format(time.RFC3339)}) + // Patch the resource to add the deletion attempt annotation + if err := h.client.Patch(ctx, u, client.MergeFrom(orig)); err != nil { + h.log.Debug("Error when patching the resource to add the deletion attempt annotation", "apiVersion", u.GetAPIVersion(), "kind", u.GetKind(), "name", u.GetName(), "err", err) + return admission.Errored(http.StatusInternalServerError, err) + } + } + return admission.Response{ AdmissionResponse: admissionv1.AdmissionResponse{ Allowed: false, diff --git a/internal/usage/handler_test.go b/internal/usage/handler_test.go index 0b41ba8e1..734f3da30 100644 --- a/internal/usage/handler_test.go +++ b/internal/usage/handler_test.go @@ -42,7 +42,7 @@ var errBoom = errors.New("boom") func TestHandle(t *testing.T) { protected := "This resource is protected!" type args struct { - reader client.Reader + client client.Client request admission.Request } type want struct { @@ -121,8 +121,8 @@ func TestHandle(t *testing.T) { "DeleteAllowedNoUsages": { reason: "We should allow a delete request if there is no usages for the given object.", args: args{ - reader: &test.MockClient{ - MockList: func(ctx context.Context, list client.ObjectList, opts ...client.ListOption) error { + client: &test.MockClient{ + MockList: func(_ context.Context, _ client.ObjectList, _ ...client.ListOption) error { return nil }, }, @@ -147,8 +147,8 @@ func TestHandle(t *testing.T) { "DeleteRejectedCannotList": { reason: "We should reject a delete request if we cannot list usages.", args: args{ - reader: &test.MockClient{ - MockList: func(ctx context.Context, list client.ObjectList, opts ...client.ListOption) error { + client: &test.MockClient{ + MockList: func(_ context.Context, _ client.ObjectList, _ ...client.ListOption) error { return errBoom }, }, @@ -173,8 +173,11 @@ func TestHandle(t *testing.T) { "DeleteBlockedWithUsageBy": { reason: "We should reject a delete request if there are usages for the given object with \"by\" defined.", args: args{ - reader: &test.MockClient{ - MockList: func(ctx context.Context, list client.ObjectList, opts ...client.ListOption) error { + client: &test.MockClient{ + MockPatch: func(_ context.Context, _ client.Object, _ client.Patch, _ ...client.PatchOption) error { + return nil + }, + MockList: func(_ context.Context, list client.ObjectList, _ ...client.ListOption) error { l := list.(*v1alpha1.UsageList) l.Items = []v1alpha1.Usage{ { @@ -231,8 +234,11 @@ func TestHandle(t *testing.T) { "DeleteBlockedWithUsageReason": { reason: "We should reject a delete request if there are usages for the given object with \"reason\" defined.", args: args{ - reader: &test.MockClient{ - MockList: func(ctx context.Context, list client.ObjectList, opts ...client.ListOption) error { + client: &test.MockClient{ + MockPatch: func(_ context.Context, _ client.Object, _ client.Patch, _ ...client.PatchOption) error { + return nil + }, + MockList: func(_ context.Context, list client.ObjectList, _ ...client.ListOption) error { l := list.(*v1alpha1.UsageList) l.Items = []v1alpha1.Usage{ { @@ -283,8 +289,11 @@ func TestHandle(t *testing.T) { "DeleteBlockedWithUsageNone": { reason: "We should reject a delete request if there are usages for the given object without \"reason\" or \"by\" defined.", args: args{ - reader: &test.MockClient{ - MockList: func(ctx context.Context, list client.ObjectList, opts ...client.ListOption) error { + client: &test.MockClient{ + MockPatch: func(_ context.Context, _ client.Object, _ client.Patch, _ ...client.PatchOption) error { + return nil + }, + MockList: func(_ context.Context, list client.ObjectList, _ ...client.ListOption) error { l := list.(*v1alpha1.UsageList) l.Items = []v1alpha1.Usage{ { @@ -334,7 +343,7 @@ func TestHandle(t *testing.T) { } for name, tc := range cases { t.Run(name, func(t *testing.T) { - h := NewHandler(tc.args.reader, WithLogger(logging.NewNopLogger())) + h := NewHandler(tc.args.client, WithLogger(logging.NewNopLogger())) got := h.Handle(context.Background(), tc.args.request) if diff := cmp.Diff(tc.want.resp, got); diff != "" { t.Errorf("%s\nHandle(...): -want response, +got:\n%s", tc.reason, diff) From c38fb3535dc7d7fedf988e3d8b8f9d8212583c1a Mon Sep 17 00:00:00 2001 From: Hasan Turken Date: Sat, 9 Mar 2024 15:36:46 +0300 Subject: [PATCH 2/4] Replay deletion with the same policy Signed-off-by: Hasan Turken (cherry picked from commit ef512a0950ad0f4adc8ab4b0a5965020ae4385c7) --- .../apiextensions/usage/reconciler.go | 26 +++++++++-------- .../apiextensions/usage/reconciler_test.go | 2 +- internal/usage/handler.go | 28 +++++++++++++------ .../usage/standalone/with-by/usage.yaml | 1 + .../usage/standalone/with-by/used.yaml | 3 -- .../usage/standalone/with-by/using.yaml | 3 -- .../usage/standalone/with-reason/used.yaml | 3 -- test/e2e/usage_test.go | 6 ++-- 8 files changed, 38 insertions(+), 34 deletions(-) diff --git a/internal/controller/apiextensions/usage/reconciler.go b/internal/controller/apiextensions/usage/reconciler.go index 4dfd3416f..06d7ac88b 100644 --- a/internal/controller/apiextensions/usage/reconciler.go +++ b/internal/controller/apiextensions/usage/reconciler.go @@ -316,17 +316,21 @@ func (r *Reconciler) Reconcile(ctx context.Context, req reconcile.Request) (reco } } - if u.Spec.ReplayDeletion != nil && *u.Spec.ReplayDeletion && used.GetAnnotations() != nil && used.GetAnnotations()[usage.AnnotationKeyFirstDeletionAttempt] != "" { - // We have already recorded a deletion attempt and want to replay deletion, let's delete the used resource. - log.Debug("Replaying deletion of the used resource", "apiVersion", used.GetAPIVersion(), "kind", used.GetKind(), "name", used.GetName()) - go func() { - // We do the deletion async and after some delay to make sure the usage is deleted before the - // deletion attempt. We remove the finalizer on this Usage right below, so, we know it will disappear - // very soon. - time.Sleep(2 * time.Second) - // We cannot use the context from the reconcile function since it will be cancelled after the reconciliation. - _ = r.client.Delete(context.Background(), used) - }() + if u.Spec.ReplayDeletion != nil && *u.Spec.ReplayDeletion && used.GetAnnotations() != nil { + if policy, ok := used.GetAnnotations()[usage.AnnotationKeyDeletionAttempt]; ok { + // We have already recorded a deletion attempt and want to replay deletion, let's delete the used resource. + go func() { + // We do the deletion async and after some delay to make sure the usage is deleted before the + // deletion attempt. We remove the finalizer on this Usage right below, so, we know it will disappear + // very soon. + time.Sleep(2 * time.Second) + log.Info("Replaying deletion of the used resource", "apiVersion", used.GetAPIVersion(), "kind", used.GetKind(), "name", used.GetName(), "policy", policy) + // We cannot use the context from the reconcile function since it will be cancelled after the reconciliation. + if err = r.client.Delete(context.Background(), used, client.PropagationPolicy(policy)); err != nil { + log.Info("Error when replaying deletion of the used resource", "apiVersion", used.GetAPIVersion(), "kind", used.GetKind(), "name", used.GetName(), "err", err) + } + }() + } } // Remove the finalizer from the usage diff --git a/internal/controller/apiextensions/usage/reconciler_test.go b/internal/controller/apiextensions/usage/reconciler_test.go index 7f69830cd..95b82190f 100644 --- a/internal/controller/apiextensions/usage/reconciler_test.go +++ b/internal/controller/apiextensions/usage/reconciler_test.go @@ -740,7 +740,7 @@ func TestReconcile(t *testing.T) { return nil } if o, ok := obj.(*composed.Unstructured); ok { - o.SetAnnotations(map[string]string{usage.AnnotationKeyFirstDeletionAttempt: time.Now().String()}) + o.SetAnnotations(map[string]string{usage.AnnotationKeyDeletionAttempt: string(metav1.DeletePropagationBackground)}) o.SetLabels(map[string]string{inUseLabelKey: "true"}) return nil } diff --git a/internal/usage/handler.go b/internal/usage/handler.go index 8ef0fb4c9..3956a536e 100644 --- a/internal/usage/handler.go +++ b/internal/usage/handler.go @@ -21,7 +21,6 @@ import ( "context" "fmt" "net/http" - "time" admissionv1 "k8s.io/api/admission/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" @@ -31,6 +30,7 @@ import ( "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/webhook" "sigs.k8s.io/controller-runtime/pkg/webhook/admission" + "sigs.k8s.io/yaml" "github.com/crossplane/crossplane-runtime/pkg/controller" "github.com/crossplane/crossplane-runtime/pkg/errors" @@ -46,9 +46,10 @@ const ( // indexing and retrieving needed CRDs InUseIndexKey = "inuse.apiversion.kind.name" - // AnnotationKeyFirstDeletionAttempt is the annotation key used to record the timestamp for first deletion attempt - // which was blocked due to usage. - AnnotationKeyFirstDeletionAttempt = "usage.crossplane.io/first-deletion-attempt" + // AnnotationKeyDeletionAttempt is the annotation key used to record whether + // a deletion attempt was made and blocked by the Usage. The value stored is + // the propagation policy used with the deletion attempt. + AnnotationKeyDeletionAttempt = "usage.crossplane.io/deletion-attempt-with-policy" // Error strings. errFmtUnexpectedOp = "unexpected operation %q, expected \"DELETE\"" @@ -132,14 +133,18 @@ func (h *Handler) Handle(ctx context.Context, request admission.Request) admissi if err := u.UnmarshalJSON(request.OldObject.Raw); err != nil { return admission.Errored(http.StatusBadRequest, err) } - return h.validateNoUsages(ctx, u) + opts := &metav1.DeleteOptions{} + if err := yaml.Unmarshal(request.Options.Raw, opts); err != nil { + return admission.Errored(http.StatusBadRequest, err) + } + return h.validateNoUsages(ctx, u, opts) default: return admission.Errored(http.StatusBadRequest, errors.Errorf(errFmtUnexpectedOp, request.Operation)) } } -func (h *Handler) validateNoUsages(ctx context.Context, u *unstructured.Unstructured) admission.Response { - h.log.Debug("Validating no usages", "apiVersion", u.GetAPIVersion(), "kind", u.GetKind(), "name", u.GetName()) +func (h *Handler) validateNoUsages(ctx context.Context, u *unstructured.Unstructured, opts *metav1.DeleteOptions) admission.Response { + h.log.Debug("Validating no usages", "apiVersion", u.GetAPIVersion(), "kind", u.GetKind(), "name", u.GetName(), "policy", opts.PropagationPolicy) usageList := &v1alpha1.UsageList{} if err := h.client.List(ctx, usageList, client.MatchingFields{InUseIndexKey: IndexValueForObject(u)}); err != nil { h.log.Debug("Error when getting Usages", "apiVersion", u.GetAPIVersion(), "kind", u.GetKind(), "name", u.GetName(), "err", err) @@ -149,11 +154,16 @@ func (h *Handler) validateNoUsages(ctx context.Context, u *unstructured.Unstruct msg := inUseMessage(usageList) h.log.Debug("Usage found, deletion not allowed", "apiVersion", u.GetAPIVersion(), "kind", u.GetKind(), "name", u.GetName(), "msg", msg) + // Use the default propagation policy if not provided + policy := metav1.DeletePropagationBackground + if opts.PropagationPolicy != nil { + policy = *opts.PropagationPolicy + } // If the resource is being deleted, we want to record the first deletion attempt // so that we can track whether a deletion was attempted at least once. - if u.GetAnnotations() == nil || u.GetAnnotations()[AnnotationKeyFirstDeletionAttempt] == "" { + if u.GetAnnotations() == nil || u.GetAnnotations()[AnnotationKeyDeletionAttempt] != string(policy) { orig := u.DeepCopy() - xpmeta.AddAnnotations(u, map[string]string{AnnotationKeyFirstDeletionAttempt: metav1.Now().Format(time.RFC3339)}) + xpmeta.AddAnnotations(u, map[string]string{AnnotationKeyDeletionAttempt: string(policy)}) // Patch the resource to add the deletion attempt annotation if err := h.client.Patch(ctx, u, client.MergeFrom(orig)); err != nil { h.log.Debug("Error when patching the resource to add the deletion attempt annotation", "apiVersion", u.GetAPIVersion(), "kind", u.GetKind(), "name", u.GetName(), "err", err) diff --git a/test/e2e/manifests/apiextensions/usage/standalone/with-by/usage.yaml b/test/e2e/manifests/apiextensions/usage/standalone/with-by/usage.yaml index 6c54d3b9b..e1cd38bcc 100644 --- a/test/e2e/manifests/apiextensions/usage/standalone/with-by/usage.yaml +++ b/test/e2e/manifests/apiextensions/usage/standalone/with-by/usage.yaml @@ -3,6 +3,7 @@ kind: Usage metadata: name: using-uses-used spec: + replayDeletion: true of: apiVersion: nop.crossplane.io/v1alpha1 kind: NopResource diff --git a/test/e2e/manifests/apiextensions/usage/standalone/with-by/used.yaml b/test/e2e/manifests/apiextensions/usage/standalone/with-by/used.yaml index da0ec9f17..3fa17b096 100644 --- a/test/e2e/manifests/apiextensions/usage/standalone/with-by/used.yaml +++ b/test/e2e/manifests/apiextensions/usage/standalone/with-by/used.yaml @@ -7,9 +7,6 @@ metadata: spec: forProvider: conditionAfter: - - conditionType: "Synced" - conditionStatus: "True" - time: "5s" - conditionType: "Ready" conditionStatus: "True" time: "10s" \ No newline at end of file diff --git a/test/e2e/manifests/apiextensions/usage/standalone/with-by/using.yaml b/test/e2e/manifests/apiextensions/usage/standalone/with-by/using.yaml index b182b1d5a..62a6edcac 100644 --- a/test/e2e/manifests/apiextensions/usage/standalone/with-by/using.yaml +++ b/test/e2e/manifests/apiextensions/usage/standalone/with-by/using.yaml @@ -5,9 +5,6 @@ metadata: spec: forProvider: conditionAfter: - - conditionType: "Synced" - conditionStatus: "True" - time: "5s" - conditionType: "Ready" conditionStatus: "True" time: "10s" \ No newline at end of file diff --git a/test/e2e/manifests/apiextensions/usage/standalone/with-reason/used.yaml b/test/e2e/manifests/apiextensions/usage/standalone/with-reason/used.yaml index ace8b8701..af0060395 100644 --- a/test/e2e/manifests/apiextensions/usage/standalone/with-reason/used.yaml +++ b/test/e2e/manifests/apiextensions/usage/standalone/with-reason/used.yaml @@ -7,9 +7,6 @@ metadata: spec: forProvider: conditionAfter: - - conditionType: "Synced" - conditionStatus: "True" - time: "5s" - conditionType: "Ready" conditionStatus: "True" time: "10s" \ No newline at end of file diff --git a/test/e2e/usage_test.go b/test/e2e/usage_test.go index 02eaa629a..4328f10dc 100644 --- a/test/e2e/usage_test.go +++ b/test/e2e/usage_test.go @@ -59,10 +59,8 @@ func TestUsageStandalone(t *testing.T) { funcs.DeleteResources(manifests, "with-by/using.yaml"), funcs.ResourcesDeletedWithin(30*time.Second, manifests, "with-by/using.yaml"), funcs.ResourcesDeletedWithin(30*time.Second, manifests, "with-by/usage.yaml"), - - // Deletion of used resource should be allowed after usage is cleared. - funcs.DeleteResources(manifests, "with-by/used.yaml"), - funcs.ResourcesDeletedWithin(30*time.Second, manifests, "with-by/used.yaml"), + // We have "replayDeletion: true" on the usage, deletion of used resource should be replayed after usage is cleared. + funcs.ResourcesDeletedWithin(1*time.Minute, manifests, "with-by/used.yaml"), ), }, { From 6cc7c14e985bd9ef742bf1a496d9b2b802083bbf Mon Sep 17 00:00:00 2001 From: Hasan Turken Date: Wed, 13 Mar 2024 16:11:09 +0300 Subject: [PATCH 3/4] Allow group changes in composed templates Fixes #5473 Signed-off-by: Hasan Turken (cherry picked from commit d347ff385c1701ba2dbc545b04c1bb56e9fe4767) --- .../composite/composition_render.go | 14 ++++++----- .../composite/composition_render_test.go | 23 +++++++++---------- 2 files changed, 19 insertions(+), 18 deletions(-) diff --git a/internal/controller/apiextensions/composite/composition_render.go b/internal/controller/apiextensions/composite/composition_render.go index cb46f4670..605ec1d9c 100644 --- a/internal/controller/apiextensions/composite/composition_render.go +++ b/internal/controller/apiextensions/composite/composition_render.go @@ -39,8 +39,8 @@ const ( errGenerateName = "cannot generate a name for a composed resource" errSetControllerRef = "cannot set controller reference" - errFmtKindOrGroupChanged = "cannot change the kind or group of a composed resource from %s to %s (possible composed resource template mismatch)" - errFmtNamePrefixLabel = "cannot find top-level composite resource name label %q in composite resource metadata" + errFmtKindChanged = "cannot change the kind of a composed resource from %s to %s (possible composed resource template mismatch)" + errFmtNamePrefixLabel = "cannot find top-level composite resource name label %q in composite resource metadata" // TODO(negz): Include more detail such as field paths if they exist. // Perhaps require each patch type to have a String() method to help @@ -68,16 +68,18 @@ func RenderFromJSON(o resource.Object, data []byte) error { o.SetName(name) o.SetNamespace(namespace) - // This resource already had a GK (probably because it already exists), but + // This resource already had a Kind (probably because it already exists), but // when we rendered its template it changed. This shouldn't happen. Either - // someone changed the kind or group in the template, or we're trying to use the + // someone changed the kind in the template, or we're trying to use the // wrong template (e.g. because the order of an array of anonymous templates // changed). // Please note, we don't check for version changes, as versions can change. For example, // if a composed resource was created with a template that has a version of "v1alpha1", // and then the template is updated to "v1beta1", the composed resource will still be valid. - if !gvk.Empty() && o.GetObjectKind().GroupVersionKind().GroupKind() != gvk.GroupKind() { - return errors.Errorf(errFmtKindOrGroupChanged, gvk, o.GetObjectKind().GroupVersionKind()) + // We also don't check for group changes, as groups can change during + // migrations. + if !gvk.Empty() && o.GetObjectKind().GroupVersionKind().Kind != gvk.Kind { + return errors.Errorf(errFmtKindChanged, gvk, o.GetObjectKind().GroupVersionKind()) } return nil diff --git a/internal/controller/apiextensions/composite/composition_render_test.go b/internal/controller/apiextensions/composite/composition_render_test.go index 5999a0ba4..2502127f8 100644 --- a/internal/controller/apiextensions/composite/composition_render_test.go +++ b/internal/controller/apiextensions/composite/composition_render_test.go @@ -67,38 +67,37 @@ func TestRenderFromJSON(t *testing.T) { err: errors.Wrap(errInvalidChar, errUnmarshalJSON), }, }, - "ExistingGroupChanged": { - reason: "We should return an error if unmarshalling the base template changed the composed resource's group.", + "ExistingKindChanged": { + reason: "We should return an error if unmarshalling the base template changed the composed resource's kind.", args: args{ o: composed.New(composed.FromReference(corev1.ObjectReference{ APIVersion: "example.org/v1", Kind: "Potato", })), - data: []byte(`{"apiVersion": "foo.io/v1", "kind": "Potato"}`), + data: []byte(`{"apiVersion": "example.org/v1", "kind": "Different"}`), }, want: want{ o: composed.New(composed.FromReference(corev1.ObjectReference{ - APIVersion: "foo.io/v1", - Kind: "Potato", + APIVersion: "example.org/v1", + Kind: "Different", })), - err: errors.Errorf(errFmtKindOrGroupChanged, "example.org/v1, Kind=Potato", "foo.io/v1, Kind=Potato"), + err: errors.Errorf(errFmtKindChanged, "example.org/v1, Kind=Potato", "example.org/v1, Kind=Different"), }, }, - "ExistingKindChanged": { - reason: "We should return an error if unmarshalling the base template changed the composed resource's kind.", + "GroupCanChange": { + reason: "We should accept group changes in the base template.", args: args{ o: composed.New(composed.FromReference(corev1.ObjectReference{ APIVersion: "example.org/v1", Kind: "Potato", })), - data: []byte(`{"apiVersion": "example.org/v1", "kind": "Different"}`), + data: []byte(`{"apiVersion": "foo.io/v1", "kind": "Potato"}`), }, want: want{ o: composed.New(composed.FromReference(corev1.ObjectReference{ - APIVersion: "example.org/v1", - Kind: "Different", + APIVersion: "foo.io/v1", + Kind: "Potato", })), - err: errors.Errorf(errFmtKindOrGroupChanged, "example.org/v1, Kind=Potato", "example.org/v1, Kind=Different"), }, }, "VersionCanChange": { From de1d8957eac7c5968b76d5c631b52fdbbc921d20 Mon Sep 17 00:00:00 2001 From: Hasan Turken Date: Thu, 14 Mar 2024 11:00:07 +0300 Subject: [PATCH 4/4] Bump default max reconcile rate and resource limits (cherry picked from commit 76fdf072330959e526a3a2435321b6a08829212e) Signed-off-by: Hasan Turken --- cluster/charts/crossplane/README.md | 4 ++-- cluster/charts/crossplane/values.yaml | 4 ++-- cmd/crossplane/core/core.go | 2 +- 3 files changed, 5 insertions(+), 5 deletions(-) diff --git a/cluster/charts/crossplane/README.md b/cluster/charts/crossplane/README.md index b691a9177..348efccf8 100644 --- a/cluster/charts/crossplane/README.md +++ b/cluster/charts/crossplane/README.md @@ -104,8 +104,8 @@ and their default values. | `registryCaBundleConfig.key` | The ConfigMap key containing a custom CA bundle to enable fetching packages from registries with unknown or untrusted certificates. | `""` | | `registryCaBundleConfig.name` | The ConfigMap name containing a custom CA bundle to enable fetching packages from registries with unknown or untrusted certificates. | `""` | | `replicas` | The number of Crossplane pod `replicas` to deploy. | `1` | -| `resourcesCrossplane.limits.cpu` | CPU resource limits for the Crossplane pod. | `"100m"` | -| `resourcesCrossplane.limits.memory` | Memory resource limits for the Crossplane pod. | `"512Mi"` | +| `resourcesCrossplane.limits.cpu` | CPU resource limits for the Crossplane pod. | `"500m"` | +| `resourcesCrossplane.limits.memory` | Memory resource limits for the Crossplane pod. | `"1024Mi"` | | `resourcesCrossplane.requests.cpu` | CPU resource requests for the Crossplane pod. | `"100m"` | | `resourcesCrossplane.requests.memory` | Memory resource requests for the Crossplane pod. | `"256Mi"` | | `resourcesRBACManager.limits.cpu` | CPU resource limits for the RBAC Manager pod. | `"100m"` | diff --git a/cluster/charts/crossplane/values.yaml b/cluster/charts/crossplane/values.yaml index 1f2a758bb..cd859bda2 100755 --- a/cluster/charts/crossplane/values.yaml +++ b/cluster/charts/crossplane/values.yaml @@ -90,9 +90,9 @@ priorityClassName: "" resourcesCrossplane: limits: # -- CPU resource limits for the Crossplane pod. - cpu: 100m + cpu: 500m # -- Memory resource limits for the Crossplane pod. - memory: 512Mi + memory: 1024Mi requests: # -- CPU resource requests for the Crossplane pod. cpu: 100m diff --git a/cmd/crossplane/core/core.go b/cmd/crossplane/core/core.go index dbeea29c2..d4815cffd 100644 --- a/cmd/crossplane/core/core.go +++ b/cmd/crossplane/core/core.go @@ -99,7 +99,7 @@ type startCommand struct { SyncInterval time.Duration `short:"s" help:"How often all resources will be double-checked for drift from the desired state." default:"1h"` PollInterval time.Duration `help:"How often individual resources will be checked for drift from the desired state." default:"1m"` - MaxReconcileRate int `help:"The global maximum rate per second at which resources may checked for drift from the desired state." default:"10"` + MaxReconcileRate int `help:"The global maximum rate per second at which resources may checked for drift from the desired state." default:"100"` WebhookEnabled bool `help:"Enable webhook configuration." default:"true" env:"WEBHOOK_ENABLED"`