chore!: remove legacy patch pods fallback (#13100)

Signed-off-by: Anton Gilgur <[email protected]> Signed-off-by: Garett MacGowan <[email protected]> Co-authored-by: Anton Gilgur <[email protected]> Co-authored-by: Alan Clucas <[email protected]>
argoproj · Sep 14, 2024 · 31493d3 · 31493d3
1 parent 8065edb
commit 31493d3
Show file tree

Hide file tree

Showing 25 changed files with 115 additions and 134 deletions.
diff --git a/docs/upgrading.md b/docs/upgrading.md
@@ -15,6 +15,10 @@ Previously it was `--basehref` (no dash in between) and `ARGO_BASEHREF` (no unde
 `ALLOWED_LINK_PROTOCOL` and `BASE_HREF` have been removed as redundant.
 Use `ARGO_ALLOWED_LINK_PROTOCOL` and `ARGO_BASE_HREF` instead.
 
+### Legacy insecure pod patch fallback removed. ([#13100](https://github.com/argoproj/argo-workflows/pull/13100))
+
+For the Emissary executor to work properly, you must set up RBAC. See [workflow RBAC](workflow-rbac.md)
+
 ### Metrics changes
 
 You can now retrieve metrics using the OpenTelemetry Protocol using the [OpenTelemetry collector](https://opentelemetry.io/docs/collector/), and this is the recommended mechanism.

diff --git a/docs/workflow-templates.md b/docs/workflow-templates.md
@@ -138,7 +138,6 @@ kind: WorkflowTemplate
 metadata:
   name: hello-world-template-global-arg
 spec:
-  serviceAccountName: argo
   templates:
     - name: hello-world
       container:
@@ -151,7 +150,6 @@ kind: Workflow
 metadata:
   generateName: hello-world-wf-global-arg-
 spec:
-  serviceAccountName: argo
   entrypoint: print-message
   arguments:
     parameters:

diff --git a/examples/arguments-parameters-from-configmap.yaml b/examples/arguments-parameters-from-configmap.yaml
@@ -11,7 +11,6 @@ metadata:
     workflows.argoproj.io/verify.py: |
       assert status["phase"] == "Succeeded"
 spec:
-  serviceAccountName: argo
   entrypoint: print-message-from-configmap
 
   templates:

diff --git a/examples/global-parameters-from-configmap-referenced-as-local-variable.yaml b/examples/global-parameters-from-configmap-referenced-as-local-variable.yaml
@@ -9,7 +9,6 @@ metadata:
       This example demonstrates how a global parameter from a ConfigMap can be referenced as a template local variable.
       Note that the "simple-parameters" ConfigMap (defined in `examples/configmaps/simple-parameters-configmap.yaml`) needs to be created first before submitting this workflow.
 spec:
-  serviceAccountName: argo
   entrypoint: print-message
   arguments:
     parameters:

diff --git a/examples/global-parameters-from-configmap.yaml b/examples/global-parameters-from-configmap.yaml
@@ -9,7 +9,6 @@ metadata:
       This example demonstrates loading global parameter values from a ConfigMap.
       Note that the "simple-parameters" ConfigMap (defined in `examples/configmaps/simple-parameters-configmap.yaml`) needs to be created first before submitting this workflow.
 spec:
-  serviceAccountName: argo
   entrypoint: print-message
   # Parameters can also be passed via configmap reference.
   arguments:

diff --git a/manifests/namespace-install/workflow-controller-rbac/workflow-controller-rolebinding.yaml b/manifests/namespace-install/workflow-controller-rbac/workflow-controller-rolebinding.yaml
@@ -7,6 +7,6 @@ roleRef:
   kind: Role
   name: argo-role
 subjects:
-- kind: ServiceAccount
-  name: argo
+  - kind: ServiceAccount
+    name: argo
 
diff --git a/manifests/quick-start/base/workflow-default-rolebinding.yaml b/manifests/quick-start/base/workflow-default-rolebinding.yaml
diff --git a/test/e2e/manifests/minimal/kustomization.yaml b/test/e2e/manifests/minimal/kustomization.yaml
@@ -9,6 +9,7 @@ resources:
 - ../mixins/argo-workflows-agent-ca-certificates.yaml
 - ../mixins/argo-server.service-account-token-secret.yaml
 - ../mixins/argo.service-account-token-secret.yaml
+- ../mixins/get-cm-rbac.yaml
 
 patches:
 - path: ../mixins/argo-server-deployment.yaml

diff --git a/test/e2e/manifests/mixins/get-cm-rbac.yaml b/test/e2e/manifests/mixins/get-cm-rbac.yaml
@@ -0,0 +1,49 @@
+apiVersion: v1
+kind: ServiceAccount
+metadata:
+  name: get-cm
+---
+apiVersion: v1
+kind: Secret
+metadata:
+  name: get-cm.service-account-token
+  annotations:
+    kubernetes.io/service-account.name: get-cm
+type: kubernetes.io/service-account-token
+---
+apiVersion: rbac.authorization.k8s.io/v1
+kind: ClusterRole
+metadata:
+  name: get-cm
+rules:
+- apiGroups:
+  - ""
+  resources:
+  - configmaps
+  verbs:
+  - get
+  - list
+---
+apiVersion: rbac.authorization.k8s.io/v1
+kind: ClusterRoleBinding
+metadata:
+  name: get-cm-get-cm
+roleRef:
+  apiGroup: rbac.authorization.k8s.io
+  kind: ClusterRole
+  name: get-cm
+subjects:
+  - kind: ServiceAccount
+    name: get-cm
+---
+apiVersion: rbac.authorization.k8s.io/v1
+kind: RoleBinding
+metadata:
+  name: executor-get-cm
+roleRef:
+  apiGroup: rbac.authorization.k8s.io
+  kind: Role
+  name: executor
+subjects:
+- kind: ServiceAccount
+  name: get-cm
diff --git a/test/e2e/resource_template_test.go b/test/e2e/resource_template_test.go
@@ -64,11 +64,9 @@ kind: Workflow
 metadata:
   generateName: k8s-resource-tmpl-with-pod-
 spec:
-  serviceAccount: argo
   entrypoint: main
   templates:
     - name: main
-      serviceAccountName: argo
       resource:
         action: create
         setOwnerReference: true
@@ -80,7 +78,6 @@ spec:
           metadata:
             generateName: k8s-pod-resource-
           spec:
-            serviceAccountName: argo
             containers:
             - name: argosay-container
               image: argoproj/argosay:v2
@@ -104,11 +101,9 @@ kind: Workflow
 metadata:
   generateName: k8s-resource-tmpl-with-artifact-
 spec:
-  serviceAccount: argo
   entrypoint: main
   templates:
     - name: main
-      serviceAccountName: argo
       inputs:
         artifacts:
         - name: manifest
@@ -120,7 +115,6 @@ spec:
               metadata:
                 generateName: k8s-pod-resource-
               spec:
-                serviceAccountName: argo
                 containers:
                 - name: argosay-container
                   image: argoproj/argosay:v2

diff --git a/test/e2e/workflow_configmap_substitution_test.go b/test/e2e/workflow_configmap_substitution_test.go
@@ -194,7 +194,6 @@ metadata:
   label:
     workflows.argoproj.io/test: "true"
 spec:
-  serviceAccountName: argo
   entrypoint: whalesay
   arguments:
     parameters:

diff --git a/test/e2e/workflow_inputs_orverridable_test.go b/test/e2e/workflow_inputs_orverridable_test.go
@@ -27,7 +27,6 @@ metadata:
   label:
     workflows.argoproj.io/test: "true"
 spec:
-  serviceAccountName: argo
   entrypoint: whalesay
   arguments:
     parameters:
@@ -73,7 +72,6 @@ metadata:
   label:
     workflows.argoproj.io/test: "true"
 spec:
-  serviceAccountName: argo
   entrypoint: whalesay
   arguments:
     parameters:
@@ -127,7 +125,6 @@ metadata:
   label:
     workflows.argoproj.io/test: "true"
 spec:
-  serviceAccountName: argo
   entrypoint: whalesay
   arguments:
     parameters:
@@ -164,7 +161,6 @@ metadata:
   label:
     workflows.argoproj.io/test: "true"
 spec:
-  serviceAccountName: argo
   entrypoint: whalesay
   arguments:
     parameters:

diff --git a/test/e2e/workflow_test.go b/test/e2e/workflow_test.go
@@ -33,7 +33,7 @@ spec:
   serviceAccountName: argo
   automountServiceAccountToken: false
   executor:
-    serviceAccountName: argo 
+    serviceAccountName: get-cm
   entrypoint: main
   templates:
     - name: main
@@ -67,7 +67,7 @@ spec:
   serviceAccountName: argo
   automountServiceAccountToken: false
   executor:
-    serviceAccountName: argo
+    serviceAccountName: get-cm
   entrypoint: main
   templates:
     - name: main

diff --git a/workflow/common/common.go b/workflow/common/common.go
@@ -34,8 +34,6 @@ const (
 	AnnotationKeyRBACRule           = workflow.WorkflowFullName + "/rbac-rule"
 	AnnotationKeyRBACRulePrecedence = workflow.WorkflowFullName + "/rbac-rule-precedence"
 
-	// AnnotationKeyOutputs is the pod metadata annotation key containing the container outputs
-	AnnotationKeyOutputs = workflow.WorkflowFullName + "/outputs"
 	// AnnotationKeyCronWfScheduledTime is the workflow metadata annotation key containing the time when the workflow
 	// was scheduled to run by CronWorkflow.
 	AnnotationKeyCronWfScheduledTime = workflow.WorkflowFullName + "/scheduled-time"
@@ -51,10 +49,6 @@ const (
 	// AnnotationKeyProgress is N/M progress for the node
 	AnnotationKeyProgress = workflow.WorkflowFullName + "/progress"
 
-	// AnnotationKeyReportOutputsCompleted is an annotation on a workflow pod indicating outputs have completed.
-	// Only used as a backup in case LabelKeyReportOutputsCompleted can't be added to WorkflowTaskResult.
-	AnnotationKeyReportOutputsCompleted = workflow.WorkflowFullName + "/report-outputs-completed"
-
 	// AnnotationKeyArtifactGCStrategy is listed as an annotation on the Artifact GC Pod to identify
 	// the strategy whose artifacts are being deleted
 	AnnotationKeyArtifactGCStrategy = workflow.WorkflowFullName + "/artifact-gc-strategy"

diff --git a/workflow/controller/controller_test.go b/workflow/controller/controller_test.go
@@ -27,6 +27,7 @@ import (
 
 	"github.com/argoproj/argo-workflows/v3/config"
 	"github.com/argoproj/argo-workflows/v3/persist/sqldb"
+	"github.com/argoproj/argo-workflows/v3/pkg/apis/workflow"
 	wfv1 "github.com/argoproj/argo-workflows/v3/pkg/apis/workflow/v1alpha1"
 	fakewfclientset "github.com/argoproj/argo-workflows/v3/pkg/client/clientset/versioned/fake"
 	"github.com/argoproj/argo-workflows/v3/pkg/client/clientset/versioned/scheme"
@@ -447,20 +448,42 @@ func listPods(woc *wfOperationCtx) (*apiv1.PodList, error) {
 	return woc.controller.kubeclientset.CoreV1().Pods(woc.wf.Namespace).List(context.Background(), metav1.ListOptions{})
 }
 
-type with func(pod *apiv1.Pod)
+type with func(pod *apiv1.Pod, woc *wfOperationCtx)
 
-func withOutputs(v interface{}) with {
-	switch x := v.(type) {
-	case string:
-		return withAnnotation(common.AnnotationKeyOutputs, x)
-	default:
-		return withOutputs(wfv1.MustMarshallJSON(x))
+func withOutputs(outputs wfv1.Outputs) with {
+	return func(pod *apiv1.Pod, woc *wfOperationCtx) {
+		nodeId := woc.nodeID(pod)
+		taskResult := &wfv1.WorkflowTaskResult{
+			TypeMeta: metav1.TypeMeta{
+				APIVersion: workflow.APIVersion,
+				Kind:       workflow.WorkflowTaskResultKind,
+			},
+			ObjectMeta: metav1.ObjectMeta{
+				Name: nodeId,
+				Labels: map[string]string{
+					common.LabelKeyWorkflow:               woc.wf.Name,
+					common.LabelKeyReportOutputsCompleted: "true",
+				},
+			},
+			NodeResult: wfv1.NodeResult{
+				Phase:   wfv1.NodeSucceeded,
+				Outputs: &outputs,
+			},
+		}
+		_, err := woc.controller.wfclientset.ArgoprojV1alpha1().WorkflowTaskResults(woc.wf.Namespace).
+			Create(
+				context.Background(),
+				taskResult,
+				metav1.CreateOptions{},
+			)
+		if err != nil {
+			panic(err)
+		}
 	}
 }
-func withProgress(v string) with { return withAnnotation(common.AnnotationKeyProgress, v) }
 
 func withExitCode(v int32) with {
-	return func(pod *apiv1.Pod) {
+	return func(pod *apiv1.Pod, _ *wfOperationCtx) {
 		for _, c := range pod.Spec.Containers {
 			pod.Status.ContainerStatuses = append(pod.Status.ContainerStatuses, apiv1.ContainerStatus{
 				Name: c.Name,
@@ -474,10 +497,6 @@ func withExitCode(v int32) with {
 	}
 }
 
-func withAnnotation(key, val string) with {
-	return func(pod *apiv1.Pod) { pod.Annotations[key] = val }
-}
-
 // createRunningPods creates the pods that are marked as running in a given test so that they can be accessed by the
 // pod assessor
 func createRunningPods(ctx context.Context, woc *wfOperationCtx) {
@@ -532,7 +551,7 @@ func makePodsPhase(ctx context.Context, woc *wfOperationCtx, phase apiv1.PodPhas
 				pod.Status.Message = "Pod failed"
 			}
 			for _, w := range with {
-				w(&pod)
+				w(&pod, woc)
 			}
 			updatedPod, err := podcs.Update(ctx, &pod, metav1.UpdateOptions{})
 			if err != nil {

diff --git a/workflow/controller/exit_handler_test.go b/workflow/controller/exit_handler_test.go
@@ -9,7 +9,6 @@ import (
 	"github.com/stretchr/testify/require"
 	apiv1 "k8s.io/api/core/v1"
 	v1 "k8s.io/apimachinery/pkg/apis/meta/v1"
-	"k8s.io/utils/pointer"
 
 	wfv1 "github.com/argoproj/argo-workflows/v3/pkg/apis/workflow/v1alpha1"
 )
@@ -357,7 +356,7 @@ func TestStepsTmplOnExit(t *testing.T) {
 	woc := newWorkflowOperationCtx(wf, controller)
 	woc.operate(ctx)
 	assert.Equal(t, wfv1.WorkflowRunning, woc.wf.Status.Phase)
-	makePodsPhase(ctx, woc, apiv1.PodSucceeded, withOutputs(wfv1.Outputs{Result: pointer.String("ok"), Parameters: []wfv1.Parameter{{}}}))
+	makePodsPhase(ctx, woc, apiv1.PodSucceeded)
 	woc1 := newWorkflowOperationCtx(woc.wf, controller)
 	woc1.operate(ctx)
 	assert.Equal(t, wfv1.WorkflowRunning, woc1.wf.Status.Phase)
@@ -462,7 +461,7 @@ func TestDAGOnExit(t *testing.T) {
 	woc := newWorkflowOperationCtx(wf, controller)
 	woc.operate(ctx)
 	assert.Equal(t, wfv1.WorkflowRunning, woc.wf.Status.Phase)
-	makePodsPhase(ctx, woc, apiv1.PodSucceeded, withOutputs(wfv1.Outputs{Parameters: []wfv1.Parameter{{}}}))
+	makePodsPhase(ctx, woc, apiv1.PodSucceeded)
 	woc1 := newWorkflowOperationCtx(woc.wf, controller)
 	woc1.operate(ctx)
 	assert.Equal(t, wfv1.WorkflowRunning, woc1.wf.Status.Phase)

diff --git a/workflow/controller/operator.go b/workflow/controller/operator.go
@@ -1427,35 +1427,6 @@ func (woc *wfOperationCtx) assessNodeStatus(ctx context.Context, pod *apiv1.Pod,
 		new.PodIP = pod.Status.PodIP
 	}
 
-	// If `AnnotationKeyReportOutputsCompleted` is set, it means RBAC prevented WorkflowTaskResult from being written.
-	if x, ok := pod.Annotations[common.AnnotationKeyReportOutputsCompleted]; ok {
-		woc.log.Warn("workflow uses legacy/insecure pod patch, see https://argo-workflows.readthedocs.io/en/latest/workflow-rbac/")
-		resultName := woc.nodeID(pod)
-		if x == "true" {
-			woc.wf.Status.MarkTaskResultComplete(resultName)
-		} else {
-			woc.wf.Status.MarkTaskResultIncomplete(resultName)
-		}
-
-		// Get node outputs from pod annotations instead if RBAC prevented WorkflowTaskResult from being written.
-		if x, ok = pod.Annotations[common.AnnotationKeyOutputs]; ok {
-			if new.Outputs == nil {
-				new.Outputs = &wfv1.Outputs{}
-			}
-			if err := json.Unmarshal([]byte(x), new.Outputs); err != nil {
-				new.Phase = wfv1.NodeError
-				new.Message = err.Error()
-			}
-		}
-
-		// Get node progress from pod annotations instead if RBAC prevented WorkflowTaskResult from being written.
-		if x, ok = pod.Annotations[common.AnnotationKeyProgress]; ok {
-			if p, ok := wfv1.ParseProgress(x); ok {
-				new.Progress = p
-			}
-		}
-	}
-
 	new.HostNodeName = pod.Spec.NodeName
 
 	if !new.Progress.IsValid() {

diff --git a/workflow/controller/operator_concurrency_test.go b/workflow/controller/operator_concurrency_test.go
@@ -1118,7 +1118,7 @@ spec:
 	assert.True(t, job1AcquiredLock)
 
 	// Make job-1's pod succeed
-	makePodsPhase(ctx, woc, apiv1.PodSucceeded, func(pod *apiv1.Pod) {
+	makePodsPhase(ctx, woc, apiv1.PodSucceeded, func(pod *apiv1.Pod, _ *wfOperationCtx) {
 		if pod.ObjectMeta.Name == "job-1" {
 			pod.Status.Phase = apiv1.PodSucceeded
 		}