Skip to content

feat: add keycloak rolebindings #144

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

Open
wants to merge 11 commits into
base: main
Choose a base branch
from

Conversation

udaltsovra
Copy link
Contributor

@udaltsovra udaltsovra commented Jan 17, 2025

Summary

Task: WRDOC-823

  • Added RoleBinding and ClusterRoleBinding for auth to cluster via keycloak in infra-v3

Tested on wrdoc-prod

Related PRs:

@udaltsovra udaltsovra marked this pull request as ready for review January 17, 2025 06:11
@udaltsovra udaltsovra self-assigned this Jan 17, 2025
@udaltsovra udaltsovra added enhancement New feature or request Needs Review labels Jan 17, 2025
@udaltsovra udaltsovra changed the title Added keycloak rolebindings feat: add keycloak rolebindings Jan 17, 2025
kind: RoleBinding
metadata:
name: keycloak:developer
namespace: prod
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't it be namespace: {{ .Release.Namespace }}?

Copy link
Contributor

@dmitry-mightydevops dmitry-mightydevops left a comment

Choose a reason for hiding this comment

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

I made changes
82cf922..f5596cd
you don't need to add new roles or rolebindings
use existing ones.

So test my new release version: 0.1.13-dev.2 in wrdoc and then reopen or fix as needed

@udaltsovra udaltsovra marked this pull request as draft January 20, 2025 13:09
@udaltsovra udaltsovra marked this pull request as ready for review January 21, 2025 04:25
Copy link
Contributor

@kseniyashaydurova kseniyashaydurova left a comment

Choose a reason for hiding this comment

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

Approved, but pls add more comments

@@ -6,7 +6,7 @@ type: application
# This is the chart version. This version number should be incremented each time you make changes
# to the chart and its templates, including the app version.
# Versions are expected to follow Semantic Versioning (https://semver.org/)
version: 0.1.12
version: 0.1.13-dev.4
Copy link
Contributor

Choose a reason for hiding this comment

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

Don't forget to change it on merge to have just 0.1.13

@@ -43,5 +43,7 @@ subjects:
- apiGroup: rbac.authorization.k8s.io
Copy link
Contributor

Choose a reason for hiding this comment

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

Pls add comment where this group comes from?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@@ -24,5 +24,8 @@ subjects:
- apiGroup: rbac.authorization.k8s.io
kind: Group
name: saritasa:sso:developers

# this group is coming from keycloak sso via oidc
- apiGroup: rbac.authorization.k8s.io
Copy link
Contributor

Choose a reason for hiding this comment

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

  1. The same comment as for apps.yaml
  2. Pls specify comments in similar way everywhere

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@@ -31,5 +31,8 @@ subjects:
- apiGroup: rbac.authorization.k8s.io
kind: Group
name: saritasa:sso:developers
- apiGroup: rbac.authorization.k8s.io # this group is coming from keycloak sso via oidc
Copy link
Contributor

Choose a reason for hiding this comment

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

The same comment as for apps.yaml

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@@ -31,5 +31,8 @@ subjects:
- apiGroup: rbac.authorization.k8s.io
kind: Group
name: saritasa:sso:developers
- apiGroup: rbac.authorization.k8s.io # this group is coming from keycloak sso via oidc
kind: Group
name: developer

Copy link
Contributor

Choose a reason for hiding this comment

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

Remove empty line pls

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@@ -24,5 +24,7 @@ subjects:
- apiGroup: rbac.authorization.k8s.io
kind: Group
name: saritasa:sso:developers

- apiGroup: rbac.authorization.k8s.io # this group is coming from keycloak sso via oidc
Copy link
Contributor

Choose a reason for hiding this comment

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

The same comment as for apps.yaml

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

- apiGroup: rbac.authorization.k8s.io
kind: User
name: saritasa-sso-devops

- apiGroup: rbac.authorization.k8s.io # this group is coming from keycloak sso via oidc
Copy link
Contributor

Choose a reason for hiding this comment

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

Pls add comments for other roles, where do they come from too

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@@ -40,8 +40,10 @@ roleRef:
kind: Role
name: saritasa-developers-readonly-role
subjects:
- apiGroup: rbac.authorization.k8s.io
- apiGroup: rbac.authorization.k8s.io # this group is creating when sso stack applied in infra-v3 and when saritasa-sso module applied in infra-v2
Copy link
Contributor

Choose a reason for hiding this comment

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

May be move comments to line above? Now it looks like it's a comment for this particular value, not the whole subject?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request Needs Review
Projects
None yet
Development

Successfully merging this pull request may close these issues.