⚠️ Add CAPI owned kubeadm types #4255

fabriziopandini · 2021-03-04T22:30:21Z

What this PR does / why we need it:
This PR:

Adds a CAPI owned copy of kubeadm types.
Implements conversion from the CAPI owned copy of kubeadm types to the actual kubeadm types (v1beta1 and v1beta2).
Uses conversions for generating the Kubeadm Config files in CABPK

Which issue(s) this PR fixes:
Fixes #

/milestone v0.4.0
/hold
For the CAEP to merge

@vincepri

fabriziopandini · 2021-03-05T09:46:39Z

/retest

fabriziopandini · 2021-03-05T10:05:24Z

/test pull-cluster-api-test-main

fabriziopandini · 2021-03-11T12:28:19Z

/hold cancel

fabriziopandini · 2021-03-11T13:07:07Z

@vincepri this one is ready for review now

fabriziopandini · 2021-03-12T11:33:04Z

/test pull-cluster-api-test-main

vincepri · 2021-03-13T16:17:40Z

util/conversion/conversion.go

+				// NOTE: Kubeadm types does not have ObjectMeta, so this step is executed only when required.
+				metaAfter, ok := spokeAfter.(metav1.Object)
+				if ok {
+					delete(metaAfter.GetAnnotations(), DataAnnotation)
+				}


If there is not ObjectMeta in the kubeadm types, why use this function to test? Could we pass the encapsulating object instead?

With the new conversions, kubeadm v1beta1 and v1beta2 two types are defined as conversion spoke, and the don't have encapsulating objects anymore.

So, given that v1beta1 and v1beta2 (which are mirrored from kubeadm) do not have ObjectMeta, it was require to introduce this type check before calling delete

Wouldn't it be better to just use the super-types instead each inner type? Without this logic, we have no way to preserve conversion between types

@vincepri I have added a commit where I'm trying to make skipping this operation something that should be required explicitly (vs trying to assume intent)

controlplane/kubeadm/api/v1alpha3/conversion_test.go

bootstrap/kubeadm/types/v1beta2/doc.go

bootstrap/kubeadm/types/v1beta1/doc.go

vincepri · 2021-03-13T16:23:14Z

bootstrap/kubeadm/api/v1alpha4/kubeadm_types.go

+type BootstrapTokenString struct {
+	ID     string `json:"-"`
+	Secret string `json:"-"`
+}


Do we need to keep the token as a struct?

@fabriziopandini

According to the CAEP as a first step we are going to create v1alpha4 types equal to v1beta1 (thus including token struct).
As soon as conversion are in place then we could start simplifying Cluster API owned types

k8s-ci-robot · 2021-04-06T19:46:39Z

@fabriziopandini: The following test failed, say /retest to rerun all failed tests:

Test name	Commit	Details	Rerun command
pull-cluster-api-apidiff-main	`e8cb88e`	link	`/test pull-cluster-api-apidiff-main`

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

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. I understand the commands that are listed here.

fabriziopandini · 2021-04-06T20:03:26Z

@vincepri this is back in shape now (Problems with go-bindata were resolved by #4378)

vincepri

/approve
/lgtm

k8s-ci-robot · 2021-04-06T21:05:17Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: vincepri

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~~ [vincepri]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

k8s-ci-robot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. labels Mar 4, 2021

k8s-ci-robot added this to the v0.4.0 milestone Mar 4, 2021

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Mar 4, 2021

k8s-ci-robot requested review from justinsb and vincepri March 4, 2021 22:30

k8s-ci-robot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Mar 4, 2021

fabriziopandini force-pushed the add-CAPI-owned-kubeadm-types branch from a6b0926 to 6fd499e Compare March 5, 2021 09:17

fabriziopandini force-pushed the add-CAPI-owned-kubeadm-types branch from 6fd499e to 15a6ffb Compare March 11, 2021 12:26

fabriziopandini changed the title ~~[WIP] ⚠️ Add CAPI owned kubeadm types~~ ⚠️ Add CAPI owned kubeadm types Mar 11, 2021

k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Mar 11, 2021

k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Mar 11, 2021

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 11, 2021

fabriziopandini force-pushed the add-CAPI-owned-kubeadm-types branch from 15a6ffb to 02e6697 Compare March 12, 2021 10:19

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 12, 2021

vincepri reviewed Mar 13, 2021

View reviewed changes

fabriziopandini force-pushed the add-CAPI-owned-kubeadm-types branch from ad73e03 to 3c3f298 Compare March 22, 2021 15:32

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 22, 2021

fabriziopandini force-pushed the add-CAPI-owned-kubeadm-types branch from 3c3f298 to 35c2ffe Compare March 26, 2021 13:25

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 26, 2021

Add CAPI owned kubeadm types

e8cb88e

fabriziopandini force-pushed the add-CAPI-owned-kubeadm-types branch from 35c2ffe to e8cb88e Compare April 6, 2021 19:44

vincepri approved these changes Apr 6, 2021

View reviewed changes

k8s-ci-robot assigned vincepri Apr 6, 2021

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 6, 2021

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 6, 2021

k8s-ci-robot merged commit 5f08b4a into kubernetes-sigs:master Apr 6, 2021

fabriziopandini deleted the add-CAPI-owned-kubeadm-types branch April 7, 2021 14:11

fabriziopandini mentioned this pull request Apr 7, 2021

🌱 Make KCP using embedded kubeadm types while manipulating the kubeadm-config ConfigMap #4443

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

⚠️ Add CAPI owned kubeadm types #4255

⚠️ Add CAPI owned kubeadm types #4255

fabriziopandini commented Mar 4, 2021 •

edited

Loading

fabriziopandini commented Mar 5, 2021

fabriziopandini commented Mar 5, 2021

fabriziopandini commented Mar 11, 2021

fabriziopandini commented Mar 11, 2021

fabriziopandini commented Mar 12, 2021

vincepri Mar 13, 2021

fabriziopandini Mar 15, 2021 •

edited

Loading

vincepri Mar 15, 2021

fabriziopandini Mar 18, 2021

vincepri Mar 13, 2021

vincepri Mar 15, 2021

fabriziopandini Mar 17, 2021

k8s-ci-robot commented Apr 6, 2021 •

edited

Loading

fabriziopandini commented Apr 6, 2021

vincepri left a comment

k8s-ci-robot commented Apr 6, 2021

⚠️ Add CAPI owned kubeadm types #4255

⚠️ Add CAPI owned kubeadm types #4255

Conversation

fabriziopandini commented Mar 4, 2021 • edited Loading

fabriziopandini commented Mar 5, 2021

fabriziopandini commented Mar 5, 2021

fabriziopandini commented Mar 11, 2021

fabriziopandini commented Mar 11, 2021

fabriziopandini commented Mar 12, 2021

vincepri Mar 13, 2021

Choose a reason for hiding this comment

fabriziopandini Mar 15, 2021 • edited Loading

Choose a reason for hiding this comment

vincepri Mar 15, 2021

Choose a reason for hiding this comment

fabriziopandini Mar 18, 2021

Choose a reason for hiding this comment

vincepri Mar 13, 2021

Choose a reason for hiding this comment

vincepri Mar 15, 2021

Choose a reason for hiding this comment

fabriziopandini Mar 17, 2021

Choose a reason for hiding this comment

k8s-ci-robot commented Apr 6, 2021 • edited Loading

fabriziopandini commented Apr 6, 2021

vincepri left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Apr 6, 2021

fabriziopandini commented Mar 4, 2021 •

edited

Loading

fabriziopandini Mar 15, 2021 •

edited

Loading

k8s-ci-robot commented Apr 6, 2021 •

edited

Loading