feat: add support for additional CLI arg/env var extensions + add azure/kubelogin based example #27
Conversation
Signed-off-by: michaelawyu <chenyu1@microsoft.com>
k8s-ci-robot
added
the
cncf-cla: yes
|
Welcome @michaelawyu! |
k8s-ci-robot
added
the
size/L
michaelawyu
changed the title
Signed-off-by: michaelawyu <chenyu1@microsoft.com>
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: michaelawyu, ryanzhang-oss 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 |
k8s-ci-robot
added
the
approved
….20.4 🌱 Update to controller-runtime v0.20.4
pkg/credentials/config.go
Outdated
| ExecConfig *clientcmdapi.ExecConfig `json:"execConfig"` | ||
| Name string `json:"name"` | ||
| ExecConfig *clientcmdapi.ExecConfig `json:"execConfig"` | ||
| AdditionalCLIArgEnvVarExtensionFlag AdditionalCLIArgEnvVarExtensionFlag `json:"additionalCLIArgEnvVarExtensionFlag,omitempty"` |
There was a problem hiding this comment.
That looks like a verify long name...and if the value is int, I think it would be confusing in json format?
There was a problem hiding this comment.
Hi qiujian16! Yeah, let me make the edits. corentone also suggested using separate flags for CLI args and env vars; I will change accordingly as well.
pkg/credentials/config.go
Outdated
| for idx := range clusterAccessor.Cluster.Extensions { | ||
| ext := &clusterAccessor.Cluster.Extensions[idx] | ||
|
|
||
| switch { |
There was a problem hiding this comment.
might be
if additionalCLIArgEnvVarsExtFlag {
switch ext.Name {
case additionalCLIArgsExtensionKey:
....
}
}There was a problem hiding this comment.
Hi qiujian16! I've made some edits based on the suggestion and the latest KEP changes; thanks 🙏
pkg/credentials/config.go
Outdated
| env := &execConfig.Env[idx] | ||
| if _, exists := additionalEnvs[env.Name]; exists { | ||
| env.Value = additionalEnvs[env.Name] | ||
| delete(additionalEnvs, env.Name) |
There was a problem hiding this comment.
to make it simpler, how about adding any item in execConfig.Env that does not exist in additionalEnvs into additionalEnvs. So in the loop in line 121, you can construct a new env list.
…entory-api into feat/add-support-for-additional-cli-arg-env-var-extension
Signed-off-by: michaelawyu <chenyu1@microsoft.com>
| // two reserved extensions defined in KEP 5339, which allows users to pass in (usually cluster-specific) | ||
| // additional command-line arguments and environment variables to the exec plugin from | ||
| // the ClusterProfile API side. | ||
| additionalCLIArgsExtensionKey = "multicluster.x-k8s.io/clusterprofiles/auth/exec/additional-args" |
There was a problem hiding this comment.
I would suggest
clusterprofiles.multicluster.x-k8s.io/exec/additional-args
clusterprofiles.multicluster.x-k8s.io/exec/additional-envs
| ExecConfig *clientcmdapi.ExecConfig `json:"execConfig"` | ||
| Name string `json:"name"` | ||
| ExecConfig *clientcmdapi.ExecConfig `json:"execConfig"` | ||
| AllowProfileSourcedCLIArgs bool `json:"allowProfileSourcedCLIArgs,omitempty"` |
There was a problem hiding this comment.
Instead of boolean, how do you think if we can set a this as Merge, Replace, or Ignore ?
4 participants
This PR adds support for the proposed additional CLI argument/environment variable extensions in the ClusterProfile API.
The PR is submitted right now to showcase how the system would function with the new extensions; it can be merged when PR #5559 (to the k8s/enhancements repo) is merged and all changes look good.