Decide on how to handle the extra arg we use in kubeadm join

[neolit123]

we treat the extra arg for kubaedm join as an endpoint of the API server (or LB) this node want's to join.

[lubomir] the extra unnamed arg in `kubeadm join <some-endpoint>` has opened a can of worms for us and the `join phases`:
[lubomir] [here is how it’s handled](https://github.com/kubernetes/kubernetes/blob/master/cmd/kubeadm/app/cmd/join.go#L345-L352)

i known phase that needs this arg is "boostrap".

cc @ereslibre @fabriziopandini

[rosti]

Another phase is the pre-flight join phase. An API server endpoint is required to validate the JoinConfiguration if bootstrap tokens are used for discovery.
I do become more and more convinced, that we need to go down the --api-endpoint= road.

[neolit123]

I do become more and more convinced, that we need to go down the --api-endpoint= road.

💯

[fabriziopandini]

copying here/elaborating some notes from the various threads

general cosideration about args vs flags
I'm not in favor of introducing a guildeline for deprecating args in all commands in favor of flags, because there are plenty of example were this is good UX (see eg. kubectl).

general cosideration about args with phased commands
In v1.13 it was decided that

1. flags apply to top-level commands and to leaf phases (not to intermediate phase container command);
2. each phase should be allowed to customize the list its own flags

IMO the same should go for args, and AFAIK, currently, only 2 is missing; however this can be fixed by allowing to specifify command.Args validator for each phase.

focus on args with phased kubeadm join
Hypothetically speaking, if kubeadm join <master> accepts only IP address, there is no problem at all in the co-hesistance of args and phases for kubeadm join.

The domain of allowed values for <master> address includes all the possible hostnames, and this set of values overlaps with the name of the autogenerated phase subcommand.

The effect on the user of the overlap between<master> values and phase are the following:

• phase cannot be used as hostname

If the user types kubeadm join phase cobra kicks in and executes the phase subcommand, but what if phase was the hostname of the master?
I personally consider 1) a corner case, and if phase is actually the hostname a possible workaround should be to use phase:6443 instead (not tested, but it should work)

• mistype of phase are considered valid hostname

if the user mispeslls phase, typing e.g. kubeadm join phasee, cobra doens't prints any automatic suggestions, the args validation logic accept phasee as a valid host name, and the join workflow continues, but what if phasee was just a typo?

Assuming that this problem cannot be fixed by transforming <master> into a flag due to deprecation policy, my proposal is to improve error message when the join workflow fails to connect to phasee not existing master.

[chuckha]

I think making kubeadm init phase and kubeadm join phase could be improved by flipping the commands around. I think about it like kubeadm init is a command that runs a set of phases. If I want to see which phases kubeadm init runs, I would do something like kubeadm phases init or some other method of filtering ALL phases by which command(s) they are associated with. Same logic applies to kubeadm join.

I see this problem coming from breaking the single responsibility principle. Since both init and join are now responsible for two things, running their command and listing/running subphases, the UX is breaking down. There are many workarounds like @fabriziopandini has pointed out, but I think we could consider moving the phase subcommand out from init and join and into its own command, kubeadm phases with some mechanism for showing phases by how they are used. Tagging phases might make sense here since phases can be reused across various kubeadm sub commands. I could see some UX like kubeadm phases lists all phases while kubeadm phases init lists just phases tagged as phases that run during the init command.

[fabriziopandini]

@chuckha I see your point about the single responsibility principle, but such change is not easy to achieve

In the short term, what I'm trying to do is to ensure that

• From a user perspective, kubeadm init --help will continue to show the init workflow/set of phases, while kubeadm join --help will show the join workflow/set of phases
• Even if technically possible, we are not considering to use the same phase in both workflows (e.g certs phase in init is different from certs phase in join; eventually we can choose to use a different name, but this has backside as well)
• #73725 introduced a cleaner separation in the codebase between init and join actions

This doesn't solve the arg problem, but hopefully avoids new problems

[neolit123]

@ereslibre @fabriziopandini
i got this command line from a user today in a k/k issue thread:

kubeadm join 10.109.0.80:6443 --ignore-preflight-errors --token 3i2jzo.gicrm3jjbz0y64zg --discovery-token-ca-cert-hash sha256:5b20e87a257ea5551d8f5b3e1d502de099b485011d6b0e6062ad571fa97f5acb

this results in the following error:

Error: accepts at most 1 arg(s), received 2

the cause for the error is "interesting" (hint: it's caused by a flag)

[ereslibre]

Error: accepts at most 1 arg(s), received 2

Colour me confused, ❓

[bart0sh]

@ereslibre It's caused by missing parameter for --ignore-preflight-errors I guess.

[ereslibre]

kubeadm join 172.28.128.5:6443 --ignore-preflight-errors --token <token> --discovery-token-ca-cert-hash <hash> is parsed as:

• Unnamed arg 1: 172.28.128.5:6443
• --ignore-preflight-errors with argument --token
• Unnamed arg 2: <token>
• --discovery-token-ca-cert-hash with argument <hash>

😢

[neolit123]

had a chat with @ereslibre on slack. the above is a cobra issue and we can improve it upstream, but probably only if we get a lot of user reports.

[fabriziopandini]

I agree that cobra issue should be addressed upstream.

However, I think that it would be nice to have the possibility to set the Args validation function for leaf phases (if not set this should default to what set for the top-level command).

More specifically, we would like to have leaf phases without discovery flags using Args: cobra.NoArgs (while the top-level command/all the other leaf phases will use Args: cobra.MaximumNArgs(1))

@ereslibre, if I'm not wrong you already prototyped this. Would you like to take charge of this?

[ereslibre]

@ereslibre, if I'm not wrong you already prototyped this. Would you like to take charge of this?

Yes, I can investigate during the next cycle on this.

/assign

[fejta-bot]

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle stale

[fabriziopandini]

/close
With kubernetes/kubernetes#77400 we already did what is possible within the scope of kubeadm.
Potential conflict between args and subcommands are intrinsic in cobra design, but in our specific use case, one probability of conflict between arg and the "phase" subcommand are low; in case of conflict, as a workaround, the user can add https:// prefix and/or :6443 suffix to disambiguate.

This can be eventually reconsidered when wg Component Standard has a proposal for handling flags Vs Config

[k8s-ci-robot]

@fabriziopandini: Closing this issue.

In response to this:

/close
With kubernetes/kubernetes#77400 we already did what is possible within the scope of kubeadm.
Potential conflict between args and subcommands are intrinsic in cobra design, but in our specific use case, one probability of conflict between arg and the "phase" subcommand are low; in case of conflict, as a workaround, the user can add https:// prefix and/or :6443 suffix to disambiguate.

This can be eventually reconsidered when wg Component Standard has a proposal for handling flags Vs Config

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.