-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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
feat(profile-controller): propagate labels from CR to namespace #7481
base: master
Are you sure you want to change the base?
Conversation
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
829655b
to
116b665
Compare
Signed-off-by: droctothorpe <mythicalsunlight@gmail.com> Co-authored-by: jsonmp-k8 <paul.jaison@gmail.com> Co-authored-by: tarat44 <32471142+tarat44@users.noreply.github.com> Co-authored-by: moorthy156 <narayanamoorthy.mari@gmail.com>
@thesuperzapper per the WG meeting, let me know if you think it makes more sense to add something like |
Bump. |
I think this PR is a security issue for many users, holding for review. /hold |
I am really not sure how (or even if) we can do this safely without causing the following issues:
|
Thanks, @thesuperzapper. Regarding (1), could I trouble you to clarify your concerns with the first option? Is the concern that if profiles are created used ArgoCD, the labels will be propagated to the namespace? Why would that be a problem? Regarding (2), in our use case, end users don't have permission to edit namespaces or profiles directly. It's hard to imagine a scenario where namespace CRUD access would be disabled but profile CRUD access would be enabled, except via the UI as a privileged service. In addition, what if we contributed this feature but disabled it by default and made it contingent on a specific env var being set (that can be configured via the chart)? |
Resolves kubeflow/dashboard#51.
We noticed that there were no tests for the
Reconcile
function inprofile_controller_test.go
, otherwise we would have extended those tests to validate this specific scenario, even though it's just one line of code with very limited impact. We'd potentially be open, bandwidth permitting, to contributing broader tests forReconcile
down the road.Confirmed working E2E on our clusters. Labels applied to
profile
CRs are propagated to the corresponding namespaces.