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

fix: restrict permissions to only access specific CRB #436

Merged
merged 4 commits into from
Apr 3, 2023

Conversation

bacherfl
Copy link
Contributor

@bacherfl bacherfl commented Apr 3, 2023

This PR reduces the required permissions for the OFO to only be able to modify the open-feature-operator-flagd-kubernetes-sync ClusterRoleBinding.
Once #377 has been implemented, this permission can likely be dropped completely

@bacherfl bacherfl force-pushed the fix/reduce-permissions branch from e1b9184 to 7fa9471 Compare April 3, 2023 07:18
@codecov
Copy link

codecov bot commented Apr 3, 2023

Codecov Report

Merging #436 (8ab48d8) into main (08a50ac) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main     #436   +/-   ##
=======================================
  Coverage   78.00%   78.00%           
=======================================
  Files          21       21           
  Lines        1323     1323           
=======================================
  Hits         1032     1032           
  Misses        251      251           
  Partials       40       40           
Impacted Files Coverage Δ
webhooks/pod_webhook.go 80.61% <ø> (ø)
Flag Coverage Δ
unit-tests 78.00% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

@bacherfl bacherfl force-pushed the fix/reduce-permissions branch from 33c8260 to 203f638 Compare April 3, 2023 07:40
@bacherfl bacherfl marked this pull request as ready for review April 3, 2023 07:46
@bacherfl bacherfl requested a review from a team as a code owner April 3, 2023 07:46
Copy link
Member

@thisthat thisthat left a comment

Choose a reason for hiding this comment

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

LGTM but I would double check with the helm package step if we lock down the name there as well

@toddbaert toddbaert self-requested a review April 3, 2023 12:56
Copy link
Member

@toddbaert toddbaert left a comment

Choose a reason for hiding this comment

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

Reviewing the BackfillPermissions method, this looks right to me. I dont think we need anything else.

Once #377 has been implemented, this permission can likely be dropped completely

@bacherfl I think the above issue is basically done by @james-milligan with #412, but I think the plan was not to get rid of the direct k8s sync at least for now. So I think we will need to keep this until we decide to move away from that (if we do at all).

@beeme1mr beeme1mr merged commit 6f1f93c into open-feature:main Apr 3, 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.

5 participants