feat: Introduce RBAC based approach to pod logs #7211 #8353

reggie-k · 2022-02-03T11:43:44Z

The required changes in the cli were performed, the UI changes will come in a separate PR.
The enforcement is disabled by default, to mitigate the breaking change.
The switch will reside in the main argocd-cm.
Not sure whether I should document the switch explicitly myself or will it be documented in the release blog?

Note on DCO:

If the DCO action in the integration test fails, one or more of your commits are not signed off. Please click on the Details link next to the DCO action for instructions on how to resolve this.

Checklist:

[V] Either (a) I've created an enhancement proposal and discussed it with the community, (b) this is a bug fix, or (c) this does not need to be in the release notes.
[V] The title of the PR states what changed and the related issues number (used for the release note).
I've included "Closes [ISSUE #]" or "Fixes [ISSUE #]" in the description to automatically close the associated issue.
[V] I've updated both the CLI and UI to expose my feature, or I plan to submit a second PR with them.
[V] Does this PR require documentation updates?
I've updated documentation as required by this PR.
Optional. My organization is added to USERS.md.
I have signed off all my commits as required by DCO
[V] I have written unit and/or e2e tests for my change. PRs without these are unlikely to be merged.
[V] My build is green (troubleshooting builds).

codecov · 2022-02-03T11:55:56Z

Codecov Report

Merging #8353 (4e4330d) into master (ebc89ce) will increase coverage by 0.02%.
The diff coverage is 50.00%.

@@            Coverage Diff             @@
##           master    #8353      +/-   ##
==========================================
+ Coverage   42.57%   42.60%   +0.02%     
==========================================
  Files         177      177              
  Lines       22988    23007      +19     
==========================================
+ Hits         9787     9801      +14     
- Misses      11804    11805       +1     
- Partials     1397     1401       +4

Impacted Files	Coverage Δ
cmd/argocd/commands/admin/settings_rbac.go	`23.24% <ø> (ø)`
server/application/application.go	`32.06% <0.00%> (-0.21%)`	⬇️
server/account/account.go	`69.62% <66.66%> (+3.73%)`	⬆️
util/settings/settings.go	`48.10% <71.42%> (+0.18%)`	⬆️
server/rbacpolicy/rbacpolicy.go	`82.35% <100.00%> (ø)`

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 ebc89ce...4e4330d. Read the comment docs.

pasha-codefresh · 2022-02-03T13:30:23Z

server/account/account.go

+	// Otherwise, no RBAC enforcement for logs will take place (meaning, can-i request on a logs resource will result in "yes")
+	// In the future, logs RBAC will be always enforced and the parameter along with this check will be removed
+	if r.Resource == "logs" {
+		// logsRBACEnforceEnable := env.ParseBoolFromEnv("ARGOCD_SERVER_RBAC_LOG_ENFORCE_ENABLE", false)


i think it is redundant changes

pasha-codefresh · 2022-02-03T13:31:16Z

server/application/application.go

+	// Temporarily, logs RBAC will be enforced only if an intermediate env var serverRBACLogEnforceEnable is defined and has a "true" value
+	// Otherwise, no RBAC enforcement for logs will take place (meaning, can-i request on a logs resource will result in "yes")
+	// In the future, logs RBAC will be always enforced and the parameter along with this check will be removed
+	logsRBACEnforceEnable, _ := s.settingsMgr.GetServerRBACLogEnforceEnable()


not ignore error here

pasha-codefresh · 2022-02-03T13:33:47Z

util/settings/settings.go

+	if argoCDCM.Data[settingsServerRBACLogEnforceEnableKey] == "" {
+		return false, err
+	}
+	return argoCDCM.Data[settingsServerRBACLogEnforceEnableKey] == "true", nil


maybe better use strconv.ParseBool(

pasha-codefresh · 2022-02-03T13:34:33Z

util/settings/settings.go

@@ -91,6 +91,8 @@ type ArgoCDSettings struct {
 	PasswordPattern string `json:"passwordPattern,omitempty"`
 	// BinaryUrls contains the URLs for downloading argocd binaries
 	BinaryUrls map[string]string `json:"binaryUrls,omitempty"`
+	// ServerRBACLogEnforceEnable temporary var indicates whether rbac will be enforced on logs
+	ServerRBACLogEnforceEnable bool `json:"serverRBACLogEnforceEnable"`


i dont think you need it anymore after migrate to settings manager

reggie-k · 2022-02-04T15:01:17Z

Fixed according to the remarks.
Signed off the commits.
Perhaps additional discussion needed on util/settings/settings.go

crenshaw-dev

Could you also do a quick update in rbac.md? Seems like a good place to document this.

crenshaw-dev · 2022-02-16T13:59:04Z

server/account/account.go

@@ -126,6 +126,21 @@ func (s *Server) CanI(ctx context.Context, r *account.CanIRequest) (*account.Can
 	if !slice.ContainsString(rbacpolicy.Resources, r.Resource, nil) {
 		return nil, status.Errorf(codes.InvalidArgument, "%v does not contain %s", rbacpolicy.Resources, r.Resource)
 	}
+
+	// Temporarily, logs RBAC will be enforced only if an intermediate env var serverRBACLogEnforceEnable is defined and has a "true" value


Is serverRBACLogEnforceEnable the name of the env var or some internal variable?

Internal var based on a value of an env var. Will clarify the comment, thanks.

Fixed the comment in application.go and account.go section to explain better what the switch does + renamed the var to comply to its name in settings manager.
Added docs in the rbac.md as per the "log" rbac resource, but not sure at what section should I document the switch itself?

Signed-off-by: reggie-k <reginakagan@gmail.com>

crenshaw-dev

LGTM!

reggie-k · 2022-02-18T14:55:24Z

Great!
As for the UI changes needed (both backend and UI changes will have to appear in a release), should they come after this PR is merged or can they be based on my branch as it is now? They will be performed by my colleague.

pasha-codefresh · 2022-02-18T15:52:11Z

Well done @reggie-k

yonimoses · 2022-02-18T16:11:53Z

Great! As for the UI changes needed (both backend and UI changes will have to appear in a release), should they come after this PR is merged or can they be based on my branch as it is now? They will be performed by my colleague.

Awesome!

crenshaw-dev

Oh! You asked a question that I failed to answer. settingsServerRBACLogEnforceEnableKey should be documented in docs/operator-manual/argocd-cm.yaml.

Nitpick: could you replace the hard-coded instances of the config key in tests with the variable?

crenshaw-dev · 2022-02-18T19:17:06Z

As for the UI changes needed (both backend and UI changes will have to appear in a release), should they come after this PR is merged or can they be based on my branch as it is now?

If the backend changes shouldn't be allowed to go out without frontend changes, I'd recommend making them all part of one PR.

Signed-off-by: reggie-k <reginakagan@gmail.com>

reggie-k · 2022-02-23T16:03:11Z

Oh! You asked a question that I failed to answer. settingsServerRBACLogEnforceEnableKey should be documented in docs/operator-manual/argocd-cm.yaml.

Done, now wondering whether part of that belongs in the release blog :) What do you think?

Nitpick: could you replace the hard-coded instances of the config key in tests with the variable?

Am a total go newbie, but the const key names in settings.go are private and used within the package, as far as I understand, do you want me to make this particular one a global const so that it can be referenced outside?

reggie-k · 2022-02-23T16:19:20Z

As for the UI changes needed (both backend and UI changes will have to appear in a release), should they come after this PR is merged or can they be based on my branch as it is now?

If the backend changes shouldn't be allowed to go out without frontend changes, I'd recommend making them all part of one PR.

I see the reason behind this. How can this be achieved, given my colleague will work on the UI changes, and will need the backend to have my changes? Can you guide me through the process from Github perspective? Where does he fork from, where does he create PRs to, etc?

crenshaw-dev · 2022-02-23T16:30:03Z

Done, now wondering whether part of that belongs in the release blog :) What do you think?

I think it probably does. @alexmt do we keep a running list anywhere of things that need to be in the release blog?

Am a total go newbie, but the const key names in settings.go are private and used within the package, as far as I understand, do you want me to make this particular one a global const so that it can be referenced outside?

I'm relatively new to go as well, so I missed that problem. 😆 I think hard-coding is fine in this case. Searching text is easy.

Can you guide me through the process from Github perspective? Where does he fork from, where does he create PRs to, etc?

I think he could 1) fork argo-cd, 2) add your fork as a remote, 3) checkout your branch, 4) create a new branch based on yours, 5) make changes and then open a PR against your branch. Once the UI changes are merged into your branch, this PR will be ready for final review/merge.

alexmt · 2022-03-11T21:45:28Z

Thank you for review @pasha-codefresh and @crenshaw-dev . Doing one final round and merging

alexmt

LGTM.

I think we should resolve https://github.com/argoproj/argo-cd/pull/8353/files#r798567363 but I propose to merge PR for the sake of time and create follow up PR to do it.

crenshaw-dev

LGTM!

pasha-codefresh · 2022-03-18T19:20:08Z

Great job @reggie-k !

alexmt · 2022-03-18T19:25:29Z

Thank you @reggie-k !

…oj#8353) * initial changes in settings, app, account, admin, rbac, doc and tests Signed-off-by: reggie-k <reginakagan@gmail.com> * rbac.md docs and better comments in account and app Signed-off-by: reggie-k <reginakagan@gmail.com> * initial changes in settings, app, account, admin, rbac, doc and tests Signed-off-by: reggie-k <reginakagan@gmail.com> * rbac.md docs and better comments in account and app Signed-off-by: reggie-k <reginakagan@gmail.com> * initial changes in settings, app, account, admin, rbac, doc and tests Signed-off-by: reggie-k <reginakagan@gmail.com> * rbac.md docs and better comments in account and app Signed-off-by: reggie-k <reginakagan@gmail.com> * rebase fix Signed-off-by: reggie-k <reginakagan@gmail.com> * updated docs for argocd-cm.yaml Signed-off-by: reggie-k <reginakagan@gmail.com> Signed-off-by: wojtekidd <wojtek.cichon@protonmail.com>

pasha-codefresh requested changes Feb 3, 2022

View reviewed changes

reggie-k closed this Feb 4, 2022

reggie-k force-pushed the master branch from 97222fc to ac12540 Compare February 4, 2022 06:58

reggie-k reopened this Feb 4, 2022

crenshaw-dev requested changes Feb 16, 2022

View reviewed changes

reggie-k force-pushed the master branch from 3898e83 to 5a5a851 Compare February 16, 2022 17:04

reggie-k added 6 commits February 16, 2022 19:04

initial changes in settings, app, account, admin, rbac, doc and tests

5a5a851

Signed-off-by: reggie-k <reginakagan@gmail.com>

rbac.md docs and better comments in account and app

46b52a8

Signed-off-by: reggie-k <reginakagan@gmail.com>

initial changes in settings, app, account, admin, rbac, doc and tests

4d1bc0a

Signed-off-by: reggie-k <reginakagan@gmail.com>

rbac.md docs and better comments in account and app

0a26a9d

Signed-off-by: reggie-k <reginakagan@gmail.com>

Merge branch 'argoproj:master' into master

e59a6a3

Merge branch 'master' of https://github.com/reggie-k/argo-cd

45fd2e8

crenshaw-dev approved these changes Feb 18, 2022

View reviewed changes

pasha-codefresh approved these changes Feb 18, 2022

View reviewed changes

crenshaw-dev requested changes Feb 18, 2022

View reviewed changes

reggie-k added 6 commits February 23, 2022 15:14

Merge branch 'master' into master

f54173c

initial changes in settings, app, account, admin, rbac, doc and tests

000c16a

Signed-off-by: reggie-k <reginakagan@gmail.com>

rbac.md docs and better comments in account and app

656e9ff

Signed-off-by: reggie-k <reginakagan@gmail.com>

Merge branch 'master' of https://github.com/reggie-k/argo-cd

b1894dc

rebase fix

98cfc13

Signed-off-by: reggie-k <reginakagan@gmail.com>

updated docs for argocd-cm.yaml

4e4330d

Signed-off-by: reggie-k <reginakagan@gmail.com>

alexmt self-requested a review March 11, 2022 21:45

alexmt approved these changes Mar 18, 2022

View reviewed changes

crenshaw-dev approved these changes Mar 18, 2022

View reviewed changes

crenshaw-dev merged commit af5f234 into argoproj:master Mar 18, 2022

reggie-k mentioned this pull request Oct 31, 2022

REQUEST: New membership for reggie-k argoproj/argoproj#162

Closed

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Introduce RBAC based approach to pod logs #7211 #8353

feat: Introduce RBAC based approach to pod logs #7211 #8353

reggie-k commented Feb 3, 2022

codecov bot commented Feb 3, 2022 •

edited

Loading

pasha-codefresh Feb 3, 2022

pasha-codefresh Feb 3, 2022

pasha-codefresh Feb 3, 2022

pasha-codefresh Feb 3, 2022

reggie-k commented Feb 4, 2022

crenshaw-dev left a comment

crenshaw-dev Feb 16, 2022

reggie-k Feb 16, 2022

reggie-k Feb 17, 2022 •

edited

Loading

crenshaw-dev left a comment

reggie-k commented Feb 18, 2022

pasha-codefresh commented Feb 18, 2022

yonimoses commented Feb 18, 2022 •

edited

Loading

crenshaw-dev left a comment

crenshaw-dev commented Feb 18, 2022

reggie-k commented Feb 23, 2022 •

edited

Loading

reggie-k commented Feb 23, 2022

crenshaw-dev commented Feb 23, 2022

alexmt commented Mar 11, 2022

alexmt left a comment

crenshaw-dev left a comment

pasha-codefresh commented Mar 18, 2022

alexmt commented Mar 18, 2022

feat: Introduce RBAC based approach to pod logs #7211 #8353

feat: Introduce RBAC based approach to pod logs #7211 #8353

Conversation

reggie-k commented Feb 3, 2022

codecov bot commented Feb 3, 2022 • edited Loading

Codecov Report

pasha-codefresh Feb 3, 2022

Choose a reason for hiding this comment

pasha-codefresh Feb 3, 2022

Choose a reason for hiding this comment

pasha-codefresh Feb 3, 2022

Choose a reason for hiding this comment

pasha-codefresh Feb 3, 2022

Choose a reason for hiding this comment

reggie-k commented Feb 4, 2022

crenshaw-dev left a comment

Choose a reason for hiding this comment

crenshaw-dev Feb 16, 2022

Choose a reason for hiding this comment

reggie-k Feb 16, 2022

Choose a reason for hiding this comment

reggie-k Feb 17, 2022 • edited Loading

Choose a reason for hiding this comment

crenshaw-dev left a comment

Choose a reason for hiding this comment

reggie-k commented Feb 18, 2022

pasha-codefresh commented Feb 18, 2022

yonimoses commented Feb 18, 2022 • edited Loading

crenshaw-dev left a comment

Choose a reason for hiding this comment

crenshaw-dev commented Feb 18, 2022

reggie-k commented Feb 23, 2022 • edited Loading

reggie-k commented Feb 23, 2022

crenshaw-dev commented Feb 23, 2022

alexmt commented Mar 11, 2022

alexmt left a comment

Choose a reason for hiding this comment

crenshaw-dev left a comment

Choose a reason for hiding this comment

pasha-codefresh commented Mar 18, 2022

alexmt commented Mar 18, 2022

codecov bot commented Feb 3, 2022 •

edited

Loading

reggie-k Feb 17, 2022 •

edited

Loading

yonimoses commented Feb 18, 2022 •

edited

Loading

reggie-k commented Feb 23, 2022 •

edited

Loading