kubeadm: Refactor the .Etcd substruct in the v1alpha2 API #64066

luxas · 2018-05-19T13:10:45Z

What this PR does / why we need it:
Splits the monolithic .Etcd struct with all the options as fields to a more modular and clear design with two sub-structs for the different modes of hosting etcd we support.

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Part of kubernetes/community#2131

Special notes for your reviewer:
Depends on:

Add roundtrip, defaulting, upgrading and validation unit tests for the kubeadm API types #63917

Follows up: #63871
TODO: I still need to write unit tests for this.

Release note:

[action required] kubeadm: The `:Etcd` struct has been refactored in the v1alpha2 API. All the options now reside under either `.Etcd.Local` or `.Etcd.External`. Automatic conversions from the v1alpha1 API are supported.

@kubernetes/sig-cluster-lifecycle-pr-reviews @liztio

k8s-ci-robot · 2018-05-19T13:11:26Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: luxas

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:

~~cmd/kubeadm/OWNERS~~ [luxas]

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

dixudx

The old configurations for Etcd are replaced by Etcd.Local and Etcd.External respectively. The is a big change, but makes the configurations more concise.

Better add a notice somewhere to warn the users, since this new change will block in-place updates with old config file.

luxas · 2018-05-21T16:59:35Z

@timothysc this is ready to merge.

Better add a notice somewhere to warn the users, since this new change will block in-place updates with old config file.

@dixudx we support full backwards-compability. All existing users will be upgraded to the new config automatically, thanks to the automatic API machinery conversations from v1alpha1 to v1alpha2. Also, a release note with action required is automatically created from this PR.

luxas · 2018-05-21T16:59:46Z

/retest

neolit123 · 2018-05-21T18:19:57Z

/test pull-kubernetes-e2e-kops-aws

timothysc · 2018-05-21T20:53:21Z

/assign @detiber @stealthybox

They are the last two to update this area, they should have a say.

fabriziopandini · 2018-05-22T06:44:27Z

cmd/kubeadm/app/apis/kubeadm/validation/validation.go

+		allErrs = append(allErrs, ValidateCertSANs(e.Local.PeerCertSANs, localPath.Child("peerCertSANs"))...)
+	}
+	if e.External != nil {
+		allowHTTP := false


I think that requireHTTPS is more clear (the logic should be changed accordingly)

Can you/I please follow-up on that?

Might be a new issue help/wanted for new contributors 😉

fabriziopandini · 2018-05-22T06:45:53Z

cmd/kubeadm/app/phases/certs/certs.go

@@ -51,8 +51,7 @@ func CreatePKIAssets(cfg *kubeadmapi.MasterConfiguration) error {
 		CreateAPIServerEtcdClientCertAndKeyFiles,
 	}

-	// Currently this is the only way we have to identify static pod etcd vs external etcd
-	if len(cfg.Etcd.Endpoints) == 0 {
+	if cfg.Etcd.Local != nil || cfg.Etcd.External == nil {


|| cfg.Etcd.External == nil seems not necessary

There is an edge case here actually when we don't know which one of the structs are defined or the other undefined (in unit tests), so hence I added this. It doesn't hurt I guess...?

Ok. If possible this should be annotated in comments because this is the only place where this is required

Okay, now I changed the unit tests instead, so now it works as you wanted it 👍

fabriziopandini

+💯 for cleaning up the etcd configuration
Only two minor comments from my side

stealthybox · 2018-05-22T22:23:00Z

@luxas should we bump the api version for the featureGates struct and regenerate the config conversions before we merge this so that we have an independent patch for fixing the previous config issue we introduced with v1.10.0 ?

luxas · 2018-05-23T13:26:32Z

@luxas should we bump the api version for the featureGates struct and regenerate the config conversions before we merge this so that we have an independent patch for fixing the previous config issue we introduced with v1.10.0 ?

I've worked a lot the latest week on building on top of @liztio's initial great patch for that in order to make this all work flawlessly. See the earlier PRs linked in kubernetes/community#2131

Basically what happens can be read in https://github.com/kubernetes/kubernetes/blob/master/cmd/kubeadm/app/util/config/masterconfig.go#L109-L160.
TL;DR; The featureGates and lack of TypeMeta when marshalled issues are resolved for the v1alpha1 version. That's the first that happens. Internally, when loading a v1alpha1 version in kubeadm v1.11, it will be automatically converted to the internal version which equals v1alpha2 which is the output format.
Also see the testcases in util/config/testdata

Does that answer your question?
I'm rebasing now and addressing @fabriziopandini's nits so we can merge.

stealthybox · 2018-05-23T16:51:18Z

/test pull-kubernetes-bazel-test

@luxas that does answer my question -- I'm glad the serialization issues are remedied.

luxas · 2018-05-23T20:42:55Z

/retest

k8s-github-robot · 2018-05-24T17:45:17Z

Automatic merge from submit-queue (batch tested with PRs 64127, 63895, 64066, 64215, 64202). If you want to cherry-pick this change to another branch, please follow the instructions here.

luxas assigned timothysc May 19, 2018

k8s-ci-robot requested review from detiber and dixudx May 19, 2018 13:11

k8s-ci-robot added the area/kubeadm label May 19, 2018

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 19, 2018

dixudx reviewed May 20, 2018

View reviewed changes

luxas force-pushed the kubeadm_etcd_refactor branch from aac8aae to 213f8f0 Compare May 21, 2018 15:41

k8s-ci-robot assigned detiber and stealthybox May 21, 2018

fabriziopandini reviewed May 22, 2018

View reviewed changes

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

luxas force-pushed the kubeadm_etcd_refactor branch 2 times, most recently from 6adb790 to 0038aba Compare May 23, 2018 14:02

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

luxas added 2 commits May 23, 2018 21:13

kubeadm: Refactor the .Etcd substruct in the v1alpha2 API

099e60b

autogenerated

2d0efea

luxas force-pushed the kubeadm_etcd_refactor branch from 0038aba to 2d0efea Compare May 23, 2018 18:13

luxas added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 23, 2018

k8s-github-robot merged commit b712eaf into kubernetes:master May 24, 2018

luxas mentioned this pull request Jun 12, 2018

v1alpha2 versioning for kubeadm configuration structs kubernetes/kubeadm#750

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

kubeadm: Refactor the .Etcd substruct in the v1alpha2 API #64066

kubeadm: Refactor the .Etcd substruct in the v1alpha2 API #64066

luxas commented May 19, 2018 •

edited

k8s-ci-robot commented May 19, 2018

dixudx left a comment

luxas commented May 21, 2018

luxas commented May 21, 2018

neolit123 commented May 21, 2018

timothysc commented May 21, 2018

fabriziopandini May 22, 2018

luxas May 22, 2018

fabriziopandini May 23, 2018

fabriziopandini May 22, 2018 •

edited

luxas May 22, 2018

fabriziopandini May 23, 2018

luxas May 23, 2018

fabriziopandini left a comment

stealthybox commented May 22, 2018

luxas commented May 23, 2018 •

edited

stealthybox commented May 23, 2018 •

edited

luxas commented May 23, 2018

k8s-github-robot commented May 24, 2018

kubeadm: Refactor the .Etcd substruct in the v1alpha2 API #64066

kubeadm: Refactor the .Etcd substruct in the v1alpha2 API #64066

Conversation

luxas commented May 19, 2018 • edited

k8s-ci-robot commented May 19, 2018

dixudx left a comment

Choose a reason for hiding this comment

luxas commented May 21, 2018

luxas commented May 21, 2018

neolit123 commented May 21, 2018

timothysc commented May 21, 2018

fabriziopandini May 22, 2018

Choose a reason for hiding this comment

luxas May 22, 2018

Choose a reason for hiding this comment

fabriziopandini May 23, 2018

Choose a reason for hiding this comment

fabriziopandini May 22, 2018 • edited

Choose a reason for hiding this comment

luxas May 22, 2018

Choose a reason for hiding this comment

fabriziopandini May 23, 2018

Choose a reason for hiding this comment

luxas May 23, 2018

Choose a reason for hiding this comment

fabriziopandini left a comment

Choose a reason for hiding this comment

stealthybox commented May 22, 2018

luxas commented May 23, 2018 • edited

stealthybox commented May 23, 2018 • edited

luxas commented May 23, 2018

k8s-github-robot commented May 24, 2018

luxas commented May 19, 2018 •

edited

fabriziopandini May 22, 2018 •

edited

luxas commented May 23, 2018 •

edited

stealthybox commented May 23, 2018 •

edited