fix(controller): Revert prepending ExecutorScriptSourcePath which brought a breaking change in args handling

[MrFreezeex]

This reverts commit 64ae330.

This PR reverts #4492 which changes the way args are passed to a script template. Due to this change we are not able to override args with a docker image and an entrypoint with an exec "$@" in argo workflow 2.12.x.

This is convenient to execute the docker entrypoint of the base image and then execute the script in the workflow. We use it to do some privileged action and then drop those privileges before executing the script passed in the workflow. You can find more information in #4782 with an example to test the behavior that brokes in 2.12.x.

This revert will obviously break the behavior that the author of this commit has intended to introduce so I don't know if reverting is the best things to do...

Resolves #4782

[alexec]

Did you author these changes originally?

[MrFreezeex]

Did you author these changes originally?

No I didn't. The original author is @ivancili in PR #4492. You can find an explanation of why this has been introduced in issue #4481. This broke our use of script template as described in this PR and in issue #4782.

[alexec]

@ivancili would you like to chip it? Would you like to propose a fix so everyone is happy?

[ghost]

@alexec @MrFreezeex In my opinion, the correct behavior of a script template should include the ability to pass arbitrary args to the script (one way, or another). The example provided in this issue seems like an incorrect usage to me, though. I would just put the [sh] into command section of the template, or prepare the entrypoint of the base image fully. Nevertheless, I see your point, and I realize my PR introduced a breaking change and it should be fixed, I apologize for the inconvenience.

Here are my proposals on how to fix this:

1. Add a flag to the script template which indicates whether args should be added before or after the script, notice the flag argsPosition in this example (name and data type of the flag can be further discussed):

apiVersion: argoproj.io/v1alpha1
kind: Workflow
metadata:
  name: test-args-script
spec:
  entrypoint: test
  templates:
  - name: test
    script:
      image: mrfreezeex/test-argow-script:v1
      args: [sh]
      argsPosition: prepend  # other option is append
      source: |
        echo "This message will be shown!"

2. Add separate field called sourceArgs, which will then allow users to specify both container args and script args at the same time, without these kinds of problems. Container args will be added before script, and sourceArgs after the script. But in this case you have to be careful in how you write your container image entrypoint/args. Example:

apiVersion: argoproj.io/v1alpha1
kind: Workflow
metadata:
  name: test-args-script
spec:
  entrypoint: test
  templates:
  - name: test
    script:
      image: mrfreezeex/test-argow-script:v1
      args: [sh]
      sourceArgs:
        - "Hello"
      source: |
        echo "This message will be shown!"
        echo $1 there                  # prints "Hello there"

3. Revert my commit and just use Go templates to parametrize the script source (as it were before)

Let me know your thoughts.

[MrFreezeex]

Let me know your thoughts.

From my point of view, I think that the sourceArgs would be the best options of the three. Kubernetes users are used to having default commands/args handling and breaking this behavior in argo workflow would be a bit weird. This also would be a non breaking change as we would add a new field with a totally different behavior.

[MrFreezeex]

I updated my PR to add sourceArgs instead of reverting the commit.

[jessesuen]

Since #4492 introduced a regression/change in behavior, it should be reverted. If there is confusion about how script templates work, we should document the behavior better rather than implement additional knobs, especially since there are alternatives to achieve the desired use case. Can we simplify this PR to just a revert of #4492?

[MrFreezeex]

Since #4492 introduced a regression/change in behavior, it should be reverted. If there is confusion about how script templates work, we should document the behavior better rather than implement additional knobs, especially since there are alternatives to achieve the desired use case. Can we simplify this PR to just a revert of #4492?

I just did that but for some reasons now a test is failing:

    --- FAIL: TestFunctionalSuite/TestDeletingRunningPod (44.63s)
Error:         functional_test.go:61: timeout after 30s waiting for condition to be done

I don't think that's something related to this code change but I retried It once already... Do you know if this test can be a bit flaky sometimes ?

[alexec]

TestDeletingRunningPod is broken

[alexec]

@ivancili we've reverted your commit because it was a breaking change that resulted in a regression. Would you like to consider suggesting a new way to do this that is non-breaking?

[henrywangx]

Hi guys @alexec @ivancili @MrFreezeex , looks like this revert broken our script

Submitted a MR #7467 to fix this issue