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

Refactor kyma roles #3473

Conversation

Tomasz-Smelcerz-SAP
Copy link
Member

@Tomasz-Smelcerz-SAP Tomasz-Smelcerz-SAP commented Apr 1, 2019

Description

Refactor three main kyma cluster roles (kyma-admin, kyma-view, kyma-edit) so that each role only aggregates other roles.

Purpose: improving maintainability of roles.
Note: This PR should not introduce any new permissions nor modify existing ones, it's just refactoring.

Related issue(s)
See also #2896

@Tomasz-Smelcerz-SAP Tomasz-Smelcerz-SAP self-assigned this Apr 1, 2019
@Tomasz-Smelcerz-SAP Tomasz-Smelcerz-SAP added the area/security Issues or PRs related to security label Apr 1, 2019
@Tomasz-Smelcerz-SAP Tomasz-Smelcerz-SAP added this to the Sprint_Goat_9 milestone Apr 1, 2019
@Tomasz-Smelcerz-SAP
Copy link
Member Author

/test pre-master-kyma-gke-integration

@Tomasz-Smelcerz-SAP
Copy link
Member Author

/test pre-master-kyma-gke-upgrade

@Demonsthere
Copy link

Good job, but as you expected the number of roles exploded, which makes it a lot more difficult to manage. How about we split the definition into multiple files, separated by ApiGroups (istio, k8s, kyma) to reduce the size of a single file?

apiVersion: rbac.authorization.k8s.io/v1beta1
kind: ClusterRole
metadata:
name: kyma-crd-list

Choose a reason for hiding this comment

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

This might me more useable as kyma-crd-view, with list, get verbs. List alone seems lacking

Copy link
Member Author

@Tomasz-Smelcerz-SAP Tomasz-Smelcerz-SAP Apr 1, 2019

Choose a reason for hiding this comment

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

I changed the name, but I don't want to change actual verb - this is only refactoring PR. Changes/fixes/improvements will be introduced in a separate PR.

"helm.sh/hook-weight": "0"
rules:
- apiGroups:
- "applicationconnector.kyma-project.io"

Choose a reason for hiding this comment

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

How about set all kyma apiGroups as a helm variable? This will allow to modify the list in one place when a new one is introduced

Copy link
Member Author

Choose a reason for hiding this comment

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

Good point - I'll extract common groups to a helm value.

@Tomasz-Smelcerz-SAP
Copy link
Member Author

/test pre-master-kyma-integration

@Tomasz-Smelcerz-SAP
Copy link
Member Author

/test pre-master-kyma-gke-upgrade

1 similar comment
@Tomasz-Smelcerz-SAP
Copy link
Member Author

/test pre-master-kyma-gke-upgrade

@Tomasz-Smelcerz-SAP
Copy link
Member Author

/test pre-master-kyma-integration

@Tomasz-Smelcerz-SAP
Copy link
Member Author

/test pre-master-kyma-gke-integration

@Tomasz-Smelcerz-SAP
Copy link
Member Author

/test pre-master-kyma-gke-central-connector

@Tomasz-Smelcerz-SAP
Copy link
Member Author

/test pre-master-kyma-gke-upgrade

@Tomasz-Smelcerz-SAP
Copy link
Member Author

/test pre-master-kyma-gke-central-connector

@Tomasz-Smelcerz-SAP
Copy link
Member Author

/test pre-master-kyma-integration

@Tomasz-Smelcerz-SAP
Copy link
Member Author

/retest

@Tomasz-Smelcerz-SAP Tomasz-Smelcerz-SAP merged commit 1d02d67 into kyma-project:master Apr 2, 2019
@Tomasz-Smelcerz-SAP Tomasz-Smelcerz-SAP deleted the refactor-kyma-roles branch April 2, 2019 23:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/security Issues or PRs related to security
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants