APIEndpoint to ClusterNetworkingConfig cluster-types

[alejandroEsc]

What this PR does / why we need it: clusterclient.go assumes that the port to be used to contact apiserver is 443, meanwhile the default value for kubeadm is 6443. Meaning that a user would have to manually configure on their script the 443 value, which may not be what they want. Here we allow the cluster-type to contain and APIEndpoint object under the clusternetwork configuration so that it can be passed to kubeadm or whatever provisioning tool of choice.

The change will also keep us from making breaking changes to cluster-client, though It suspect that in the many clusters case some additional changes may be required (as of yet unknown). Currently without this change, most users who are unaware of the need to set kubeadm to bindPort 443 may not be able to have nodes join their cluster.

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #

Special notes for your reviewer:

Please confirm that if this PR changes any image versions, then that's the sole change this PR makes.

Release note:

cluster-type change to allow APIEndpoint object under clusternetworkconfig. Should allow one to define what port to use and pass that to the kubernetes provisioning tool or fall back to the default 443.

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: alejandroEsc
To fully approve this pull request, please assign additional approvers.
We suggest the following additional approver: jessicaochen

If they are not already assigned, you can assign the PR to them by writing /assign @jessicaochen in a comment when ready.

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

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

[davidewatson]

I agree that the port should not be hard-coded within the deployer, however, I am not sure this should be a part of either ClusterNetworkingConfig or ClusterSpec since some providers may not allow the APIEndpoint to be specified (e.g. it is only known as status). Cf. https://github.com/kubernetes-sigs/cluster-api/issues/158

Maybe it does make sense as an optional part of ClusterSpec. This would match the corresponding APIEndpoints filed of ClusterStatus.

[alejandroEsc]

Well, its optional, and doesnt have to be defined if one doesnt. I am not opposed to placing this info elsewhere but here it seems like a natural fit? Im am open to suggestions, anyway this could really block others so I would like some thoughts.

[frobware]

At first blush this PR seems to be solely adding APIEndpoint to type ClusterNetworkingConfig, but why are so many other types changing [name] as part of this commit?

[alejandroEsc]

Those are done by autogeneration which for whatever reason moved things around.

…

On Tue, Aug 7, 2018, 8:40 AM Andrew McDermott ***@***.***> wrote: At first blush this PR seems to be solely adding APIEndpoint to type ClusterNetworkingConfig, but why are so many other types changing [name] as part of this commit? — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#467 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AT0KkpCIGvjJvMX2N2JKv6C3pS_7ciFjks5uObT7gaJpZM4VsoBQ> .

[roberthbailey]

@alejandroEsc - can you take a look at https://github.com/kubernetes-sigs/cluster-api/issues/158? As we discussed during the working group meeting today, fixing that issue might also solve the underlying problem you are trying to address with this PR.

[roberthbailey]

/hold

[alejandroEsc]

@roberthbailey I think the ticket you are pointing to is referring to a large degree a change to cluster controller as the thing responsible for updating api-endpoints . That may be holistic and may cover this piece, but not the how and the details. I agree my change here is looking more and more like a bandaid than the solution.

My thinking on this is that the cluster controller not update itself with this details but that the machine actuator do the update as part of my description below:

So thinking here about how this would work all together my thought is that the machine actuator code should have probably contain the following methods:

• GetKubeconfig (master should point to this, could call on updateAPIEndpoints, etc.., exists already)

• UpdateAPIEndpoints (to update status endpoints, provider specific details + master machine details)

and we remove the machine actuator code

• GetIP

The methods would then be implemented by a provider with details and since the cluster object is passed in each of the above code for the machine actuator, we can rely here as the place where the updates can be addressed. Anytime that a machine is created/updated/deleted, the code to udpateAPIEndpoints can/should be invoked at some point (details provider specific) or we can make it part of each of the other reconcile processes for updating/deleting/creating, etc... . My only problem with this scheme is how to enforce it, other than making it part of the machine code itself as something that must be added to the actuator interface code.

Anyway thats my rant sorry about that, hope it makes some sense.

[roberthbailey]

We discussed this during the working group meeting yesterday but I wanted to post some comments here as well for folks that didn't see the notes or recording.

The proposed scheme won't scale well to a cluster with multiple apiservers (commonly called HA) because each machine actuator that deploys an apiserver would update the endpoints. While the endpoints is an array because we should theoretically be able to have more than one, most client tooling requires a single endpoint for the cluster. In practice this is achieved by having a load balancer sitting in front of the apiservers and using the load balancer ip as the cluster endpoint.

Hence the suggestion in that issue for the cluster controller to be responsible for the IP. In a "single master" cluster the cluster controller would take the address of the master machine. In a "multi-master" cluster the cluster controller would provision a LB, point it to each master machine, and then set the IP to that of the LB.

It would also make it easier to decouple the "master machine" concept from the cluster endpoint, which is tied in with the removal of machine roles from the API (#338).

[alejandroEsc]

@roberthbailey so the LB argument is an interesting one, simply because if the concern of this tool is to bring up a cluster, why should we extend the concern of an additional component which would be the LB? That seems like it should be something out of the scope of the requirements for the clusterctl tool.

[roberthbailey]

Not the clusterctl tool, but the cluster controller. Right now the gce cluster controller already does things like set up firewall rules, having it create a LB doesn't seem terribly different.

[alejandroEsc]

@roberthbailey thats more or less where i am getting that. It seems like that should be a provider detail that is/should be done at that level. Not necessarily a concern of cluster controller (sorry mispoke above) directly, though i suppose i can see why the logic would belong there. So yeah sounds like a good plan.

For the example you gave there. are those things created at the level of the cluster controller? Also is there guidance about what things belong in the cluster controller, vs machine controller? You can see where the concerns can be sometimes involved, where updating APIEndpoints is somewhat mixed as we could easily argue either point to varying degrees.

[alejandroEsc]

also feel free to close this PR. and thanks for considering it! If there is someplace you think i can help on, let me know :)

[roberthbailey]

We have provider specific cluster controllers in addition to the machine controllers. I think keeping the machine controllers scoped to managing the lifecycle of machines (vs the control plane) makes a lot of sense.

There was some discussion a while back on trying to standardize an actuator-like interface for the cluster controllers but it wasn't as clear that a simple consistent interface would be possible for all providers.

[roberthbailey]

I don't think we want to put APIEnpoints into the ClusterNetworkingConfig (at least not yet), so I'm going to close this PR to keep the backlog in check. Let's re-assess after we address #158.