kubernetes-sigs / cluster-api

Home for Cluster API, a subproject of sig-cluster-lifecycle
https://cluster-api.sigs.k8s.io
Apache License 2.0
3.6k stars 1.33k forks source link

Propagate `skipPhases` from Kubeadm config to Cluster API objects #5357

Closed mkjpryor closed 2 years ago

mkjpryor commented 3 years ago

User Story

I want to be able to specify skipPhases in my Kubeadm configuration in order to prevent kube-proxy from being deployed because the CNI I am using (Cilium) includes a more performant alternative.

Detailed Description

Upstream Kubeadm configuration resources now support skipPhases for init and join configurations. We need to add this to the Cluster API resources and propagate it.

/kind feature

sbueringer commented 3 years ago

/cc @chrischdi

neolit123 commented 3 years ago

@mkjpryor this was briefly discussed in the CAPI meeting today, since it has been a recurring ask (at least on slack). response from the maintainers is that if someone wishes to work on this they can go ahead.

/help

k8s-ci-robot commented 3 years ago

@neolit123: This request has been marked as needing help from a contributor.

Guidelines

Please ensure that the issue body includes answers to the following questions:

For more details on the requirements of such an issue, please see here and ensure that they are met.

If this request no longer meets these requirements, the label can be removed by commenting with the /remove-help command.

In response to [this](https://github.com/kubernetes-sigs/cluster-api/issues/5357): >@mkjpryor this was briefly discussed in the CAPI meeting today, since it has been a recurring ask (at least on slack). >response from the maintainers is that if someone wishes to work on this they can go ahead. > >/help > Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md). If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository.
dharmjit commented 3 years ago

Hi @sbueringer, @neolit123, I could look into this. Can I assign this issue to myself.

sbueringer commented 3 years ago

@dharmjit Yes

dharmjit commented 3 years ago

/assign

vincepri commented 3 years ago

/milestone v1.0

dharmjit commented 3 years ago

Hi @fabriziopandini @sbueringer, I have below in mind to implement this. Please let me know if I missed something.

Also, I am getting this go:build !ignore_autogenerated_core in generated files. I am using Go1.17, Do I need to use some specific Go version.

fabriziopandini commented 3 years ago

This really depends by what is the expected behaviour.

Skip phases has been added in the latest version of Kubeadm config, but KubeadmConfig allows to create clusters with older version of kubeadm as well.

If I'm right, the proposed approach silently ignore the flag for older version, which could be confusing for the users.

So the question is do we consider this acceptable? if yes, then how do we document this properly; if not, how do we translate skipPhases for older version? e.g will it be possible to translate skipPhases into kubeadm command flags?

I don't have strong opinions about this, but given that this is user facing, it should probably be discussed in the channel or in the next office hours (in the latest office hours I have raised the point, but as far as I understood we didn't reach an agreement on this point).

Also, I am getting this go:build !ignore_autogenerated_core in generated files. I am using Go1.17, Do I need to use some specific Go version.

yes, you use go v1.16 (ClusterAPI is in sync with the version of Kuberentes we are importing)

sbueringer commented 3 years ago

Do we already have some fields which are only supported in newer kubeadm versions? If yes, how do we handle them currently?

mkjpryor commented 3 years ago

If you are worried about backwards compatibility, why not always translate it into command line arguments until there are no supported Kubeadm versions that don't have the config file option? Even when the config file option is available, the flag is still supported...

You can transition from one to the other in the future without changing the Cluster API interface that is used.

jayunit100 commented 3 years ago

after about 2 hours of grepping and googling, i just realized this :) what is the current workaround ?

jayunit100 commented 3 years ago

yes silently ignoreing the flag is fine - imo as a user :) i have a selfish motivation for this

vincepri commented 3 years ago

Do we already have some fields which are only supported in newer kubeadm versions? If yes, how do we handle them currently?

@fabriziopandini might know more, but AFAIK today all configuration options have been compatible with each Kubeadm/Kubernetes version.

If you are worried about backwards compatibility, why not always translate it into command line arguments until there are no supported Kubeadm versions that don't have the config file option? Even when the config file option is available, the flag is still supported...

That could be a potential solution, although translating only some options to CLI flags still requires Cluster API to be aware of which Kubernetes version a flag is supported in.

If I'm right, the proposed approach silently ignore the flag for older version, which could be confusing for the users.

Silently ignoring flags isn't ideal, we should try to warn users as much as possible and potentially throw an error.


The compatibility matrix for Kubeadm is expanding with this issue to be not only the Kubeadm API version, but also the Kubernetes version that supports each field. Thankfully, we're carrying our own Kubeadm shim-like types, so adding a new field and cross-checking the kubeadm version shouldn't be an issue if we go down that path, although it does require us to introduce a new layer of compatibility, which might be ok in this case.