KEP-5339: add additional cluster-specific auth info field to the cluster profile object #5559
Conversation
michaelawyu
commented
Sep 22, 2025
michaelawyu
commented
- One-line PR description: this PR updates KEP-5339 to add a field for cluster-specific auth information in the cluster profile API
- Issue link: sig-multicluster's ClusterProfile should have a way to support credentials issuance #5339
- Other comments:
k8s-ci-robot
added
the
do-not-merge/work-in-progress
k8s-ci-robot
added
kind/kep
|
Welcome @michaelawyu! |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: michaelawyu 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 |
k8s-ci-robot
added
sig/multicluster
|
Hi @michaelawyu. 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. |
k8s-ci-robot
added
the
size/M
michaelawyu
changed the title
k8s-ci-robot
added
cncf-cla: yes
k8s-ci-robot
removed
the
do-not-merge/work-in-progress
|
/ok-to-test |
k8s-ci-robot
added
ok-to-test
|
Triage notes:
|
Signed-off-by: michaelawyu <chenyu1@microsoft.com>
Signed-off-by: michaelawyu <chenyu1@microsoft.com>
Signed-off-by: michaelawyu <chenyu1@microsoft.com>
Signed-off-by: michaelawyu <chenyu1@microsoft.com>
michaelawyu
force-pushed
the
kep-5339-extensions-update
branch
from
133f634 to
bce1a46
Compare
Signed-off-by: michaelawyu <chenyu1@microsoft.com>
Signed-off-by: michaelawyu <chenyu1@microsoft.com>
|
This looks good to me, thanks; before I approve it, do you have an example handy of a real-world situation showing how these extensions would be used in practice? |
|
|
||
| The `Extensions` field in the `Cluster` struct, can hold various form of data, each associated with an extension name. | ||
| [KEP 541](https://github.com/kubernetes/enhancements/blob/master/keps/sig-auth/541-external-credential-providers/README.md) | ||
| further reserves a an extension name, `client.authentication.k8s.io/exec`, that can be used to pass cluster-specific |
There was a problem hiding this comment.
| further reserves a an extension name, `client.authentication.k8s.io/exec`, that can be used to pass cluster-specific | |
| further reserves an extension name, `client.authentication.k8s.io/exec`, that can be used to pass cluster-specific |
There was a problem hiding this comment.
Thanks, coretone! Let me fix this.
| cluster and its CA bundle), to an environment variable, `KUBERNETES_EXEC_INFO`. An exec plugin may choose to read | ||
| the environment variable, and make use of the extension data as it sees fit. | ||
|
|
||
| For the authentication workflow described in this KEP, users may, too, choose to specify a `client.authentication.k8s.io/exec` |
There was a problem hiding this comment.
how is this paragraph different from the previous one? You seem to imply that its additive to the previous one, but it seems to just explain the in-code behavior that the Extension content always overwrites any ExecConfig.Config ?
There was a problem hiding this comment.
Hi coretone! Let me simplify this sections about client.authentication.k8s.io/exec a bit so that the PR can be more focused.
| `ProvideClusterInfo` flag is unset (the default), this variable will be absent. There are a few cases where the | ||
| variable cannot be easily set: for example, the `KUBERNETES_EXEC_INFO` environment variable does feature the CA bundle for the | ||
| target cluster, which can potentially get quite large; depending on the target environment, it might not be OK to | ||
| write such data as an environment variable. Exec plugins are not mandated to support this environment variable either; |
There was a problem hiding this comment.
Exec plugins are not mandated to support this environment variable either;
I don't think its an actual issue. if they don't support the envvar, it means they don't need data that would be in it...
There was a problem hiding this comment.
Hi coretone! It is true, though I am more thinking in the way that the plugins might have other ways to take the inputs needed... Anyway, as mentioned earlier, let me simplify the sections a bit more.
| variable cannot be easily set: for example, the `KUBERNETES_EXEC_INFO` environment variable does feature the CA bundle for the | ||
| target cluster, which can potentially get quite large; depending on the target environment, it might not be OK to | ||
| write such data as an environment variable. Exec plugins are not mandated to support this environment variable either; | ||
| even for those that do read this environment variable, it is not guaranteed that the included extension data can be |
There was a problem hiding this comment.
it is not guaranteed that the included extension data can be properly extracted
This would actually be a misconfiguration? I don't think anyone must protect the extension. The paragraph from line l.319 to l.326 gives a good summary that things may breakdown if marshalling/unmarshalling doesnt work.
There was a problem hiding this comment.
Hi coretone! For this part the idea was that both the marshaller and the unmarshaller must know the expected object type (scheme) to make it work, but from the communit library's perspective it is a bit difficult to achieve so. Once again, let me simplify this a bit.
| properly extracted. | ||
|
|
||
| On the other hand, it is quite common for multi-cluster users to have a need for specifying cluster-specific information | ||
| when performing the authentication workflow: the authentication solution in use by a cluster might be expecting a token of |
There was a problem hiding this comment.
can we remove this intro or shorten it?
Maybe something like: Extensions are useful for ClusterProfile to communicate additional elements such as audience, identity guidance etc.
There was a problem hiding this comment.
Sure, let me simplify this part.
| On the other hand, it is quite common for multi-cluster users to have a need for specifying cluster-specific information | ||
| when performing the authentication workflow: the authentication solution in use by a cluster might be expecting a token of | ||
| a specific audience, or a token from a specific identity. Most, if not all, exec plugins would expect such information | ||
| from the CLI arguments or some environment variables. In a single-cluster setup, it is trivial to set them up, as the |
There was a problem hiding this comment.
Most, if not all, exec plugins would expect such information from the CLI arguments or some environment variables.
This I disagree with the wording. The mechanism described above is via KUBERNETES_EXEC_INFO and it would mostly be good for the vast majority.
I would prefer to point out on possible shortcomings of the envvar, which are:
- Length limitations (which is proposed above as a blocker, though I'd question if args don't suffer the same issues)
- Ability to leverage existing binaries that require usage of arguments instead of the envvar.
There was a problem hiding this comment.
Hi coretone! I might have a little bit different idea on the vast majority part given how well the KUBERNETES_EXEC_INFO env var is supported right now but I do get the point. Will tweak the wordings a bit to be more focused.
| To address this decifiency, we further proposes that: | ||
|
|
||
| * this KEP reserve a name in the extensions, `multicluster.x-k8s.io/clusterprofiles/auth/exec/additional-args`, which holds | ||
| additional CLI arguments that need to be supplied to the exec plugin when the Cluster Profile API and community-provided |
There was a problem hiding this comment.
| additional CLI arguments that need to be supplied to the exec plugin when the Cluster Profile API and community-provided | |
| additional CLI arguments that would be supplied to the exec plugin when the Cluster Profile API and community-provided |
There was a problem hiding this comment.
Hi coretone! Thanks, let me fix the part.
| additional CLI arguments that need to be supplied to the exec plugin when the Cluster Profile API and community-provided | ||
| library are used for authentication. | ||
|
|
||
| If an extension under this name is present, the community-provided library will extract the data, and append the |
There was a problem hiding this comment.
Hi coretone! Thanks, let me fix the part.
| that the community-provided library implementation must allow users to specify whether additional CLI arguments or environment | ||
| variables can be set by a `ClusterProfile` object. Available options include: | ||
|
|
||
| * `Ignore`: ignore any `multicluster.x-k8s.io/clusterprofiles/auth/exec/additional-args` or `multicluster.x-k8s.io/clusterprofiles/auth/exec/additional-envs` extension; no additional CLI arguments or environment variables will be set. |
There was a problem hiding this comment.
I agree with the section and default recommendation. But I'd love more details here.
Could we recommend a field similar to ProvideClusterInfo that would be AllowClusterProfileSourcedArgs (defaults to false) and AllowClusterProfileSourcedEnv (defaults to false).
Those arguments would be set in the Provider, defined in Configuring plugins in the controller
There was a problem hiding this comment.
Hi coretone! I do not have a preference on this part, let me make the edits.
|
See also this PR: kubernetes-sigs/cluster-inventory-api#27 |
|
I created the following PR, and discussed and agreed with corentone to incorporate the extensions specification. kubernetes-sigs/cluster-inventory-api#21 After this implementation was merged, I immediately created the PR for #5643 and requested a review on Slack. This PR seems to have been opened before my PR, but the commit dates suggest my PR is older. |
Hi @kahirokunn! I don't think this PR is a duplicate of #5643 though; as you can see from the changes in the PR, the primary goal of this PR was to reserve additional, multi-cluster specific extensions, a) it is an existing mechanism defined in previous KEPs that we are supposed to support; and and the discussion scope about With this being said, would it make things easier if I simplify the PR a bit to focus more on the newly reserved extensions and less about |
Signed-off-by: michaelawyu <chenyu1@microsoft.com>
k8s-ci-robot
added
size/L
Signed-off-by: michaelawyu <chenyu1@microsoft.com>
|
Thank you for the clarification! That makes perfect sense to me. |
|
I approved #5643; I think we can get it in and rebase that one on top. Indeed there is alignment and just the wording is different. Thank you for applying my suggestions and editing the text! Let's align the PRs content, do a final review and push this! |
7 participants
