Allow API server LB frontend port to be configured #1207
Conversation
This change allows API server LB frontend port to be configured separately from the API server listen port. The code uses Cluster CR's ControlPlaneEndpoint.Port as LB frontend port if it is defined. Otherwise the API server listen port is used as the implementation was before.
k8s-ci-robot
added
release-note
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
k8s-ci-robot
added
area/provider/azure
|
Welcome @tuommaki! |
|
Hi @tuommaki. Thanks for your PR. I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
k8s-ci-robot
added
the
needs-ok-to-test
k8s-ci-robot
added
cncf-cla: yes
|
Thanks for the PR @tuommaki! Could you please document how to set both ports in https://github.com/kubernetes-sigs/cluster-api-provider-azure/blob/master/docs/book/src/topics/api-server-endpoint.md ? Also, does this change impact the guidance on allowing inbound ports in vnet security groups https://github.com/kubernetes-sigs/cluster-api-provider-azure/blob/master/docs/book/src/topics/custom-vnet.md ? And does it impact the default ingress rules we generate https://github.com/kubernetes-sigs/cluster-api-provider-azure/blob/master/azure/scope/cluster.go#L507 |
|
Hi Cecile and thanks for the initial feedback!
Sure, I will.
Based on my understanding no, because this change won't affect the CP node listen port.
This one I'm not sure and I'll perform further tests to find out. 👍 |
|
/ok-to-test |
k8s-ci-robot
added
ok-to-test
|
@tuommaki Thanks for the PR! Can you create an issue for this and link it in the pr description for tracking? |
Ok. I did additional testing to verify this and I cannot observe any impacts. The default ingress rule linked earlier matches for apiserver listen port, but this PR doesn't change any behavior related to that. This only affects the LB public facing configuration which affects LB rules but those work both with defaults and with configuration e.g. from 443 -> 6443. I also created the issue and linked it in proposal. |
|
I'll write the missing documentation next. |
| @@ -457,6 +458,15 @@ func (s *ClusterScope) AdditionalTags() infrav1.Tags { | |||
| return tags | |||
| } | |||
|
|
|||
| // APIServerLBFrontendPort returns the frontend listen port to use when creating the load balancer. | |||
| func (s *ClusterScope) APIServerLBFrontendPort() int32 { | |||
| if s.Cluster.Spec.ControlPlaneEndpoint.Port > 0 { | |||
There was a problem hiding this comment.
What is still not 100% clear to me is the relationship between Cluster.Spec.ControlPlaneEndpoint and AzureCluster.Spec.ControlPlaneEndpoint
We set AzureCluster.Spec.ControlPlaneEndpoint in the AzureCluster controller here (to follow the CAPI provider contract): https://github.com/kubernetes-sigs/cluster-api-provider-azure/blob/master/controllers/azurecluster_controller.go#L256-L260
I had opened a discussion in CAPI a while back about moving it to Status since it's basically being used as status right now (and any user value will be overridden) kubernetes-sigs/cluster-api#3715
There was a problem hiding this comment.
in the CAPI cluster_controller, Cluster.Spec.ControlPlaneEndpoint is set to the value of AzureCluster.Spec.ControlPlaneEndpoint https://github.com/kubernetes-sigs/cluster-api/blob/master/controllers/cluster_controller_phases.go#L184
There was a problem hiding this comment.
Hmm... I didn't realize to think from this perspective.
My original use case for this change was to provide migration compatibility when converting existing Kubernetes cluster (Giant Swarm flavor in this case) into one managed by CAPZ. The cluster is already reconciled by v1alpha3 CAPI + CAPZ CRs and there's DNS zone managed for the workload cluster.
I was also viewing the Cluster CR as main spec for provider independent workload cluster configuration since it also has several important network configuration fields (e.g. Service and POD CIDRs as well as API server listen port).
Now I see that my understanding / interpretation on that is slightly off from the present implementation and interoperability between different controllers.
Do you think this change would still make sense in CAPZ?
As far as I was able to reason & test, this change should be fully compatible with present state, but I do understand that there's the corner case that one might not be able to configure just port because the host must be set as well and something needs to manage that. I'm not sure if that can be fixed in any way right now.
There was a problem hiding this comment.
I was also viewing the Cluster CR as main spec for provider independent workload cluster configuration since it also has several important network configuration fields (e.g. Service and POD CIDRs as well as API server listen port).
Right, that's why it's confusing. I think it's a bug that providers don't honor that field, that's why I opened kubernetes-sigs/cluster-api#3715.
I think as long as it has no effect on current behaviors for those who don't set the port & for existing clusters that have the default port, it should be ok to make this change to unblock your use case. However in that case we should remove the user facing documentation since it's not really a configuration option, but more of a workaround. Also let's make sure there are solid comments in the code so we know exactly why this was done and what the expected bahavior is when we look at this code again 6 months from now.
There was a problem hiding this comment.
Sorry for long delays, we have been recently working on different areas so this was not high priority. Because of this controversial nature of this feature and the fact that we don't urgently need this right now, I'll take more time to consider this. I'll get back to this later. Thanks for your patience 🙂
k8s-ci-robot
added
the
needs-rebase
|
@tuommaki: PR needs rebase. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
Going to close the PR for now, feel free to reopen when it's ready for review again /close |
|
@CecileRobertMichon: Closed this PR. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
/kind feature
Description:
This change allows API server LB frontend port to be configured separately from the API server listen port. The code uses
ClusterCR'sSpec.ControlPlaneEndpoint.Portas LB frontend port, given it is defined. Otherwise the API server listen port is used as the implementation was before.This change implements following proposal: #1240
Common use case for this would be:
API LB is listening in default HTTPS port (443) while workload cluster control plane node uses default (6443) for API server listen port.
Special notes for your reviewer:
Depending on situation, this might require user action on existing clusters if control plane endpoint port is configured but earlier LB defaulting has been used.
TODOs:
Release note: