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

Jump to bottom

feat(rbac) : add editor and viewer role for crds #1165

Merged
merged 1 commit into from Nov 19, 2019
Merged

feat(rbac) : add editor and viewer role for crds #1165

merged 1 commit into from Nov 19, 2019

Conversation

camilamacedo86
Copy link
Member

@camilamacedo86 camilamacedo86 commented Nov 8, 2019 •
edited

Description

  • Add editor and viewer roles for each CRD

Motivation
Closes: #886

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Nov 8, 2019
@k8s-ci-robot k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Nov 8, 2019
@camilamacedo86
Copy link
Member Author

/assign @mengqiy

mengqiy
mengqiy requested changes Nov 9, 2019
View reviewed changes
pkg/scaffold/v2/crd_editor_rbac.go Outdated Show resolved Hide resolved
pkg/scaffold/v2/crd_viewer_rbac.go Outdated Show resolved Hide resolved
pkg/scaffold/v2/crd_viewer_rbac.go Outdated Show resolved Hide resolved
pkg/scaffold/v2/crd_editor_rbac.go Outdated Show resolved Hide resolved
@camilamacedo86
Copy link
Member Author

camilamacedo86 commented Nov 9, 2019 •
edited

Hi @mengqiy,

Really thank you. I was looking for exactly this kind of review. I will be working on to address it.

mengqiy
mengqiy reviewed Nov 11, 2019
View reviewed changes
testdata/project-v2/config/rbac/admiral_viewer_role.yaml Outdated Show resolved Hide resolved
pkg/scaffold/v2/crd_editor_rbac.go Show resolved Hide resolved
pkg/scaffold/v2/crd_editor_rbac.go Outdated Show resolved Hide resolved
pkg/scaffold/v2/crd_editor_rbac.go Outdated Show resolved Hide resolved
pkg/scaffold/v2/crd_viewer_rbac.go Outdated Show resolved Hide resolved
@camilamacedo86 camilamacedo86 force-pushed the gen-crd-roles branch 2 times, most recently from 8bf7d1a to 4472e50 Compare November 11, 2019 22:43
mengqiy
mengqiy reviewed Nov 12, 2019
View reviewed changes
pkg/scaffold/v2/crd_editor_rbac.go Outdated Show resolved Hide resolved
pkg/scaffold/v2/crd_viewer_rbac.go Outdated Show resolved Hide resolved
@mengqiy
Copy link
Member

mengqiy commented Nov 16, 2019

Travis is failing. Please fix.

@camilamacedo86 camilamacedo86 force-pushed the gen-crd-roles branch 2 times, most recently from 18db06b to 3e6ff53 Compare November 16, 2019 13:47
@k8s-ci-robot
Copy link
Contributor

@camilamacedo86: The following tests failed, say /retest to rerun them all:

Test name Commit Details Rerun command
pull-kubebuilder-e2e 081a114 link /test pull-kubebuilder-e2e
pull-kubebuilder-test 081a114 link /test pull-kubebuilder-test

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

@camilamacedo86
Copy link
Member Author

Hi @mengqiy,

Now, the test is fixed and all is passing. Please, feel free to check this one.

mengqiy
mengqiy approved these changes Nov 19, 2019
View reviewed changes
Copy link
Member

@mengqiy mengqiy left a comment

Choose a reason for hiding this comment

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

/lgtm
/approve

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 19, 2019
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: camilamacedo86, mengqiy

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 19, 2019
@k8s-ci-robot k8s-ci-robot merged commit 9ca3349 into kubernetes-sigs:master Nov 19, 2019
erictune
erictune reviewed Nov 19, 2019
View reviewed changes
testdata/project-v2/config/rbac/firstmate_editor_role.yaml
- crew.testproject.org
resources:
- firstmates/status
verbs:
Copy link

Choose a reason for hiding this comment

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

I was comparing this with the build-in cluster role edit which is you can see with kubectl get clusterroles edit -o yaml. The edit role gives read and write to most of the built-in kubernetes resources. I noted that it does not give patch or update permissions on any */status resources. Is it by design that these generated roles are different?

Copy link
Member

Choose a reason for hiding this comment

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

If */status should be included depends on who will be granted these permission.
For controllers, permissions to edit */status is necessary.
For human, they should never edit */status.

Reading #886 again, it seems that means human users.
I guess we can still revise it and make it clear that it is for end users.

Copy link
Member Author

Choose a reason for hiding this comment

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

I was comparing this with the build-in cluster role edit which is you can see with kubectl get clusterroles edit -o yaml.

The roles done here are to give permissions to the specific resource.

I noted that it does not give patch or update permissions on any */status resources.

I understand that the idea is to allow admin using this role to create a group of permissions. So, the status is required for the operator to update the CR's and users would also would like to check the status. Am I right?

I am not sure if I understood what should be done in another way here. May the question in order to clarify could be what behaviour are you facing or should not be facing when these roles are applied?

@mengqiy mengqiy mentioned this pull request Nov 20, 2019
Closed
@camilamacedo86 camilamacedo86 deleted the gen-crd-roles branch November 20, 2019 19:24
@mengqiy mengqiy mentioned this pull request Nov 25, 2019
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mengqiy mengqiy mengqiy approved these changes

@erictune erictune erictune left review comments

@droot droot Awaiting requested review from droot

@Liujingfang1 Liujingfang1 Awaiting requested review from Liujingfang1

Assignees

@mengqiy mengqiy

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Generate suggested Roles for CR users
4 participants
@camilamacedo86 @mengqiy @k8s-ci-robot @erictune