fix: Reduce agent permissions. Fixes #7986 (#7987)

Signed-off-by: Alex Collins <[email protected]>
argoproj · Feb 25, 2022 · 06d4bf7 · 06d4bf7
1 parent 20f7516
commit 06d4bf7
Show file tree

Hide file tree

Showing 12 changed files with 101 additions and 55 deletions.
diff --git a/docs/workflow-rbac.md b/docs/workflow-rbac.md
@@ -1,35 +1,26 @@
 # Workflow RBAC
 
-All pods in a workflow run with the service account specified in `workflow.spec.serviceAccountName`,
-or if omitted, the `default` service account of the workflow's namespace. The amount of access which
-a workflow needs is dependent on what the workflow needs to do. For example, if your workflow needs
-to deploy a resource, then the workflow's service account will require 'create' privileges on that
-resource.
+All pods in a workflow run with the service account specified in `workflow.spec.serviceAccountName`, or if omitted,
+the `default` service account of the workflow's namespace. The amount of access which a workflow needs is dependent on
+what the workflow needs to do. For example, if your workflow needs to deploy a resource, then the workflow's service
+account will require 'create' privileges on that resource.
 
-The bare minimum for a workflow to function is outlined below:
+The bare minimum for a workflow running using the Emissary executor to function is outlined below:
 
 ```yaml
 apiVersion: rbac.authorization.k8s.io/v1
 kind: Role
 metadata:
   name: workflow-role
 rules:
-# pod get/watch is used to identify the container IDs of the current pod
-# pod patch is used to annotate the step's outputs back to controller (e.g. artifact location)
-- apiGroups:
-  - ""
-  resources:
-  - pods
-  verbs:
-  - get
-  - watch
-  - patch
-# logs get/watch are used to get the pods logs for script outputs, and for log archival
-- apiGroups:
-  - ""
-  resources:
-  - pods/log
-  verbs:
-  - get
-  - watch
+  - apiGroups:
+      - ""
+    resources:
+      - pods
+    verbs:
+      - patch
 ```
+
+If you are using another executor, or using resource template, you'll need additional permissions,
+see [workflow-role](https://github.com/argoproj/argo-workflows/blob/master/manifests/quick-start/base/workflow-role.yaml)
+.
diff --git a/manifests/base/crds/full/argoproj.io_workflowtasksets.yaml b/manifests/base/crds/full/argoproj.io_workflowtasksets.yaml
@@ -7098,3 +7098,5 @@ spec:
         type: object
     served: true
     storage: true
+    subresources:
+      status: {}
diff --git a/manifests/base/crds/minimal/argoproj.io_workflowtasksets.yaml b/manifests/base/crds/minimal/argoproj.io_workflowtasksets.yaml
@@ -37,3 +37,5 @@ spec:
         type: object
     served: true
     storage: true
+    subresources:
+      status: {}
diff --git a/manifests/install.yaml b/manifests/install.yaml
@@ -203,6 +203,8 @@ spec:
         type: object
     served: true
     storage: true
+    subresources:
+      status: {}
 ---
 apiVersion: apiextensions.k8s.io/v1
 kind: CustomResourceDefinition

diff --git a/manifests/namespace-install.yaml b/manifests/namespace-install.yaml
@@ -203,6 +203,8 @@ spec:
         type: object
     served: true
     storage: true
+    subresources:
+      status: {}
 ---
 apiVersion: apiextensions.k8s.io/v1
 kind: CustomResourceDefinition

diff --git a/manifests/quick-start-minimal.yaml b/manifests/quick-start-minimal.yaml
@@ -203,6 +203,8 @@ spec:
         type: object
     served: true
     storage: true
+    subresources:
+      status: {}
 ---
 apiVersion: apiextensions.k8s.io/v1
 kind: CustomResourceDefinition
@@ -461,14 +463,19 @@ kind: Role
 metadata:
   name: workflow-role
 rules:
+- apiGroups:
+  - ""
+  resources:
+  - pods
+  verbs:
+  - patch
 - apiGroups:
   - ""
   resources:
   - pods
   verbs:
   - get
   - watch
-  - patch
 - apiGroups:
   - ""
   resources:
@@ -501,12 +508,14 @@ rules:
   - argoproj.io
   resources:
   - workflowtasksets
-  - workflowtasksets/finalizers
   verbs:
   - list
   - watch
-  - get
-  - update
+- apiGroups:
+  - argoproj.io
+  resources:
+  - workflowtasksets/status
+  verbs:
   - patch
 ---
 apiVersion: rbac.authorization.k8s.io/v1

diff --git a/manifests/quick-start-mysql.yaml b/manifests/quick-start-mysql.yaml
@@ -203,6 +203,8 @@ spec:
         type: object
     served: true
     storage: true
+    subresources:
+      status: {}
 ---
 apiVersion: apiextensions.k8s.io/v1
 kind: CustomResourceDefinition
@@ -461,14 +463,19 @@ kind: Role
 metadata:
   name: workflow-role
 rules:
+- apiGroups:
+  - ""
+  resources:
+  - pods
+  verbs:
+  - patch
 - apiGroups:
   - ""
   resources:
   - pods
   verbs:
   - get
   - watch
-  - patch
 - apiGroups:
   - ""
   resources:
@@ -501,12 +508,14 @@ rules:
   - argoproj.io
   resources:
   - workflowtasksets
-  - workflowtasksets/finalizers
   verbs:
   - list
   - watch
-  - get
-  - update
+- apiGroups:
+  - argoproj.io
+  resources:
+  - workflowtasksets/status
+  verbs:
   - patch
 ---
 apiVersion: rbac.authorization.k8s.io/v1

diff --git a/manifests/quick-start-postgres.yaml b/manifests/quick-start-postgres.yaml
@@ -203,6 +203,8 @@ spec:
         type: object
     served: true
     storage: true
+    subresources:
+      status: {}
 ---
 apiVersion: apiextensions.k8s.io/v1
 kind: CustomResourceDefinition
@@ -461,14 +463,19 @@ kind: Role
 metadata:
   name: workflow-role
 rules:
+- apiGroups:
+  - ""
+  resources:
+  - pods
+  verbs:
+  - patch
 - apiGroups:
   - ""
   resources:
   - pods
   verbs:
   - get
   - watch
-  - patch
 - apiGroups:
   - ""
   resources:
@@ -501,12 +508,14 @@ rules:
   - argoproj.io
   resources:
   - workflowtasksets
-  - workflowtasksets/finalizers
   verbs:
   - list
   - watch
-  - get
-  - update
+- apiGroups:
+  - argoproj.io
+  resources:
+  - workflowtasksets/status
+  verbs:
   - patch
 ---
 apiVersion: rbac.authorization.k8s.io/v1

diff --git a/manifests/quick-start/base/workflow-role.yaml b/manifests/quick-start/base/workflow-role.yaml
@@ -4,18 +4,23 @@ kind: Role
 metadata:
   name: workflow-role
 rules:
-  # pod get/watch is used to identify the container IDs of the current pod
+  - apiGroups:
+      - ""
+    resources:
+      - pods
+    verbs:
+      - patch
+  # pod get/watch is used to identify the container IDs of the current pod.
+  # This is not needed for the emissary executor.
   - apiGroups:
       - ""
     resources:
       - pods
     verbs:
       - get
       - watch
-      # pod patch is used to annotate the step's outputs back to controller (e.g. artifact location)
-      # This is ONLY needed if the step/task has outputs that are used by another step or task.
-      - patch
   # logs get/watch are used to get the pods logs for script outputs, and for log archival
+  # This is not needed for the emissary executor.
   - apiGroups:
       - ""
     resources:
@@ -48,16 +53,17 @@ rules:
     verbs:
       - create
       - get
-  # This allows agent pod to update the taskset.
-  # This is only needed for http templates.
+  # This allows executor to patch the taskset.
   - apiGroups:
       - argoproj.io
     resources:
       - workflowtasksets
-      - workflowtasksets/finalizers
     verbs:
       - list
       - watch
-      - get
-      - update
+  - apiGroups:
+      - argoproj.io
+    resources:
+      - workflowtasksets/status
+    verbs:
       - patch
diff --git a/pkg/apis/workflow/v1alpha1/generated.proto b/pkg/apis/workflow/v1alpha1/generated.proto
diff --git a/pkg/apis/workflow/v1alpha1/task_set_types.go b/pkg/apis/workflow/v1alpha1/task_set_types.go
@@ -7,6 +7,7 @@ import (
 // +genclient
 // +kubebuilder:resource:shortName=wfts
 // +k8s:deepcopy-gen:interfaces=k8s.io/apimachinery/pkg/runtime.Object
+// +kubebuilder:subresource:status
 type WorkflowTaskSet struct {
 	metav1.TypeMeta   `json:",inline"`
 	metav1.ObjectMeta `json:"metadata" protobuf:"bytes,1,opt,name=metadata"`

diff --git a/workflow/executor/agent.go b/workflow/executor/agent.go
@@ -15,9 +15,11 @@ import (
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/types"
 	runtimeutil "k8s.io/apimachinery/pkg/util/runtime"
+	"k8s.io/apimachinery/pkg/util/wait"
 	"k8s.io/apimachinery/pkg/watch"
 	"k8s.io/client-go/kubernetes"
 	"k8s.io/client-go/rest"
+	"k8s.io/client-go/util/retry"
 	"k8s.io/utils/pointer"
 
 	wfv1 "github.com/argoproj/argo-workflows/v3/pkg/apis/workflow/v1alpha1"
@@ -176,23 +178,33 @@ func (ae *AgentExecutor) patchWorker(ctx context.Context, taskSetInterface v1alp
 
 			ae.log.Info("Processing Patch")
 
-			_, err = taskSetInterface.Patch(ctx, ae.WorkflowName, types.MergePatchType, patch, metav1.PatchOptions{})
-			if err != nil {
-				isTransientErr := errors.IsTransientErr(err)
+			err = retry.OnError(wait.Backoff{
+				Duration: time.Second,
+				Factor:   2,
+				Jitter:   0.1,
+				Steps:    5,
+				Cap:      30 * time.Second,
+			}, errors.IsTransientErr, func() error {
+				_, err := taskSetInterface.Patch(ctx, ae.WorkflowName, types.MergePatchType, patch, metav1.PatchOptions{}, "status")
+				if apierr.IsForbidden(err) {
+					ae.log.Warn("forbidden to patch workflowtaskset/status,  falling back to less secure patching workflowtaskset, please updated your agent's RBAC")
+					_, err = taskSetInterface.Patch(ctx, ae.WorkflowName, types.MergePatchType, patch, metav1.PatchOptions{})
+				}
+				return err
+			})
+
+			if err != nil && !errors.IsTransientErr(err) {
 				ae.log.WithError(err).
-					WithField("is_transient_error", isTransientErr).
 					Error("TaskSet Patch Failed")
 
 				// If this is not a transient error, then it's likely that the contents of the patch have caused the error.
 				// To avoid a deadlock with the workflow overall, or an infinite loop, fail and propagate the error messages
 				// to the nodes.
 				// If this is a transient error, then simply do nothing and another patch will be retried in the next tick.
-				if !isTransientErr {
-					for node := range nodeResults {
-						nodeResults[node] = wfv1.NodeResult{
-							Phase:   wfv1.NodeError,
-							Message: fmt.Sprintf("HTTP request completed successfully but an error occurred when patching its result: %s", err),
-						}
+				for node := range nodeResults {
+					nodeResults[node] = wfv1.NodeResult{
+						Phase:   wfv1.NodeError,
+						Message: fmt.Sprintf("HTTP request completed successfully but an error occurred when patching its result: %s", err),
 					}
 				}
 				continue