📖 CAEP: Insulate users from kubeadm API version changes #4170
Conversation
k8s-ci-robot
added
the
cncf-cla: yes
k8s-ci-robot
added
the
size/L
|
/milestone v0.4.0 |
k8s-ci-robot
added
the
kind/api-change
docs/proposals/20210210-insulate-users-from-kubeadm-API-changes.md
Outdated
Show resolved
Hide resolved
docs/proposals/20210210-insulate-users-from-kubeadm-API-changes.md
Outdated
Show resolved
Hide resolved
docs/proposals/20210210-insulate-users-from-kubeadm-API-changes.md
Outdated
Show resolved
Hide resolved
docs/proposals/20210210-insulate-users-from-kubeadm-API-changes.md
Outdated
Show resolved
Hide resolved
|
|
||
| While moving to a more recent version of the kubeadm API is a required move, Cluster API | ||
| should take this opportunity stop to exposing the kubeadm API types in the | ||
| KubeadmConfig/KubeadmControlPlane specs, and instead use our own types. |
There was a problem hiding this comment.
i think embedding the kubeadm v1beta1 type was a mistake.
a non-v1 type should not be embedded in another type as as a field. but there is also a risk for v1 types in a dynamic area of k8s. if v2 comes out (inevitable in some places) you'd have to bump your API if you want to use the v2 features.
wrapping kubeadm types will inevitably introduce burden on CAPI and just more types to maintain. how much are you going to expose out of the kubeadm API?
i think the right pattern is to:
- pass the kubeadm types as separate structures (separated with
---\n) - let users configure everything
- document what cannot be configured and CAP* will override it
- error out on outdated APIs
- support conversion on public types, where both CAP* and users can do it on demand.
There was a problem hiding this comment.
There is a little bit of confusion here, I rephrased it to make it more clear, PTAL.
docs/proposals/20210210-insulate-users-from-kubeadm-API-changes.md
Outdated
Show resolved
Hide resolved
| v1.16 could work with v1beta2 types. Same for all the other versions up to latest(1.20) | ||
| and next (1.21). | ||
| - limitations: this approach is not future proof, and it should be reconsidered | ||
| whenever a new version of kubeadm types is created while v1alpha3 is still supported. |
There was a problem hiding this comment.
it does feel like a temporary hack.
There was a problem hiding this comment.
there are pro and cons.
it has limitations and it is not future proof but it limits the impact/the risks on the v1alpha3 branch.
but let's see other opinions on the two alternatives
docs/proposals/20210210-insulate-users-from-kubeadm-API-changes.md
Outdated
Show resolved
Hide resolved
docs/proposals/20210210-insulate-users-from-kubeadm-API-changes.md
Outdated
Show resolved
Hide resolved
|
/test pull-cluster-api-test-main |
1 similar comment
|
/test pull-cluster-api-test-main |
fabriziopandini
changed the title
docs/proposals/20210210-insulate-users-from-kubeadm-API-changes.md
Outdated
Show resolved
Hide resolved
fabriziopandini
force-pushed
the
kubeadm-types-KEP
branch
from
cdc8261 to
bbae2a2
Compare
|
/test pull-cluster-api-test-main |
|
/test pull-cluster-api-test-main |
docs/proposals/20210210-insulate-users-from-kubeadm-API-changes.md
Outdated
Show resolved
Hide resolved
docs/proposals/20210210-insulate-users-from-kubeadm-API-changes.md
Outdated
Show resolved
Hide resolved
|
|
||
| Planned actions are: | ||
|
|
||
| - introduce a Cluster API owned version of the kubeadm config types |
There was a problem hiding this comment.
would this "version" be unversioned?
There was a problem hiding this comment.
No, those types will be part of Cluster API types, and thus initially versioned as v1alpha4.
The advantage is that once conversion will be in place, those types are not required anymore to have the same serialization format of the real kubeadm types.
Added a sentence to make this more clear
| __Alternative 1:__ | ||
|
|
||
| Keep kubeadm v1beta1 types as a Hub type (1); implement conversion to kubeadm API | ||
| version f(Kubernetes Version) when generating the kubeadm config for init/join. |
There was a problem hiding this comment.
I'm not 100% clear on what implement conversion to kubeadm API version f(Kubernetes Version) means
There was a problem hiding this comment.
Added an example to make this more clear
|
@CecileRobertMichon thanks for review, really appreciated! |
k8s-ci-robot
added
the
lgtm
There was a problem hiding this comment.
A couple of minor comments, otherwise this proposal lgtm. Thanks @fabriziopandini for all the effort you've put into this.
| anymore to have the same serialization format of the real kubeadm types. | ||
| - preserve `bootstrap/kubeadm/types/v1beta1` as a serialization/deserialization | ||
| spoke (1) for v1alpha4 (also, this will be used by v1alpha3 API types until removal) | ||
| - preserve `bootstrap/kubeadm/types/v1beta2` as serialization/deserialization |
There was a problem hiding this comment.
Weren't there plans to move the kubeadm types out of k/k? Ideally that would be a long term goal to reference the official kubeadm types even if we need to generate our own conversions to/from rather than having to worry about potential config drift between two different sources of truth.
There was a problem hiding this comment.
Unfortunately the for moving kubeadm out of k/k is getting stuck.
However I added a note to document this.
| Planned actions are: | ||
|
|
||
| - introduce a Cluster API owned version of the kubeadm config types | ||
| (starting from kubeadm v1beta1?) to be used by KubeadmConfig/KubeadmControlPlane |
There was a problem hiding this comment.
If there are additional features that are not available in v1beta1 and there is any possibility that we want to support those features, then it might be a good idea to start with the v1beta2 types as the hub type.
There was a problem hiding this comment.
The only feature that not available in v1beta1 is IgnorePreflightError field, but after some thinking my idea is to start from v1beta1, get all the conversion and fuzz test in place, and only in a follow up PR, add the IgnorePreflightError to the Cluster API owned types.
I have added a bulled in the planned actions to track this
k8s-ci-robot
removed
the
lgtm
|
/test pull-cluster-api-test-main |
2 similar comments
|
/test pull-cluster-api-test-main |
|
/test pull-cluster-api-test-main |
fabriziopandini
force-pushed
the
kubeadm-types-KEP
branch
from
d779f75 to
4fc3399
Compare
|
squashed commits, all comment addressed. |
k8s-ci-robot
added
the
lgtm
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: detiber 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:
Approvers can indicate their approval by writing |
k8s-ci-robot
added
the
approved
What this PR does / why we need it:
As per #2769 (comment)
IMPORTANT! This task is a release blocker for CABPK/v1alpha4. Changes in v1alpha3 are required to support Kubernetes version >= v1.21 (assuming removal of v1beta1 types in kubeadm happens in v1.21)
Which issue(s) this PR fixes:
Rif #2769