RBAC Support for Actions #2110

simster7 · 2019-08-06T20:55:21Z

See #2002

codecov · 2019-08-06T21:24:11Z

Codecov Report

Merging #2110 into master will decrease coverage by 0.27%.
The diff coverage is 16.32%.

@@            Coverage Diff             @@
##           master    #2110      +/-   ##
==========================================
- Coverage   38.53%   38.25%   -0.28%     
==========================================
  Files         103      103              
  Lines       14732    14547     -185     
==========================================
- Hits         5677     5565     -112     
+ Misses       8294     8239      -55     
+ Partials      761      743      -18

Impacted Files	Coverage Δ
server/rbacpolicy/rbacpolicy.go	`79.68% <ø> (ø)`	⬆️
pkg/apis/application/v1alpha1/types.go	`59.24% <ø> (ø)`	⬆️
cmd/argocd/commands/app.go	`1.81% <0%> (ø)`	⬆️
server/application/application.go	`25.19% <0%> (+0.09%)`	⬆️
cmd/argocd/commands/app_actions.go	`0% <0%> (ø)`	⬆️
util/lua/lua.go	`76.75% <72.72%> (-0.31%)`	⬇️
util/ksonnet/ksonnet.go	`34.61% <0%> (-8.79%)`	⬇️
util/helm/helm.go	`66.07% <0%> (-7.05%)`	⬇️
util/kube/ctl.go	`19.9% <0%> (-4.68%)`	⬇️
util/kustomize/kustomize.go	`71.84% <0%> (-3.84%)`	⬇️
... and 1 more

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 b502555...81dcae8. Read the comment docs.

server/rbacpolicy/rbacpolicy.go

alexec · 2019-08-07T18:37:52Z

CI failure looks unrelated to change. Re-building.

evrardjp · 2019-08-10T12:02:51Z

Not sure which tests are running, but it seems there is an error in build just after a git diff showing a spacing issue

-	ActionAction = "action"
+	ActionAction   = "action"

simster7 · 2019-08-12T14:35:01Z

Thanks @evrardjp. Seems current failure is due to unrelated tests? @alexec

alexec · 2019-08-12T18:10:03Z

The failed test is known flaky.

simster7 · 2019-08-12T19:37:42Z

Perfect. @jessesuen should be ready to merge

jessesuen

There is a UI change to this which is not captured here.

jessesuen · 2019-08-21T00:01:35Z

server/application/application.go

@@ -1093,7 +1093,8 @@ func (s *Server) RunResourceAction(ctx context.Context, q *application.ResourceA
 		Version:      q.Version,
 		Group:        q.Group,
 	}
-	res, config, _, err := s.getAppResource(ctx, rbacpolicy.ActionOverride, resourceRequest)
+	actionRequest := fmt.Sprintf("%s/%s", rbacpolicy.ActionAction, q.Action)
+	res, config, _, err := s.getAppResource(ctx, actionRequest, resourceRequest)


Thinking about this more, action/<actionname> is not specific enough. We may have the same action name "pause", which works for both a Deployment, Rollout, and CronJob object. Granting "pause" privileges to all object types is potentially undesirable. We need a convention for actions which reflect the finer grained rbac. For example:

action/argoproj.io/Rollout/resume

Responded in #2181

As I understand, the convention being introduced is action/argoproj.io/Rollout:resume instead of action/argoproj.io/Rollout/resume. I feel the slash is more consistent with all our other RBAC as opposed to the colon.

For example, rbac rules may be:
action/argoproj.io/Rollout/*

Which seems better than:
action/argoproj.io/Rollout:*

jessesuen · 2019-08-21T00:03:46Z

For the UI to understand and present the available actions. We need to implement action discovery better, as described in item (1) in #2181

simster7 · 2019-09-16T20:51:38Z

@jessesuen @alexec Sorry for the crazy delay, but points 1 and 2 from Jesse's issue (#2110) are now done. I intend to open a separate PR for the UI changes. A few points:

The new syntax for actions is group/kind:actionName. Because of this, I have removed the group and kind parameters in argocd app actions run as they are now a part of the action name. Therefore, this is currently a breaking change as the call

argocd app actions run APPNAME resume --kind Rollout

is no longer supported and instead must be

argocd app actions run APPNAME argoproj.io/Rollout:resume

Not sure how to approach this, should I allow backwards compatibility with resume commands?

argocd app actions list APPNAME now lists all actions across all resources by default and shows if they are available:

$ argocd app actions list blue-green

GROUP        KIND     NAME                       ACTION                      AVAILABLE
argoproj.io  Rollout  blue-green-helm-guestbook  argoproj.io/Rollout:resume  true

Running an action no longer checks if it is available or not.

jessesuen

Very close to what I was looking for. Great job.

pkg/apis/application/v1alpha1/types.go

jessesuen · 2019-09-21T01:43:25Z

cmd/argocd/commands/app_actions.go

+	if len(groupKindSplit) != 2 {
+		log.Fatal("Action name is malformed")
+	}
+	return groupKindSplit[0], groupKindSplit[1], actionName


It appears we are making breaking changes here by requiring a new convention for running actions. I think it's okay if we start introducing a new convention, but we may need to preserve previous flags for backwards compatibility since many pipelines are depending on this behavior.

Introduced backwards compatibility with the following message:

$ argocd app actions run blue-green resume --kind Rollout Warning: "resume" action has been deprecated. Please run the action as argocd app run blue-green argoproj.io/Rollout/resume

server/application/application.go

jessesuen · 2019-09-21T01:49:10Z

server/application/application.go

@@ -1093,7 +1093,8 @@ func (s *Server) RunResourceAction(ctx context.Context, q *application.ResourceA
 		Version:      q.Version,
 		Group:        q.Group,
 	}
-	res, config, _, err := s.getAppResource(ctx, rbacpolicy.ActionOverride, resourceRequest)
+	actionRequest := fmt.Sprintf("%s/%s", rbacpolicy.ActionAction, q.Action)
+	res, config, _, err := s.getAppResource(ctx, actionRequest, resourceRequest)


As I understand, the convention being introduced is action/argoproj.io/Rollout:resume instead of action/argoproj.io/Rollout/resume. I feel the slash is more consistent with all our other RBAC as opposed to the colon.

For example, rbac rules may be:
action/argoproj.io/Rollout/*

Which seems better than:
action/argoproj.io/Rollout:*

jessesuen · 2019-09-21T01:52:00Z

My review comments above were made without first reading comment #2110 (comment). But yes, I am glad you recognized the breaking change.

is no longer supported and instead must be argocd app actions run APPNAME argoproj.io/Rollout:resume

How does this work when a user wants to run it against a specific rollout by name?

simster7 · 2019-09-21T22:08:44Z

@jessesuen Addressed all of your comments.

How does this work when a user wants to run it against a specific rollout by name?

There is a --resource-name argument available

jessesuen

Conditional approval: there's one place where we still use the colon, but otherwise this LGTM. Please fix the last statement.

server/application/application.go

simster7 · 2019-10-03T16:10:10Z

@alexec @jessesuen This should also be good to go!

alexec · 2019-10-03T16:53:15Z

@jessesuen do you want to merge please?

alexec · 2019-10-07T18:17:22Z

Yay!

…tions on App Details page

… App Details page (#2450)

simster7 added 4 commits August 6, 2019 11:36

Action roles

3206fe7

Removed e2e test for now

63e8816

minor

dc551d9

Codegen

f2c34b8

alexec added this to the v1.3 milestone Aug 6, 2019

jessesuen reviewed Aug 7, 2019

View reviewed changes

server/rbacpolicy/rbacpolicy.go Outdated Show resolved Hide resolved

Rename

e53d44b

alexec assigned jessesuen Aug 7, 2019

alexec approved these changes Aug 7, 2019

View reviewed changes

alexec self-assigned this Aug 7, 2019

Minor fix

52f9d2b

alexec removed their assignment Aug 14, 2019

jessesuen requested changes Aug 21, 2019

View reviewed changes

simster7 mentioned this pull request Aug 21, 2019

Improvements to resource actions #2181

Open

Merge branch 'master' into action_roles

f64c940

simster7 added 2 commits August 28, 2019 10:50

Merge branch 'master' into action_roles

0d65f0f

WIP

a73bb6a

simster7 added 3 commits September 16, 2019 13:20

Should be done now, running tests

2d6df04

Merge branch 'master' into action_roles

17cc958

Minor

fc81ff0

Pre-commit

614b047

alexec requested a review from jessesuen September 20, 2019 22:26

jessesuen requested changes Sep 21, 2019

View reviewed changes

simster7 added 2 commits September 21, 2019 14:35

Done

6937767

Done

1fea62b

alexec requested a review from jessesuen September 23, 2019 17:57

jessesuen approved these changes Oct 2, 2019

View reviewed changes

server/application/application.go Outdated Show resolved Hide resolved

simster7 added 3 commits October 2, 2019 14:18

Merge branch 'master' into action_roles

a52cd9b

Minor

573daa9

Code-gen

81dcae8

jessesuen merged commit dd21ab9 into argoproj:master Oct 4, 2019

alexmt pushed a commit to alexmt/argo-cd that referenced this pull request Oct 9, 2019

Issue argoproj#2110 - Disable menu item for non available resource ac…

8001934

…tions on App Details page

alexmt mentioned this pull request Oct 9, 2019

Issue #2110 - Disable menu item for non available resource actions on App Details page #2450

Merged

alexmt pushed a commit that referenced this pull request Oct 9, 2019

Issue #2110 - Disable menu item for non available resource actions on…

c2dce9c

… App Details page (#2450)

alexmt mentioned this pull request Oct 9, 2019

Custom resource actions should be available by default #2451

Closed

simster7 mentioned this pull request Oct 20, 2019

argocd app actions list APPNAME should work without specify flags #1588

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

RBAC Support for Actions #2110

RBAC Support for Actions #2110

simster7 commented Aug 6, 2019

codecov bot commented Aug 6, 2019 •

edited

Loading

alexec commented Aug 7, 2019

evrardjp commented Aug 10, 2019 •

edited

Loading

simster7 commented Aug 12, 2019

alexec commented Aug 12, 2019

simster7 commented Aug 12, 2019

jessesuen left a comment

jessesuen Aug 21, 2019

simster7 Aug 21, 2019

jessesuen Sep 21, 2019

jessesuen commented Aug 21, 2019 •

edited

Loading

simster7 commented Sep 16, 2019

jessesuen left a comment

jessesuen Sep 21, 2019

simster7 Sep 21, 2019

jessesuen Sep 21, 2019

jessesuen commented Sep 21, 2019 •

edited

Loading

simster7 commented Sep 21, 2019

jessesuen left a comment •

edited

Loading

simster7 commented Oct 3, 2019

alexec commented Oct 3, 2019

alexec commented Oct 7, 2019

RBAC Support for Actions #2110

RBAC Support for Actions #2110

Conversation

simster7 commented Aug 6, 2019

codecov bot commented Aug 6, 2019 • edited Loading

Codecov Report

alexec commented Aug 7, 2019

evrardjp commented Aug 10, 2019 • edited Loading

simster7 commented Aug 12, 2019

alexec commented Aug 12, 2019

simster7 commented Aug 12, 2019

jessesuen left a comment

Choose a reason for hiding this comment

jessesuen Aug 21, 2019

Choose a reason for hiding this comment

simster7 Aug 21, 2019

Choose a reason for hiding this comment

jessesuen Sep 21, 2019

Choose a reason for hiding this comment

jessesuen commented Aug 21, 2019 • edited Loading

simster7 commented Sep 16, 2019

jessesuen left a comment

Choose a reason for hiding this comment

jessesuen Sep 21, 2019

Choose a reason for hiding this comment

simster7 Sep 21, 2019

Choose a reason for hiding this comment

jessesuen Sep 21, 2019

Choose a reason for hiding this comment

jessesuen commented Sep 21, 2019 • edited Loading

simster7 commented Sep 21, 2019

jessesuen left a comment • edited Loading

Choose a reason for hiding this comment

simster7 commented Oct 3, 2019

alexec commented Oct 3, 2019

alexec commented Oct 7, 2019

codecov bot commented Aug 6, 2019 •

edited

Loading

evrardjp commented Aug 10, 2019 •

edited

Loading

jessesuen commented Aug 21, 2019 •

edited

Loading

jessesuen commented Sep 21, 2019 •

edited

Loading

jessesuen left a comment •

edited

Loading