From c952c18811edd93c01e989e7590a57680073bc0b Mon Sep 17 00:00:00 2001 From: Francisc Munteanu Date: Fri, 5 Apr 2024 17:13:12 +0200 Subject: [PATCH 1/5] clear last applied configuration --- pkg/client/client.go | 8 ++++++-- pkg/client/client_test.go | 36 ++++++++++++++++++++++++++++++++++++ 2 files changed, 42 insertions(+), 2 deletions(-) diff --git a/pkg/client/client.go b/pkg/client/client.go index 6ed5c3f3..698c34aa 100644 --- a/pkg/client/client.go +++ b/pkg/client/client.go @@ -113,7 +113,11 @@ func (c ApplyClient) applyObject(ctx context.Context, obj client.Object, options if config.saveConfiguration { // set current object as annotation annotations := obj.GetAnnotations() - newConfiguration = getNewConfiguration(obj) + // reset the previous config to avoid recursive embedding of the object + if _, found := obj.GetAnnotations()[LastAppliedConfigurationAnnotationKey]; found { + delete(obj.GetAnnotations(), LastAppliedConfigurationAnnotationKey) + } + newConfiguration = GetNewConfiguration(obj) if annotations == nil { annotations = map[string]string{} } @@ -205,7 +209,7 @@ func clusterIP(obj runtime.Object) (string, bool, error) { } } -func getNewConfiguration(newResource runtime.Object) string { +func GetNewConfiguration(newResource runtime.Object) string { newJSON, err := marshalObjectContent(newResource) if err != nil { log.Error(err, "unable to marshal the object", "object", newResource) diff --git a/pkg/client/client_test.go b/pkg/client/client_test.go index 78ebcf84..321ad7e8 100644 --- a/pkg/client/client_test.go +++ b/pkg/client/client_test.go @@ -339,6 +339,42 @@ func TestApplySingle(t *testing.T) { assert.Equal(t, "second-value", configMap.Data["first-param"]) }) }) + + t.Run("updates of ServiceAccount", func(t *testing.T) { + + t.Run("should update service account with last applied configuration", func(t *testing.T) { + // given + // there's an existing SA with secret refs + existingSA := newSA() + secretRefs := []corev1.ObjectReference{ + { + Name: "secret", + Namespace: existingSA.Namespace, + }, + } + existingSA.Secrets = secretRefs + cl, cli := newClient(t) + _, err := cl.ApplyRuntimeObject(context.TODO(), existingSA) + require.NoError(t, err) + + // when + // we update with existing annotations + newSA := existingSA.DeepCopy() + existingLastAppliedAnnotation := map[string]string{ + client.LastAppliedConfigurationAnnotationKey: client.GetNewConfiguration(newSA), + } + newSA.SetAnnotations(existingLastAppliedAnnotation) // let's set the last applied annotation + _, err = cl.ApplyRuntimeObject(context.TODO(), newSA) // then + + // then + require.NoError(t, err) + var actualSa corev1.ServiceAccount + err = cli.Get(context.TODO(), types.NamespacedName{Name: "appstudio-user-sa", Namespace: "john-dev"}, &actualSa) // assert sa was created + require.NoError(t, err) + assert.Equal(t, secretRefs, actualSa.Secrets) // secret refs are still there + assert.Equal(t, existingLastAppliedAnnotation[client.LastAppliedConfigurationAnnotationKey], actualSa.Annotations[client.LastAppliedConfigurationAnnotationKey]) // the last apply configuration should match the previous object + }) + }) } func toUnstructured(obj runtime.Object) (*unstructured.Unstructured, error) { From aacc2c6479568c7e2dfba7bef4749098ea3336f6 Mon Sep 17 00:00:00 2001 From: Francisc Munteanu Date: Mon, 8 Apr 2024 07:42:32 +0200 Subject: [PATCH 2/5] move cleanup --- pkg/client/client.go | 13 +++++++------ pkg/client/client_test.go | 7 ++++--- 2 files changed, 11 insertions(+), 9 deletions(-) diff --git a/pkg/client/client.go b/pkg/client/client.go index 698c34aa..2d500572 100644 --- a/pkg/client/client.go +++ b/pkg/client/client.go @@ -113,10 +113,6 @@ func (c ApplyClient) applyObject(ctx context.Context, obj client.Object, options if config.saveConfiguration { // set current object as annotation annotations := obj.GetAnnotations() - // reset the previous config to avoid recursive embedding of the object - if _, found := obj.GetAnnotations()[LastAppliedConfigurationAnnotationKey]; found { - delete(obj.GetAnnotations(), LastAppliedConfigurationAnnotationKey) - } newConfiguration = GetNewConfiguration(obj) if annotations == nil { annotations = map[string]string{} @@ -209,7 +205,12 @@ func clusterIP(obj runtime.Object) (string, bool, error) { } } -func GetNewConfiguration(newResource runtime.Object) string { +func GetNewConfiguration(newResource client.Object) string { + // reset the previous config to avoid recursive embedding of the object + copyResource := newResource.DeepCopyObject().(client.Object) + if _, found := copyResource.GetAnnotations()[LastAppliedConfigurationAnnotationKey]; found { + delete(copyResource.GetAnnotations(), LastAppliedConfigurationAnnotationKey) + } newJSON, err := marshalObjectContent(newResource) if err != nil { log.Error(err, "unable to marshal the object", "object", newResource) @@ -282,7 +283,7 @@ func ApplyUnstructuredObjectsWithNewLabels(ctx context.Context, cl client.Client for _, unstructuredObj := range unstructuredObjects { log.Info("applying object", "object_namespace", unstructuredObj.GetNamespace(), "object_name", unstructuredObj.GetObjectKind().GroupVersionKind().Kind+"/"+unstructuredObj.GetName()) MergeLabels(unstructuredObj, newLabels) - _, err := applyClient.ApplyObject(ctx, unstructuredObj) + _, err := applyClient.ApplyObject(ctx, unstructuredObj, SaveConfiguration(false)) if err != nil { return err } diff --git a/pkg/client/client_test.go b/pkg/client/client_test.go index 321ad7e8..c3daa088 100644 --- a/pkg/client/client_test.go +++ b/pkg/client/client_test.go @@ -353,6 +353,10 @@ func TestApplySingle(t *testing.T) { }, } existingSA.Secrets = secretRefs + existingLastAppliedAnnotation := map[string]string{ + client.LastAppliedConfigurationAnnotationKey: client.GetNewConfiguration(existingSA), + } + existingSA.SetAnnotations(existingLastAppliedAnnotation) // let's set the last applied annotation cl, cli := newClient(t) _, err := cl.ApplyRuntimeObject(context.TODO(), existingSA) require.NoError(t, err) @@ -360,9 +364,6 @@ func TestApplySingle(t *testing.T) { // when // we update with existing annotations newSA := existingSA.DeepCopy() - existingLastAppliedAnnotation := map[string]string{ - client.LastAppliedConfigurationAnnotationKey: client.GetNewConfiguration(newSA), - } newSA.SetAnnotations(existingLastAppliedAnnotation) // let's set the last applied annotation _, err = cl.ApplyRuntimeObject(context.TODO(), newSA) // then From 7ed09c15dd9e6ed8753200366a78c53d1290ef7d Mon Sep 17 00:00:00 2001 From: Francisc Munteanu Date: Tue, 9 Apr 2024 14:49:42 +0200 Subject: [PATCH 3/5] remove resourceversion and create removeAnnotation func --- pkg/client/client.go | 18 +++++++++++------- 1 file changed, 11 insertions(+), 7 deletions(-) diff --git a/pkg/client/client.go b/pkg/client/client.go index 2d500572..b6e317b9 100644 --- a/pkg/client/client.go +++ b/pkg/client/client.go @@ -124,6 +124,7 @@ func (c ApplyClient) applyObject(ctx context.Context, obj client.Object, options namespacedName := types.NamespacedName{Namespace: obj.GetNamespace(), Name: obj.GetName()} if err := c.Client.Get(ctx, namespacedName, existing); err != nil { if apierrors.IsNotFound(err) { + obj.SetResourceVersion("") // reset resource version when creating to avoid error: resourceVersion should not be set on objects to be created return true, c.createObj(ctx, obj, config.owner) } return false, errors.Wrapf(err, "unable to get the resource '%v'", existing) @@ -207,18 +208,21 @@ func clusterIP(obj runtime.Object) (string, bool, error) { func GetNewConfiguration(newResource client.Object) string { // reset the previous config to avoid recursive embedding of the object - copyResource := newResource.DeepCopyObject().(client.Object) - if _, found := copyResource.GetAnnotations()[LastAppliedConfigurationAnnotationKey]; found { - delete(copyResource.GetAnnotations(), LastAppliedConfigurationAnnotationKey) - } - newJSON, err := marshalObjectContent(newResource) + copyResource := removeAnnotation(newResource, LastAppliedConfigurationAnnotationKey) + newJSON, err := marshalObjectContent(copyResource) if err != nil { - log.Error(err, "unable to marshal the object", "object", newResource) - return fmt.Sprintf("%v", newResource) + log.Error(err, "unable to marshal the object", "object", copyResource) + return fmt.Sprintf("%v", copyResource) } return string(newJSON) } +func removeAnnotation(newResource client.Object, annotationKey string) client.Object { + copyResource := newResource.DeepCopyObject().(client.Object) + delete(copyResource.GetAnnotations(), annotationKey) + return copyResource +} + func marshalObjectContent(newResource runtime.Object) ([]byte, error) { if newRes, ok := newResource.(runtime.Unstructured); ok { return json.Marshal(newRes.UnstructuredContent()) From cf8634bae91180a43272ef5b29ed7e1ca2468103 Mon Sep 17 00:00:00 2001 From: Francisc Munteanu Date: Mon, 15 Apr 2024 22:04:21 +0200 Subject: [PATCH 4/5] fix apply object when saveconfiguration is disabled and annotation is present --- pkg/client/client.go | 3 +- pkg/client/client_test.go | 63 +++++++++++++++++++++++++++++++++++++++ 2 files changed, 65 insertions(+), 1 deletion(-) diff --git a/pkg/client/client.go b/pkg/client/client.go index b6e317b9..14f8ab1c 100644 --- a/pkg/client/client.go +++ b/pkg/client/client.go @@ -134,7 +134,8 @@ func (c ApplyClient) applyObject(ctx context.Context, obj client.Object, options if !config.forceUpdate { existingAnnotations := existing.GetAnnotations() if existingAnnotations != nil { - if newConfiguration == existingAnnotations[LastAppliedConfigurationAnnotationKey] { + lastApplied, lastAppliedFound := existingAnnotations[LastAppliedConfigurationAnnotationKey] + if lastAppliedFound && newConfiguration != "" && newConfiguration == lastApplied { return false, nil } } diff --git a/pkg/client/client_test.go b/pkg/client/client_test.go index c3daa088..c0417f47 100644 --- a/pkg/client/client_test.go +++ b/pkg/client/client_test.go @@ -376,6 +376,69 @@ func TestApplySingle(t *testing.T) { assert.Equal(t, existingLastAppliedAnnotation[client.LastAppliedConfigurationAnnotationKey], actualSa.Annotations[client.LastAppliedConfigurationAnnotationKey]) // the last apply configuration should match the previous object }) }) + + t.Run("should update object when save configuration is disabled and another annotation is present", func(t *testing.T) { + // given + cl, cli := newClient(t) + existingRules := []rbac.PolicyRule{ + { + APIGroups: []string{"toolchain.dev.openshift.com"}, + Resources: []string{"bannedusers", "masteruserrecords"}, + Verbs: []string{"*"}, + }, + } + existingRole := rbac.Role{ + ObjectMeta: metav1.ObjectMeta{ + Name: "toolchaincluster-host", + Namespace: HostOperatorNs, + Annotations: map[string]string{"kubectl.kubernetes.io/last-applied-configuration": "{\"apiVersion\":\"rbac.authorization.k8s.io/v1\",\"kind\":\"Role\",\"metadata\":{\"annotations\":{},\"name\":\"toolchaincluster-host\",\"namespace\":\"toolchain-host-operator\"},\"rules\":[{\"apiGroups\":[\"toolchain.dev.openshift.com\"],\"resources\":[\"bannedusers\",\"masteruserrecords\"],\"verbs\":[\"*\"]}]}"}, + }, + Rules: existingRules, + } + + // when + _, err := cl.ApplyRuntimeObject(context.TODO(), &existingRole, client.SaveConfiguration(false)) + require.NoError(t, err) + + // then + // the object should match the existing role + foundRole := &rbac.Role{} + err = cli.Get(context.TODO(), types.NamespacedName{ + Name: "toolchaincluster-host", + Namespace: HostOperatorNs, + }, foundRole) + require.NoError(t, err) + require.Equal(t, existingRules, foundRole.Rules) + + // when + // we updated the role + newRules := []rbac.PolicyRule{ + { + APIGroups: []string{"toolchain.dev.openshift.com"}, + Resources: []string{"*"}, + Verbs: []string{"*"}, + }, + } + newRole := rbac.Role{ + ObjectMeta: metav1.ObjectMeta{ + Name: "toolchaincluster-host", + Namespace: HostOperatorNs, + }, + Rules: newRules, + } + _, err = cl.ApplyRuntimeObject(context.TODO(), &newRole, client.SaveConfiguration(false)) + require.NoError(t, err) + + // then + // the new rules should be there + foundRole = &rbac.Role{} + err = cl.Get(context.TODO(), types.NamespacedName{ + Name: "toolchaincluster-host", + Namespace: HostOperatorNs, + }, foundRole) + require.NoError(t, err) + require.Equal(t, newRules, foundRole.Rules) + }) } func toUnstructured(obj runtime.Object) (*unstructured.Unstructured, error) { From 91ec6712cad89744600f375013fff20bcbadbe55 Mon Sep 17 00:00:00 2001 From: Francisc Munteanu Date: Wed, 17 Apr 2024 10:39:51 +0200 Subject: [PATCH 5/5] PR comments --- pkg/client/client_test.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/pkg/client/client_test.go b/pkg/client/client_test.go index c0417f47..6233d819 100644 --- a/pkg/client/client_test.go +++ b/pkg/client/client_test.go @@ -342,7 +342,7 @@ func TestApplySingle(t *testing.T) { t.Run("updates of ServiceAccount", func(t *testing.T) { - t.Run("should update service account with last applied configuration", func(t *testing.T) { + t.Run("last-applied-configuration is used and SA secret ref is not updated", func(t *testing.T) { // given // there's an existing SA with secret refs existingSA := newSA() @@ -358,7 +358,7 @@ func TestApplySingle(t *testing.T) { } existingSA.SetAnnotations(existingLastAppliedAnnotation) // let's set the last applied annotation cl, cli := newClient(t) - _, err := cl.ApplyRuntimeObject(context.TODO(), existingSA) + _, err := cl.ApplyRuntimeObject(context.TODO(), existingSA.DeepCopyObject()) require.NoError(t, err) // when