-
Notifications
You must be signed in to change notification settings - Fork 1.6k
Update KEP-4322 to account for ClusterProfile CR being namespaced #5457
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
Update KEP-4322 to account for ClusterProfile CR being namespaced #5457
Conversation
Welcome @ostrain! |
Hi @ostrain. Thanks for your PR. I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
/cc ryanzhang-oss |
@ostrain: GitHub didn't allow me to request PR reviews from the following users: ryanzhang-oss. Note that only kubernetes members and repo collaborators can review this PR, and authors cannot review their own PRs. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
FYI @ryanzhang-oss Thanks @ostrain! /ok-to-test |
An alternative thought I had is we could consider removing this stuff and making this section less prescriptive about what the secret should look like. Instead, we could recommend using the new external credential provider mechanism (https://github.com/kubernetes/enhancements/blob/master/keps/sig-multicluster/5339-clusterprofile-plugin-credentials/README.md), and then there could be one (or more) such plugins that reads creds from a secret, and the plugin could define what the secret should look like. That KEP has a trivial sketch of such a secret-based plugin: https://github.com/kubernetes/enhancements/blob/master/keps/sig-multicluster/5339-clusterprofile-plugin-credentials/README.md#secret-reader-plugin |
This would also remove ambiguity of how the consumer should behave if the secret described by this section exists but the ClusterProfile also specifies |
This looks good to me, but I'll pop the merge if I hard lgtm and I want @ryanzhang-oss to look. I see what you mean in #5457 (comment) however. If we are staying prescriptive, I think a follow on with an API example of Push Model via Credentials in Secret inline here or in an appendix would be nice. I feel its implied that CredentialProvider should take preference over the "Not Recommended" secret provider but is not explicit. Are we ok to leave that to implementations to decide? Seems ok, possibly even preferred, to me but depends what you are going for. |
/assign @ryanzhang-oss I'd love your input on this when you get a chance, thanks! |
@ostrain: GitHub didn't allow me to assign the following users: ryanzhang-oss. Note that only kubernetes members with read permissions, repo collaborators and people who have commented on this issue/PR can be assigned. Additionally, issues/PRs can only have 10 assignees at the same time. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
/lgtm |
/approve |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: ostrain, ryanzhang-oss, skitt 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 |
The "Push Model via Credentials in Secret" section defines a label to associate the secret with a ClusterProfile but only accounts for the ClusterProfile's name.
ClusterProfiles are namespaced objects so we need a way to specify the namespace as well (the secret is in a per-consumer namespace whereas the ClusterProfile is shared across consumers so is likely in a different namespace).
This PR adds a second label to specify the ClusterProfile namespace.