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

Remove duplicate pod RBAC #2336

Closed
wants to merge 1 commit into from

Conversation

dfarrell07
Copy link
Member

@dfarrell07 dfarrell07 commented Nov 10, 2022

The RBAC for pods duplicates the get permission, as it's granted * elsewhere.

This was recently modified in #2225, #2214, and #2008.

Signed-off-by: Daniel Farrell [email protected]

@submariner-bot
Copy link
Contributor

🤖 Created branch: z_pr2336/dfarrell07/gateway_pod_rbac
🚀 Full E2E won't run until the "ready-to-test" label is applied. I will add it automatically once the PR has 2 approvals, or you can add it manually.

@dfarrell07
Copy link
Member Author

@skitt How did you determine in #2008 that pods didn't need some permissions? Just looking at the code?

@tpantelis How did you spot the error in #2225, as it didn't seem to fail CI?

I ask because I don't know if pods really needs *, or if we can get away with a subset.

@tpantelis
Copy link
Contributor

How did you spot the error in #2225, as it didn't seem to fail CI?

It did fail CI according to the PR summary.

@dfarrell07
Copy link
Member Author

How did you spot the error in #2225, as it didn't seem to fail CI?

It did fail CI according to the PR summary.

Ah okay, I see. I actually linked to the wrong PR there, this is the gateway which was PR #2214. But still, the point is that it should have failed CI on the PR that made the change, not have been merged.

@dfarrell07 dfarrell07 changed the title Remove duplicate gateway pod RBAC Remove duplicate pod RBAC Nov 11, 2022
@tpantelis
Copy link
Contributor

Ah okay, I see. I actually linked to the wrong PR there, this is the gateway which was PR #2214. But still, the point is that it should have failed CI on the PR that made the change, not have been merged.

It changed the RBAC yaml and the associated Go file but those are used by subctl so they won't take effect until the Go dependency version for submariner-operator is updated in the subctl repo. This is a side-effect of splitting out the subctl repo.

@dfarrell07
Copy link
Member Author

Ah okay, I see. I actually linked to the wrong PR there, this is the gateway which was PR #2214. But still, the point is that it should have failed CI on the PR that made the change, not have been merged.

It changed the RBAC yaml and the associated Go file but those are used by subctl so they won't take effect until the Go dependency version for submariner-operator is updated in the subctl repo. This is a side-effect of splitting out the subctl repo.

Thanks for clarifying Tom, that makes sense.

@stale
Copy link

stale bot commented Dec 16, 2022

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the wontfix This will not be worked on label Dec 16, 2022
@submariner-bot submariner-bot added the ready-to-test When a PR is ready for full E2E testing label Dec 17, 2022
@stale stale bot removed the wontfix This will not be worked on label Dec 17, 2022
The  RBAC for pods duplicates the get permission, as it's granted *
elsewhere.

This was recently modified in submariner-io#2225, submariner-io#2214, and submariner-io#2008.

Signed-off-by: Daniel Farrell <[email protected]>
@stale
Copy link

stale bot commented Dec 31, 2022

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the wontfix This will not be worked on label Dec 31, 2022
@tpantelis tpantelis removed the wontfix This will not be worked on label Jan 2, 2023
Copy link
Member

@skitt skitt left a comment

Choose a reason for hiding this comment

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

I would rather move in the other direction: instead of removing explicit permissions which are subsumed by wildcard permissions, remove the wildcard permissions and explicitly list all the permissions we really need (ideally, auto-generated from code comments).

@dfarrell07
Copy link
Member Author

Closing in favor of #1105

@tpantelis tpantelis closed this Jan 18, 2023
@submariner-bot
Copy link
Contributor

🤖 Closed branches: [z_pr2336/dfarrell07/gateway_pod_rbac]

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready-to-test When a PR is ready for full E2E testing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants