Skip to content

Revert proposal change of ClusterStatus.APIEndpoints to APIEndpoint #1193

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

Merged
merged 1 commit into from
Jul 25, 2019

Conversation

moshloop
Copy link
Contributor

APIEndpoint is only relevant in environments with native load balancers, For on-premise APIEndpoints is the logical field for client-side load balancers to use.

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Jul 24, 2019
@k8s-ci-robot k8s-ci-robot requested review from detiber and justinsb July 24, 2019 20:14
@k8s-ci-robot k8s-ci-robot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Jul 24, 2019
@detiber
Copy link
Member

detiber commented Jul 24, 2019

@moshloop I'm not sure I agree. In general this has been used centrally for determining how to contact the API server for a cluster, which for the purpose of Kubernetes clients is required to be a single endpoint. If we need to track multiple ControlPlane Endpoints, we should probably track those separately rather than overload a field intended to be used by clients.

@detiber
Copy link
Member

detiber commented Jul 24, 2019

/assign @ncdc

@moshloop
Copy link
Contributor Author

It doesn't need to be a single endpoint, just a single endpoint exposed to clients.

Currently I have a client-side load balancer using haproxy + consul, and all clients (kubelet, kubeadm, etcc) connect to haproxy on localhost:8443 as the single endpoint.

I was planning on replacing the consul component with APIEndpoints,

@vincepri
Copy link
Member

How are you using multiple endpoints in a kubeconfig or in client-go?

@detiber
Copy link
Member

detiber commented Jul 24, 2019

@moshloop how would an external management server know where to contact the Cluster's API Server in order to check Node status for the NodeRef controller in that model? Or how would a workload cluster administrator determine how to interact with their Cluster?

@moshloop
Copy link
Contributor Author

How are you using multiple endpoints in a kubeconfig or in client-go?

With a client-side loadbalancer deployed out-of-band from k8s e.g. localhost:8443

@moshloop
Copy link
Contributor Author

how would an external management server know where to contact the Cluster's API Server

Pick any random APIEndpoint from the list (performing a type of client-side load balancing)

ow would a workload cluster administrator determine how to interact with their Cluster?

Pick any random APIEndpoint from the list or rely on something like DNS round-robin

@ncdc
Copy link
Contributor

ncdc commented Jul 25, 2019

Are there any v1alpha1 Cluster API providers that set more than 1 entry in the APIEndpoints array today?

@detiber
Copy link
Member

detiber commented Jul 25, 2019

Are there any v1alpha1 Cluster API providers that set more than 1 entry in the APIEndpoints array today?

I know the vSphere provider does this today.

@ncdc
Copy link
Contributor

ncdc commented Jul 25, 2019

Given that we originally said we didn't want to make any data model or behavioral changes to the Cluster other than removing the actuators, I think it's fair to merge this and we'll file a separate issue to discuss apiserver endpoint(s) and how internal CAPI consumers use them vs what an external client should use.

/approve
/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 25, 2019
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: moshloop, ncdc

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-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 25, 2019
@moshloop
Copy link
Contributor Author

From what I can see CAPV sets the API Endpoint to the endpoint used in the saved kubeconfig:

https://github.com/kubernetes-sigs/cluster-api-provider-vsphere/blob/master/pkg/cloud/vsphere/actuators/cluster/actuator.go#L196

This is flawed and something I want to fix, the flow should be:

  1. Pick a random endpoint to create a remote client (using saved or dynamically generated certs)
    1a) Requeue if the endpoint is not working
  2. Update APIEndpoints with the list of active ready masters.

@akutz / @timothysc any thoughts on this?

@detiber
Copy link
Member

detiber commented Jul 25, 2019

Pick a random endpoint to create a remote client (using saved or dynamically generated certs)
1a) Requeue if the endpoint is not working

This isn't default behavior for kubernetes clients, which expect a single stable endpoint.

@k8s-ci-robot k8s-ci-robot merged commit 4007f42 into kubernetes-sigs:master Jul 25, 2019
@akutz
Copy link
Contributor

akutz commented Jul 25, 2019

CAPV doesn’t set APIEndpoints to more than one value, and Moshe’s evaluation isn’t completely accurate either.

In other words, CAPV uses a single endpoint for APIEndpoints.

If there’s HA then the load balancer FQDN is used, otherwise the API endpoint of the oldest control plane node is used.

@akutz
Copy link
Contributor

akutz commented Jul 25, 2019

Seems odd to merge this when there were outstanding questions on CAPV for which no authoritative answer was provided. I was pinged to answer a question, but apparently there was a time limit to respond of which I was unaware.

@ncdc
Copy link
Contributor

ncdc commented Jul 25, 2019

@akutz this reverts a data model change (in the proposal, at least) so it is back in line with v1alpha1 pending further discussion in #1197

@akutz
Copy link
Contributor

akutz commented Jul 25, 2019

I liked the change and was I favor of it, and didn’t think CAPV should be used to support the revert, that’s all.

@texascloud
Copy link

@moshloop I don't understand why your client-side load balancing solution requires this change. It sounds like it's working for you

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants