Skip to content
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

KEP-3104: Introduce kuberc #3392

Merged
merged 1 commit into from
Jun 23, 2022
Merged

Conversation

eddiezane
Copy link
Member

  • One-line PR description: Add initial KEP.
  • Other comments:

/cc @liggitt @soltysh

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. kind/kep Categorizes KEP tracking issues and PRs modifying the KEP directory sig/cli Categorizes an issue or PR as relevant to SIG CLI. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Jun 13, 2022
Copy link
Member

@puerco puerco left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

how are subcommands indicated? e.g. kubectl create secret or kubectl auth reconcile?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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`.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is this an opportunity to address some of the issues raised in kubernetes/kubernetes#80399, #2229, or #2111 for this new file?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

keps/sig-cli/3104-introduce-kuberc/README.md Outdated Show resolved Hide resolved
keps/sig-cli/3104-introduce-kuberc/README.md Outdated Show resolved Hide resolved
apiVersion: v1alpha1

commandAliases:
- alias: getdbprod
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

how are subcommand aliases indicated? e.g. kubectl create mycustomobj

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we should at least consider the shape, in case it informs this field structure (e.g. list instead of string)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@soltysh soltysh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A couple of comments, and you're also missing PRR file in https://github.com/kubernetes/enhancements/tree/master/keps/prod-readiness/sig-cli

keps/sig-cli/3104-introduce-kuberc/README.md Show resolved Hide resolved
keps/sig-cli/3104-introduce-kuberc/README.md Outdated Show resolved Hide resolved
keps/sig-cli/3104-introduce-kuberc/README.md Show resolved Hide resolved
apiVersion: v1alpha1

commandAliases:
- alias: getdbprod
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

keps/sig-cli/3104-introduce-kuberc/README.md Show resolved Hide resolved
keps/sig-cli/3104-introduce-kuberc/README.md Outdated Show resolved Hide resolved
keps/sig-cli/3104-introduce-kuberc/README.md Show resolved Hide resolved

commandAliases:
- alias: getdbprod
command: get pods -l what=database --namespace us-2-production
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 😅.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd again advise to start simple, w/o any replacements, and slowly iterate the bits we add.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But yes would like to do that. Unsure of how.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably something we'll need to figure out at alpha, add that to questions section, or elements to figure out before beta.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't want to go that far, I'd start simple with just command aliases.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can always consider other bits as we move forward.

@johnbelamaric
Copy link
Member

@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 k8s-ci-robot removed the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 21, 2022
Signed-off-by: Eddie Zaneski <eddiezane@gmail.com>
@johnbelamaric
Copy link
Member

Can we get an approval from a SIG CLI approver that is not the KEP author?

@eddiezane
Copy link
Member Author

Can we get an approval from a SIG CLI approver that is not the KEP author?

@soltysh is going to tag it.

Copy link
Contributor

@sftim sftim left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some feedback

[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.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
[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.

Comment on lines +257 to +262
<!--
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.
-->
Copy link
Contributor

@sftim sftim Jun 22, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should define the behavior if somebody specifies true, unquoted.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@sftim
Copy link
Contributor

sftim commented Jun 22, 2022

  • What happens if I do have a kubeconfig preferences field and a kuberc?
  • Can I have a kuberc.d directory for dropins?

@sftim
Copy link
Contributor

sftim commented Jun 22, 2022

  • Does kubectl config view tell me about the presence of a kuberc file? (maybe printed to STDERR)

@sftim
Copy link
Contributor

sftim commented Jun 22, 2022

  • is there an equivalent of kubectl config view to show me the kuberc and maybe even the merged configuration?

@sftim
Copy link
Contributor

sftim commented Jun 22, 2022

  • 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 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.

Copy link
Contributor

@soltysh soltysh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's a couple of good points from Tim worth adding in the notes section
/lgtm
/approve
/hold
if you want to add them before merging or drop the hold and open a followup.

- command: apply
flags:
- name: server-side
default: "true"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 k8s-ci-robot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. lgtm "Looks good to me", indicates that a PR is ready to be merged. labels Jun 22, 2022
@johnbelamaric
Copy link
Member

/approve

@k8s-ci-robot
Copy link
Contributor

[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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 22, 2022
@sftim
Copy link
Contributor

sftim commented Jun 22, 2022

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.

@soltysh
Copy link
Contributor

soltysh commented Jun 23, 2022

I'm happy to see those extra questions addressed / noted / whatever, they shouldn't block a merge.

Thanks Tim for your valuable input.

/hold cancel

I would like to see the name “flags” not part of this by the time it reaches beta. But that's a personal opinion.

I totally agree with you on that one.

@k8s-ci-robot k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jun 23, 2022
@k8s-ci-robot k8s-ci-robot merged commit d3df65f into kubernetes:master Jun 23, 2022
@k8s-ci-robot k8s-ci-robot added this to the v1.25 milestone Jun 23, 2022
@eddiezane eddiezane deleted the ez/add-kuberc branch June 29, 2022 16:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/kep Categorizes KEP tracking issues and PRs modifying the KEP directory lgtm "Looks good to me", indicates that a PR is ready to be merged. sig/cli Categorizes an issue or PR as relevant to SIG CLI. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants