kubernetes-retired / cluster-api-bootstrap-provider-kubeadm

LEGACY REPO. NEW CODE IS https://github.com/kubernetes-sigs/cluster-api/tree/master/bootstrap/kubeadm
Apache License 2.0
62 stars 67 forks source link

✨ Initialize JoinConfiguration if missing #234

Closed fabriziopandini closed 4 years ago

fabriziopandini commented 4 years ago

What this PR does / why we need it:

Which issue(s) this PR fixes: Fixes #213

k8s-ci-robot commented 4 years ago

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: fabriziopandini

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: - ~~[OWNERS](https://github.com/kubernetes-sigs/cluster-api-bootstrap-provider-kubeadm/blob/master/OWNERS)~~ [fabriziopandini] Approvers can indicate their approval by writing `/approve` in a comment Approvers can cancel approval by writing `/approve cancel` in a comment
accepting commented 4 years ago

@fabriziopandini In our use cases, we use a template to generate bootstrap configs for all control plane machines. Hence, these configs all contains initconfiguration, clusterconfiguration and joinconfiguration. If adding the requirements in your PR, it will make the work of setting kubeadmconfig objects more manual.

fabriziopandini commented 4 years ago

@accepting here a thread explaining why it was decided to make this change

chuckha commented 4 years ago

I'm really torn on this PR

I'm leaning towards not being restrictive to allow more use cases, but I'm concerned about how people are using this. I don't think I have enough information about what is more common, using duplicated configs across all machines or being specific about which machine will be the initial control plane and which will be a secondary control plane. I'll talk to some people this week and see if I can get some consensus on this.

Any additional thoughts are welcome.

Please see the associated issue for more details. The TL;DR is: Should we allow users to supply all configs to every machine and let the controller figure it out? or should we be prescriptive to prevent accidental user errors and nice error messages.

fabriziopandini commented 4 years ago

/hold

chuckha commented 4 years ago

For this PR can we have just the initializing a join configuration if one doesn't exist and leave the other checks out for now? That way our behavior doesn't change and we can close #213.

SataQiu commented 4 years ago

/retitle ✨ Initialize config objects if missing

fabriziopandini commented 4 years ago

@chuckha Reduced the scope to

chuckha commented 4 years ago

/hold cancel

looks good to me. assigning to @detiber in case I've misunderstood his comment

/assign @detiber

detiber commented 4 years ago

/lgtm