KEP-3104: Introduce kuberc #3392
Conversation
k8s-ci-robot
added
cncf-cla: yes
There was a problem hiding this comment.
This is such a great feature, thank you @eddiezane, I am really looking forward to it!
Reading through the proposal I am assuming that the kuberc definitions will apply globally to every cluster kubectl connects to, right? Whatever the answer may be (global or per cluster) I think it should be stated in the document to fully document the file behavior.
Again, great idea!
| command: get pods -l what=database --namespace us-2-production | ||
|
|
||
| commandOverrides: | ||
| - command: apply |
There was a problem hiding this comment.
how are subcommands indicated? e.g. kubectl create secret or kubectl auth reconcile?
There was a problem hiding this comment.
I think we should pattern match on the string create and create secret? Implementation wise this lets up grab the initial argv and then match in the commands?
Unsure but great question. Do you have any thoughts?
There was a problem hiding this comment.
This will really come down to what Cobra enables. It doesn't seem to be much right now but I am hoping we can expose some more information to commands.
| proposal will be implemented, this is the place to discuss them. | ||
| --> | ||
|
|
||
| The file will default to being located in `~/.kube/kuberc`. A flag will allow overriding this default location with a path i.e. `kubectl --kuberc /var/kube/rc`. |
There was a problem hiding this comment.
is this an opportunity to address some of the issues raised in kubernetes/kubernetes#80399, #2229, or #2111 for this new file?
There was a problem hiding this comment.
We opted out of XDG because it would cause an awful partition across tooling that didn't upgrade client-go.
@BenTheElder had an idea on how we can introduce a ~/.kube/config.d folder separate from this.
There was a problem hiding this comment.
We opted out of XDG because it would cause an awful partition across tooling that didn't upgrade client-go.
For existing files, sure. For this net new file, it's worth considering, at least.
There was a problem hiding this comment.
@soltysh what are your thoughts here? My first thought is introducing a new location will just confuse users when kubeconfigs will not be loaded from there.
There was a problem hiding this comment.
I think it might be worth explicitly addressing in the KEP but I also lean towards it being confusing to have two sets of rules as to what directory to use as for reasoning why we aren't doing that.
There was a problem hiding this comment.
I agree with Ben, referring to that XDG (especially it's there as rejected) and make it explicit why we choose this approach is good idea.
| apiVersion: v1alpha1 | ||
|
|
||
| commandAliases: | ||
| - alias: getdbprod |
There was a problem hiding this comment.
how are subcommand aliases indicated? e.g. kubectl create mycustomobj
There was a problem hiding this comment.
Maybe we can start without sub-command aliases, (I haven't used those in git, not sure if that's possible at all), but we can say that we might re-visit this decision in the future, if needed.
There was a problem hiding this comment.
we should at least consider the shape, in case it informs this field structure (e.g. list instead of string)
There was a problem hiding this comment.
git doesn't seem to support this. I think this would be really tricky to do with little gain.
We could potentially match on the full path alias: create mycustomobj but that will have issues with shadowing.
Open to ideas.
There was a problem hiding this comment.
A couple of comments, and you're also missing PRR file in https://github.com/kubernetes/enhancements/tree/master/keps/prod-readiness/sig-cli
| apiVersion: v1alpha1 | ||
|
|
||
| commandAliases: | ||
| - alias: getdbprod |
There was a problem hiding this comment.
Maybe we can start without sub-command aliases, (I haven't used those in git, not sure if that's possible at all), but we can say that we might re-visit this decision in the future, if needed.
|
|
||
| commandAliases: | ||
| - alias: getdbprod | ||
| command: get pods -l what=database --namespace us-2-production |
There was a problem hiding this comment.
How do you see this command being executed? Is it just going to run whatever text you put here as a shell command?
Would there be a benefit to allowing variables to be substituted into the command, maybe just positionally like with a bash script ($1, $2, etc) or possibly named? That is one of the limitations of bash aliases.
Should I be able to combine commands? (ie. kubectl wait --for=condition=Ready pod/foo && kubectl logs foo)
I'm just throwing some questions out that come to mind when I look at this. Maybe some of this is out of scope or at least out of scope initially.
There was a problem hiding this comment.
How do you see this command being executed? Is it just going to run whatever text you put here as a shell command?
To be determined. I hope that doesn't wind up being the only option. I am thinking we might be able to get clever with Cobra exposing more.
Would there be a benefit to allowing variables to be substituted into the command, maybe just positionally like with a bash script ($1, $2, etc) or possibly named? That is one of the limitations of bash aliases.
I like that idea. We can play with it when we go to implement.
Should I be able to combine commands? (ie. kubectl wait --for=condition=Ready pod/foo && kubectl logs foo)
My initial thought is no, that's what bash aliases are for. You could bash alias multiple kubectl aliases 😅.
There was a problem hiding this comment.
I'd again advise to start simple, w/o any replacements, and slowly iterate the bits we add.
There was a problem hiding this comment.
Added to open questions.
| nitty-gritty. | ||
| --> | ||
|
|
||
| We propose introducing a new file that will be versioned and intended for user provided preferences. This new file is entirely opt-in. |
There was a problem hiding this comment.
Currently, besides defining contexts, .kube/config also stores the user's currently selected context...
current-context: cluster1
Do you see that remaining in .kube/config or will it be moving here?
There was a problem hiding this comment.
I think that's a good idea but will have the same issue as the XDG change. Any tooling that hasn't been updated could use a different context.
Maybe we could reject any value in kuberc if current-context is defined in the kubeconfig?
Thoughts?
There was a problem hiding this comment.
But yes would like to do that. Unsure of how.
There was a problem hiding this comment.
Probably something we'll need to figure out at alpha, add that to questions section, or elements to figure out before beta.
There was a problem hiding this comment.
Added to open questions.
| * `aliases` for users to declare their own aliases for commands with flags and values. | ||
| * `overrides` for users to set default flags to apply to commands. | ||
|
|
||
| `command.aliases` will not be allowed to override builtins but take precedence of plugins i.e. builtins -> aliases -> plugins. Additional flags and values will be appended to the end of the aliased command. It is the responsibility of the user to define aliases with this in mind. |
There was a problem hiding this comment.
Could we consider different aliasing rules/types? For example aliasing namespaces:
instead of oc get pods -n kube-system, we would do oc get pods -n ks
I suppose we could also move defining such aliases to flag overrides instead of aliases section. Although I worry generic aliases for flags would be too much in the beginning and could cause problems when running scripts, etc.
There was a problem hiding this comment.
I don't want to go that far, I'd start simple with just command aliases.
There was a problem hiding this comment.
We can always consider other bits as we move forward.
|
@eddiezane please add the PRR file. Your answers look fine, so I will be able to approve once you have SIG approval from someone other than yourself :) I see the issue you raised and it's certainly a reasonable discussion but let's follow the process for now. FWIW, we have asked for changes for past kubectl KEPs (specifically to use the env var to hide alpha features and avoid user confusion). But your point is well taken; there are often KEPs where various sections are not relevant (PRR a common one). @kikisdeliveryservice @palnabarun FYI |
k8s-ci-robot
removed
the
approved
Signed-off-by: Eddie Zaneski <eddiezane@gmail.com>
|
Can we get an approval from a SIG CLI approver that is not the KEP author? |
@soltysh is going to tag it. |
| [documentation style guide]: https://github.com/kubernetes/community/blob/master/contributors/guide/style-guide.md | ||
| --> | ||
|
|
||
| This proposal introduces an optional `kuberc` file that is used to separate cluster credentials and server configuration from user preferences. |
There was a problem hiding this comment.
I recommend making this a few sentences to explain more about the existing mechanism (~/.kube/config) and how this is different.
| --> | ||
| kubectl is one of the oldest parts of the Kubernetes project and has a strong guarantee on backwards compatibility. We want users to be able to opt in to new behaviors (like delete confirmation) that may be considered breaking changes for existing CI jobs and scripts. | ||
|
|
||
| [kubeconfigs already have a field for preferences](https://github.com/kubernetes/kubernetes/blob/474fd40e38bc4e7f968f7f6dbb741b7783b0740b/staging/src/k8s.io/client-go/tools/clientcmd/api/types.go#L43) that is currently underutilized. The reason for not using this existing field is that creating a new cluster generally yields a new kubeconfig file which contains credentials and host information. While kubeconfigs can be merged and specified by path, we feel there should be a clear separation between server configuration and user preferences. |
There was a problem hiding this comment.
| [kubeconfigs already have a field for preferences](https://github.com/kubernetes/kubernetes/blob/474fd40e38bc4e7f968f7f6dbb741b7783b0740b/staging/src/k8s.io/client-go/tools/clientcmd/api/types.go#L43) that is currently underutilized. The reason for not using this existing field is that creating a new cluster generally yields a new kubeconfig file which contains credentials and host information. While kubeconfigs can be merged and specified by path, we feel there should be a clear separation between server configuration and user preferences. | |
| Existing Kubernetes configuration files have [a field for preferences](https://github.com/kubernetes/kubernetes/blob/474fd40e38bc4e7f968f7f6dbb741b7783b0740b/staging/src/k8s.io/client-go/tools/clientcmd/api/types.go#L43) that is currently underutilized. The reason for not using this existing field is that creating a new cluster generally yields a new kubeconfig file which contains credentials and host information. While kubeconfigs can be merged and specified by path, we feel there should be a clear separation between server configuration and user preferences. |
Also, clarify who “we” are here.
| <!-- | ||
| What are the caveats to the proposal? | ||
| What are some important details that didn't come across above? | ||
| Go in to as much detail as necessary here. | ||
| This might be a good place to talk about core concepts and how they relate. | ||
| --> |
There was a problem hiding this comment.
If I specify KUBECONFIG via an environment variable, is there any way that a kuberc could affect the outcome of an API call?
It's OK to affect how the results are rendered, beep when the rollout is done, turn on my coffee machine once the rollout is done, etc - so long as those side effects are confined to my client side and aren't visible to the cluster. It's also OK for my kuberc to act as a trigger for a second interaction that does have side effects: a locally-run kubectl create CoffeeMakingRequest etc.
|
|
||
| overrides: | ||
| - command: apply | ||
| flags: |
There was a problem hiding this comment.
I wouldn't call these flags. https://superuser.com/a/1070071 (and the common use, at least when I was learning computers) was to reserve the term flag for boolean and maybe tristate options.
I know Golang coders often use the term to mean any kind of command line option, but we shouldn't assume that every tool and user consuming a kuberc is written in Go or knows this Go quirk.
| - command: apply | ||
| flags: | ||
| - name: server-side | ||
| default: "true" |
There was a problem hiding this comment.
We should define the behavior if somebody specifies true, unquoted.
There was a problem hiding this comment.
Since this will most likely be a string field b/c we'll need to accept any values, we'll have to deal with that in the parsing logic, in which case both true and "true" should resolve to the same, but good point.
|
|
|
It's OK to affect how the results are rendered, beep when the rollout is done, turn on my coffee machine once the rollout is done, etc - so long as those side effects are confined to my and aren't visible to the cluster. It's also OK for my kuberc to act as a trigger for a second interaction that does have side effects: a locally-run |
| - command: apply | ||
| flags: | ||
| - name: server-side | ||
| default: "true" |
There was a problem hiding this comment.
Since this will most likely be a string field b/c we'll need to accept any values, we'll have to deal with that in the parsing logic, in which case both true and "true" should resolve to the same, but good point.
k8s-ci-robot
added
do-not-merge/hold
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: eddiezane, johnbelamaric, soltysh 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
|
I'm happy to see those extra questions addressed / noted / whatever, they shouldn't block a merge. I would like to see the name “flags” not part of this by the time it reaches beta. But that's a personal opinion. |
Thanks Tim for your valuable input. /hold cancel
I totally agree with you on that one. |
k8s-ci-robot
removed
the
do-not-merge/hold
/cc @liggitt @soltysh