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.

Sign up for GitHub

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

Auto-generate RBAC manifests by the controller-gen #1815

Merged
merged 1 commit into from
Jun 2, 2023
Merged

Auto-generate RBAC manifests by the controller-gen #1815

merged 1 commit into from
Jun 2, 2023

Conversation

Syulin7
Copy link
Contributor

@Syulin7 Syulin7 commented May 31, 2023

Which issue(s) this PR fixes (optional, in Fixes #<issue number>, #<issue number>, ... format, will close the issue(s) when PR gets merged):
Fixes #1814

Auto-generate RBAC manifests by the controller-gen and remove unused permissions：

  • endpoints
  • deployments
  • secrets

@coveralls
Copy link

coveralls commented May 31, 2023 •
edited

Pull Request Test Coverage Report for Build 5139444961

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • 9 unchanged lines in 1 file lost coverage.
  • Overall coverage decreased (-0.03%) to 33.791%
Files with Coverage Reduction New Missed Lines %
pkg/controller.v1/pytorch/pytorchjob_controller.go 9 58.19%
Totals Coverage Status
Change from base Build 5135774776: -0.03%
Covered Lines: 3324
Relevant Lines: 9837
💛 - Coveralls

tenzen-y
tenzen-y reviewed May 31, 2023
View reviewed changes
Copy link
Member

@tenzen-y tenzen-y left a comment

Choose a reason for hiding this comment

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

Thanks for this.
Overall is lgtm.
I left comments for some nits.

Makefile
@@ -37,7 +37,7 @@ help: ## Display this help.
##@ Development

manifests: controller-gen ## Generate WebhookConfiguration, ClusterRole and CustomResourceDefinition objects.
$(CONTROLLER_GEN) $(CRD_OPTIONS) rbac:roleName=manager-role webhook paths="./pkg/apis/..." output:crd:artifacts:config=manifests/base/crds
$(CONTROLLER_GEN) $(CRD_OPTIONS) rbac:roleName=training-operator webhook paths="./pkg/..." output:crd:artifacts:config=manifests/base/crds output:rbac:artifacts:config=manifests/base/rbac
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
$(CONTROLLER_GEN) $(CRD_OPTIONS) rbac:roleName=training-operator webhook paths="./pkg/..." output:crd:artifacts:config=manifests/base/crds output:rbac:artifacts:config=manifests/base/rbac
$(CONTROLLER_GEN) $(CRD_OPTIONS) rbac:roleName=manager-role webhook paths="./pkg/..." output:crd:artifacts:config=manifests/base/crds output:rbac:artifacts:config=manifests/base/rbac

Can we keep using manager-role for backward compatibility?

Copy link
Contributor Author

@Syulin7 Syulin7 May 31, 2023 •
edited

Choose a reason for hiding this comment

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

Actually, RBAC manifests were not generated by controller-gen previously（Because the path=./pkg/apis/... does not include the kubebuilder RBAC flag, which is in ./pkg/controller.v1/...）, and the role name was training-operator. Therefore, to maintain backward compatibility, using training-operator.

training-operator/manifests/base/cluster-role.yaml

Lines 2 to 7 in fc8a644

apiVersion: rbac.authorization.k8s.io/v1
kind: ClusterRole
metadata:
labels:
app: training-operator
name: training-operator

Copy link
Member

Choose a reason for hiding this comment

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

Oh, I see. Thank you for the clarification!

pkg/controller.v1/tensorflow/tfjob_controller.go
Comment on lines +136 to +138
//+kubebuilder:rbac:groups=scheduling.volcano.sh,resources=podgroups,verbs=get;list;watch;create;update;patch;delete
//+kubebuilder:rbac:groups=scheduling.sigs.k8s.io,resources=podgroups,verbs=get;list;watch;create;update;patch;delete
//+kubebuilder:rbac:groups="",resources=events,verbs=get;list;watch;create;update;patch;delete
Copy link
Member

Choose a reason for hiding this comment

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

Can we add these markers to all controllers to clarify the needed roles in each controller?

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

tenzen-y
tenzen-y reviewed May 31, 2023
View reviewed changes
Copy link
Member

@tenzen-y tenzen-y left a comment

Choose a reason for hiding this comment

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

/lgtm
/assign @johnugeorge

@google-oss-prow google-oss-prow bot added the lgtm label May 31, 2023
andreyvelich
andreyvelich reviewed May 31, 2023
View reviewed changes
manifests/base/rbac/role.yaml Outdated
apiVersion: rbac.authorization.k8s.io/v1
kind: ClusterRole
metadata:
creationTimestamp: null
Copy link
Member

Choose a reason for hiding this comment

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

Why it generates creationTimestamp ?

Copy link
Member

Choose a reason for hiding this comment

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

That is a bug of the controller-gen.
We should upgrade controller-tools to v0.11 or later.

Copy link
Member

Choose a reason for hiding this comment

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

Currently, we use v0.10.0.

training-operator/Makefile

Line 111 in 93c758c

GOBIN=$(PROJECT_DIR)/bin go install sigs.k8s.io/controller-tools/cmd/controller-gen@v0.10.0

Copy link
Member

Choose a reason for hiding this comment

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

Although, we should upgrade k8s libraries version before we upgrade controller-gen version.

Copy link
Member

Choose a reason for hiding this comment

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

Oh, I missed putting a link: kubernetes-sigs/controller-tools#800

andreyvelich
andreyvelich reviewed May 31, 2023
View reviewed changes
Copy link
Member

@andreyvelich andreyvelich left a comment

Choose a reason for hiding this comment

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

Thank you for adding this @Syulin7!
Can we move ServiceAccount to RBAC folder also ?
Since it is needed for ClusterRole.

@google-oss-prow google-oss-prow bot removed the lgtm label May 31, 2023
@Syulin7
Copy link
Contributor Author

Syulin7 commented May 31, 2023

@andreyvelich @tenzen-y Thanks for the review! please take a look again.

@tenzen-y
Copy link
Member

@andreyvelich @tenzen-y Thanks for the review! please take a look again.

Looks good. Once #1817 is merged into the master branch, let's merge this PR.

/lgtm
/hold

@johnugeorge
Copy link
Member

@Syulin7 Can you do a rebase?

@Syulin7
Auto-generate RBAC manifests by the controller-gen
4dc69a4 
Signed-off-by: Syulin7 <735122171@qq.com>
@google-oss-prow google-oss-prow bot removed the lgtm label Jun 1, 2023
tenzen-y
tenzen-y reviewed Jun 1, 2023
View reviewed changes
Copy link
Member

@tenzen-y tenzen-y left a comment

Choose a reason for hiding this comment

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

/lgtm
/hold cancel
/assign @johnugeorge

@johnugeorge
Copy link
Member

Thanks for this feature

/approve

@google-oss-prow
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: johnugeorge, Syulin7

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

@google-oss-prow google-oss-prow bot merged commit 4043955 into kubeflow:master Jun 2, 2023
21 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@andreyvelich andreyvelich andreyvelich left review comments

@tenzen-y tenzen-y tenzen-y left review comments

@kuizhiqing kuizhiqing Awaiting requested review from kuizhiqing

Assignees

@johnugeorge johnugeorge

@tenzen-y tenzen-y

Labels
approved lgtm size/L
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Remove the permissions related to secrets.
5 participants
@Syulin7 @coveralls @tenzen-y @johnugeorge @andreyvelich