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

Add reminders to avoid RBAC synchronization bug #576

Merged
merged 2 commits into from
Sep 20, 2022

Conversation

kevin85421
Copy link
Member

Why are these changes needed?

In #164, @Jeffwan mentions that RBAC files cannot be updated automatically with the command make manifests. The root cause is the lack of a newline after kubebuilder markers. This issue is same as kubernetes-sigs/controller-tools#551.

In addition, the permissions of config/rbac/role.yaml is the union of kubebuilder markers in raycluster_controller.go, rayjob_controller.go, and rayservice_controller.go.

Related issue number

closes #164

Checks

  • I've made sure the tests are passing.
  • Testing Strategy
    • Unit tests
    • Manual tests
    • This PR is not tested :(
  • Step1: Add the following marker in one of "xxxxx_controller.go"
    • // +kubebuilder:rbac:groups=apps,resources=deployments,verbs=get;list;watch;create;update;delete
  • Step2: make manifests
  • Step3: Check config/role.yaml
    • There is no kubebuilder marker defining permissions of Deployment, and thus role.yaml will be updated.

@@ -80,6 +80,7 @@ type RayClusterReconciler struct {
// +kubebuilder:rbac:groups="rbac.authorization.k8s.io",resources=roles,verbs=get;list;watch;create;delete;update
// +kubebuilder:rbac:groups="rbac.authorization.k8s.io",resources=rolebindings,verbs=get;list;watch;create;delete

// [WARNING]: There MUST be a newline after kubebuilder markers.
Copy link
Collaborator

Choose a reason for hiding this comment

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

em. is not line 82 a new line? I may forgot the tricks here.

Copy link
Member Author

Choose a reason for hiding this comment

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

The new line is added by #577. Originally, it does not have a newline.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I mean the new line was added in #577 and the bug should be fixed? This PR is to add some comment to reminder developer? But the title is the bug fix?

Copy link
Member Author

Choose a reason for hiding this comment

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

Got it. The title is confusing. I opened this PR prior to #577, but #577 was merged prior to this PR. Then, I rebased this branch with upstream. I will update the title. Thank you for pointing this out!

@kevin85421 kevin85421 changed the title [Bug] RBAC file can not be updated automatically Add reminders to avoid RBAC synchronization bug Sep 20, 2022
@Jeffwan Jeffwan merged commit e9544fc into ray-project:master Sep 20, 2022
lowang-bh pushed a commit to lowang-bh/kuberay that referenced this pull request Sep 24, 2023
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.

[Bug] RBAC file can not be updated automatically
2 participants