feat: Allow to pass optional flags to resource template #1779

CermakM · 2019-11-19T15:11:03Z

In some cases, it might be useful to have the option to pass optional flags to a resource template.

For example, to turn off kubectl default validation when submitting a resource manifest. OpenShift has some strict requirements (and in case of OpenShift 3.11 even discrepancies) in the oapi spec, which prevents resources from being submitted via kubectl

This patch introduces the Template.Spec.Resource.Flags parameter which allows to pass optional flags to a resource template.

Example:

resource:
  action: create
  flags: [
    "--validate=false"  # validation will be turned off
  ]
  manifest: |
    apiVersion: route.openshift.io/v1
    kind: Route
    metadata:
      name: host-route
    spec:
      to:
        kind: Service
        name: service-name

sarabala1979

Please add examples

CermakM · 2019-11-20T22:02:52Z

The example references openshift/origin#24060

CermakM · 2019-11-21T07:17:37Z

Could I ask for help with the CI check? I am kinda struggling to understand the reason it failed.

simster7 · 2019-11-21T15:05:04Z

Looks like a network issue when running dep ensure. Doesn't seem like you changed any dependencies, so don't worry about it. @sarabala1979 will re-run the CI when he reviews your PR.

simster7 · 2019-12-16T19:49:19Z

I think that this PR could become more generalized by allowing the user to pass arbitrary arguments to kubectl. If we merge this, other users could request a similar implementation for other flags, which could cause it to get cluttered.

Something like:

  - name: create-route-without-validation
    resource:
      action: create
      flags: ["--validate=false"]  # validation will be turned off

could work.

Will open this as a new issue. @CermakM do you want to own this?

(P.S. Not that this is slightly different from the open #1834 as that requires executor changes, so it still requires its own PR)

CermakM · 2019-12-17T08:31:06Z

@simster7 This looks reasonable to me and I'll be happy to extend the implementation :) I suppose we therefore want to have just flags, instead of both flags and validate, correct?

EDIT: Commented on the issue as well.

CermakM · 2020-01-02T12:06:06Z

Fixes: #1857

codecov · 2020-01-02T12:13:03Z

Codecov Report

❗ No coverage uploaded for pull request base (master@f3df966). Click here to learn what that means.
The diff coverage is 13.15%.

@@            Coverage Diff            @@
##             master    #1779   +/-   ##
=========================================
  Coverage          ?   11.39%           
=========================================
  Files             ?       74           
  Lines             ?    31163           
  Branches          ?        0           
=========================================
  Hits              ?     3550           
  Misses            ?    27134           
  Partials          ?      479

Impacted Files	Coverage Δ
pkg/apis/workflow/v1alpha1/workflow_types.go	`11.54% <ø> (ø)`
...kg/apis/workflow/v1alpha1/zz_generated.deepcopy.go	`0% <0%> (ø)`
pkg/apis/workflow/v1alpha1/generated.pb.go	`0.45% <0%> (ø)`
workflow/executor/resource.go	`6.46% <28.57%> (ø)`

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 f3df966...c182b3b. Read the comment docs.

See argoproj/argo-workflows#1779 Signed-off-by: Marek Cermak <macermak@redhat.com> modified: workflows/templates/inspection-build-template.yaml modified: workflows/templates/inspection-build-with-cpu-template.yaml

simster7

This LGTM! Could you add some tests please?

CermakM · 2020-01-08T14:10:37Z

Uh, well, I hoped you wouldn't say that :D I'll see what I can do.

See argoproj/argo-workflows#1779 Signed-off-by: Marek Cermak <macermak@redhat.com> modified: workflows/templates/inspection-build-template.yaml modified: workflows/templates/inspection-build-with-cpu-template.yaml

maximmold · 2020-01-24T20:09:32Z

Does this implementation require a manifest? I want to delete given a particular label in an argo work flow and I seem to only be able to do it with a specific manifest that requires a metadata.name

CermakM · 2020-01-26T18:34:22Z

@maximmold I am sorry, struggling to understand your use case a little bit. Could you please elaborate or provide an example of how would you imagine that being used other than in the ResourceTemplate?

workflow/executor/resource_test.go

See argoproj/argo-workflows#1779 Signed-off-by: Marek Cermak <macermak@redhat.com> modified: workflows/templates/inspection-build-template.yaml modified: workflows/templates/inspection-build-with-cpu-template.yaml

maximmold · 2020-01-27T12:31:32Z

I simply want to be able to do a delete of resources given a particular label selector.

- name: delete-resources-without-validation
    resource:
      action: delete
      flags: ["seldondeployment -l github-branch={{workflow.parameters.github-branch}}"]

Providing a manifest requires you know the same of the resource.

CermakM · 2020-01-27T14:05:05Z

Ah, gotcha. Let me take a look at it. I think it shouldn't require many changes.

simster7 · 2020-01-28T15:22:36Z

Will try to get some internal comments w.r.t this PR today

See argoproj/argo-workflows#1779 Signed-off-by: Marek Cermak <macermak@redhat.com> modified: workflows/templates/inspection-build-template.yaml modified: workflows/templates/inspection-build-with-cpu-template.yaml

alexec · 2020-03-18T23:39:43Z

@CermakM is this still wanted please?

CermakM · 2020-03-19T07:08:44Z

@alexec Yes it is. I guess I have quite a lot of rebasing to do. :/

alexec · 2020-03-19T16:05:52Z

@simster7 do you want to continue as reviewer?

CermakM · 2020-03-20T09:10:57Z

Is the failure of Golang CI setup on my side? 🤔

simster7 · 2020-03-20T20:27:19Z

@simster7 do you want to continue as reviewer?

Yes, sorry this got out of my radar. I'll give another review today

simster7

Two minor changes, otherwise LGTM.

I have reservations about security. With this PR we effectively give the workflow submitter full access to kubectl. I'm going to bring this up with @jessesuen, but feel free to chime in with your opinions.

workflow/executor/resource.go

CermakM · 2020-03-23T09:16:59Z

@simster7 you're right, but IMHO it is not Argo who should impose security restrictions, it is the cluster/namespace admin, I.e. if users have the rights to execute a certain command via kubectl, they should be able to do that in Argo as well, shouldn't they?

simster7 · 2020-03-23T16:12:41Z

@simster7 you're right, but IMHO it is not Argo who should impose security restrictions, it is the cluster/namespace admin, I.e. if users have the rights to execute a certain command via kubectl, they should be able to do that in Argo as well, shouldn't they?

You're right, except that Argo its own ServiceAccount with kubectl – not the user's – so with this PR we give free reign to the submitter to use Argo's ServiceAccount. However, @jessesuen brought up a good point by saying that we effectively already do this since users can submit arbitrary containers... so it looks like we're going to go ahead with this.

- added an example for resource flags - added a test for resource flags - allow to use resource flags without manifest Signed-off-by: Marek Cermak <macermak@redhat.com> modified: cmd/argoexec/commands/resource.go modified: pkg/apis/workflow/v1alpha1/workflow_types.go modified: workflow/executor/resource.go new file: examples/resource-delete-with-flags.yaml new file: examples/resource-flags.yaml new file: workflow/executor/resource_test.go

CermakM · 2020-04-14T11:49:39Z

@simster7 I rebased it once again and pushed it without updated codegen, which causes the conflicts (which, in turn, I assume, have been blocking the merging the whole time). Please, if / when you decide to merge this, generate the codegen as well.

Cheers,
M

sonarcloud · 2020-04-15T16:21:18Z

SonarCloud Quality Gate failed.

0 Bugs
0 Vulnerabilities (and 0 Security Hotspots to review)
0 Code Smells

51.4% Coverage
0.0% Duplication

Stale review

simster7 · 2020-04-15T16:41:22Z

Thanks for your work @CermakM! Glad we finally got this merged

CermakM · 2020-04-20T12:21:16Z

Thanks @simster7 !

m-yosefpor · 2020-11-03T20:18:38Z

I'm new to Argo. Will this solve the issue with Openshift resources when used with ArgoCD as well? or will it only affect argo workflows?

m-yosefpor · 2020-11-03T20:50:41Z

I found this option in Application object:

syncPolicy:
  syncOptions:
    - Validate=false

However is it possible to disable it per resource (instead of the whole app)?

CermakM changed the title ~~Resource validation~~ Allow to disable kubectl resource validation Nov 19, 2019

sarabala1979 previously requested changes Nov 20, 2019

View reviewed changes

CermakM force-pushed the resource-validation branch from 5645d9c to 4397256 Compare November 20, 2019 22:04

CermakM requested a review from sarabala1979 November 20, 2019 22:04

sarabala1979 changed the title ~~Allow to disable kubectl resource validation~~ feat: Allow to disable kubectl resource validation Dec 16, 2019

simster7 mentioned this pull request Dec 16, 2019

Allow resources to pass arbitrary flags to kubectl #1857

Open

CermakM force-pushed the resource-validation branch 2 times, most recently from 020c8a9 to 03c3f4d Compare January 2, 2020 12:03

CermakM changed the title ~~feat: Allow to disable kubectl resource validation~~ feat: Allow to pass optional flags to resource template Jan 2, 2020

simster7 self-assigned this Jan 7, 2020

simster7 suggested changes Jan 7, 2020

View reviewed changes

CermakM force-pushed the resource-validation branch from 03c3f4d to 7b64016 Compare January 26, 2020 18:32

golangcibot reviewed Jan 26, 2020

View reviewed changes

workflow/executor/resource_test.go Outdated Show resolved Hide resolved

CermakM force-pushed the resource-validation branch from 7b64016 to 2062de8 Compare January 26, 2020 19:14

CermakM requested a review from simster7 January 26, 2020 20:36

CermakM force-pushed the resource-validation branch from 9b8d5a5 to cc94c54 Compare March 19, 2020 08:24

CermakM force-pushed the resource-validation branch from cc94c54 to fb6522a Compare March 20, 2020 11:25

simster7 suggested changes Mar 20, 2020

View reviewed changes

workflow/executor/resource.go Outdated Show resolved Hide resolved

workflow/executor/resource.go Outdated Show resolved Hide resolved

CermakM force-pushed the resource-validation branch from b2c42d1 to 4ed2795 Compare March 23, 2020 09:20

simster7 approved these changes Mar 23, 2020

View reviewed changes

CermakM force-pushed the resource-validation branch from c182b3b to 8f772da Compare April 14, 2020 11:40

simster7 added 4 commits April 14, 2020 10:20

Merge branch 'master' into resource-validation

00fcbf9

Codegen

b31f176

Merge branch 'master' into HEAD

7a60971

Latest

f30dfe4

simster7 merged commit 2737c0a into argoproj:master Apr 15, 2020

simster7 mentioned this pull request May 12, 2020

feat: IgnoreNotFound prevents a workflow error when the resource does not exist #1834

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Allow to pass optional flags to resource template #1779

feat: Allow to pass optional flags to resource template #1779

CermakM commented Nov 19, 2019 •

edited

Loading

sarabala1979 left a comment

CermakM commented Nov 20, 2019

CermakM commented Nov 21, 2019

simster7 commented Nov 21, 2019

simster7 commented Dec 16, 2019

CermakM commented Dec 17, 2019 •

edited

Loading

CermakM commented Jan 2, 2020

codecov bot commented Jan 2, 2020 •

edited

Loading

simster7 left a comment

CermakM commented Jan 8, 2020

maximmold commented Jan 24, 2020

CermakM commented Jan 26, 2020

maximmold commented Jan 27, 2020

CermakM commented Jan 27, 2020

simster7 commented Jan 28, 2020

alexec commented Mar 18, 2020

CermakM commented Mar 19, 2020

alexec commented Mar 19, 2020

CermakM commented Mar 20, 2020

simster7 commented Mar 20, 2020

simster7 left a comment

CermakM commented Mar 23, 2020

simster7 commented Mar 23, 2020

CermakM commented Apr 14, 2020

sonarcloud bot commented Apr 15, 2020

simster7 commented Apr 15, 2020

CermakM commented Apr 20, 2020

m-yosefpor commented Nov 3, 2020

m-yosefpor commented Nov 3, 2020 •

edited

Loading

feat: Allow to pass optional flags to resource template #1779

feat: Allow to pass optional flags to resource template #1779

Conversation

CermakM commented Nov 19, 2019 • edited Loading

sarabala1979 left a comment

Choose a reason for hiding this comment

CermakM commented Nov 20, 2019

CermakM commented Nov 21, 2019

simster7 commented Nov 21, 2019

simster7 commented Dec 16, 2019

CermakM commented Dec 17, 2019 • edited Loading

CermakM commented Jan 2, 2020

codecov bot commented Jan 2, 2020 • edited Loading

Codecov Report

simster7 left a comment

Choose a reason for hiding this comment

CermakM commented Jan 8, 2020

maximmold commented Jan 24, 2020

CermakM commented Jan 26, 2020

maximmold commented Jan 27, 2020

CermakM commented Jan 27, 2020

simster7 commented Jan 28, 2020

alexec commented Mar 18, 2020

CermakM commented Mar 19, 2020

alexec commented Mar 19, 2020

CermakM commented Mar 20, 2020

simster7 commented Mar 20, 2020

simster7 left a comment

Choose a reason for hiding this comment

CermakM commented Mar 23, 2020

simster7 commented Mar 23, 2020

CermakM commented Apr 14, 2020

sonarcloud bot commented Apr 15, 2020

simster7 commented Apr 15, 2020

CermakM commented Apr 20, 2020

m-yosefpor commented Nov 3, 2020

m-yosefpor commented Nov 3, 2020 • edited Loading

CermakM commented Nov 19, 2019 •

edited

Loading

CermakM commented Dec 17, 2019 •

edited

Loading

codecov bot commented Jan 2, 2020 •

edited

Loading

m-yosefpor commented Nov 3, 2020 •

edited

Loading