From 6e1ce1bfa1201914794c2b25ca6ba779ec455300 Mon Sep 17 00:00:00 2001 From: Matous Jobanek Date: Thu, 3 Aug 2023 14:11:47 +0200 Subject: [PATCH 1/3] directly create SA if does not already exist --- controllers/nstemplateset/client.go | 20 +++++++++----- controllers/nstemplateset/client_test.go | 33 +++++++++++++++++++++--- 2 files changed, 44 insertions(+), 9 deletions(-) diff --git a/controllers/nstemplateset/client.go b/controllers/nstemplateset/client.go index f3405b14..1c4f1efb 100644 --- a/controllers/nstemplateset/client.go +++ b/controllers/nstemplateset/client.go @@ -2,6 +2,7 @@ package nstemplateset import ( "context" + "strings" toolchainv1alpha1 "github.com/codeready-toolchain/api/api/v1alpha1" applycl "github.com/codeready-toolchain/toolchain-common/pkg/client" @@ -39,15 +40,22 @@ 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" { + if strings.EqualFold(object.GetObjectKind().GroupVersionKind().Kind, "ServiceAccount") { + logger.Info("the object is a ServiceAccount so we do the special handling for it...") sa := &v1.ServiceAccount{} - err := applyClient.Client.Get(context.TODO(), runtimeclient.ObjectKeyFromObject(object), sa) + 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 +63,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) }) } From 91038c07680d2f91af396cb346f9404ebb2559d1 Mon Sep 17 00:00:00 2001 From: Matous Jobanek Date: Thu, 3 Aug 2023 14:15:30 +0200 Subject: [PATCH 2/3] additional params --- controllers/nstemplateset/client.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/controllers/nstemplateset/client.go b/controllers/nstemplateset/client.go index 1c4f1efb..46b1f200 100644 --- a/controllers/nstemplateset/client.go +++ b/controllers/nstemplateset/client.go @@ -41,7 +41,7 @@ func (c APIClient) ApplyToolchainObjects(logger logr.Logger, toolchainObjects [] // 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...") + 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 := &v1.ServiceAccount{} err := applyClient.Get(context.TODO(), runtimeclient.ObjectKeyFromObject(object), sa) if err != nil && !errors.IsNotFound(err) { From 48d28e73852e63b9e45f0a1b7a08e1f9d5745107 Mon Sep 17 00:00:00 2001 From: Matous Jobanek Date: Thu, 3 Aug 2023 16:26:47 +0200 Subject: [PATCH 3/3] make the copy --- controllers/nstemplateset/client.go | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/controllers/nstemplateset/client.go b/controllers/nstemplateset/client.go index 46b1f200..a6ffdf20 100644 --- a/controllers/nstemplateset/client.go +++ b/controllers/nstemplateset/client.go @@ -8,7 +8,6 @@ import ( 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" @@ -42,12 +41,11 @@ func (c APIClient) ApplyToolchainObjects(logger logr.Logger, toolchainObjects [] // 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 := &v1.ServiceAccount{} + 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 ServiceAccount does not exists - creating...") applycl.MergeLabels(object, newLabels)