diff --git a/pkg/client/client.go b/pkg/client/client.go index 6ed5c3f3..14f8ab1c 100644 --- a/pkg/client/client.go +++ b/pkg/client/client.go @@ -113,7 +113,7 @@ 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) + newConfiguration = GetNewConfiguration(obj) if annotations == nil { annotations = map[string]string{} } @@ -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) @@ -133,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 } } @@ -205,15 +207,23 @@ func clusterIP(obj runtime.Object) (string, bool, error) { } } -func getNewConfiguration(newResource runtime.Object) string { - newJSON, err := marshalObjectContent(newResource) +func GetNewConfiguration(newResource client.Object) string { + // reset the previous config to avoid recursive embedding of the object + 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()) @@ -278,7 +288,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 78ebcf84..6233d819 100644 --- a/pkg/client/client_test.go +++ b/pkg/client/client_test.go @@ -339,6 +339,106 @@ 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("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() + secretRefs := []corev1.ObjectReference{ + { + Name: "secret", + Namespace: existingSA.Namespace, + }, + } + 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.DeepCopyObject()) + require.NoError(t, err) + + // when + // we update with existing annotations + newSA := existingSA.DeepCopy() + 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 + }) + }) + + 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) {