kubeadm config add support for more than one APIEndpoint #67832

fabriziopandini · 2018-08-24T15:39:36Z

What this PR does / why we need it:
This PR completes the changes in kubeadm for management of more than one control plane instances introducing the possibility to configure more than one APIEndpoints

Which issue(s) this PR fixes :
refs kubernetes/kubeadm#911, refs kubernetes/kubeadm#963

Special notes for your reviewer:
Depends on:

kubeadm config move ControlPlaneEndpoint to ClusterConfiguration #67830

Release note:

kubeadm: The kubeadm configuration now support definition of more than one control plane instances with their own APIEndpoint. The APIEndpoint for the "bootstrap" control plane instance should be defined using `InitConfiguration.APIEndpoint`, while the APIEndpoints for additional control plane instances should be added using `JoinConfiguration.APIEndpoint`.

/cc @kubernetes/sig-cluster-lifecycle-pr-reviews
/sig cluster-lifecycle
/area kubeadm
/kind api-change
/kind enhancement
/assign @luxas
/assign @timothysc
/cc @chuckha @rosti @neolit123 @liztio

fabriziopandini · 2018-08-24T15:47:06Z

/hold
until #67830 merge

timothysc

This doesn't look right.

Also perhaps I'm missing the [] or map

timothysc · 2018-08-24T20:25:10Z

cmd/kubeadm/app/apis/kubeadm/types.go

-	// API server will be installed only on nodes hosting an additional control plane instance.
-	AdvertiseAddress string
+	// APIEndpoint represents the endpoint of the instance of the API server eventually to be deployed on this node.
+	APIEndpoint APIEndpoint


The Naming is confusing here.

The APIEndpoint struct has an APIEndpoint member.

@timothysc Sorry but I don't understand you comment 😕

APIEndpoint struct has two attributes, AdvertiseAddress and BindPort:

Here we have an attribute that has the same name of its type. is that the thing you find confusing? Do you prefer naming the struct APIEndpointConfiguration

Sorry, the way github collapsed this changeset made me misinterpret this. I expanded it looks ok.

Or... are you looking for the "status" we discussed at the end of the call yesterday?

If this is the case, please consider that this PR implements only the "spec", while the status will be implemented the next PR (in the form of map[bindaddress]APIEndoint, but I'm figuring out details right now)

map[bindaddress]APIEndoint

i though more about this. we ideally need to map against the address the user provides instead of always defaulting to the address of the network interface, because we do have --apiserver-advertise-address.

if i understand this correctly this might result in an interesting situation, but i guess we can comment more in the next PR. :\

neolit123

thanks a lot @fabriziopandini
added some small comments LGTM!

neolit123 · 2018-08-24T21:35:16Z

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

-			},
+		APIEndpoint: kubeadmapiv1alpha3.APIEndpoint{
+			// GetAPIServerAltNames errors without an AdvertiseAddress; this is as good as any.
+			AdvertiseAddress: "127.0.0.1",


Maybe in such scenarios where we don't have good IP address, it is better to skip it in cert generation phase, but having default to localhost doesn't really make sense, as it leads to non-functional control plane.

@kad @neolit123 this PR doesn't implemented/changed this bit, only refactored the API struct.
If something should be fixed, this should be address in a separated PR (this one is already XXL 😉)

yeah, i'm just pointing it out because we had a discussion about 127.0.0.1 earlier.

neolit123 · 2018-08-24T21:37:45Z

cmd/kubeadm/app/util/config/masterconfig.go

+	if err := SetBootstrapTokensDynamicDefaults(&cfg.BootstrapTokens); err != nil {
+		return err
+	}
+


just a nit: some of these empty new lines in the function can be removed.
please, live as is for now if you'd prefer.

neolit123 · 2018-08-24T21:39:55Z

cmd/kubeadm/app/util/config/masterconfig.go

+	// Default all the embedded ComponentConfig structs
+	componentconfigs.Known.Default(cfg)
+
+	ip := net.ParseIP(advertiseAddress)


this will probably conflict with:
#67397
either this or that PR will need a rebase.

timothysc

looking good, but I think some the meta-data in your structs needs cleanup, otherwise LGTM. Please clean that up and we're good to go.

/approve

timothysc · 2018-08-26T16:29:59Z

cmd/kubeadm/app/apis/kubeadm/v1alpha2/types.go

@@ -285,6 +285,10 @@ type JoinConfiguration struct {
 	// API server will be installed only on nodes hosting an additional control plane instance.
 	AdvertiseAddress string `json:"advertiseAddress,omitempty"`

+	// BindPort sets the secure port for the API Server to bind to.
+	// Defaults to 6443.
+	BindPort int32 `json:"bindPort"`


Shouldn't this be omitempty, b/c previous versions of v1alpha2 didn't have this field.

timothysc · 2018-08-26T16:32:00Z

cmd/kubeadm/app/apis/kubeadm/v1alpha3/types.go

+
+	// APIEndpoints currently available in the cluster, one for each control plane/api server instance.
+	// The key of the map is the IP of the host's default interface
+	APIEndpoints map[string]APIEndpoint `json:"featureGates,`


This doesn't appear correct json:"featureGates,

rosti

Looks OK to me. A few places were converted back to InitConfiguration (from ClusterConfiguration), but I assume, that this way the change is narrower and PR is much more easy to review.

timothysc

/lgtm
/approve

Thx @fabriziopandini

k8s-ci-robot · 2018-08-27T14:24:26Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: fabriziopandini, timothysc

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~~ [fabriziopandini,timothysc]

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

k8s-github-robot · 2018-08-27T14:46:32Z

/test all [submit-queue is verifying that this PR is safe to merge]

k8s-github-robot · 2018-08-27T16:04:49Z

Automatic merge from submit-queue. If you want to cherry-pick this change to another branch, please follow the instructions here.

k8s-ci-robot added the release-note Denotes a PR that will be considered when it comes time to generate release notes. label Aug 24, 2018

k8s-ci-robot assigned luxas Aug 24, 2018

k8s-ci-robot added the sig/cluster-lifecycle Categorizes an issue or PR as relevant to SIG Cluster Lifecycle. label Aug 24, 2018

k8s-ci-robot assigned timothysc Aug 24, 2018

k8s-ci-robot added area/kubeadm size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API labels Aug 24, 2018

k8s-ci-robot requested review from chuckha, liztio, neolit123 and rosti August 24, 2018 15:39

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Aug 24, 2018

k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Aug 24, 2018

timothysc suggested changes Aug 24, 2018

View reviewed changes

neolit123 reviewed Aug 24, 2018

View reviewed changes

fabriziopandini force-pushed the kubeadm-config-APIEndpoint branch from 6a3109f to 1b90e44 Compare August 25, 2018 08:44

k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Aug 25, 2018

timothysc approved these changes Aug 26, 2018

View reviewed changes

fabriziopandini added 2 commits August 27, 2018 09:00

kubeadm config add support for more than one APIEndpoint

7dfb3c7

autogenerated

0add7f9

fabriziopandini force-pushed the kubeadm-config-APIEndpoint branch from 1b90e44 to 0add7f9 Compare August 27, 2018 07:01

rosti approved these changes Aug 27, 2018

View reviewed changes

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 27, 2018

timothysc approved these changes Aug 27, 2018

View reviewed changes

timothysc removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Aug 27, 2018

k8s-github-robot merged commit ed3c32c into kubernetes:master Aug 27, 2018

fabriziopandini deleted the kubeadm-config-APIEndpoint branch September 6, 2018 10:30

neolit123 mentioned this pull request Sep 18, 2018

Tracking issue for "Config to v1beta1" kubernetes/kubeadm#963

Closed

28 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

kubeadm config add support for more than one APIEndpoint #67832

kubeadm config add support for more than one APIEndpoint #67832

fabriziopandini commented Aug 24, 2018 •

edited

Loading

fabriziopandini commented Aug 24, 2018

timothysc left a comment

timothysc Aug 24, 2018

fabriziopandini Aug 24, 2018

timothysc Aug 24, 2018

fabriziopandini Aug 24, 2018 •

edited

Loading

neolit123 Aug 24, 2018

neolit123 left a comment

neolit123 Aug 24, 2018

kad Aug 25, 2018

fabriziopandini Aug 25, 2018

neolit123 Aug 25, 2018

neolit123 Aug 24, 2018

neolit123 Aug 24, 2018

timothysc left a comment

timothysc Aug 26, 2018

timothysc Aug 26, 2018

rosti left a comment

timothysc left a comment

k8s-ci-robot commented Aug 27, 2018

k8s-github-robot commented Aug 27, 2018

k8s-github-robot commented Aug 27, 2018

kubeadm config add support for more than one APIEndpoint #67832

kubeadm config add support for more than one APIEndpoint #67832

Conversation

fabriziopandini commented Aug 24, 2018 • edited Loading

fabriziopandini commented Aug 24, 2018

timothysc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fabriziopandini Aug 24, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

neolit123 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

timothysc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rosti left a comment

Choose a reason for hiding this comment

timothysc left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Aug 27, 2018

k8s-github-robot commented Aug 27, 2018

k8s-github-robot commented Aug 27, 2018

fabriziopandini commented Aug 24, 2018 •

edited

Loading

fabriziopandini Aug 24, 2018 •

edited

Loading