use SSA to apply the template objects

controllers/nstemplateset/client.go

@@ -2,12 +2,10 @@
 
 import (
 	"context"
-	"strings"
+	"fmt"
 
 	toolchainv1alpha1 "github.com/codeready-toolchain/api/api/v1alpha1"
-	applycl "github.com/codeready-toolchain/toolchain-common/pkg/client"
 	"github.com/codeready-toolchain/toolchain-common/pkg/cluster"
-	"k8s.io/apimachinery/pkg/api/errors"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/runtime"
 	"k8s.io/apimachinery/pkg/runtime/schema"
@@ -26,55 +24,63 @@
 // ApplyToolchainObjects applies the given ToolchainObjects with the given labels.
 // If any object is marked as optional, then it checks if the API group is available - if not, then it skips the object.
 func (c APIClient) ApplyToolchainObjects(ctx context.Context, toolchainObjects []runtimeclient.Object, newLabels map[string]string) (bool, error) {
-	applyClient := applycl.NewApplyClient(c.Client)
 	anyApplied := false
 	logger := log.FromContext(ctx)
-
-	for _, object := range toolchainObjects {
-		if _, exists := object.GetAnnotations()[toolchainv1alpha1.TierTemplateObjectOptionalResourceAnnotation]; exists {
-			if !apiGroupIsPresent(c.AvailableAPIGroups, object.GetObjectKind().GroupVersionKind()) {
-				logger.Info("the object is marked as optional and the API group is not present - skipping...", "gvk", object.GetObjectKind().GroupVersionKind().String(), "name", object.GetName())
+	for _, o := range toolchainObjects {
+		gvk, err := getGvk(o, c.Client.Scheme())
+		if err != nil {
+			return anyApplied, err
+		}
+		if _, exists := o.GetAnnotations()[toolchainv1alpha1.TierTemplateObjectOptionalResourceAnnotation]; exists {
+			if !apiGroupIsPresent(c.AvailableAPIGroups, gvk) {
+				logger.Info("the object is marked as optional and the API group is not present - skipping...", "gvk", o.GetObjectKind().GroupVersionKind().String(), "name", o.GetName())
 				continue
 			}
 		}
-		// Special handling of ServiceAccounts is required because if a ServiceAccount is reapplied when it already exists, it causes Kubernetes controllers to
-		// automatically create new Secrets for the ServiceAccounts. After enough time the number of Secrets created will hit the Secrets quota and then no new
-		// Secrets can be created. To prevent this from happening, we fetch the already existing SA, update labels and annotations only, and then call update using the same object (keeping the refs to secrets).
-		if strings.EqualFold(object.GetObjectKind().GroupVersionKind().Kind, "ServiceAccount") {
-			logger.Info("the object is a ServiceAccount so we do the special handling for it...", "object_namespace", object.GetNamespace(), "object_name", object.GetObjectKind().GroupVersionKind().Kind+"/"+object.GetName())
-			sa := object.DeepCopyObject().(runtimeclient.Object)
-			err := applyClient.Get(ctx, runtimeclient.ObjectKeyFromObject(object), sa)
-			if err != nil && !errors.IsNotFound(err) {
-				return anyApplied, err
-			}
-			if err != nil {
-				logger.Info("the ServiceAccount does not exists - creating...")
-				applycl.MergeLabels(object, newLabels)
-				if err := applyClient.Create(ctx, object); err != nil {
-					return anyApplied, err
-				}
-			} else {
-				logger.Info("the ServiceAccount already exists - updating labels and annotations...")
-				applycl.MergeLabels(sa, newLabels)                    // add new labels to existing one
-				applycl.MergeLabels(sa, object.GetLabels())           // add new labels from template
-				applycl.MergeAnnotations(sa, object.GetAnnotations()) // add new annotations from template
-				err = applyClient.Update(ctx, sa)
-				if err != nil {
-					return anyApplied, err
-				}
-			}
-			anyApplied = true
-			continue
+		// we could theoretically work on the "o" itself, but let's not change the contents of the incoming parameters
+		toPatch := o.DeepCopyObject().(runtimeclient.Object)
+
+		// SSA requires the GVK to be set on the object (which is not the case usually) and the managedFields to be nil
+		toPatch.SetManagedFields(nil)
+		toPatch.GetObjectKind().SetGroupVersionKind(gvk)
+
+		toPatch.SetLabels(mergeLabels(toPatch.GetLabels(), newLabels))
+
+		if err := c.Client.Patch(ctx, toPatch, runtimeclient.Apply, runtimeclient.FieldOwner("kubesaw"), runtimeclient.ForceOwnership); err != nil {
+			return anyApplied, fmt.Errorf("unable to patch '%s' called '%s' in namespace '%s': %w", gvk, o.GetName(), o.GetNamespace(), err)
+		}
+
+		anyApplied = true
+	}
+	return anyApplied, nil
+}
+
+func mergeLabels(a, b map[string]string) map[string]string {
+	if a == nil {
+		return b
+	} else {
+		for k, v := range b {
+			a[k] = v
 		}
-		logger.Info("applying object", "object_namespace", object.GetNamespace(), "object_name", object.GetObjectKind().GroupVersionKind().Kind+"/"+object.GetName())
-		_, err := applyClient.Apply(ctx, []runtimeclient.Object{object}, newLabels)
+		return a
+	}
+}
+
+func getGvk(obj runtimeclient.Object, scheme *runtime.Scheme) (schema.GroupVersionKind, error) {
+	gvk := obj.GetObjectKind().GroupVersionKind()
+	if gvk.Empty() {
+		gvks, _, err := scheme.ObjectKinds(obj)
 		if err != nil {
-			return anyApplied, err
+			return schema.GroupVersionKind{}, err
 		}
-		anyApplied = true
+		if len(gvks) != 1 {
+			return schema.GroupVersionKind{}, fmt.Errorf("the scheme maps the object of type %T into more than 1 GVK. This is not supported at the moment", obj)
+		}
+
+		return gvks[0], nil
 	}
 
-	return anyApplied, nil
+	return gvk, nil
 }
 
 func apiGroupIsPresent(availableAPIGroups []metav1.APIGroup, gvk schema.GroupVersionKind) bool {

controllers/nstemplateset/client_test.go

@@ -14,6 +14,7 @@ import (
 	"github.com/stretchr/testify/require"
 	appsv1 "k8s.io/api/apps/v1"
 	corev1 "k8s.io/api/core/v1"
+	"k8s.io/apimachinery/pkg/api/errors"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/runtime/serializer"
 	"k8s.io/client-go/kubernetes/scheme"
@@ -159,14 +160,6 @@ func TestApplyToolchainObjects(t *testing.T) {
 			originalSA := sa.DeepCopy()
 			client.MergeLabels(originalSA, additionalLabel)
 			apiClient, fakeClient := prepareAPIClient(t, originalSA)
-			called := false
-			fakeClient.MockGet = func(ctx context.Context, key runtimeclient.ObjectKey, obj runtimeclient.Object, opts ...runtimeclient.GetOption) error {
-				if key.Name == "appstudio-user-sa" {
-					require.False(t, called, "should be called only once for SA")
-					called = true
-				}
-				return fakeClient.Client.Get(ctx, key, obj, opts...)
-			}
 
 			// when
 			changed, err := apiClient.ApplyToolchainObjects(ctx, copyObjects(newSaObject), newlabels)
@@ -183,7 +176,6 @@ func TestApplyToolchainObjects(t *testing.T) {
 				assert.Equal(t, expectedLabels, actualSA.Labels)
 				// check new annotations were applied
 			})
-
 		})
 
 		t.Run("update SA annotations", func(t *testing.T) {
@@ -365,6 +357,33 @@ func prepareAPIClient(t *testing.T, initObjs ...runtimeclient.Object) (*APIClien
 		obj.SetGeneration(o.GetGeneration())
 		return nil
 	}
+
+	fakeClient.MockPatch = func(ctx context.Context, obj runtimeclient.Object, patch runtimeclient.Patch, opts ...runtimeclient.PatchOption) error {
+		// fake client doesn't support SSA yet, so we have to be creative here and try to mock it out. Hopefully, SSA will be merged soon and we will
+		// be able to remove this.
+		//
+		// NOTE: this doesn't really implement SSA in any sense. It is just here so that the existing tests pass.
+
+		// A non-SSA patch assumes the object must already exist and should break if it doesn't. The SSA patch, on the other hand, creates the object
+		// if it doesn't exist.
+
+		if patch == runtimeclient.Apply {
+			copy := obj.DeepCopyObject().(runtimeclient.Object)
+			if err := fakeClient.Get(ctx, runtimeclient.ObjectKeyFromObject(copy), copy); err != nil {
+				if !errors.IsNotFound(err) {
+					return err
+				}
+				if err = fakeClient.Create(ctx, copy); err != nil {
+					return err
+				}
+			}
+			// the fake client actively complains if it sees an SSA patch...
+			patch = runtimeclient.Merge
+		}
+
+		return fakeClient.Client.Patch(ctx, obj, patch, opts...)
+	}
+
 	return &APIClient{
 		AllNamespacesClient: fakeClient,
 		Client:              fakeClient,

controllers/nstemplateset/cluster_resources_test.go

@@ -4,11 +4,12 @@ import (
 	"context"
 	"errors"
 	"fmt"
-	"k8s.io/utils/strings/slices"
 	"strings"
 	"testing"
 	"time"
 
+	"k8s.io/utils/strings/slices"
+
 	toolchainv1alpha1 "github.com/codeready-toolchain/api/api/v1alpha1"
 	"github.com/codeready-toolchain/member-operator/pkg/apis"
 	. "github.com/codeready-toolchain/member-operator/test"
@@ -388,7 +389,6 @@ func TestEnsureClusterResourcesOK(t *testing.T) {
 }
 
 func TestEnsureClusterResourcesFail(t *testing.T) {
-
 	// given
 	logger := zap.New(zap.UseDevMode(true))
 	log.SetLogger(logger)
@@ -451,12 +451,11 @@ func TestEnsureClusterResourcesFail(t *testing.T) {
 		AssertThatNSTemplateSet(t, namespaceName, spacename, fakeClient).
 			HasFinalizer().
 			HasConditions(UnableToProvisionClusterResources(
-				"failed to apply cluster resource of type 'quota.openshift.io/v1, Kind=ClusterResourceQuota': unable to create resource of kind: ClusterResourceQuota, version: v1: unable to create resource of kind: ClusterResourceQuota, version: v1: some error"))
+				"failed to apply cluster resource of type 'quota.openshift.io/v1, Kind=ClusterResourceQuota': unable to patch 'quota.openshift.io/v1, Kind=ClusterResourceQuota' called 'for-johnsmith-space' in namespace '': some error"))
 	})
 }
 
 func TestDeleteClusterResources(t *testing.T) {
-
 	// given
 	logger := zap.New(zap.UseDevMode(true))
 	log.SetLogger(logger)
@@ -606,7 +605,6 @@ func TestDeleteClusterResources(t *testing.T) {
 }
 
 func TestPromoteClusterResources(t *testing.T) {
-
 	restore := test.SetEnvVarAndRestore(t, commonconfig.WatchNamespaceEnvVar, "my-member-operator-namespace")
 	t.Cleanup(restore)
 
@@ -619,7 +617,6 @@ func TestPromoteClusterResources(t *testing.T) {
 	crb := newTektonClusterRoleBinding(spacename, "advanced")
 
 	t.Run("success", func(t *testing.T) {
-
 		t.Run("upgrade from advanced to team tier by changing only the CRQ", func(t *testing.T) {
 			// given
 			nsTmplSet := newNSTmplSet(namespaceName, spacename, "team", withNamespaces("abcde11", "dev"), withClusterResources("abcde11"))
@@ -735,7 +732,6 @@ func TestPromoteClusterResources(t *testing.T) {
 					HasResource(spacename+"-tekton-view", &rbacv1.ClusterRoleBinding{},
 						WithLabel("toolchain.dev.openshift.com/templateref", "withemptycrq-clusterresources-abcde11"),
 						WithLabel("toolchain.dev.openshift.com/tier", "withemptycrq"))
-
 			})
 		})
 
@@ -962,7 +958,6 @@ func TestPromoteClusterResources(t *testing.T) {
 					HasNoResource(spacename+"-tekton-view", &rbacv1.ClusterRoleBinding{}).
 					HasResource("for-another-user", &quotav1.ClusterResourceQuota{}).
 					HasResource("another-tekton-view", &rbacv1.ClusterRoleBinding{})
-
 			})
 		})
 
@@ -1024,7 +1019,6 @@ func TestPromoteClusterResources(t *testing.T) {
 	})
 
 	t.Run("failure", func(t *testing.T) {
-
 		t.Run("promotion to another tier fails because it cannot list current resources", func(t *testing.T) {
 			// given
 			nsTmplSet := newNSTmplSet(namespaceName, spacename, "basic", withNamespaces("abcde11", "dev"), withConditions(Updating()))
@@ -1083,7 +1077,6 @@ func TestPromoteClusterResources(t *testing.T) {
 }
 
 func TestUpdateClusterResources(t *testing.T) {
-
 	restore := test.SetEnvVarAndRestore(t, commonconfig.WatchNamespaceEnvVar, "my-member-operator-namespace")
 	t.Cleanup(restore)
 
@@ -1097,7 +1090,6 @@ func TestUpdateClusterResources(t *testing.T) {
 	crq := newClusterResourceQuota(spacename, "advanced")
 
 	t.Run("success", func(t *testing.T) {
-
 		t.Run("update from abcde11 revision to abcde12 revision as part of the advanced tier by updating CRQ", func(t *testing.T) {
 			// given
 			nsTmplSet := newNSTmplSet(namespaceName, spacename, "advanced", withNamespaces("abcde12", "dev"), withClusterResources("abcde12"))
@@ -1219,7 +1211,6 @@ func TestUpdateClusterResources(t *testing.T) {
 	})
 
 	t.Run("failure", func(t *testing.T) {
-
 		t.Run("update to abcde11 fails because it cannot list current resources", func(t *testing.T) {
 			// given
 			nsTmplSet := newNSTmplSet(namespaceName, spacename, "advanced", withClusterResources("abcde11"), withConditions(Updating()))
@@ -1303,23 +1294,27 @@ func TestRetainObjectsOfSameGVK(t *testing.T) {
 		Object: map[string]interface{}{
 			"kind":       "ClusterRole",
 			"apiVersion": "rbac.authorization.k8s.io/v1",
-		}}}
+		},
+	}}
 
 	namespace := runtime.RawExtension{Object: &unstructured.Unstructured{
 		Object: map[string]interface{}{
 			"kind":       "Namespace",
 			"apiVersion": "v1",
-		}}}
+		},
+	}}
 	clusterResQuota := runtime.RawExtension{Object: &unstructured.Unstructured{
 		Object: map[string]interface{}{
 			"kind":       "ClusterResourceQuota",
 			"apiVersion": "quota.openshift.io/v1",
-		}}}
+		},
+	}}
 	clusterRoleBinding := runtime.RawExtension{Object: &unstructured.Unstructured{
 		Object: map[string]interface{}{
 			"kind":       "ClusterRoleBinding",
 			"apiVersion": "rbac.authorization.k8s.io/v1",
-		}}}
+		},
+	}}
 
 	t.Run("verify retainObjectsOfSameGVK function for ClusterRole", func(t *testing.T) {
 		// given
@@ -1338,7 +1333,6 @@ func TestRetainObjectsOfSameGVK(t *testing.T) {
 		})
 
 		t.Run("should return true since the GVK matches", func(t *testing.T) {
-
 			// when
 			ok := retain(clusterRole)