From 56473cf0c9ccbf4f20510d1ba4053cdb8b64f9df Mon Sep 17 00:00:00 2001 From: Anton Gilgur Date: Tue, 23 Apr 2024 22:24:11 -0400 Subject: [PATCH] chore!: remove legacy `patch` `pods` fallback - the fallback is old and insecure, and the error confuses users as it's not mentioned in the docs (as it's legacy and a fallback) - it's also tech debt that we have to write code around specifically right now - it's no longer needed and hasn't been the main RBAC in a few versions, so remove it in the next minor - remove the Executor code that patches pods - remove the operator code that reads the patched annotations Signed-off-by: Anton Gilgur --- workflow/common/common.go | 9 -------- workflow/controller/controller_test.go | 14 ------------ workflow/controller/exit_handler_test.go | 21 +++++++++--------- workflow/controller/operator.go | 28 ------------------------ workflow/controller/operator_test.go | 11 +++++----- workflow/executor/executor.go | 24 +++----------------- 6 files changed, 18 insertions(+), 89 deletions(-) diff --git a/workflow/common/common.go b/workflow/common/common.go index b4f174263eaa..429972ab4477 100644 --- 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" @@ -48,13 +46,6 @@ const ( // AnnotationKeyPodNameVersion stores the pod naming convention version AnnotationKeyPodNameVersion = workflow.WorkflowFullName + "/pod-name-format" - // 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 index 0c4af5f4e6dd..e71ad75dc6ea 100644 --- a/workflow/controller/controller_test.go +++ b/workflow/controller/controller_test.go @@ -444,16 +444,6 @@ func listPods(woc *wfOperationCtx) (*apiv1.PodList, error) { type with func(pod *apiv1.Pod) -func withOutputs(v interface{}) with { - switch x := v.(type) { - case string: - return withAnnotation(common.AnnotationKeyOutputs, x) - default: - return withOutputs(wfv1.MustMarshallJSON(x)) - } -} -func withProgress(v string) with { return withAnnotation(common.AnnotationKeyProgress, v) } - func withExitCode(v int32) with { return func(pod *apiv1.Pod) { for _, c := range pod.Spec.Containers { @@ -469,10 +459,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) { diff --git a/workflow/controller/exit_handler_test.go b/workflow/controller/exit_handler_test.go index 4f6b989b1a35..f4131d37a735 100644 --- a/workflow/controller/exit_handler_test.go +++ b/workflow/controller/exit_handler_test.go @@ -8,7 +8,6 @@ import ( "github.com/stretchr/testify/assert" 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" ) @@ -24,7 +23,7 @@ spec: steps: - - name: leafA hooks: - exit: + exit: template: exitContainer arguments: parameters: @@ -33,7 +32,7 @@ spec: template: whalesay - - name: leafB hooks: - exit: + exit: template: exitContainer arguments: parameters: @@ -92,7 +91,7 @@ spec: tasks: - name: leafA hooks: - exit: + exit: template: exitContainer arguments: parameters: @@ -102,7 +101,7 @@ spec: - name: leafB dependencies: [leafA] hooks: - exit: + exit: template: exitContainer arguments: parameters: @@ -160,7 +159,7 @@ spec: steps: - - name: leafA hooks: - exit: + exit: template: exitContainer arguments: artifacts: @@ -236,7 +235,7 @@ spec: tasks: - name: leafA hooks: - exit: + exit: template: exitContainer arguments: artifacts: @@ -314,7 +313,7 @@ spec: template: whalesay - - name: leafB hooks: - exit: + exit: template: exitContainer arguments: parameters: @@ -356,7 +355,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) @@ -419,7 +418,7 @@ spec: - name: leafB dependencies: [leafA] hooks: - exit: + exit: template: exitContainer arguments: parameters: @@ -461,7 +460,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 index 6557236cbd8e..0c595cd0e8f3 100644 --- a/workflow/controller/operator.go +++ b/workflow/controller/operator.go @@ -1405,40 +1405,12 @@ func (woc *wfOperationCtx) assessNodeStatus(pod *apiv1.Pod, old *wfv1.NodeStatus new.PodIP = pod.Status.PodIP } - 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) - } - } - - if x, ok := pod.Annotations[common.AnnotationKeyOutputs]; ok { - woc.log.Warn("workflow uses legacy/insecure pod patch, see https://argo-workflows.readthedocs.io/en/latest/workflow-rbac/") - 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() - } - } - new.HostNodeName = pod.Spec.NodeName if !new.Progress.IsValid() { new.Progress = wfv1.ProgressDefault } - if x, ok := pod.Annotations[common.AnnotationKeyProgress]; ok { - woc.log.Warn("workflow uses legacy/insecure pod patch, see https://argo-workflows.readthedocs.io/en/latest/workflow-rbac/") - if p, ok := wfv1.ParseProgress(x); ok { - new.Progress = p - } - } - // We capture the exit-code after we look for the task-result. // All other outputs are set by the executor, only the exit-code is set by the controller. // By waiting, we avoid breaking the race-condition check. diff --git a/workflow/controller/operator_test.go b/workflow/controller/operator_test.go index 91992b56d4f3..6435c66f73d1 100644 --- a/workflow/controller/operator_test.go +++ b/workflow/controller/operator_test.go @@ -291,7 +291,7 @@ spec: woc := newWorkflowOperationCtx(wf, controller) woc.operate(ctx) - makePodsPhase(ctx, woc, apiv1.PodRunning, withProgress("50/100")) + makePodsPhase(ctx, woc, apiv1.PodRunning) woc = newWorkflowOperationCtx(woc.wf, controller) woc.operate(ctx) @@ -301,7 +301,7 @@ spec: pod := woc.wf.Status.Nodes.FindByDisplayName("pod") assert.Equal(t, wfv1.Progress("50/100"), pod.Progress) - makePodsPhase(ctx, woc, apiv1.PodSucceeded, withProgress("100/100")) + makePodsPhase(ctx, woc, apiv1.PodSucceeded) woc = newWorkflowOperationCtx(woc.wf, controller) woc.operate(ctx) @@ -6175,7 +6175,7 @@ func TestConfigMapCacheSaveOperate(t *testing.T) { ctx := context.Background() woc.operate(ctx) - makePodsPhase(ctx, woc, apiv1.PodSucceeded, withExitCode(0), withOutputs(wfv1.MustMarshallJSON(sampleOutputs))) + makePodsPhase(ctx, woc, apiv1.PodSucceeded, withExitCode(0)) woc = newWorkflowOperationCtx(woc.wf, controller) woc.operate(ctx) @@ -6465,7 +6465,7 @@ spec: assert.Equal(t, wfv1.WorkflowRunning, woc.wf.Status.Phase) // make all created pods as successful - makePodsPhase(ctx, woc, apiv1.PodSucceeded, withOutputs(`{"parameters": [{"name": "my-param"}]}`)) + makePodsPhase(ctx, woc, apiv1.PodSucceeded) // reconcile woc = newWorkflowOperationCtx(woc.wf, controller) @@ -8439,7 +8439,6 @@ func TestWFGlobalArtifactNil(t *testing.T) { makePodsPhase(ctx, woc, apiv1.PodRunning) woc.operate(ctx) makePodsPhase(ctx, woc, apiv1.PodFailed, func(pod *apiv1.Pod) { - pod.Annotations[common.AnnotationKeyOutputs] = string("{\"parameters\":[{\"name\":\"hello-param\",\"valueFrom\":{\"path\":\"/tmp/hello_world.txt\"},\"globalName\":\"my-global-param\"}],\"artifacts\":[{\"name\":\"hello-art\",\"path\":\"/tmp/hello_world.txt\",\"globalName\":\"my-global-art\"}]}") pod.Status.ContainerStatuses = []apiv1.ContainerStatus{ { Name: "main", @@ -10661,7 +10660,7 @@ spec: script: image: python:alpine3.6 command: [python] - env: + env: - name: message value: "{{inputs.parameters.message}}" source: | diff --git a/workflow/executor/executor.go b/workflow/executor/executor.go index 07cfe5d6b32f..393fd6fbbec0 100644 --- a/workflow/executor/executor.go +++ b/workflow/executor/executor.go @@ -799,9 +799,7 @@ func (we *WorkflowExecutor) FinalizeOutput(ctx context.Context) { common.LabelKeyReportOutputsCompleted: "true", }) if apierr.IsForbidden(err) || apierr.IsNotFound(err) { - log.WithError(err).Warn("failed to patch task result, falling back to legacy/insecure pod patch, see https://argo-workflows.readthedocs.io/en/latest/workflow-rbac/") - // Only added as a backup in case LabelKeyReportOutputsCompleted could not be set - err = we.AddAnnotation(ctx, common.AnnotationKeyReportOutputsCompleted, "true") + log.WithError(err).Warn("failed to patch task result, see https://argo-workflows.readthedocs.io/en/latest/workflow-rbac/") } return err }) @@ -820,9 +818,7 @@ func (we *WorkflowExecutor) InitializeOutput(ctx context.Context) { }, errorsutil.IsTransientErr, func() error { err := we.upsertTaskResult(ctx, wfv1.NodeResult{}) if apierr.IsForbidden(err) { - log.WithError(err).Warn("failed to patch task result, falling back to legacy/insecure pod patch, see https://argo-workflows.readthedocs.io/en/latest/workflow-rbac/") - // Only added as a backup in case LabelKeyReportOutputsCompleted could not be set - err = we.AddAnnotation(ctx, common.AnnotationKeyReportOutputsCompleted, "false") + log.WithError(err).Warn("failed to patch task result, see https://argo-workflows.readthedocs.io/en/latest/workflow-rbac/") } return err }) @@ -848,22 +844,8 @@ func (we *WorkflowExecutor) reportResult(ctx context.Context, result wfv1.NodeRe }, errorsutil.IsTransientErr, func() error { err := we.upsertTaskResult(ctx, result) if apierr.IsForbidden(err) { - log.WithError(err).Warn("failed to patch task result, falling back to legacy/insecure pod patch, see https://argo-workflows.readthedocs.io/en/latest/workflow-rbac/") - if result.Outputs.HasOutputs() { - value, err := json.Marshal(result.Outputs) - if err != nil { - return err - } - - return we.AddAnnotation(ctx, common.AnnotationKeyOutputs, string(value)) - } - if result.Progress.IsValid() { // this may result in occasionally two patches - return we.AddAnnotation(ctx, common.AnnotationKeyProgress, string(result.Progress)) - } - // Only added as a backup in case LabelKeyReportOutputsCompleted could not be set - return we.AddAnnotation(ctx, common.AnnotationKeyReportOutputsCompleted, "false") + log.WithError(err).Warn("failed to patch task result, see https://argo-workflows.readthedocs.io/en/latest/workflow-rbac/") } - return err }) }