Skip to content
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.

Sign up for GitHub

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

Allow API server LB frontend port to be configured #1207

Closed
wants to merge 2 commits into from
Closed

Allow API server LB frontend port to be configured #1207

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
db3f193
Allow API server LB frontend port to be configured
Mar 4, 2021
2b7e7ca
Add documentation for LB frontend port configuration
Mar 17, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 10 additions & 0 deletions azure/scope/cluster.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -144,6 +144,7 @@ func (s *ClusterScope) LBSpecs() []azure.LBSpec {
Name: s.APIServerLB().Name,
SubnetName: s.ControlPlaneSubnet().Name,
FrontendIPConfigs: s.APIServerLB().FrontendIPs,
FrontendPort: s.APIServerLBFrontendPort(),
APIServerPort: s.APIServerPort(),
Type: s.APIServerLB().Type,
SKU: infrav1.SKUStandard,
Expand Down Expand Up @@ -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 {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 🙂

return s.Cluster.Spec.ControlPlaneEndpoint.Port
}

return s.APIServerPort()
}

// APIServerPort returns the APIServerPort to use when creating the load balancer.
func (s *ClusterScope) APIServerPort() int32 {
if s.Cluster.Spec.ClusterNetwork != nil && s.Cluster.Spec.ClusterNetwork.APIServerPort != nil {
Expand Down
2 changes: 1 addition & 1 deletion azure/services/loadbalancers/loadbalancers.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -259,7 +259,7 @@ func (s *Service) getLoadBalancingRules(lbSpec azure.LBSpec, frontendIDs []netwo
LoadBalancingRulePropertiesFormat: &network.LoadBalancingRulePropertiesFormat{
DisableOutboundSnat: to.BoolPtr(true),
Protocol: network.TransportProtocolTCP,
FrontendPort: to.Int32Ptr(lbSpec.APIServerPort),
FrontendPort: to.Int32Ptr(lbSpec.FrontendPort),
BackendPort: to.Int32Ptr(lbSpec.APIServerPort),
IdleTimeoutInMinutes: to.Int32Ptr(4),
EnableFloatingIP: to.BoolPtr(false),
Expand Down
10 changes: 8 additions & 2 deletions azure/services/loadbalancers/loadbalancers_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -77,6 +77,7 @@ func TestReconcileLoadBalancer(t *testing.T) {
},
},
},
FrontendPort: 443,
APIServerPort: 6443,
},
})
Expand Down Expand Up @@ -104,6 +105,7 @@ func TestReconcileLoadBalancer(t *testing.T) {
PrivateIPAddress: "10.0.0.10",
},
},
FrontendPort: 443,
APIServerPort: 6443,
},
})
Expand Down Expand Up @@ -152,12 +154,14 @@ func TestReconcileLoadBalancer(t *testing.T) {
{
Name: "my-lb",
SubnetName: "my-subnet",
FrontendPort: 443,
APIServerPort: 6443,
Role: infrav1.APIServerRole,
Type: infrav1.Internal,
},
{
Name: "my-lb-2",
FrontendPort: 443,
APIServerPort: 6443,
Role: infrav1.APIServerRole,
Type: infrav1.Public,
Expand Down Expand Up @@ -202,6 +206,7 @@ func TestReconcileLoadBalancer(t *testing.T) {
},
},
},
FrontendPort: 443,
APIServerPort: 6443,
},
})
Expand Down Expand Up @@ -232,6 +237,7 @@ func TestReconcileLoadBalancer(t *testing.T) {
},
},
},
FrontendPort: 443,
APIServerPort: 6443,
},
})
Expand Down Expand Up @@ -433,7 +439,7 @@ func newDefaultPublicAPIServerLB() network.LoadBalancer {
LoadBalancingRulePropertiesFormat: &network.LoadBalancingRulePropertiesFormat{
DisableOutboundSnat: to.BoolPtr(true),
Protocol: network.TransportProtocolTCP,
FrontendPort: to.Int32Ptr(6443),
FrontendPort: to.Int32Ptr(443),
BackendPort: to.Int32Ptr(6443),
IdleTimeoutInMinutes: to.Int32Ptr(4),
EnableFloatingIP: to.BoolPtr(false),
Expand Down Expand Up @@ -512,7 +518,7 @@ func newDefaultInternalAPIServerLB() network.LoadBalancer {
LoadBalancingRulePropertiesFormat: &network.LoadBalancingRulePropertiesFormat{
DisableOutboundSnat: to.BoolPtr(true),
Protocol: network.TransportProtocolTCP,
FrontendPort: to.Int32Ptr(6443),
FrontendPort: to.Int32Ptr(443),
BackendPort: to.Int32Ptr(6443),
IdleTimeoutInMinutes: to.Int32Ptr(4),
EnableFloatingIP: to.BoolPtr(false),
Expand Down
1 change: 1 addition & 0 deletions azure/types.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,7 @@ type LBSpec struct {
SubnetName string
BackendPoolName string
FrontendIPConfigs []infrav1.FrontendIP
FrontendPort int32
APIServerPort int32
}

Expand Down