Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(server): Add list sa and create secret to argo-server roles. Closes #4526 #4514

Merged
merged 7 commits into from
Nov 14, 2020
Merged

Conversation

Boolman
Copy link
Contributor

@Boolman Boolman commented Nov 11, 2020

In 2.12.0-rc1 when using SSO RBAC - argo fails to create the secret, due to missing permissions in the argo-server-role
https://github.com/argoproj/argo/blob/0931baf5fbe48487278b9a6c2fa206ab02406e5b/server/auth/sso/sso.go#L135-L138

@CLAassistant
Copy link

CLAassistant commented Nov 11, 2020

CLA assistant check
All committers have signed the CLA.

@Boolman Boolman changed the title fix - argo-server-role is missing 'CREATE' on secrets resource fix: roles for 2.12.0-rc1 is missing create on secerts and list on sa Nov 11, 2020
@Boolman
Copy link
Contributor Author

Boolman commented Nov 11, 2020

Signed-off-by: boolman <boolman@gmail.com>
Signed-off-by: boolman <boolman@gmail.com>
@alexec
Copy link
Contributor

alexec commented Nov 12, 2020

Please run make codegen to fix build.

@alexec
Copy link
Contributor

alexec commented Nov 12, 2020

I'm not 100% sure we should add list sa and create secret to argo-server role. It is new privileges only needed for SSO + RBAC.

@jessesuen thoughts?

@alexec alexec changed the title fix: roles for 2.12.0-rc1 is missing create on secerts and list on sa fix(server): Add list sa and create secret to argo-server roles. Closes #4526 Nov 13, 2020
Copy link
Contributor

@alexec alexec left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Needs back-porting to v2.12.0-rc3

@sarabala1979
Copy link
Member

@Boolman Can you fix the codegen and DCO? We are targeting this PR for RC3 release

Signed-off-by: boolman <boolman@gmail.com>
@Boolman
Copy link
Contributor Author

Boolman commented Nov 13, 2020

I dont understand why the codegen fails, it reports no change in my local branch.
And the DCO says: 'Commit sha: af85335, Author: Alex Collins, Committer: GitHub; The sign-off is missing.'

@jessesuen
Copy link
Member

I'm not 100% sure we should add list sa and create secret to argo-server role. It is new privileges only needed for SSO + RBAC.

So long as this is in the namespace of the argo-server, I don't see a problem with it

@alexec
Copy link
Contributor

alexec commented Nov 14, 2020

This looks good, but it needs either (a) run make codegen or (b) apply this patch:

diff --git a/manifests/quick-start-minimal.yaml b/manifests/quick-start-minimal.yaml
index f70741e9..5468fc25 100644
--- a/manifests/quick-start-minimal.yaml
+++ b/manifests/quick-start-minimal.yaml
@@ -612,7 +612,8 @@ spec:
         runAsNonRoot: true
       serviceAccountName: argo-server
       volumes:
-      - name: tmp
+      - emptyDir: {}
+        name: tmp
 ---
 apiVersion: apps/v1
 kind: Deployment
diff --git a/manifests/quick-start-mysql.yaml b/manifests/quick-start-mysql.yaml
index ae1b29f1..4021bb39 100644
--- a/manifests/quick-start-mysql.yaml
+++ b/manifests/quick-start-mysql.yaml
@@ -656,7 +656,8 @@ spec:
         runAsNonRoot: true
       serviceAccountName: argo-server
       volumes:
-      - name: tmp
+      - emptyDir: {}
+        name: tmp
 ---
 apiVersion: apps/v1
 kind: Deployment
diff --git a/manifests/quick-start-postgres.yaml b/manifests/quick-start-postgres.yaml
index c77ffede..12688edc 100644
--- a/manifests/quick-start-postgres.yaml
+++ b/manifests/quick-start-postgres.yaml
@@ -656,7 +656,8 @@ spec:
         runAsNonRoot: true
       serviceAccountName: argo-server
       volumes:
-      - name: tmp
+      - emptyDir: {}
+        name: tmp
 ---
 apiVersion: apps/v1
 kind: Deployment

Copy link
Contributor

@alexec alexec left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changes requested

Signed-off-by: boolman <boolman@gmail.com>
@alexec alexec merged commit 02e1f0e into argoproj:master Nov 14, 2020
@alexec
Copy link
Contributor

alexec commented Nov 14, 2020

LGTM

alexcapras pushed a commit to alexcapras/argo that referenced this pull request Dec 2, 2020
…Closes argoproj#4526 (argoproj#4514)

Signed-off-by: boolman <boolman@gmail.com>
Signed-off-by: Alex Capras <alexcapras@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants