Skip to content

Commit

Permalink
UPSTREAM: <carry>: legacy-cloud-providers: azure: do not detach maste…
Browse files Browse the repository at this point in the history 
…rs from lb when unready

workaround to mitigate issue: kubernetes-sigs/cloud-provider-azure#3500
bug: https://issues.redhat.com/browse/OCPBUGS-7359

UPSTREAM: <carry>: legacy-cloud-providers: azure: use kube-proxy based health probes by default

See
issue: kubernetes-sigs/cloud-provider-azure#3499
bug: https://issues.redhat.com/browse/OCPBUGS-7359
  • Loading branch information
@damdo @bertinatto
damdo authored and bertinatto committed Jun 9, 2023
1 parent bc569ac commit f6a82e5
Show file tree
Hide file tree
Showing 4 changed files with 85 additions and 18 deletions.
13 changes: 12 additions & 1 deletion staging/src/k8s.io/legacy-cloud-providers/azure/azure.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -858,7 +858,7 @@ func (az *Cloud) updateNodeCaches(prevNode, newNode *v1.Node) {
case hasExcludeBalancerLabel:
az.excludeLoadBalancerNodes.Insert(newNode.ObjectMeta.Name)

case !isNodeReady(newNode) && getCloudTaint(newNode.Spec.Taints) == nil:
case !isNodeReady(newNode) && !isNodeMaster(newNode) && getCloudTaint(newNode.Spec.Taints) == nil:
// If not in ready state and not a newly created node, add to excludeLoadBalancerNodes cache.
// New nodes (tainted with "node.cloudprovider.kubernetes.io/uninitialized") should not be
// excluded from load balancers regardless of their state, so as to reduce the number of
Expand Down Expand Up @@ -1005,6 +1005,17 @@ func isNodeReady(node *v1.Node) bool {
return false
}

func isNodeMaster(node *v1.Node) bool {
labels := node.GetLabels()
for l := range labels {
if l == "node-role.kubernetes.io/master" || l == "node-role.kubernetes.io/control-plane" {
return true
}
}

return false
}

func getCloudTaint(taints []v1.Taint) *v1.Taint {
for _, taint := range taints {
if taint.Key == cloudproviderapi.TaintExternalCloudProvider {
Expand Down
26 changes: 21 additions & 5 deletions staging/src/k8s.io/legacy-cloud-providers/azure/azure_loadbalancer.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -123,6 +123,12 @@ const (
serviceUsingDNSKey = "kubernetes-dns-label-service"

defaultLoadBalancerSourceRanges = "0.0.0.0/0"

// NOTE: Please keep the following port in sync with ProxyHealthzPort in k/k pkg/cluster/ports/ports.go
// ports.ProxyHealthzPort was not used here to avoid dependencies to k8s.io/kubernetes in the
// GCE cloud provider which is required as part of the out-of-tree cloud provider efforts.
// TODO: use a shared constant once ports in pkg/cluster/ports are in a common external repo.
lbNodesHealthCheckPort = 10256
)

// GetLoadBalancer returns whether the specified load balancer and its components exist, and
Expand Down Expand Up @@ -1642,7 +1648,7 @@ func (az *Cloud) reconcileLoadBalancerRule(
lbRuleName := az.getLoadBalancerRuleName(service, port.Protocol, port.Port)
klog.V(2).Infof("reconcileLoadBalancerRule lb name (%s) rule name (%s)", lbName, lbRuleName)

transportProto, _, probeProto, err := getProtocolsFromKubernetesProtocol(port.Protocol)
transportProto, _, _, err := getProtocolsFromKubernetesProtocol(port.Protocol)
if err != nil {
return expectedProbes, expectedRules, err
}
Expand Down Expand Up @@ -1670,24 +1676,34 @@ func (az *Cloud) reconcileLoadBalancerRule(
},
})
} else if port.Protocol != v1.ProtocolUDP && port.Protocol != v1.ProtocolSCTP {
// we only add the expected probe if we're doing TCP
var actualPath *string
probePort := port.NodePort

if probeProtocol == "" {
probeProtocol = string(*probeProto)
// If no protocol is specified we want to fall back to HTTP
// as we want to leverage kube-proxy's HTTP health endpoint
// unless otherwise specified.
probeProtocol = string(network.ProbeProtocolHTTP)
}
var actualPath *string

if !strings.EqualFold(probeProtocol, string(network.ProbeProtocolTCP)) {
// In case this is not TCP, instead of using NodePort, we want to probe against
// kube-proxy's HTTP health endpoint which, unless otherwise specified,
// is available at port lbNodesHealthCheckPort and /healthz path.
probePort = lbNodesHealthCheckPort
if requestPath != "" {
actualPath = pointer.String(requestPath)
} else {
actualPath = pointer.String("/healthz")
}
}

expectedProbes = append(expectedProbes, network.Probe{
Name: &lbRuleName,
ProbePropertiesFormat: &network.ProbePropertiesFormat{
Protocol: network.ProbeProtocol(probeProtocol),
RequestPath: actualPath,
Port: pointer.Int32(port.NodePort),
Port: pointer.Int32(probePort),
IntervalInSeconds: pointer.Int32(5),
NumberOfProbes: pointer.Int32(2),
},
Expand Down
62 changes: 51 additions & 11 deletions staging/src/k8s.io/legacy-cloud-providers/azure/azure_loadbalancer_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1675,23 +1675,23 @@ func TestReconcileLoadBalancerRule(t *testing.T) {
service: getTestService("test1", v1.ProtocolTCP, map[string]string{"service.beta.kubernetes.io/azure-load-balancer-disable-tcp-reset": "true"}, false, 80),
loadBalancerSku: "basic",
wantLb: true,
expectedProbes: getDefaultTestProbes("Tcp", ""),
expectedProbes: getDefaultTestProbes("Http", "/healthz", lbNodesHealthCheckPort),
expectedRules: getDefaultTestRules(false),
},
{
desc: "reconcileLoadBalancerRule shall return corresponding probe and lbRule (slb without tcp reset)",
service: getTestService("test1", v1.ProtocolTCP, map[string]string{"service.beta.kubernetes.io/azure-load-balancer-disable-tcp-reset": "True"}, false, 80),
loadBalancerSku: "standard",
wantLb: true,
expectedProbes: getDefaultTestProbes("Tcp", ""),
expectedProbes: getDefaultTestProbes("Http", "/healthz", lbNodesHealthCheckPort),
expectedRules: getDefaultTestRules(true),
},
{
desc: "reconcileLoadBalancerRule shall return corresponding probe and lbRule(slb with tcp reset)",
service: getTestService("test1", v1.ProtocolTCP, nil, false, 80),
loadBalancerSku: "standard",
wantLb: true,
expectedProbes: getDefaultTestProbes("Tcp", ""),
expectedProbes: getDefaultTestProbes("Http", "/healthz", lbNodesHealthCheckPort),
expectedRules: getDefaultTestRules(true),
},
{
Expand All @@ -1701,7 +1701,7 @@ func TestReconcileLoadBalancerRule(t *testing.T) {
wantLb: true,
probeProtocol: "http",
probePath: "/healthy",
expectedProbes: getDefaultTestProbes("http", "/healthy"),
expectedProbes: getDefaultTestProbes("http", "/healthy", lbNodesHealthCheckPort),
expectedRules: getDefaultTestRules(true),
},
{
Expand All @@ -1712,7 +1712,7 @@ func TestReconcileLoadBalancerRule(t *testing.T) {
}, false, 80),
loadBalancerSku: "standard",
wantLb: true,
expectedProbes: getDefaultTestProbes("Tcp", ""),
expectedProbes: getDefaultTestProbes("Http", "/healthz", lbNodesHealthCheckPort),
expectedRules: getHATestRules(true),
},
{
Expand All @@ -1723,7 +1723,7 @@ func TestReconcileLoadBalancerRule(t *testing.T) {
}, false, 80, 8080),
loadBalancerSku: "standard",
wantLb: true,
expectedProbes: getDefaultTestProbes("Tcp", ""),
expectedProbes: getDefaultTestProbes("Http", "/healthz", lbNodesHealthCheckPort),
expectedRules: getHATestRules(true),
},
{
Expand All @@ -1732,7 +1732,7 @@ func TestReconcileLoadBalancerRule(t *testing.T) {
loadBalancerSku: "standard",
wantLb: true,
probeProtocol: "Tcp",
expectedProbes: getDefaultTestProbes("Tcp", ""),
expectedProbes: getDefaultTestProbes("Tcp", "", 10080),
expectedRules: getDefaultTestRules(true),
},
}
Expand All @@ -1759,13 +1759,13 @@ func TestReconcileLoadBalancerRule(t *testing.T) {
}
}

func getDefaultTestProbes(protocol, path string) []network.Probe {
func getDefaultTestProbes(protocol, path string, port int32) []network.Probe {
expectedProbes := []network.Probe{
{
Name: pointer.String("atest1-TCP-80"),
ProbePropertiesFormat: &network.ProbePropertiesFormat{
Protocol: network.ProbeProtocol(protocol),
Port: pointer.Int32(10080),
Port: pointer.Int32(port),
IntervalInSeconds: pointer.Int32(5),
NumberOfProbes: pointer.Int32(2),
},
Expand Down Expand Up @@ -1970,6 +1970,19 @@ func TestReconcileLoadBalancer(t *testing.T) {
},
},
}
expectedLb1.Probes = &[]network.Probe{
{
Name: pointer.String("aservice1-" + string(service3.Spec.Ports[0].Protocol) +
"-" + strconv.Itoa(int(service3.Spec.Ports[0].Port))),
ProbePropertiesFormat: &network.ProbePropertiesFormat{
Port: pointer.Int32(lbNodesHealthCheckPort),
RequestPath: pointer.String("/healthz"),
Protocol: network.ProbeProtocol(network.ProtocolHTTP),
IntervalInSeconds: pointer.Int32(5),
NumberOfProbes: pointer.Int32(2),
},
},
}

service4 := getTestService("service1", v1.ProtocolTCP, map[string]string{"service.beta.kubernetes.io/azure-load-balancer-disable-tcp-reset": "true"}, false, 80)
existingSLB := getTestLoadBalancer(pointer.String("testCluster"), pointer.String("rg"), pointer.String("testCluster"), pointer.String("aservice1"), service4, "Standard")
Expand Down Expand Up @@ -2025,6 +2038,19 @@ func TestReconcileLoadBalancer(t *testing.T) {
},
},
}
expectedSLb.Probes = &[]network.Probe{
{
Name: pointer.String("aservice1-" + string(service3.Spec.Ports[0].Protocol) +
"-" + strconv.Itoa(int(service3.Spec.Ports[0].Port))),
ProbePropertiesFormat: &network.ProbePropertiesFormat{
Port: pointer.Int32(lbNodesHealthCheckPort),
RequestPath: pointer.String("/healthz"),
Protocol: network.ProbeProtocol(network.ProtocolHTTP),
IntervalInSeconds: pointer.Int32(5),
NumberOfProbes: pointer.Int32(2),
},
},
}

service5 := getTestService("service1", v1.ProtocolTCP, nil, false, 80)
slb5 := getTestLoadBalancer(pointer.String("testCluster"), pointer.String("rg"), pointer.String("testCluster"), pointer.String("aservice1"), service5, "Standard")
Expand Down Expand Up @@ -2082,6 +2108,19 @@ func TestReconcileLoadBalancer(t *testing.T) {
},
},
}
expectedSLb5.Probes = &[]network.Probe{
{
Name: pointer.String("aservice1-" + string(service3.Spec.Ports[0].Protocol) +
"-" + strconv.Itoa(int(service3.Spec.Ports[0].Port))),
ProbePropertiesFormat: &network.ProbePropertiesFormat{
Port: pointer.Int32(lbNodesHealthCheckPort),
RequestPath: pointer.String("/healthz"),
Protocol: network.ProbeProtocol(network.ProtocolHTTP),
IntervalInSeconds: pointer.Int32(5),
NumberOfProbes: pointer.Int32(2),
},
},
}

service6 := getTestService("service1", v1.ProtocolUDP, nil, false, 80)
lb6 := getTestLoadBalancer(pointer.String("testCluster"), pointer.String("rg"), pointer.String("testCluster"), pointer.String("aservice1"), service6, "basic")
Expand Down Expand Up @@ -2155,8 +2194,9 @@ func TestReconcileLoadBalancer(t *testing.T) {
Name: pointer.String("aservice1-" + string(service8.Spec.Ports[0].Protocol) +
"-" + strconv.Itoa(int(service7.Spec.Ports[0].Port))),
ProbePropertiesFormat: &network.ProbePropertiesFormat{
Port: pointer.Int32(10080),
Protocol: network.ProbeProtocolTCP,
Port: pointer.Int32(lbNodesHealthCheckPort),
Protocol: network.ProbeProtocolHTTP,
RequestPath: pointer.String("/healthz"),
IntervalInSeconds: pointer.Int32(5),
NumberOfProbes: pointer.Int32(2),
},
Expand Down
2 changes: 1 addition & 1 deletion staging/src/k8s.io/legacy-cloud-providers/azure/azure_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1646,7 +1646,7 @@ func validateLoadBalancer(t *testing.T, loadBalancer *network.LoadBalancer, serv
} else {
for _, actualProbe := range *loadBalancer.Probes {
if strings.EqualFold(*actualProbe.Name, wantedRuleName) &&
*actualProbe.Port == wantedRule.NodePort {
*actualProbe.Port == lbNodesHealthCheckPort {
foundProbe = true
break
}
Expand Down

0 comments on commit f6a82e5

Please sign in to comment.