feat: support resource actions on CRDs that use status subresources

cmd/argocd/commands/app.go

@@ -2317,7 +2317,7 @@ func filterResources(command *cobra.Command, resources []*argoappv1.ResourceDiff
 		if resourceName != "" && resourceName != obj.GetName() {
 			continue
 		}
-		if kind != gvk.Kind {
+		if kind != "" && kind != gvk.Kind {
 			continue
 		}
 		deepCopy := obj.DeepCopy()

cmd/argocd/commands/app_actions.go

@@ -100,7 +100,6 @@ func NewApplicationResourceActionsListCommand(clientOpts *argocdclient.ClientOpt
 		case "":
 			w := tabwriter.NewWriter(os.Stdout, 0, 0, 2, ' ', 0)
 			fmt.Fprintf(w, "GROUP\tKIND\tNAME\tACTION\tDISABLED\n")
-			fmt.Println()
 			for _, action := range availableActions {
 				fmt.Fprintf(w, "%s\t%s\t%s\t%s\t%s\n", action.Group, action.Kind, action.Name, action.Action, strconv.FormatBool(action.Disabled))
 			}

go.mod

@@ -7,7 +7,7 @@ require (
 	github.com/TomOnTime/utfutil v0.0.0-20180511104225-09c41003ee1d
 	github.com/alicebob/gopher-json v0.0.0-20180125190556-5a6b3ba71ee6 // indirect
 	github.com/alicebob/miniredis v2.5.0+incompatible
-	github.com/argoproj/gitops-engine v0.1.3-0.20201027001456-31311943a57a
+	github.com/argoproj/gitops-engine v0.1.3-0.20201027224148-eb76c93f0a2e
 	github.com/argoproj/pkg v0.2.0
 	github.com/bombsimon/logrusr v0.0.0-20200131103305-03a291ce59b4
 	github.com/casbin/casbin v1.9.1

go.sum

@@ -84,8 +84,8 @@ github.com/andreyvit/diff v0.0.0-20170406064948-c7f18ee00883/go.mod h1:rCTlJbsFo
 github.com/anmitsu/go-shlex v0.0.0-20161002113705-648efa622239 h1:kFOfPq6dUM1hTo4JG6LR5AXSUEsOjtdm0kw0FtQtMJA=
 github.com/anmitsu/go-shlex v0.0.0-20161002113705-648efa622239/go.mod h1:2FmKhYUyUczH0OGQWaF5ceTx0UBShxjsH6f8oGKYe2c=
 github.com/antihax/optional v0.0.0-20180407024304-ca021399b1a6/go.mod h1:V8iCPQYkqmusNa815XgQio277wI47sdRh1dUOLdyC6Q=
-github.com/argoproj/gitops-engine v0.1.3-0.20201027001456-31311943a57a h1:ShQaYWNaNKJ8iy65vkftl6jYbDU2kLiAdQ0XJ8LSlOo=
-github.com/argoproj/gitops-engine v0.1.3-0.20201027001456-31311943a57a/go.mod h1:7Hzxz1c5YFXpAsM+i9L4LzKPnt8oY0t9QaGgEEUbQuc=
+github.com/argoproj/gitops-engine v0.1.3-0.20201027224148-eb76c93f0a2e h1:a/Kem2t1Zs9uEI9FDVDmNn3SneBuuaJvq+wyj1iuLwQ=
+github.com/argoproj/gitops-engine v0.1.3-0.20201027224148-eb76c93f0a2e/go.mod h1:OxXp8YaT73rw9gEBnGBWg55af80nkV/uIjWCbJu1Nw0=
 github.com/argoproj/pkg v0.2.0 h1:ETgC600kr8WcAi3MEVY5sA1H7H/u1/IysYOobwsZ8No=
 github.com/argoproj/pkg v0.2.0/go.mod h1:F4TZgInLUEjzsWFB/BTJBsewoEy0ucnKSq6vmQiD/yc=
 github.com/armon/circbuf v0.0.0-20150827004946-bbbad097214e/go.mod h1:3U/XgcO3hCbHZ8TKRvWD2dDTCfh9M9ya+I9JpbB7O8o=

server/application/application.go

@@ -1439,9 +1439,6 @@ func (s *Server) RunResourceAction(ctx context.Context, q *application.ResourceA
 		return nil, err
 	}
 
-	s.logAppEvent(a, ctx, argo.EventReasonResourceActionRan, fmt.Sprintf("ran action %s on resource %s/%s/%s", q.Action, res.Group, res.Kind, res.Name))
-	s.logResourceEvent(res, ctx, argo.EventReasonResourceActionRan, fmt.Sprintf("ran action %s", q.Action))
-
 	newObjBytes, err := json.Marshal(newObj)
 	if err != nil {
 		return nil, err
@@ -1460,13 +1457,76 @@ func (s *Server) RunResourceAction(ctx context.Context, q *application.ResourceA
 		return &application.ApplicationResponse{}, nil
 	}
 
-	_, err = s.kubectl.PatchResource(ctx, config, newObj.GroupVersionKind(), newObj.GetName(), newObj.GetNamespace(), types.MergePatchType, diffBytes)
+	// The following logic detects if the resource action makes a modification to status and/or spec.
+	// If status was modified, we attempt to patch the status using status subresource, in case the
+	// CRD is configured using the status subresource feature. See:
+	// https://kubernetes.io/docs/tasks/extend-kubernetes/custom-resources/custom-resource-definitions/#status-subresource
+	// If status subresource is in use, the patch has to be split into two:
+	// * one to update spec (and other non-status fields)
+	// * the other to update only status.
+	nonStatusPatch, statusPatch, err := splitStatusPatch(diffBytes)
 	if err != nil {
 		return nil, err
 	}
+	if statusPatch != nil {
+		_, err = s.kubectl.PatchResource(ctx, config, newObj.GroupVersionKind(), newObj.GetName(), newObj.GetNamespace(), types.MergePatchType, diffBytes, "status")
+		if err != nil {
+			if !apierr.IsNotFound(err) {
+				return nil, err
+			}
+			// K8s API server returns 404 NotFound when the CRD does not support the status subresource
+			// if we get here, the CRD does not use the status subresource. We will fall back to a normal patch
+		} else {
+			// If we get here, the CRD does use the status subresource, so we must patch status and
+			// spec separately. update the diffBytes to the spec-only patch and fall through.
+			diffBytes = nonStatusPatch
+		}
+	}
+	if diffBytes != nil {
+		_, err = s.kubectl.PatchResource(ctx, config, newObj.GroupVersionKind(), newObj.GetName(), newObj.GetNamespace(), types.MergePatchType, diffBytes)
+		if err != nil {
+			return nil, err
+		}
+	}
+
+	s.logAppEvent(a, ctx, argo.EventReasonResourceActionRan, fmt.Sprintf("ran action %s on resource %s/%s/%s", q.Action, res.Group, res.Kind, res.Name))
+	s.logResourceEvent(res, ctx, argo.EventReasonResourceActionRan, fmt.Sprintf("ran action %s", q.Action))
 	return &application.ApplicationResponse{}, nil
 }
 
+// splitStatusPatch splits a patch into two: one for a non-status patch, and the status-only patch.
+// Returns nil for either if the patch doesn't have modifications to non-status, or status, respectively.
+func splitStatusPatch(patch []byte) ([]byte, []byte, error) {
+	var obj map[string]interface{}
+	err := json.Unmarshal(patch, &obj)
+	if err != nil {
+		return nil, nil, err
+	}
+	var nonStatusPatch, statusPatch []byte
+	if statusVal, ok := obj["status"]; ok {
+		// calculate the status-only patch
+		statusObj := map[string]interface{}{
+			"status": statusVal,
+		}
+		statusPatch, err = json.Marshal(statusObj)
+		if err != nil {
+			return nil, nil, err
+		}
+		// remove status, and calculate the non-status patch
+		delete(obj, "status")
+		if len(obj) > 0 {
+			nonStatusPatch, err = json.Marshal(obj)
+			if err != nil {
+				return nil, nil, err
+			}
+		}
+	} else {
+		// status was not modified in patch
+		nonStatusPatch = patch
+	}
+	return nonStatusPatch, statusPatch, nil
+}
+
 func (s *Server) plugins() ([]*v1alpha1.ConfigManagementPlugin, error) {
 	plugins, err := s.settingsMgr.GetConfigManagementPlugins()
 	if err != nil {

server/application/application_test.go

@@ -622,3 +622,34 @@ func TestGetCachedAppState(t *testing.T) {
 		assert.Equal(t, randomError, err)
 	})
 }
+
+func TestSplitStatusPatch(t *testing.T) {
+	specPatch := `{"spec":{"aaa":"bbb"}}`
+	statusPatch := `{"status":{"ccc":"ddd"}}`
+	{
+		nonStatus, status, err := splitStatusPatch([]byte(specPatch))
+		assert.NoError(t, err)
+		assert.Equal(t, specPatch, string(nonStatus))
+		assert.Nil(t, status)
+	}
+	{
+		nonStatus, status, err := splitStatusPatch([]byte(statusPatch))
+		assert.NoError(t, err)
+		assert.Nil(t, nonStatus)
+		assert.Equal(t, statusPatch, string(status))
+	}
+	{
+		bothPatch := `{"spec":{"aaa":"bbb"},"status":{"ccc":"ddd"}}`
+		nonStatus, status, err := splitStatusPatch([]byte(bothPatch))
+		assert.NoError(t, err)
+		assert.Equal(t, specPatch, string(nonStatus))
+		assert.Equal(t, statusPatch, string(status))
+	}
+	{
+		otherFields := `{"operation":{"eee":"fff"},"spec":{"aaa":"bbb"},"status":{"ccc":"ddd"}}`
+		nonStatus, status, err := splitStatusPatch([]byte(otherFields))
+		assert.NoError(t, err)
+		assert.Equal(t, `{"operation":{"eee":"fff"},"spec":{"aaa":"bbb"}}`, string(nonStatus))
+		assert.Equal(t, statusPatch, string(status))
+	}
+}

test/e2e/app_management_test.go

@@ -1404,3 +1404,87 @@ func TestCreateFromPartialFile(t *testing.T) {
 			assert.Equal(t, []HelmParameter{{Name: "foo", Value: "foo"}}, app.Spec.Source.Helm.Parameters)
 		})
 }
+
+// Ensure actions work when using a resource action that modifies status and/or spec
+func TestCRDStatusSubresourceAction(t *testing.T) {
+	actions := `
+discovery.lua: |
+  actions = {}
+  actions["update-spec"] = {["disabled"] = false}
+  actions["update-status"] = {["disabled"] = false}
+  actions["update-both"] = {["disabled"] = false}
+  return actions
+definitions:
+- name: update-both
+  action.lua: |
+    obj.spec = {}
+    obj.spec.foo = "update-both"
+    obj.status = {}
+    obj.status.bar = "update-both"
+    return obj
+- name: update-spec
+  action.lua: |
+    obj.spec = {}
+    obj.spec.foo = "update-spec"
+    return obj
+- name: update-status
+  action.lua: |
+    obj.status = {}
+    obj.status.bar = "update-status"
+    return obj
+`
+	Given(t).
+		Path("crd-subresource").
+		And(func() {
+			SetResourceOverrides(map[string]ResourceOverride{
+				"argoproj.io/StatusSubResource": {
+					Actions: actions,
+				},
+				"argoproj.io/NonStatusSubResource": {
+					Actions: actions,
+				},
+			})
+		}).
+		When().Create().Sync().Then().
+		Expect(OperationPhaseIs(OperationSucceeded)).Expect(SyncStatusIs(SyncStatusCodeSynced)).
+		When().
+		Then().
+		// tests resource actions on a CRD using status subresource
+		And(func(app *Application) {
+			_, err := RunCli("app", "actions", "run", app.Name, "--kind", "StatusSubResource", "update-both")
+			assert.NoError(t, err)
+			text := FailOnErr(Run(".", "kubectl", "-n", app.Spec.Destination.Namespace, "get", "statussubresources", "status-subresource", "-o", "jsonpath={.spec.foo}")).(string)
+			assert.Equal(t, "update-both", text)
+			text = FailOnErr(Run(".", "kubectl", "-n", app.Spec.Destination.Namespace, "get", "statussubresources", "status-subresource", "-o", "jsonpath={.status.bar}")).(string)
+			assert.Equal(t, "update-both", text)
+
+			_, err = RunCli("app", "actions", "run", app.Name, "--kind", "StatusSubResource", "update-spec")
+			assert.NoError(t, err)
+			text = FailOnErr(Run(".", "kubectl", "-n", app.Spec.Destination.Namespace, "get", "statussubresources", "status-subresource", "-o", "jsonpath={.spec.foo}")).(string)
+			assert.Equal(t, "update-spec", text)
+
+			_, err = RunCli("app", "actions", "run", app.Name, "--kind", "StatusSubResource", "update-status")
+			assert.NoError(t, err)
+			text = FailOnErr(Run(".", "kubectl", "-n", app.Spec.Destination.Namespace, "get", "statussubresources", "status-subresource", "-o", "jsonpath={.status.bar}")).(string)
+			assert.Equal(t, "update-status", text)
+		}).
+		// tests resource actions on a CRD *not* using status subresource
+		And(func(app *Application) {
+			_, err := RunCli("app", "actions", "run", app.Name, "--kind", "NonStatusSubResource", "update-both")
+			assert.NoError(t, err)
+			text := FailOnErr(Run(".", "kubectl", "-n", app.Spec.Destination.Namespace, "get", "nonstatussubresources", "non-status-subresource", "-o", "jsonpath={.spec.foo}")).(string)
+			assert.Equal(t, "update-both", text)
+			text = FailOnErr(Run(".", "kubectl", "-n", app.Spec.Destination.Namespace, "get", "nonstatussubresources", "non-status-subresource", "-o", "jsonpath={.status.bar}")).(string)
+			assert.Equal(t, "update-both", text)
+
+			_, err = RunCli("app", "actions", "run", app.Name, "--kind", "NonStatusSubResource", "update-spec")
+			assert.NoError(t, err)
+			text = FailOnErr(Run(".", "kubectl", "-n", app.Spec.Destination.Namespace, "get", "nonstatussubresources", "non-status-subresource", "-o", "jsonpath={.spec.foo}")).(string)
+			assert.Equal(t, "update-spec", text)
+
+			_, err = RunCli("app", "actions", "run", app.Name, "--kind", "NonStatusSubResource", "update-status")
+			assert.NoError(t, err)
+			text = FailOnErr(Run(".", "kubectl", "-n", app.Spec.Destination.Namespace, "get", "nonstatussubresources", "non-status-subresource", "-o", "jsonpath={.status.bar}")).(string)
+			assert.Equal(t, "update-status", text)
+		})
+}

test/e2e/testdata/crd-subresource/crd-instances.yaml

@@ -0,0 +1,13 @@
+apiVersion: argoproj.io/v1alpha1
+kind: StatusSubResource
+metadata:
+  name: status-subresource
+spec:
+  foo: bar
+---
+apiVersion: argoproj.io/v1alpha1
+kind: NonStatusSubResource
+metadata:
+  name: non-status-subresource
+spec:
+  foo: bar

test/e2e/testdata/crd-subresource/crd.yaml

@@ -0,0 +1,25 @@
+apiVersion: apiextensions.k8s.io/v1beta1
+kind: CustomResourceDefinition
+metadata:
+  name: statussubresources.argoproj.io
+spec:
+  group: argoproj.io
+  version: v1alpha1
+  scope: Namespaced
+  names:
+    kind: StatusSubResource
+    plural: statussubresources
+  subresources:
+   status: {}
+---
+apiVersion: apiextensions.k8s.io/v1beta1
+kind: CustomResourceDefinition
+metadata:
+  name: nonstatussubresources.argoproj.io
+spec:
+  group: argoproj.io
+  version: v1alpha1
+  scope: Namespaced
+  names:
+    kind: NonStatusSubResource
+    plural: nonstatussubresources

test/e2e/testdata/crd-subresource/kustomization.yaml

@@ -0,0 +1,3 @@
+resources:
+- crd.yaml
+- crd-instances.yaml