diff --git a/controllers/nstemplateset/client.go b/controllers/nstemplateset/client.go index f3405b14..a6ffdf20 100644 --- a/controllers/nstemplateset/client.go +++ b/controllers/nstemplateset/client.go @@ -2,12 +2,12 @@ package nstemplateset import ( "context" + "strings" 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" "github.com/go-logr/logr" - v1 "k8s.io/api/core/v1" "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" @@ -39,15 +39,21 @@ func (c APIClient) ApplyToolchainObjects(logger logr.Logger, toolchainObjects [] // 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 object.GetObjectKind().GroupVersionKind().Kind == "ServiceAccount" { - sa := &v1.ServiceAccount{} - err := applyClient.Client.Get(context.TODO(), runtimeclient.ObjectKeyFromObject(object), sa) + 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(context.TODO(), runtimeclient.ObjectKeyFromObject(object), sa) if err != nil && !errors.IsNotFound(err) { return anyApplied, err } - // update labels and annotations for service account - if err == nil { - logger.Info("the object is a ServiceAccount and already exists - updating labels and annotations...") + if err != nil { + logger.Info("the ServiceAccount does not exists - creating...") + applycl.MergeLabels(object, newLabels) + if err := applyClient.Create(context.TODO(), 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 @@ -55,9 +61,9 @@ func (c APIClient) ApplyToolchainObjects(logger logr.Logger, toolchainObjects [] if err != nil { return anyApplied, err } - anyApplied = true - continue } + anyApplied = true + continue } logger.Info("applying object", "object_namespace", object.GetNamespace(), "object_name", object.GetObjectKind().GroupVersionKind().Kind+"/"+object.GetName()) _, err := applyClient.Apply([]runtimeclient.Object{object}, newLabels) diff --git a/controllers/nstemplateset/client_test.go b/controllers/nstemplateset/client_test.go index f0ddf9be..7cc9434a 100644 --- a/controllers/nstemplateset/client_test.go +++ b/controllers/nstemplateset/client_test.go @@ -156,9 +156,17 @@ func TestApplyToolchainObjects(t *testing.T) { "existing_label": "new_value", // existing label value was updated "foo": "bar", // existing label was preserved } - apiClient, fakeClient := prepareAPIClient(t) - _, err := client.NewApplyClient(fakeClient).Apply(copyObjects(sa), additionalLabel) - require.NoError(t, err) + 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(logger, copyObjects(newSaObject), newlabels) @@ -166,6 +174,7 @@ func TestApplyToolchainObjects(t *testing.T) { // then require.NoError(t, err) assert.True(t, changed) + fakeClient.MockGet = nil actualSA := &corev1.ServiceAccount{} AssertObject(t, fakeClient, sa.Namespace, sa.Name, actualSA, func() { // check that the Secrets field is still there @@ -191,11 +200,20 @@ func TestApplyToolchainObjects(t *testing.T) { apiClient, fakeClient := prepareAPIClient(t) _, err := client.NewApplyClient(fakeClient).Apply(copyObjects(sa), additionalLabel) require.NoError(t, err) + 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...) + } changed, err := apiClient.ApplyToolchainObjects(logger, copyObjects(newSaObject), additionalLabel) // then require.NoError(t, err) assert.True(t, changed) + fakeClient.MockGet = nil actualSA := &corev1.ServiceAccount{} AssertObject(t, fakeClient, sa.Namespace, sa.Name, actualSA, func() { // check that the Secrets field is still there @@ -233,6 +251,14 @@ func TestApplyToolchainObjects(t *testing.T) { apiClient, fakeClient := prepareAPIClient(t) _, err := client.NewApplyClient(fakeClient).Apply(copyObjects(devNs, role), additionalLabel) require.NoError(t, err) + 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(logger, copyObjects(sa), additionalLabel) @@ -240,6 +266,7 @@ func TestApplyToolchainObjects(t *testing.T) { // then require.NoError(t, err) assert.True(t, changed) + fakeClient.MockGet = nil assertObjects(t, fakeClient, false) }) }