Add suspended annotations (#4131)

* Add suspended-by and suspended-comment annotations

* Add check when no annotations already exist when updating suspend annotations

* Add check suspended annotations existance in suspend tests

* Add principal check in suspend tests

* Check for hardcoded principal id instead of principal from context in suspend tests

* Cleanup variable names and return statements in suspend

* Add proto-linux in makefile

generate proto on linux

* Cleanup suspend test

Remove checkSpec and add getting unstructured object

Check for spec.suspend value and suspend annotations in unstructured object

* Update logger info with principal and cluster in suspend

Makefile

@@ -138,6 +138,11 @@ proto: ## Generate protobuf files
 #	This job is complaining about a missing plugin and error-ing out
 #	oapi-codegen -config oapi-codegen.config.yaml api/applications/applications.swagger.json
 
+# Sometimes we get whitespace differences when running this on linux vs mac
+# So here's how you can do it under linux, on mac
+proto-linux:
+	docker run --rm -v "$(CURRENT_DIR):/app" -w /app golang:1.20 make proto
+
 ##@ Docker
 _docker:
 	DOCKER_BUILDKIT=1 docker build $(DOCKERARGS)\

api/core/core.proto

@@ -426,6 +426,7 @@ message GetFeatureFlagsResponse {
 message ToggleSuspendResourceRequest {
     repeated ObjectRef objects = 1;
     bool     suspend           = 2;
+    string   comment           = 3;
 }
 
 message ToggleSuspendResourceResponse {

api/core/core.swagger.json

@@ -1672,6 +1672,9 @@
         },
         "suspend": {
           "type": "boolean"
+        },
+        "comment": {
+          "type": "string"
         }
       }
     },

buf.lock

@@ -4,8 +4,8 @@ deps:
   - remote: buf.build
     owner: googleapis
     repository: googleapis
-    commit: 463926e7ee924d46ad0a726e1cf4eacd
+    commit: 28151c0d0a1641bf938a7672c500e01d
   - remote: buf.build
     owner: grpc-ecosystem
     repository: grpc-gateway
-    commit: a1ecdc58eccd49aa8bea2a7a9022dc27
+    commit: 3f42134f4c564983838425bc43c7a65f

core/server/suspend.go

@@ -16,13 +16,14 @@ func (cs *coreServer) ToggleSuspendResource(ctx context.Context, msg *pb.ToggleS
 	respErrors := multierror.Error{}
 
 	for _, obj := range msg.Objects {
-		clustersClient, err := cs.clustersManager.GetImpersonatedClient(ctx, auth.Principal(ctx))
+		clusterName := obj.ClusterName
+		clustersClient, err := cs.clustersManager.GetImpersonatedClient(ctx, principal)
 		if err != nil {
 			respErrors = *multierror.Append(fmt.Errorf("error getting impersonating client: %w", err), respErrors.Errors...)
 			continue
 		}
 
-		c, err := clustersClient.Scoped(obj.ClusterName)
+		c, err := clustersClient.Scoped(clusterName)
 		if err != nil {
 			respErrors = *multierror.Append(fmt.Errorf("getting cluster client: %w", err), respErrors.Errors...)
 			continue
@@ -46,6 +47,8 @@ func (cs *coreServer) ToggleSuspendResource(ctx context.Context, msg *pb.ToggleS
 			"kind", obj.GroupVersionKind().Kind,
 			"name", key.Name,
 			"namespace", key.Namespace,
+			"principal", principal.ID,
+			"cluster", clusterName,
 		)
 
 		if err := c.Get(ctx, key, obj.AsClientObject()); err != nil {
@@ -60,6 +63,8 @@ func (cs *coreServer) ToggleSuspendResource(ctx context.Context, msg *pb.ToggleS
 			return nil, err
 		}
 
+		changeSuspendAnnotations(obj, msg.Suspend, msg.Comment, principal)
+
 		if msg.Suspend {
 			log.Info("Suspending resource")
 		} else {
@@ -73,3 +78,19 @@ func (cs *coreServer) ToggleSuspendResource(ctx context.Context, msg *pb.ToggleS
 
 	return &pb.ToggleSuspendResourceResponse{}, respErrors.ErrorOrNil()
 }
+
+func changeSuspendAnnotations(obj fluxsync.Reconcilable, suspend bool, comment string, principal *auth.UserPrincipal) {
+	annotations := obj.GetAnnotations()
+	if annotations == nil {
+		annotations = map[string]string{}
+	}
+	if suspend {
+		annotations["weave.works/suspended-by"] = principal.ID
+		annotations["weave.works/suspended-comment"] = comment
+		obj.SetAnnotations(annotations)
+	} else {
+		delete(annotations, "weave.works/suspended-by")
+		delete(annotations, "weave.works/suspended-comment")
+		obj.SetAnnotations(annotations)
+	}
+}

core/server/suspend_test.go

@@ -2,18 +2,19 @@ package server_test
 
 import (
 	"context"
-	sourcev1b2 "github.com/fluxcd/source-controller/api/v1beta2"
 	"testing"
 
 	helmv2 "github.com/fluxcd/helm-controller/api/v2beta1"
 	imgautomationv1 "github.com/fluxcd/image-automation-controller/api/v1beta1"
 	reflectorv1 "github.com/fluxcd/image-reflector-controller/api/v1beta2"
 	kustomizev1 "github.com/fluxcd/kustomize-controller/api/v1"
 	sourcev1 "github.com/fluxcd/source-controller/api/v1"
+	sourcev1b2 "github.com/fluxcd/source-controller/api/v1beta2"
 	. "github.com/onsi/gomega"
 	api "github.com/weaveworks/weave-gitops/pkg/api/core"
 	"github.com/weaveworks/weave-gitops/pkg/kube"
 	"google.golang.org/grpc/metadata"
+	"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
 	"k8s.io/apimachinery/pkg/types"
 	"sigs.k8s.io/controller-runtime/pkg/client"
 )
@@ -40,36 +41,44 @@ func TestSuspend_Suspend(t *testing.T) {
 	hr := makeHelmRepo("repo-1", *ns)
 
 	tests := []struct {
-		kind string
-		obj  client.Object
+		kind       string
+		apiVersion string
+		obj        client.Object
 	}{
 		{
-			kind: sourcev1.GitRepositoryKind,
-			obj:  gr,
+			kind:       sourcev1.GitRepositoryKind,
+			apiVersion: sourcev1.GroupVersion.String(),
+			obj:        gr,
 		},
 		{
-			kind: sourcev1b2.HelmRepositoryKind,
-			obj:  hr,
+			kind:       sourcev1b2.HelmRepositoryKind,
+			apiVersion: sourcev1b2.GroupVersion.String(),
+			obj:        hr,
 		},
 		{
-			kind: sourcev1b2.BucketKind,
-			obj:  makeBucket("bucket-1", *ns),
+			kind:       sourcev1b2.BucketKind,
+			apiVersion: sourcev1b2.GroupVersion.String(),
+			obj:        makeBucket("bucket-1", *ns),
 		},
 		{
-			kind: kustomizev1.KustomizationKind,
-			obj:  makeKustomization("kust-1", *ns, gr),
+			kind:       kustomizev1.KustomizationKind,
+			apiVersion: kustomizev1.GroupVersion.String(),
+			obj:        makeKustomization("kust-1", *ns, gr),
 		},
 		{
-			kind: helmv2.HelmReleaseKind,
-			obj:  makeHelmRelease("hr-1", *ns, hr, makeHelmChart("somechart", *ns)),
+			kind:       helmv2.HelmReleaseKind,
+			apiVersion: helmv2.GroupVersion.String(),
+			obj:        makeHelmRelease("hr-1", *ns, hr, makeHelmChart("somechart", *ns)),
 		},
 		{
-			kind: reflectorv1.ImageRepositoryKind,
-			obj:  makeImageRepository("ir-1", *ns),
+			kind:       reflectorv1.ImageRepositoryKind,
+			apiVersion: reflectorv1.GroupVersion.String(),
+			obj:        makeImageRepository("ir-1", *ns),
 		},
 		{
-			kind: imgautomationv1.ImageUpdateAutomationKind,
-			obj:  makeImageUpdateAutomation("iua-1", *ns),
+			kind:       imgautomationv1.ImageUpdateAutomationKind,
+			apiVersion: imgautomationv1.GroupVersion.String(),
+			obj:        makeImageUpdateAutomation("iua-1", *ns),
 		},
 	}
 
@@ -89,12 +98,20 @@ func TestSuspend_Suspend(t *testing.T) {
 				Objects: []*api.ObjectRef{object},
 				Suspend: true,
 			}
-			md := metadata.Pairs(MetadataUserKey, "anne", MetadataGroupsKey, "system:masters")
+			principalID := "anne"
+			md := metadata.Pairs(MetadataUserKey, principalID, MetadataGroupsKey, "system:masters")
 			outgoingCtx := metadata.NewOutgoingContext(ctx, md)
 			_, err = c.ToggleSuspendResource(outgoingCtx, req)
 			g.Expect(err).NotTo(HaveOccurred())
 			name := types.NamespacedName{Name: tt.obj.GetName(), Namespace: ns.Name}
-			g.Expect(checkSpec(t, k, name, tt.obj)).To(BeTrue())
+
+			unstructuredObj := getUnstructuredObj(t, k, name, tt.kind, tt.apiVersion)
+			suspendVal, _, err := unstructured.NestedBool(unstructuredObj.Object, "spec", "suspend")
+			g.Expect(err).NotTo(HaveOccurred())
+			g.Expect(suspendVal).To(BeTrue())
+
+			checkSuspendAnnotations(t, principalID, unstructuredObj.GetAnnotations(), name, suspendVal)
+
 			requestObjects = append(requestObjects, object)
 		})
 	}
@@ -105,14 +122,20 @@ func TestSuspend_Suspend(t *testing.T) {
 			Suspend: false,
 		}
 
-		md := metadata.Pairs(MetadataUserKey, "anne", MetadataGroupsKey, "system:masters")
+		principalID := "anne"
+		md := metadata.Pairs(MetadataUserKey, principalID, MetadataGroupsKey, "system:masters")
 		outgoingCtx := metadata.NewOutgoingContext(ctx, md)
 		_, err = c.ToggleSuspendResource(outgoingCtx, req)
 		g.Expect(err).NotTo(HaveOccurred())
 
 		for _, tt := range tests {
 			name := types.NamespacedName{Name: tt.obj.GetName(), Namespace: ns.Name}
-			g.Expect(checkSpec(t, k, name, tt.obj)).To(BeFalse())
+			unstructuredObj := getUnstructuredObj(t, k, name, tt.kind, tt.apiVersion)
+			suspendVal, _, err := unstructured.NestedBool(unstructuredObj.Object, "spec", "suspend")
+			g.Expect(err).NotTo(HaveOccurred())
+			g.Expect(suspendVal).To(BeFalse())
+
+			checkSuspendAnnotations(t, principalID, unstructuredObj.GetAnnotations(), name, suspendVal)
 		}
 	})
 
@@ -137,58 +160,41 @@ func TestSuspend_Suspend(t *testing.T) {
 	})
 }
 
-func checkSpec(t *testing.T, k client.Client, name types.NamespacedName, obj client.Object) bool {
-	switch v := obj.(type) {
-	case *sourcev1.GitRepository:
-		if err := k.Get(context.Background(), name, v); err != nil {
-			t.Error(err)
-		}
-
-		return v.Spec.Suspend
-	case *kustomizev1.Kustomization:
-		if err := k.Get(context.Background(), name, v); err != nil {
-			t.Error(err)
-		}
-
-		return v.Spec.Suspend
-
-	case *helmv2.HelmRelease:
-		if err := k.Get(context.Background(), name, v); err != nil {
-			t.Error(err)
-		}
+func getUnstructuredObj(t *testing.T, k client.Client, name types.NamespacedName, kind, apiVersion string) *unstructured.Unstructured {
+	unstructuredObj := &unstructured.Unstructured{}
+	unstructuredObj.SetKind(kind)
+	unstructuredObj.SetAPIVersion(apiVersion)
+	if err := k.Get(context.Background(), name, unstructuredObj); err != nil {
+		t.Error(err)
+	}
 
-		return v.Spec.Suspend
+	return unstructuredObj
+}
 
-	case *sourcev1b2.Bucket:
-		if err := k.Get(context.Background(), name, v); err != nil {
-			t.Error(err)
+// checkSuspendAnnotations checks for the existance of suspend annotations
+// passes if suspended and annotations exist, or not suspended and annotations don't exist
+// if annotations exist, the principal is checked in the annotation for suspended-by
+func checkSuspendAnnotations(t *testing.T, principalID string, annotations map[string]string, name types.NamespacedName, suspend bool) {
+	if suspend {
+		// suspended and annotations exist check
+		if suspendedBy, ok := annotations["weave.works/suspended-by"]; ok {
+			// principal check if suspended and annotations exist
+			if suspendedBy != principalID {
+				t.Errorf("expected annotation weave.works/suspended-by to be set to the principal %s", principalID)
+			}
+		} else {
+			t.Errorf("expected annotation weave.works/suspended-by not found for %s", name)
 		}
-
-		return v.Spec.Suspend
-
-	case *sourcev1b2.HelmRepository:
-		if err := k.Get(context.Background(), name, v); err != nil {
-			t.Error(err)
+		if _, ok := annotations["weave.works/suspended-comment"]; !ok {
+			t.Errorf("expected annotation weave.works/suspended-comment not found for %s", name)
 		}
-
-		return v.Spec.Suspend
-
-	case *reflectorv1.ImageRepository:
-		if err := k.Get(context.Background(), name, v); err != nil {
-			t.Error(err)
+	} else {
+		// not suspended and annotations don't exist check
+		if _, ok := annotations["weave.works/suspended-by"]; ok {
+			t.Errorf("expected annotation weave.works/suspended-by not found for %s", name)
 		}
-
-		return v.Spec.Suspend
-
-	case *imgautomationv1.ImageUpdateAutomation:
-		if err := k.Get(context.Background(), name, v); err != nil {
-			t.Error(err)
+		if _, ok := annotations["weave.works/suspended-comment"]; ok {
+			t.Errorf("expected annotation weave.works/suspended-comment not found for %s", name)
 		}
-
-		return v.Spec.Suspend
 	}
-
-	t.Errorf("unsupported object %T", obj)
-
-	return false
 }