[release-1.11] Improve KnativeEventing finalization for TLS resources (…

…#1590) (#1591) * 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 <[email protected]> * 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 <[email protected]> --------- Signed-off-by: Pierangelo Di Pilato <[email protected]>
knative · Oct 9, 2023 · 3159a11 · 3159a11
1 parent 4525683
commit 3159a11
Show file tree

Hide file tree

Showing 3 changed files with 28 additions and 6 deletions.
diff --git 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

diff --git a/pkg/reconciler/knativeeventing/eventing_tls.go b/pkg/reconciler/knativeeventing/eventing_tls.go
@@ -30,15 +30,17 @@ 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)
 
 	if isTLSEnabled(instance) {
 		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
@@ -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
 }