From 6b8df464f2795684f823b1eed573c2d77f047804 Mon Sep 17 00:00:00 2001 From: Pierangelo Di Pilato Date: Mon, 9 Oct 2023 17:49:03 +0200 Subject: [PATCH 1/2] Improve KnativeEventing finalization for TLS resources (#1590) For optional resources like cert-manager's Certificates and Issuers, we don't want to fail finalization when such operators are not installed, so during finalization, we split the resources in - optional resources (TLS resources, etc) - resources (core k8s resources) Then, we delete `resources` first and after we delete optional resources while also ignoring errors returned when such operators are not instaled. Signed-off-by: Pierangelo Di Pilato --- .../knativeeventing/eventing_tls.go | 6 ++++-- .../knativeeventing/knativeeventing.go | 21 ++++++++++++++++++- 2 files changed, 24 insertions(+), 3 deletions(-) diff --git a/pkg/reconciler/knativeeventing/eventing_tls.go b/pkg/reconciler/knativeeventing/eventing_tls.go index bc33620718..264101d58d 100644 --- a/pkg/reconciler/knativeeventing/eventing_tls.go +++ b/pkg/reconciler/knativeeventing/eventing_tls.go @@ -30,6 +30,10 @@ import ( "knative.dev/operator/pkg/apis/operator/v1beta1" ) +var ( + tlsResourcesPred = byGroup("cert-manager.io") +) + func (r *Reconciler) handleTLSResources(ctx context.Context, manifests *mf.Manifest, comp base.KComponent) error { instance := comp.(*v1beta1.KnativeEventing) @@ -37,8 +41,6 @@ func (r *Reconciler) handleTLSResources(ctx context.Context, manifests *mf.Manif return nil } - tlsResourcesPred := byGroup("cert-manager.io") - // Delete TLS resources (if present) toBeDeleted := manifests.Filter(tlsResourcesPred) if err := toBeDeleted.Delete(mf.IgnoreNotFound(true)); err != nil && !meta.IsNoMatchError(err) { diff --git a/pkg/reconciler/knativeeventing/knativeeventing.go b/pkg/reconciler/knativeeventing/knativeeventing.go index b93ebc1a91..30a0ac811f 100644 --- a/pkg/reconciler/knativeeventing/knativeeventing.go +++ b/pkg/reconciler/knativeeventing/knativeeventing.go @@ -21,6 +21,7 @@ import ( "fmt" mf "github.com/manifestival/manifestival" + "k8s.io/apimachinery/pkg/api/meta" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/client-go/kubernetes" @@ -90,9 +91,27 @@ func (r *Reconciler) FinalizeKind(ctx context.Context, original *v1beta1.Knative return nil } - if err = common.Uninstall(manifest); err != nil { + // For optional resources like cert-manager's Certificates and Issuers, we don't want to fail + // finalization when such operator is not installed, so we split the resources in + // - optional resources (TLS resources, etc) + // - resources (core k8s resources) + // + // Then, we delete `resources` first and after we delete optional resources while also ignoring + // errors returned when such operators are not installed. + + optionalResourcesPred := mf.Any(tlsResourcesPred) + + optionalResources := manifest.Filter(optionalResourcesPred) + resources := manifest.Filter(mf.Not(optionalResourcesPred)) + + if err = common.Uninstall(&resources); err != nil { logger.Error("Failed to finalize platform resources", err) } + + if err := common.Uninstall(&optionalResources); err != nil && !meta.IsNoMatchError(err) { + logger.Error("Failed to finalize platform resources", err) + } + return nil } From b8e3f4b7c40dfa257fb9ec3d8db053dee4d17025 Mon Sep 17 00:00:00 2001 From: Pierangelo Di Pilato Date: Mon, 9 Oct 2023 16:44:19 +0200 Subject: [PATCH 2/2] Improve manifest deletion error handling It might happen that not all resources were created successfully so we should not fail finalization when resources are not found, we can just ignore not found errors. Signed-off-by: Pierangelo Di Pilato --- pkg/reconciler/common/install.go | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/pkg/reconciler/common/install.go b/pkg/reconciler/common/install.go index 07a0f733cc..4b3b3adafb 100644 --- a/pkg/reconciler/common/install.go +++ b/pkg/reconciler/common/install.go @@ -22,9 +22,10 @@ import ( "strings" mf "github.com/manifestival/manifestival" + "knative.dev/pkg/logging" + "knative.dev/operator/pkg/apis/operator/base" "knative.dev/operator/pkg/apis/operator/v1beta1" - "knative.dev/pkg/logging" ) var ( @@ -73,11 +74,11 @@ func Install(ctx context.Context, manifest *mf.Manifest, instance base.KComponen // Uninstall removes all resources except CRDs, which are never deleted automatically. func Uninstall(manifest *mf.Manifest) error { - if err := manifest.Filter(mf.NoCRDs, mf.Not(mf.Any(role, rolebinding))).Delete(); err != nil { + if err := manifest.Filter(mf.NoCRDs, mf.Not(mf.Any(role, rolebinding))).Delete(mf.IgnoreNotFound(true)); err != nil { return fmt.Errorf("failed to remove non-crd/non-rbac resources: %w", err) } // Delete Roles last, as they may be useful for human operators to clean up. - if err := manifest.Filter(mf.Any(role, rolebinding)).Delete(); err != nil { + if err := manifest.Filter(mf.Any(role, rolebinding)).Delete(mf.IgnoreNotFound(true)); err != nil { return fmt.Errorf("failed to remove rbac: %w", err) } return nil