fix(executor/emissary): fix nonroot sidecars + input/output params & artifacts #6403

Bobgy · 2021-07-22T10:55:11Z

This fully fixes emissary + nonroot.
I've tested sidecars, input/output parameters and artifacts

Checklist:

My organization is added to USERS.md.

Tips:

Your PR needs to pass the required checks before it can be approved. If the check is not required (e.g. E2E tests) it does not need to pass
Sign-off your commits to pass the DCO check: git commit --signoff.
Run make pre-commit -B to fix codegen or lint problems.
Say how how you tested your changes. If you changed the UI, attach screenshots.

…rams & artifacts Signed-off-by: Yuan Gong <[email protected]>

Bobgy · 2021-07-22T10:55:33Z

cmd/argoexec/commands/emissary.go

- if err := os.MkdirAll(varRunArgo+"/ctr/"+containerName, 0o700); err != nil {
+ // default umask can be 022
+ // setting umask as 0 allow granting write access to other non-root users
+ syscall.Umask(0)


Should I put this into os_specific?

syscall.Umask(0) is not available on Windows. So you'll need to move this command into os_specific. The Windows implementation can be a no-op.

codecov · 2021-07-22T10:59:09Z

Codecov Report

Merging #6403 (2ca568a) into master (4da8fd9) will decrease coverage by 0.01%.
The diff coverage is 25.00%.

@@            Coverage Diff             @@
##           master    #6403      +/-   ##
==========================================
- Coverage   48.81%   48.80%   -0.02%     
==========================================
  Files         253      253              
  Lines       18153    18155       +2     
==========================================
- Hits         8861     8860       -1     
- Misses       8324     8327       +3     
  Partials      968      968

Impacted Files	Coverage Δ
workflow/executor/emissary/emissary.go	`0.00% <0.00%> (ø)`
cmd/argoexec/commands/emissary.go	`50.35% <40.00%> (+0.35%)`	⬆️
workflow/metrics/server.go	`15.78% <0.00%> (-3.51%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4da8fd9...2ca568a. Read the comment docs.

Bobgy · 2021-07-22T10:59:19Z

Hi @alexec, I wonder whether you have any docs or examples for how to add workflows as tests?
I think it'll be a lot more confident checking in workflows to verify the fix and make sure we don't break it in the future.

Bobgy · 2021-07-22T11:00:59Z

nonroot sidecar example:

#### sidecar example ####
apiVersion: argoproj.io/v1alpha1
kind: Workflow
metadata:
  generateName: nonroot-sidecar-
spec:
  serviceAccountName: pipeline-runner
  entrypoint: sidecar-example
  templates:
  - name: sidecar-example
    container:
      image: alpine:latest
      command: [sh, -c]
      args: ["
        apk update &&
        apk add curl &&
        until curl 'http:https://127.0.0.1:8080'; do sleep 5; done &&
      "]
    sidecars:
    - name: http-echo
      image: mendhak/http-https-echo:19
      command: ['docker-entrypoint.sh']
      args: ['node', './index.js']
      envs:
      - name: HTTP_PORT
        value: 8080

nonroot parameter passing

#### parameter passing test workflow for non-root ####
# Output parameters provide a way to use the contents of a file,
# as a parameter value in a workflow. In that regard, they are
# similar in concept to script templates, with the difference being
# that the output parameter values are obtained via file contents
# instead of stdout (as with script templates). Secondly, there can
# be multiple 'output.parameters.xxx' in a single template, versus
# a single 'output.result' from a script template.
# 
# In this example, the 'whalesay' template produces an output
# parameter named 'hello-param', taken from the file contents of
# /tmp/hello_world.txt. This parameter is passed to a subsequent
# step as an input parameter to the template, 'print-message'.
apiVersion: argoproj.io/v1alpha1
kind: Workflow
metadata:
  generateName: nonroot-parameter-
spec:
  serviceAccountName: pipeline-runner
  entrypoint: output-parameter
  templates:
  - name: output-parameter
    steps:
    - - name: generate-parameter
        template: whalesay
    - - name: consume-parameter
        template: print-message
        arguments:
          parameters:
          - name: message
            value: "{{steps.generate-parameter.outputs.parameters.hello-param}}"

  - name: whalesay
    container:
      image: curlimages/curl:latest
      command: [sh, -c]
      args: ["sleep 1; echo -n hello world > /tmp/hello_world.txt"]
    outputs:
      parameters:
      - name: hello-param
        valueFrom:
          default: "Foobar"   # Default value to use if retrieving valueFrom fails. If not provided workflow will fail instead
          path: /tmp/hello_world.txt

  - name: print-message
    inputs:
      parameters:
      - name: message
    container:
      image: curlimages/curl:latest
      command: [sh, -c]
      args: ["echo {{inputs.parameters.message}}"]

non root hello world workflow:

#### one step test workflow for non-root ####
apiVersion: argoproj.io/v1alpha1
kind: Workflow
metadata:
  generateName: nonroot-helloworld-
spec:
  serviceAccountName: pipeline-runner
  entrypoint: hw
  templates:
  - name: hw
    container:
      image: curlimages/curl:latest
      command: [sh, -c, "echo hello"]

nonroot artifact passing workflow:

#### artifact passing test workflow for non-root ####
# This example demonstrates the ability to pass artifacts
# from one step to the next.

apiVersion: argoproj.io/v1alpha1
kind: Workflow
metadata:
  generateName: nonroot-artifact-
spec:
  serviceAccountName: pipeline-runner
  entrypoint: artifact-example
  templates:
  - name: artifact-example
    steps:
    - - name: generate-artifact
        template: whalesay
    - - name: consume-artifact
        template: print-message
        arguments:
          artifacts:
          - name: message
            from: "{{steps.generate-artifact.outputs.artifacts.hello-art}}"

  - name: whalesay
    container:
      image: curlimages/curl:latest
      command: [sh, -c]
      args: ["sleep 1; echo hello world | tee /tmp/hello_world.txt"]
    outputs:
      artifacts:
      - name: hello-art
        path: /tmp/hello_world.txt

  - name: print-message
    inputs:
      artifacts:
      - name: message
        path: /tmp/message
    container:
      image: curlimages/curl:latest
      command: [sh, -c]
      args: ["cat /tmp/message"]

alexec

great stuff! you're correct about Umask - this needs to go into os_specific

alexec · 2021-07-22T16:28:02Z

cmd/argoexec/commands/emissary.go

- if err := os.MkdirAll(varRunArgo+"/ctr/"+containerName, 0o700); err != nil {
+ // default umask can be 022
+ // setting umask as 0 allow granting write access to other non-root users
+ syscall.Umask(0)


syscall.Umask(0) is not available on Windows. So you'll need to move this command into os_specific. The Windows implementation can be a no-op.

Signed-off-by: Yuan Gong <[email protected]>

Bobgy · 2021-07-24T08:38:29Z

@alexec thanks for the review! Updated to os specific

Bobgy · 2021-07-24T08:39:01Z

Any guidance for this?

Hi @alexec, I wonder whether you have any docs or examples for how to add workflows as tests?
I think it'll be a lot more confident checking in workflows to verify the fix and make sure we don't break it in the future.

alexec · 2021-07-25T00:18:05Z

workflow/executor/os-specific/umask_darwin.go

+
+import "syscall"
+
+func CallUmask(mask int) (oldmask int) {


minor - you ignore the return value and you only ever pass in zero - so how about:

func UmaskZero() { // ... }

What might be even better is name it after you intent.

GrantWriteAccessToNonRootUsers

??

What do you think about AllowGrantingAccessToEveryone?

OK, updated

Signed-off-by: Yuan Gong <[email protected]>

alexec · 2021-07-26T15:38:28Z

@sarabala1979 this needs to be back-ported to v3.1. Do you think we should do another patch release this week?

…artifacts (#6403) Signed-off-by: Yuan Gong <[email protected]>

fix(executor): emissary works with nonroot sidecars + input/output pa…

09eae73

…rams & artifacts Signed-off-by: Yuan Gong <[email protected]>

Bobgy commented Jul 22, 2021

View reviewed changes

alexec requested changes Jul 22, 2021

View reviewed changes

alexec added this to the v3.1 milestone Jul 22, 2021

move umask to os specific package

dd0f6a8

Signed-off-by: Yuan Gong <[email protected]>

Bobgy changed the title ~~fix(executor): emissary works with nonroot sidecars + input/output params & artifacts~~ fix(executor/emissary): fix nonroot sidecars + input/output params & artifacts Jul 24, 2021

alexec reviewed Jul 25, 2021

View reviewed changes

address feedback

ff60f4d

Signed-off-by: Yuan Gong <[email protected]>

alexec approved these changes Jul 26, 2021

View reviewed changes

Merge branch 'master' into emissary-nonroot-all

2ca568a

alexec enabled auto-merge (squash) July 26, 2021 15:37

alexec merged commit 30e2518 into argoproj:master Jul 26, 2021

sarabala1979 mentioned this pull request Jul 27, 2021

v3.1.3 Cherry-pick #6435

Closed

39 tasks

sarabala1979 pushed a commit that referenced this pull request Jul 27, 2021

fix(executor/emissary): fix nonroot sidecars + input/output params & …

1eaa381

…artifacts (#6403) Signed-off-by: Yuan Gong <[email protected]>

sarabala1979 mentioned this pull request Aug 2, 2021

v3.1.4 cherry-pick #6468

Closed

49 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(executor/emissary): fix nonroot sidecars + input/output params & artifacts #6403

fix(executor/emissary): fix nonroot sidecars + input/output params & artifacts #6403

Bobgy commented Jul 22, 2021

Bobgy Jul 22, 2021

alexec Jul 22, 2021

codecov bot commented Jul 22, 2021 •

edited

Loading

Bobgy commented Jul 22, 2021

Bobgy commented Jul 22, 2021

alexec left a comment

alexec Jul 22, 2021

Bobgy commented Jul 24, 2021

Bobgy commented Jul 24, 2021 •

edited

Loading

alexec Jul 25, 2021

alexec Jul 25, 2021

Bobgy Jul 25, 2021

Bobgy Jul 26, 2021

alexec commented Jul 26, 2021

fix(executor/emissary): fix nonroot sidecars + input/output params & artifacts #6403

fix(executor/emissary): fix nonroot sidecars + input/output params & artifacts #6403

Conversation

Bobgy commented Jul 22, 2021

Bobgy Jul 22, 2021

Choose a reason for hiding this comment

alexec Jul 22, 2021

Choose a reason for hiding this comment

codecov bot commented Jul 22, 2021 • edited Loading

Codecov Report

Bobgy commented Jul 22, 2021

Bobgy commented Jul 22, 2021

alexec left a comment

Choose a reason for hiding this comment

alexec Jul 22, 2021

Choose a reason for hiding this comment

Bobgy commented Jul 24, 2021

Bobgy commented Jul 24, 2021 • edited Loading

alexec Jul 25, 2021

Choose a reason for hiding this comment

alexec Jul 25, 2021

Choose a reason for hiding this comment

Bobgy Jul 25, 2021

Choose a reason for hiding this comment

Bobgy Jul 26, 2021

Choose a reason for hiding this comment

alexec commented Jul 26, 2021

codecov bot commented Jul 22, 2021 •

edited

Loading

Bobgy commented Jul 24, 2021 •

edited

Loading