better management of mixing flags and config

chendave commented 1 year ago

What happened?

pls see: https://github.com/kubernetes/kubernetes/pull/113583#discussion_r1254293282

the major reason to against the mix configuration is:

migrate users away from flags

but it is not clear why some of them are allowed for the mix configuration in the first place.

https://github.com/kubernetes/kubernetes/blob/0ae9aaacfa217b18652f34d6ef29f08a15851b0a/cmd/kubeadm/app/apis/kubeadm/validation/validation.go#L560-L573

func isAllowedFlag(flagName string) bool {
    allowedFlags := sets.New(kubeadmcmdoptions.CfgPath,
        kubeadmcmdoptions.IgnorePreflightErrors,
        kubeadmcmdoptions.DryRun,
        kubeadmcmdoptions.KubeconfigPath,
        kubeadmcmdoptions.NodeName,
        kubeadmcmdoptions.KubeconfigDir,
        kubeadmcmdoptions.UploadCerts,
        "print-join-command", "rootfs", "v", "log-file")
    if allowedFlags.Has(flagName) {
        return true
    }
    return strings.HasPrefix(flagName, "skip-")
}

What you expected to happen?

Kubeadm should have a consistent style for the configuration
If this is not possible, Kubeadm should make it clear why some of the flags are allowed the mix configuration, while other not.

How to reproduce it (as minimally and precisely as possible)?

Anything else we need to know?

### Tasks
- [x] `dry-run` flags should be configurable in each config API (init/join/reset/upgrade)
- [x] `skip-phases` should be configurable in the upgrade API
- [ ] deprecate flags that could be configured in the config file
- [ ] move the flags out of the allowed list for mix config which is configurable in the config
- [ ] evaluate all other flags in the list and see if we can make them configurable in the config file instead.

neolit123 commented 1 year ago

but it is not clear why some of them are allowed for the mix configuration in the first place.

some flags exist as the only option and have no alternative in config, e.g. --dry-run
some flags are not deprecated yet; also need to be removed, but the deprecatation takes 1 year+ as the CLI is GA.

neolit123 commented 1 year ago

my vote, with respect to reset's --config:

we can allow the mixture of all options that don't exist in the reset config
all flag options that also exists in config should not be allowed - i.e. kubeadm should error saying "flag not allowed with --config mix" - e.g. --skip-phases

chendave commented 1 year ago

some flags exist as the only option and have no alternative in config, e.g. --dry-run

Added a TODO item in the issue: https://github.com/kubernetes/kubeadm/issues/2890

SataQiu commented 1 year ago

we can allow the mixture of all options that don't exist in the reset config

all flag options that also exists in config should not be allowed - i.e. kubeadm should error saying "flag not allowed with --config mix" - e.g. --skip-phases

I'm +1 for this. All fields that can be defined in the configuration file should no longer be allowed to be specified on the CLI flags, and we prefer to guide users to use the configuration file instead of relying too heavily on the CLI flags.

neolit123 commented 1 year ago

if we don't enumerate some action items here, perhaps we should just close? the story is clear, it feels like - @chendave as this ticket was more of a discussion / question.

biggest blocker of course is that the cli is ga, and all non experimental flags must take one year deprecation period.

chendave commented 1 year ago

@neolit123 I added some action items, I'd suggest to keep this open so won't forget to fix it in the 1.29.

k8s-triage-robot commented 7 months ago

The Kubernetes project currently lacks enough contributors to adequately respond to all issues.

This bot triages un-triaged issues according to the following rules:

After 90d of inactivity, lifecycle/stale is applied
After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

Mark this issue as fresh with /remove-lifecycle stale
Close this issue with /close
Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

kubernetes / kubeadm