Skip to content

Commit

Permalink
Merge pull request #5042 from nilo19/fix/probe
Browse files Browse the repository at this point in the history 
fix: shared probe should not be removed if there are other services u…
  • Loading branch information
@k8s-ci-robot
k8s-ci-robot committed Dec 13, 2023
2 parents d5bab0b + 686764f commit f1ce7de
Show file tree
Hide file tree
Showing 5 changed files with 228 additions and 9 deletions.
2 changes: 1 addition & 1 deletion hack/deploy-cluster-capz.sh
View file
Original file line number Diff line number Diff line change
Expand Up @@ -118,7 +118,7 @@ function create_workload_cluster() {
echo "Get kubeconfig and store it locally."
kubectl --context=kind-"${MANAGEMENT_CLUSTER_NAME}" get secrets "${CLUSTER_NAME}"-kubeconfig -o json | jq -r .data.value | base64 --decode > ./"${CLUSTER_NAME}"-kubeconfig
echo "Waiting for the control plane nodes to show up"
timeout --foreground 1000 bash -c "while ! kubectl --kubeconfig=./${CLUSTER_NAME}-kubeconfig get nodes | grep master; do sleep 1; done"
timeout --foreground 1000 bash -c "while ! kubectl --kubeconfig=./${CLUSTER_NAME}-kubeconfig get nodes | grep -E 'master|control-plane'; do sleep 1; done"
if [ "$?" == 124 ]; then
echo "Timeout waiting for the control plane nodes"
return 124
Expand Down
14 changes: 6 additions & 8 deletions pkg/provider/azure.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -279,7 +279,7 @@ type Config struct {

// ClusterServiceLoadBalancerHealthProbeMode determines the health probe mode for cluster service load balancer.
// Supported values are `shared` and `servicenodeport`.
// `unshared`: the health probe will be created against each port of each service by watching the backend application (default).
// `servicenodeport`: the health probe will be created against each port of each service by watching the backend application (default).
// `shared`: all cluster services shares one HTTP probe targeting the kube-proxy on the node (<nodeIP>/healthz:10256).
ClusterServiceLoadBalancerHealthProbeMode string `json:"clusterServiceLoadBalancerHealthProbeMode,omitempty" yaml:"clusterServiceLoadBalancerHealthProbeMode,omitempty"`
// ClusterServiceSharedLoadBalancerHealthProbePort defines the target port of the shared health probe. Default to 10256.
Expand Down Expand Up @@ -603,13 +603,11 @@ func (az *Cloud) InitializeCloudFromConfig(ctx context.Context, config *Config,
return fmt.Errorf("clusterServiceLoadBalancerHealthProbeMode %s is not supported, supported values are %v", config.ClusterServiceLoadBalancerHealthProbeMode, supportedClusterServiceLoadBalancerHealthProbeModes.UnsortedList())
}
}
if strings.EqualFold(config.ClusterServiceLoadBalancerHealthProbeMode, consts.ClusterServiceLoadBalancerHealthProbeModeShared) {
if config.ClusterServiceSharedLoadBalancerHealthProbePort == 0 {
config.ClusterServiceSharedLoadBalancerHealthProbePort = consts.ClusterServiceLoadBalancerHealthProbeDefaultPort
}
if config.ClusterServiceSharedLoadBalancerHealthProbePath == "" {
config.ClusterServiceSharedLoadBalancerHealthProbePath = consts.ClusterServiceLoadBalancerHealthProbeDefaultPath
}
if config.ClusterServiceSharedLoadBalancerHealthProbePort == 0 {
config.ClusterServiceSharedLoadBalancerHealthProbePort = consts.ClusterServiceLoadBalancerHealthProbeDefaultPort
}
if config.ClusterServiceSharedLoadBalancerHealthProbePath == "" {
config.ClusterServiceSharedLoadBalancerHealthProbePath = consts.ClusterServiceLoadBalancerHealthProbeDefaultPath
}

env, err := ratelimitconfig.ParseAzureEnvironment(config.Cloud, config.ResourceManagerEndpoint, config.IdentitySystem)
Expand Down
2 changes: 2 additions & 0 deletions pkg/provider/azure_loadbalancer.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2152,6 +2152,8 @@ func (az *Cloud) reconcileMultipleStandardLoadBalancerConfigurationStatus(wantLb
}

func (az *Cloud) reconcileLBProbes(lb *network.LoadBalancer, service *v1.Service, serviceName string, wantLb bool, expectedProbes []network.Probe) bool {
expectedProbes, _ = az.keepSharedProbe(service, *lb, expectedProbes, wantLb)

// remove unwanted probes
dirtyProbes := false
var updatedProbes []network.Probe
Expand Down
39 changes: 39 additions & 0 deletions pkg/provider/azure_loadbalancer_healthprobe.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -291,3 +291,42 @@ func findProbe(probes []network.Probe, probe network.Probe) bool {
}
return false
}

// keepSharedProbe ensures the shared probe will not be removed if there are more than 1 service referencing it.
func (az *Cloud) keepSharedProbe(
service *v1.Service,
lb network.LoadBalancer,
expectedProbes []network.Probe,
wantLB bool,
) ([]network.Probe, error) {
var shouldConsiderRemoveSharedProbe bool
if !wantLB {
shouldConsiderRemoveSharedProbe = true
}

if lb.LoadBalancerPropertiesFormat != nil && lb.Probes != nil {
for _, probe := range *lb.Probes {
if strings.EqualFold(pointer.StringDeref(probe.Name, ""), consts.SharedProbeName) {
if !az.useSharedLoadBalancerHealthProbeMode() {
shouldConsiderRemoveSharedProbe = true
}
if probe.ProbePropertiesFormat != nil && probe.LoadBalancingRules != nil {
for _, rule := range *probe.LoadBalancingRules {
ruleName, err := getLastSegment(*rule.ID, "/")
if err != nil {
klog.Errorf("failed to parse load balancing rule name %s attached to health probe %s", *rule.ID, *probe.ID)
return []network.Probe{}, err
}
if !az.serviceOwnsRule(service, ruleName) && shouldConsiderRemoveSharedProbe {
klog.V(4).Infof("there are load balancing rule %s of another service referencing the health probe %s, so the health probe should not be removed", *rule.ID, *probe.ID)
sharedProbe := az.buildClusterServiceSharedProbe()
expectedProbes = append(expectedProbes, *sharedProbe)
return expectedProbes, nil
}
}
}
}
}
}
return expectedProbes, nil
}
180 changes: 180 additions & 0 deletions pkg/provider/azure_loadbalancer_healthprobe_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -21,8 +21,14 @@ import (
"strings"
"testing"

metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/types"

"github.com/Azure/azure-sdk-for-go/services/network/mgmt/2022-07-01/network"
"github.com/golang/mock/gomock"
"github.com/stretchr/testify/assert"

v1 "k8s.io/api/core/v1"
"k8s.io/utils/pointer"

"sigs.k8s.io/cloud-provider-azure/pkg/consts"
Expand Down Expand Up @@ -197,3 +203,177 @@ func TestFindProbe(t *testing.T) {
})
}
}

func TestShouldKeepSharedProbe(t *testing.T) {
testCases := []struct {
desc string
service *v1.Service
lb network.LoadBalancer
wantLB bool
expected bool
expectedErr error
}{
{
desc: "When the lb.Probes is nil",
service: &v1.Service{},
lb: network.LoadBalancer{},
expected: false,
},
{
desc: "When the lb.Probes is not nil but does not contain a probe with the name consts.SharedProbeName",
service: &v1.Service{},
lb: network.LoadBalancer{
LoadBalancerPropertiesFormat: &network.LoadBalancerPropertiesFormat{
Probes: &[]network.Probe{
{
Name: pointer.String("notSharedProbe"),
},
},
},
},
expected: false,
},
{
desc: "When the lb.Probes contains a probe with the name consts.SharedProbeName, but none of the LoadBalancingRules in the probe matches the service",
service: &v1.Service{},
lb: network.LoadBalancer{
LoadBalancerPropertiesFormat: &network.LoadBalancerPropertiesFormat{
Probes: &[]network.Probe{
{
Name: pointer.String(consts.SharedProbeName),
ProbePropertiesFormat: &network.ProbePropertiesFormat{
LoadBalancingRules: &[]network.SubResource{},
},
},
},
},
},
expected: false,
},
{
desc: "When the lb.Probes contains a probe with the name consts.SharedProbeName, and at least one of the LoadBalancingRules in the probe does not match the service",
service: &v1.Service{
ObjectMeta: metav1.ObjectMeta{
UID: types.UID("uid"),
},
},
lb: network.LoadBalancer{
LoadBalancerPropertiesFormat: &network.LoadBalancerPropertiesFormat{
Probes: &[]network.Probe{
{
Name: pointer.String(consts.SharedProbeName),
ID: pointer.String("id"),
ProbePropertiesFormat: &network.ProbePropertiesFormat{
LoadBalancingRules: &[]network.SubResource{
{
ID: pointer.String("other"),
},
{
ID: pointer.String("auid"),
},
},
},
},
},
},
},
expected: true,
},
{
desc: "When wantLB is true and the shared probe mode is not turned on",
service: &v1.Service{
ObjectMeta: metav1.ObjectMeta{
UID: types.UID("uid"),
},
},
lb: network.LoadBalancer{
LoadBalancerPropertiesFormat: &network.LoadBalancerPropertiesFormat{
Probes: &[]network.Probe{
{
Name: pointer.String(consts.SharedProbeName),
ID: pointer.String("id"),
ProbePropertiesFormat: &network.ProbePropertiesFormat{
LoadBalancingRules: &[]network.SubResource{
{
ID: pointer.String("other"),
},
{
ID: pointer.String("auid"),
},
},
},
},
},
},
},
wantLB: true,
},
{
desc: "When the lb.Probes contains a probe with the name consts.SharedProbeName, and all of the LoadBalancingRules in the probe match the service",
service: &v1.Service{
ObjectMeta: metav1.ObjectMeta{
UID: types.UID("uid"),
},
},
lb: network.LoadBalancer{
LoadBalancerPropertiesFormat: &network.LoadBalancerPropertiesFormat{
Probes: &[]network.Probe{
{
Name: pointer.String(consts.SharedProbeName),
ID: pointer.String("id"),
ProbePropertiesFormat: &network.ProbePropertiesFormat{
LoadBalancingRules: &[]network.SubResource{
{
ID: pointer.String("auid"),
},
},
},
},
},
},
},
expected: false,
},
{
desc: "Edge cases such as when the service or LoadBalancer is nil",
service: nil,
lb: network.LoadBalancer{},
expected: false,
},
{
desc: "Case: Invalid LoadBalancingRule ID format causing getLastSegment to return an error",
service: &v1.Service{},
lb: network.LoadBalancer{
LoadBalancerPropertiesFormat: &network.LoadBalancerPropertiesFormat{
Probes: &[]network.Probe{
{
Name: pointer.String(consts.SharedProbeName),
ID: pointer.String("id"),
ProbePropertiesFormat: &network.ProbePropertiesFormat{
LoadBalancingRules: &[]network.SubResource{
{
ID: pointer.String(""),
},
},
},
},
},
},
},
expected: false,
expectedErr: fmt.Errorf("resource name was missing from identifier"),
},
}

for _, tc := range testCases {
t.Run(tc.desc, func(t *testing.T) {
az := GetTestCloud(gomock.NewController(t))
var expectedProbes []network.Probe
result, err := az.keepSharedProbe(tc.service, tc.lb, expectedProbes, tc.wantLB)
assert.Equal(t, tc.expectedErr, err)
if tc.expected {
assert.Equal(t, 1, len(result))
}
})
}
}

0 comments on commit f1ce7de

Please sign in to comment.