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

Develop a unified policy for command like flags vs config file options #1040

Closed
liztio opened this issue Aug 7, 2018 · 26 comments
Closed

Develop a unified policy for command like flags vs config file options #1040

liztio opened this issue Aug 7, 2018 · 26 comments
Assignees
Labels
area/UX kind/documentation Categorizes issue or PR as related to documentation. kind/feature Categorizes issue or PR as related to a new feature. lifecycle/frozen Indicates that an issue or PR should not be auto-closed due to staleness. priority/important-longterm Important over the long term, but may not be staffed and/or may need multiple releases to complete.
Milestone

Comments

@liztio
Copy link

liztio commented Aug 7, 2018

Assumptions:

  • We do not want hundreds of kubeadm flags to control every possible future
  • Configuration files are easier version than cli flags
  • Exclusively using a configuration file places undue burden on users.

We are looking for a happy medium between "everything is in the CLI flags" and "everything is configuration." This issue should be a formal policy that sorts new options into the "flag and configuration option" and "configuration option only" buckets. The latter is likely for more obscure flags, but where exactly the line between "common" and "obscure" should be firmly delineated.

KEP WIP doc:
https://docs.google.com/document/d/1LSb2Ieb4XrxQ3cG6AEkwpfN1hb1L6pJdb0vMo10F41U/edit?usp=sharing

@liztio liztio self-assigned this Aug 7, 2018
@neolit123
Copy link
Member

neolit123 commented Aug 7, 2018

for reference the original proposal that the kublet went for:
https://docs.google.com/document/d/1FdaEJUEh091qf5B98HM6_8MS764iXrxxigNIdwHYW9c/edit#heading=h.nlhhig66a0v6

PR about CLI flags overriding config (kubeadm init):
kubernetes/kubernetes#66968

viper as consideration:
https://github.com/spf13/viper

@rosti
Copy link

rosti commented Aug 8, 2018

I'll have a more in-depth look at viper.

The thing is that having everything either way (config file or cmd line flags) is awkward to support. Having fewer command line flags is a good thing. My bet is to support only those, that are likely to be overridden on a per-machine and/or per-execution basis (node names, certificate files, bootstrap tokens, etc.).

My proposed implementation in kubernetes/kubernetes#66968 for kubeadm init follows to a large extent the approach undertaken by kubelet. It's essentially a huge workaround over some of the cobra limitations. Also, no flags are deprecated, they are all simply imported as overrides of the config.

@fabriziopandini
Copy link
Member

I think that before addressing the issue at technical level (viper, kubelet like or kubectl like) it would be great to figure out a list of use cases when flags and config files should be used at the same time in kubeadm. This will help in understanding when and why we really need this

@timothysc timothysc added this to the v1.12 milestone Aug 9, 2018
@timothysc timothysc added the priority/important-longterm Important over the long term, but may not be staffed and/or may need multiple releases to complete. label Aug 9, 2018
@fabriziopandini
Copy link
Member

.. from slack
"More and more I think about it, more I'convinced that a suitable way forward for the current kubeadm UX issues (flags -> cfg, version defaults, etc) is to agree on a common way to implement cobra command in kubeadm
Currently there is no consistent approach and this makes the problem more complex than it should be"

@rosti
Copy link

rosti commented Aug 10, 2018

I have spent some time today looking into viper. At first glance, I am not very impressed with it. Seems to me, that if we try to stick with our current config model, we'll have to do some workarounds.

For example, implementing --apiserver-advertise-address will require it to be setup as an alias to api.advertiseAddress in order to read it from the config. Unfortunately, this would also mean, that we have yet another command line parameter (--api.advertiseAddress).

This may just be me, looking at a library which I haven't used at all, but it may just be better to stick to Cobra and provide manual overloading of only selected flags (as opposed to all config flags).

@neolit123
Copy link
Member

as much as i looked at it i can confirm the --api.advertiseAddress observation.
if kubeadm started with viper from the beginning i think it could have worked nicely in the long run, but i'm not sure if it was even available back then.

there is also the option to YOLO the viper approach and deprecate the alias flags (i.e. those that we have right now).

@timothysc
Copy link
Member

cc @sysrich

@sysrich
Copy link

sysrich commented Aug 22, 2018

Currently there is no consistent approach and this makes the problem more complex than it should be"

Indeed, and I fear I'm about to add another layer of complexity to the problem, but I hope in doing so we end up with a comprehensive solution

it would be great to figure out a list of use cases when flags and config files should be used at the same time in kubeadm

I think there are actually 3 classes or tiers of "configuration", and we need to deal with them in a structured and consistent way.

I'd describe the two being discussed so far are "user supplied config files" and "user supplied commandline flags"

My strong personal feeling is that all of the options we want the user to be able to define must be able to be set in a configuration file. This configuration file should live in /etc and be primarily configured by the user. I think distributions should be discouraged from packaging, bundling, or editing anything in /etc, leaving that existence and content of any such configuration purely for the user. (More on that later).

I think commandline flags should be reserved for options which only make sense for "one-time" custom execution parameters. For example the existing kubeadm init --dry-run and --feature-gates would be a great example of flags that make sense as flags. The --apiserver-* flags are flags which I think would be best kept to a configuration file - I don't see the benefit of having them as flags.

Regardless, I think all (or almost all) commandline flags should also be definable in a configuration file.

This creates a need to set precedence. I think commandline flags should always have a precedence above that of a config file.

I feel my above proposal might serve as a starting point of a policy for this issue, but now I introduce my new layer of complexity.

Distributions, or any other solution providers wanting to bundle k8s/kubeadm out of the box, need to have a way of providing their default configuration also. And yet, this configuration needs to be alterable by users. This is made even more complex when you consider many container OS platforms (CoreOS, Kubic, etc) all have read-only filesystems, encompassing either /usr or a significant portion of the root filesystem.

Luckily, systemd provides a model which I think we should consider here (wow I never thought I'd say that ;))

I think we should have "distro provided" configuration files, stored in /usr. These distro defaults should be automatically overridden by any user provided configuration file.

Whether or not this solution should include a systemd-like drop-in feature where selective parameters from the /usr config can be overridden by a narrowly defined override provided in /etc is an open question - I think it only makes sense if the configuration files are structured accordingly. If not I see no problem with a file provided in /etc totally overriding it's companion file in /usr

All applications should therefore consume configuration in the following order of precedence.

  • Config file in /usr - all config options can be defined
  • Config file in /etc - all config options can be defined
  • Commandline flags - subset of config options that make sense for defining at runtime

Does this sound like a viable way forward?

@neolit123
Copy link
Member

neolit123 commented Aug 22, 2018

i would like to bring Windows in the mix, because kubeadm has plans to support the OS.

and i would leave it others to comment more on the default configs in /usr and /etc, but something to note here is that while Linux has a very unclear definition of where applications should store their data, Windows often lacks corresponding folders.

the only thing that maps well is:

  • /home/<user-name>/<app-name> -> <home-drive>\Users\<user-name>\AppData\<profile-storage-type>\<app-name>

@rosti
Copy link

rosti commented Aug 24, 2018

I think, that there are a few more things to consider here on those grounds:

  • Yes, Windows and macOS are viable options for running kubeadm, but I don't think anyone is going to actually run it in production environments on those systems. So we must think Linux first, however...

  • kubeadm is mostly executed by some sort of cluster deployment system - Ansible, Terraform, Vagrant, Cluster API, etc. The end user will use kubeadm directly only for specific tasks (such as when things go wrong) or for deploying some playground clusters. This means, that kubeadm should be usable by both various automation systems and end users.

  • The so called "config" files are actually a "recipe" on how to do a deployment. In theory, one can have several such recipes, coexisting on the machine as text files, with only one being the active deployment at the moment. This allows cluster management systems to recycle machines by simply kubeadm reset-ing and then initialize or join the machine with a different recipe for a new role.

Therefore, I am a little bit hesitant to introduce pre-defined config file locations in kubeadm. If we do this, we'll end up with more code complexity, since we'll also have to introduce workarounds for Windows and macOS some day.
Given this and the fact, that most cluster management systems will actually fetch a config file from somewhere and change a few machine specific parameters via the command line, then I don't think it's worth the complexity to introduce additional complexity by defining standard locations and multiple config files overriding each other.

The whole idea behind flags overriding the config file options is to avoid changing the config file if one needs to set the node name or the API server advertise address.

@sysrich
Copy link

sysrich commented Aug 24, 2018

The so called "config" files are actually a "recipe" on how to do a deployment

That might be true for some aspects for some aspects of the configuration files, but I'd describe things like deciding which CRI runtime is being used by kubelet as far more foundational. kubeadm reset won't even work right unless it knows which runtime to reset.

I might be able to agree with your points for some aspects of the configuration, but at the very least for those related to the choice of runtime, I strongly hold that we need to have tiering of the sort I describe in my earlier post

The model is one that is proven to work well in conjunction with other configuration management systems (eg. salt, ansibile, etc), and my experience with such tools strongly suggests they too benefit from knowing where to expect to draw their configuration from. I don't think the argument that "these other tools exist, so we don't need to worry about how to do things consistently ourselves" is a valid train of thought.

@rosti
Copy link

rosti commented Aug 24, 2018

I can agree on the CRI part, but even that can be viewed as part of the deployment recipe - one can wish to deploy a machine as part of one cluster with CRI-O and then decide to reset it and join to another cluster with Docker.

The model is one that is proven to work well in conjunction with other configuration management systems (eg. salt, ansibile, etc), and my experience with such tools strongly suggests they too benefit from knowing where to expect to draw their configuration from. I don't think the argument that "these other tools exist, so we don't need to worry about how to do things consistently ourselves" is a valid train of thought.

The thing that bothers me in that model is that it may now be /etc and /usr. Next, some other distro folks will ask, say "we don't like /etc, we want /var", then some third party comes and asks "I need both /etc and /var".
This is a Linux specific code, which we'll have to support in the future. It will also make kubeadm UX specific to different distributions.
We are already struggling with supporting debs, rpms and the quirks of different Linux distros (systemd-resolved enabled or not, different cgroups drivers, etc). And I see here the potential of expanding on that.
Another thing that is doubtful to me is, that I don't think that many of the authors of third party tools (that use kubeadm) will actually change their code and store the config files in the standard locations. They'll be happy to use --config and not bother doing anything at all.

@sysrich
Copy link

sysrich commented Aug 24, 2018

The thing that bothers me in that model is that it may now be /etc and /usr. Next, some other distro folks will ask, say "we don't like /etc, we want /var", then some third party comes and asks "I need both /etc and /var".

But the /usr and /etc split mirrors exactly what is done by systemd, which is used as the default init system by the vast majority of distributions out there.

I'm not saying my proposed way is the only way forward, but we need forward and I strongly disagree with your implications that the status quo is acceptable.

If Kubernetes doesn't have sensible, standardised, consistent models that allow distributions to provide sane config defaults to then be overridden by users, then the viability of shipping kubernetes as an integrated part of any end-to-end stack will be questionable. I think we need to consider these needs as a natural part of kubernetes growing up to a solution which seeks to be easily integrated across multiple platforms by multiple stakeholders.

We are already struggling with supporting debs, rpms and the quirks of different Linux distros (systemd-resolved enabled or not, different cgroups drivers, etc). And I see here the potential of expanding on that.

And I see the opposite - because so far every single one of the issues I've had getting Kubernetes and kubeadm integrated into openSUSE has been frustrated by the lack of consistency.
Which led me to proposing further inconsistent solutions, which increased the complexity for the distro, and by proxy, Kubernetes when my inconsistent solutions led to bugs and integration issues.

Which also then prevents me from contributing to the messy incomplete starting points like the upstream rpm specfiles, because real distributions have had to hack so many nasty things to workaround the madness there is no longer any relationship to the rpm defs you see in the k8s git.

All of which conspires together to make things painful when we want to keep up with k8s versions, which change all of the above and suddenly we have weeks or months or work to do before we can deploy the new version to our users, just to have the next k8s version out before we've completed that work.. and the cycle continues ad infinitum....

This issue seeks to establish a unified policy precisely because Kubernetes needs to 'plant a flag' in the ground so other stakeholders, like Distributions, have a standardised, documented, and well thought out framework for living with.

Expecting everything to keep working together nicely with the current status quo of inconsistent flags, config files appearing and disappearing from different locations and being consumed and unconsumed by tools like kubeadm seemingly at random between versions really is not something which can continue indefinitely if we expect to keep on being able to play well with others.

The solution doesn't need to be perfect, I'm more than happy to compromise on my proposal, but we need a solution.

@rosti
Copy link

rosti commented Aug 24, 2018

@sysrich thanks for the clarification. I agree with the expressed opinion. As long as all (or at least most of the) distros agree on the selected locations for default config files, I am perfectly fine with this.

@timothysc timothysc added kind/feature Categorizes issue or PR as related to a new feature. kind/documentation Categorizes issue or PR as related to documentation. labels Aug 30, 2018
@timothysc
Copy link
Member

/assign @neolit123

@neolit123
Copy link
Member

neolit123 commented Sep 5, 2018

as discussed in today's kubeadm office hours i have created a KEP doc for this
but it's empty ATM:
https://docs.google.com/document/d/1LSb2Ieb4XrxQ3cG6AEkwpfN1hb1L6pJdb0vMo10F41U/edit?usp=sharing

this feature is outside of the 1.12 scope, but we can try to finish the KEP soon.

one big preliminary ? here is Viper. if we want to fork Viper in k8s, at that point we can have it do what we want and possibly fix some bugs. also use it in other areas that are facing the same problem, e2e framework, kube-proxy etc.

xref: kubernetes/kubernetes#66649
the e2e framework refactor is facing the same flags vs config issue.

@fabriziopandini
Copy link
Member

@sysrich thanks for keeping this discussion moving
I think that one missing piece of the discussion is how configuration from different layers should merge in case of conflicts (tricky case are maps, lists or nested/exclusive structures like the ones for ETCD configurations)

@rosti
Copy link

rosti commented Sep 10, 2018

@neolit123 I don't think, that forking Viper is the best approach here. We should probably try contributing patches upstream to Viper first.

@fabriziopandini @sysrich merging configs from different locations seems like a bad idea to me. There are so many things that could go wrong and hard to find bugs just waiting to happen. If we have multiple configuration files, we should probably just search locations in a particular order and use the first one found.

@neolit123
Copy link
Member

viper is largely unmaintained, the idea of homebrewing an alternative received some +1 at sig-architecture.

pohly added a commit to pohly/kubernetes that referenced this issue Oct 5, 2018
Storing settings in the framework's TestContext is not something that
out-of-tree test authors can do because for them the framework is a
read-only upstream component. Conceptually the same is true for
in-tree tests, so the recommended approach is to define configuration
settings in the code that uses them.

How to do that is a bit uncertain. Viper has several
drawbacks (maintenance status uncertain, cannot list supported
options, cannot validate the configuration file). How to handle
configuration files is currently getting discussed for kubeadm, with
similar concerns about
Viper (kubernetes/kubeadm#1040).

Instead of making a choice now for E2E, the recommendation is that
test authors continue to define command line flags as before, except
that they should do it in their own code and with better flag names.

But the ability to read options also from a file is useful, so
several enhancements get added:
- all settings defined via flags can also be read from a
  configuration file, without extra work for test authors
- framework/config makes it possible to populate a struct directly
  and define flags with a single function call
- a path and file suffix can be given to --viper-config (as in
  "--viper-config /tmp/e2e.json") instead of expecting the file in
  the current directory; as before, just plain "--viper-config e2e"
  still works
- if "--viper-config" is set, the file must exist; otherwise the
  "e2e" config is optional (as before)
- errors from Viper are no longer silently ignored, so syntax errors
  are detected early
- Viper support is optional: test suite authors who don't want
  it are not forced to use it by the e2e/framework
@timothysc
Copy link
Member

/assign @rdodev @timothysc
/unassign @liztio

@k8s-ci-robot k8s-ci-robot assigned rdodev and unassigned liztio Oct 11, 2018
vithati pushed a commit to vithati/kubernetes that referenced this issue Oct 25, 2018
Storing settings in the framework's TestContext is not something that
out-of-tree test authors can do because for them the framework is a
read-only upstream component. Conceptually the same is true for
in-tree tests, so the recommended approach is to define configuration
settings in the code that uses them.

How to do that is a bit uncertain. Viper has several
drawbacks (maintenance status uncertain, cannot list supported
options, cannot validate the configuration file). How to handle
configuration files is currently getting discussed for kubeadm, with
similar concerns about
Viper (kubernetes/kubeadm#1040).

Instead of making a choice now for E2E, the recommendation is that
test authors continue to define command line flags as before, except
that they should do it in their own code and with better flag names.

But the ability to read options also from a file is useful, so
several enhancements get added:
- all settings defined via flags can also be read from a
  configuration file, without extra work for test authors
- framework/config makes it possible to populate a struct directly
  and define flags with a single function call
- a path and file suffix can be given to --viper-config (as in
  "--viper-config /tmp/e2e.json") instead of expecting the file in
  the current directory; as before, just plain "--viper-config e2e"
  still works
- if "--viper-config" is set, the file must exist; otherwise the
  "e2e" config is optional (as before)
- errors from Viper are no longer silently ignored, so syntax errors
  are detected early
- Viper support is optional: test suite authors who don't want
  it are not forced to use it by the e2e/framework
@timothysc timothysc modified the milestones: v1.13, v1.14 Oct 30, 2018
@timothysc timothysc added the help wanted Denotes an issue that needs help from a contributor. Must meet "help wanted" guidelines. label Jan 7, 2019
@timothysc
Copy link
Member

/assign @luxas

b/c this overlaps with grand-unified component config.

@neolit123 neolit123 modified the milestones: v1.14, Next Feb 3, 2019
@neolit123 neolit123 added the lifecycle/frozen Indicates that an issue or PR should not be auto-closed due to staleness. label Feb 3, 2019
@neolit123
Copy link
Member

@neolit123 neolit123 assigned neolit123 and unassigned luxas Oct 10, 2019
@neolit123 neolit123 removed the help wanted Denotes an issue that needs help from a contributor. Must meet "help wanted" guidelines. label Oct 10, 2019
@neolit123 neolit123 added the lifecycle/active Indicates that an issue or PR is actively being worked on by a contributor. label Jan 2, 2020
@neolit123
Copy link
Member

neolit123 commented Mar 18, 2020

xref https://docs.google.com/document/d/1Dvct469xfjkgy3tjWMAKvRAJo4CmGH4cgSVGTDpay6A/edit

the proposal in this doc mostly staled at this point. i can still see some benefit of allowing patches from the CLI vs having a 100 flags + config or only config without granular overrides.

@neolit123 neolit123 removed the lifecycle/active Indicates that an issue or PR is actively being worked on by a contributor. label May 30, 2020
@neolit123
Copy link
Member

neolit123 commented May 18, 2021

the policy for kubeadm is tribal knowledge at this point:

  • use config instead of flags (kubeadm blocks you from mixing some).
  • flags override config (there is one known exception, but that is a bug).
  • flags already present in config will be deprecated / removed eventually, but there is no timeline (it's low priority).

this is the case for kubeadm at least.

kubelet and other components are sparse:

  • kubelet has flags that have been deprecated for a long time.
  • kubelet flags are added without the config mirroring.
  • kubelet flags override config.
  • kube-proxy and kube-scheduler config override their flags.

other notes:

  • using viper across k8s never got approved and it saw some criticism in how it manages flag/field validation.
  • WG Component Standard is looking for leads; it would be up to this group to document a k8s-wide policy.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/UX kind/documentation Categorizes issue or PR as related to documentation. kind/feature Categorizes issue or PR as related to a new feature. lifecycle/frozen Indicates that an issue or PR should not be auto-closed due to staleness. priority/important-longterm Important over the long term, but may not be staffed and/or may need multiple releases to complete.
Projects
None yet
Development

No branches or pull requests

9 participants