New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

kubeadm config add support for more than one APIEndpoint #67832

Merged
merged 2 commits into from Aug 27, 2018

Conversation

@fabriziopandini
Contributor

fabriziopandini commented Aug 24, 2018

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:

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

This comment has been minimized.

Show comment
Hide comment
@fabriziopandini

fabriziopandini Aug 24, 2018

Contributor

/hold
until #67830 merge

Contributor

fabriziopandini commented Aug 24, 2018

/hold
until #67830 merge

@timothysc

This doesn't look right.

Also perhaps I'm missing the [] or map

// 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

This comment has been minimized.

@timothysc

timothysc Aug 24, 2018

Member

The Naming is confusing here.

The APIEndpoint struct has an APIEndpoint member.

@timothysc

timothysc Aug 24, 2018

Member

The Naming is confusing here.

The APIEndpoint struct has an APIEndpoint member.

This comment has been minimized.

@fabriziopandini

fabriziopandini Aug 24, 2018

Contributor

@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

@fabriziopandini

fabriziopandini Aug 24, 2018

Contributor

@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

This comment has been minimized.

@timothysc

timothysc Aug 24, 2018

Member

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

@timothysc

timothysc Aug 24, 2018

Member

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

This comment has been minimized.

@fabriziopandini

fabriziopandini Aug 24, 2018

Contributor

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)

@fabriziopandini

fabriziopandini Aug 24, 2018

Contributor

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)

This comment has been minimized.

@neolit123

neolit123 Aug 24, 2018

Member

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

neolit123 Aug 24, 2018

Member

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!

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

This comment has been minimized.

@neolit123

neolit123 Aug 24, 2018

Member

@kad ^ FYI

@neolit123

This comment has been minimized.

@kad

kad Aug 25, 2018

Member

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

kad Aug 25, 2018

Member

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.

This comment has been minimized.

@fabriziopandini

fabriziopandini Aug 25, 2018

Contributor

@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 😉)

@fabriziopandini

fabriziopandini Aug 25, 2018

Contributor

@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 😉)

This comment has been minimized.

@neolit123

neolit123 Aug 25, 2018

Member

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

@neolit123

neolit123 Aug 25, 2018

Member

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

Show outdated Hide outdated cmd/kubeadm/app/util/config/masterconfig.go
// Default all the embedded ComponentConfig structs
componentconfigs.Known.Default(cfg)
ip := net.ParseIP(advertiseAddress)

This comment has been minimized.

@neolit123

neolit123 Aug 24, 2018

Member

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

@neolit123

neolit123 Aug 24, 2018

Member

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

@k8s-ci-robot k8s-ci-robot added size/XL and removed size/XXL labels Aug 25, 2018

@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

@rosti

rosti approved these changes Aug 27, 2018

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.

@k8s-ci-robot k8s-ci-robot added the lgtm label Aug 27, 2018

@timothysc

/lgtm
/approve

Thx @fabriziopandini

@k8s-ci-robot

This comment has been minimized.

Show comment
Hide comment
@k8s-ci-robot

k8s-ci-robot Aug 27, 2018

Contributor

[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:

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

Contributor

k8s-ci-robot commented Aug 27, 2018

[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:

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

@k8s-merge-robot

This comment has been minimized.

Show comment
Hide comment
@k8s-merge-robot

k8s-merge-robot Aug 27, 2018

Contributor

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

Contributor

k8s-merge-robot commented Aug 27, 2018

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

@k8s-merge-robot

This comment has been minimized.

Show comment
Hide comment
@k8s-merge-robot

k8s-merge-robot Aug 27, 2018

Contributor

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

Contributor

k8s-merge-robot commented Aug 27, 2018

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

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

17 of 18 checks passed

Submit Queue Required Github CI test is not green: pull-kubernetes-kubemark-e2e-gce-big
Details
cla/linuxfoundation fabriziopandini authorized
Details
pull-kubernetes-bazel-build Job succeeded.
Details
pull-kubernetes-bazel-test Job succeeded.
Details
pull-kubernetes-cross Skipped
pull-kubernetes-e2e-gce Job succeeded.
Details
pull-kubernetes-e2e-gce-100-performance Job succeeded.
Details
pull-kubernetes-e2e-gce-device-plugin-gpu Job succeeded.
Details
pull-kubernetes-e2e-gke Skipped
pull-kubernetes-e2e-kops-aws Job succeeded.
Details
pull-kubernetes-e2e-kubeadm-gce Skipped
pull-kubernetes-integration Job succeeded.
Details
pull-kubernetes-kubemark-e2e-gce-big Job succeeded.
Details
pull-kubernetes-local-e2e Skipped
pull-kubernetes-local-e2e-containerized Skipped
pull-kubernetes-node-e2e Job succeeded.
Details
pull-kubernetes-typecheck Job succeeded.
Details
pull-kubernetes-verify Job succeeded.
Details

@fabriziopandini fabriziopandini deleted the fabriziopandini:kubeadm-config-APIEndpoint branch Sep 6, 2018

@neolit123 neolit123 referenced this pull request Sep 18, 2018

Closed

Tracking issue for "Config to v1beta1" #963

19 of 28 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment