From ba672875407186ea4780e2e0941c0d8270ea0ae0 Mon Sep 17 00:00:00 2001
From: Ole Markus With <o.with@sportradar.com>
Date: Tue, 28 Jun 2022 09:40:24 +0200
Subject: [PATCH] Apply PKI even if addon fails

---
 channels/pkg/channels/addon.go | 70 ++++++++++++++++++++--------------
 channels/pkg/channels/apply.go |  8 +++-
 2 files changed, 47 insertions(+), 31 deletions(-)

diff --git a/channels/pkg/channels/addon.go b/channels/pkg/channels/addon.go
index 5af51a11d7..e3181a22db 100644
--- a/channels/pkg/channels/addon.go
+++ b/channels/pkg/channels/addon.go
@@ -23,6 +23,7 @@ import (
 	"fmt"
 	"net/url"
 
+	"go.uber.org/multierr"
 	"k8s.io/kops/pkg/pki"
 	"k8s.io/kops/util/pkg/vfs"
 
@@ -161,44 +162,55 @@ func (a *Addon) EnsureUpdated(ctx context.Context, k8sClient kubernetes.Interfac
 		return nil, nil
 	}
 
+	var merr error
+
 	if required.NewVersion != nil {
-		manifestURL, err := a.GetManifestFullUrl()
+		err := a.updateAddon(ctx, k8sClient, pruner, required)
 		if err != nil {
-			return nil, err
-		}
-		klog.Infof("Applying update from %q", manifestURL)
-
-		// We copy the manifest to a temp file because it is likely e.g. an s3 URL, which kubectl can't read
-		data, err := vfs.Context.ReadFile(manifestURL.String())
-		if err != nil {
-			return nil, fmt.Errorf("error reading manifest: %w", err)
-		}
-
-		if err := Apply(data); err != nil {
-			return nil, fmt.Errorf("error applying update from %q: %w", manifestURL, err)
-		}
-
-		if err := pruner.Prune(ctx, data, a.Spec.Prune); err != nil {
-			return nil, fmt.Errorf("error pruning manifest from %q: %w", manifestURL, err)
-		}
-
-		if err := a.AddNeedsUpdateLabel(ctx, k8sClient, required); err != nil {
-			return nil, fmt.Errorf("error adding needs-update label: %v", err)
-		}
-
-		channel := a.buildChannel()
-		err = channel.SetInstalledVersion(ctx, k8sClient, a.ChannelVersion())
-		if err != nil {
-			return nil, fmt.Errorf("error applying annotation to record addon installation: %v", err)
+			merr = multierr.Append(merr, err)
 		}
 	}
 	if required.InstallPKI {
 		err := a.installPKI(ctx, k8sClient, cmClient)
 		if err != nil {
-			return nil, fmt.Errorf("error installing PKI: %v", err)
+			merr = multierr.Append(merr, err)
 		}
 	}
-	return required, nil
+	return required, merr
+}
+
+func (a *Addon) updateAddon(ctx context.Context, k8sClient kubernetes.Interface, pruner *Pruner, required *AddonUpdate) error {
+	manifestURL, err := a.GetManifestFullUrl()
+	if err != nil {
+		return err
+	}
+
+	klog.Infof("Applying update from %q", manifestURL)
+
+	// We copy the manifest to a temp file because it is likely e.g. an s3 URL, which kubectl can't read
+	data, err := vfs.Context.ReadFile(manifestURL.String())
+	if err != nil {
+		return fmt.Errorf("error reading manifest: %w", err)
+	}
+
+	if err := Apply(data); err != nil {
+		return fmt.Errorf("error applying update from %q: %w", manifestURL, err)
+	}
+
+	if err := pruner.Prune(ctx, data, a.Spec.Prune); err != nil {
+		return fmt.Errorf("error pruning manifest from %q: %w", manifestURL, err)
+	}
+
+	if err := a.AddNeedsUpdateLabel(ctx, k8sClient, required); err != nil {
+		return fmt.Errorf("error adding needs-update label: %v", err)
+	}
+
+	channel := a.buildChannel()
+	err = channel.SetInstalledVersion(ctx, k8sClient, a.ChannelVersion())
+	if err != nil {
+		return fmt.Errorf("error applying annotation to record addon installation: %v", err)
+	}
+	return nil
 }
 
 func (a *Addon) AddNeedsUpdateLabel(ctx context.Context, k8sClient kubernetes.Interface, required *AddonUpdate) error {
diff --git a/channels/pkg/channels/apply.go b/channels/pkg/channels/apply.go
index 85bf63dc42..d787221216 100644
--- a/channels/pkg/channels/apply.go
+++ b/channels/pkg/channels/apply.go
@@ -45,20 +45,24 @@ func Apply(data []byte) error {
 	if err := os.WriteFile(localManifestFile, data, 0o600); err != nil {
 		return fmt.Errorf("error writing temp file: %v", err)
 	}
+	// First do an apply. This may fail when removing things from lists/arrays and required fields are not removed.
 	{
 		_, err := execKubectl("apply", "-f", localManifestFile, "--server-side", "--force-conflicts", "--field-manager=kops")
 		if err != nil {
 			klog.Errorf("failed to apply the manifest: %v", err)
 		}
+
 	}
+
+	// Replace will force ownership on all fields to kops. But on some k8s versions, this will fail on e.g trying to set clusterIP to "".
 	{
 		_, err := execKubectl("replace", "-f", localManifestFile, "--field-manager=kops")
 		if err != nil {
-			return fmt.Errorf("failed to replace manifest: %w", err)
+			klog.Errorf("failed to replace manifest: %v", err)
 		}
 	}
 
-	// Remove this one. Just to show that apply works properly after replace
+	// Do a final replace to ensure resources are correctly apply. This should always succeed if the addon is updated as expected.
 	{
 		_, err := execKubectl("apply", "-f", localManifestFile, "--server-side", "--force-conflicts", "--field-manager=kops")
 		if err != nil {