Skip to content

Commit

Permalink
Merge pull request #2247 from jwtty/cp_dual_1.1
Browse files Browse the repository at this point in the history 
[release-1.1] fix: NSG destination IP version same as LB IP for floating ip disabled services
  • Loading branch information
@k8s-ci-robot
k8s-ci-robot committed Sep 1, 2022
2 parents 19ee981 + d55d64a commit 6e4a39e
Show file tree
Hide file tree
Showing 3 changed files with 133 additions and 53 deletions.
51 changes: 36 additions & 15 deletions pkg/provider/azure_loadbalancer.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -108,9 +108,8 @@ func (az *Cloud) reconcileService(ctx context.Context, clusterName string, servi
serviceIP = &lbStatus.Ingress[0].IP
}

backendPrivateIPs := az.GetBackendPrivateIPs(clusterName, service, lb)
klog.V(2).Infof("reconcileService: reconciling security group for service %q with IP %q, wantLb = true", serviceName, logSafe(serviceIP))
if _, err := az.reconcileSecurityGroup(clusterName, service, serviceIP, &backendPrivateIPs, true /* wantLb */); err != nil {
if _, err := az.reconcileSecurityGroup(clusterName, service, serviceIP, lb.Name, true /* wantLb */); err != nil {
klog.Errorf("reconcileSecurityGroup(%s) failed: %#v", serviceName, err)
return nil, err
}
Expand Down Expand Up @@ -139,41 +138,47 @@ func (az *Cloud) reconcileService(ctx context.Context, clusterName string, servi
return lbStatus, nil
}

func (az *Cloud) GetBackendPrivateIPs(clusterName string, service *v1.Service, lb *network.LoadBalancer) []string {
func (az *Cloud) GetBackendPrivateIPs(clusterName string, service *v1.Service, lb *network.LoadBalancer) ([]string, []string) {
serviceName := getServiceName(service)
lbBackendPoolName := getBackendPoolName(clusterName, service)
if lb.LoadBalancerPropertiesFormat == nil || lb.LoadBalancerPropertiesFormat.BackendAddressPools == nil {
return nil
return nil, nil
}

backendPrivateIPs := sets.NewString()
backendPrivateIPv4s, backendPrivateIPv6s := sets.NewString(), sets.NewString()
for _, bp := range *lb.BackendAddressPools {
if strings.EqualFold(to.String(bp.Name), lbBackendPoolName) {
klog.V(10).Infof("bc.GetBackendPrivateIPs for service (%s): found wanted backendpool %s", serviceName, to.String(bp.Name))
klog.V(10).Infof("az.GetBackendPrivateIPs for service (%s): found wanted backendpool %s", serviceName, to.String(bp.Name))
if bp.BackendAddressPoolPropertiesFormat != nil && bp.BackendIPConfigurations != nil {
for _, backendIPConfig := range *bp.BackendIPConfigurations {
ipConfigID := to.String(backendIPConfig.ID)
nodeName, _, err := az.VMSet.GetNodeNameByIPConfigurationID(ipConfigID)
if err != nil {
klog.Errorf("bc.GetBackendPrivateIPs for service (%s): GetNodeNameByIPConfigurationID failed with error: %v", serviceName, err)
klog.Errorf("az.GetBackendPrivateIPs for service (%s): GetNodeNameByIPConfigurationID failed with error: %v", serviceName, err)
continue
}
privateIPs, err := az.VMSet.GetPrivateIPsByNodeName(nodeName)
if err != nil {
klog.Errorf("bc.GetBackendPrivateIPs for service (%s): GetPrivateIPsByNodeName(%s) failed with error: %v", serviceName, nodeName, err)
klog.Errorf("az.GetBackendPrivateIPs for service (%s): GetPrivateIPsByNodeName(%s) failed with error: %v", serviceName, nodeName, err)
continue
}
if privateIPs != nil {
klog.V(2).Infof("bc.GetBackendPrivateIPs for service (%s): lb backendpool - found private IPs %v of node %s", serviceName, privateIPs, nodeName)
backendPrivateIPs.Insert(privateIPs...)
klog.V(2).Infof("az.GetBackendPrivateIPs for service (%s): lb backendpool - found private IPs %v of node %s", serviceName, privateIPs, nodeName)
for _, ip := range privateIPs {
if utilnet.IsIPv4String(ip) {
backendPrivateIPv4s.Insert(ip)
} else {
backendPrivateIPv6s.Insert(ip)
}
}
}
}
}
} else {
klog.V(10).Infof("bc.GetBackendPrivateIPs for service (%s): found unmanaged backendpool %s", serviceName, to.String(bp.Name))
klog.V(10).Infof("az.GetBackendPrivateIPs for service (%s): found unmanaged backendpool %s", serviceName, to.String(bp.Name))
}
}
return backendPrivateIPs.List()
return backendPrivateIPv4s.List(), backendPrivateIPv6s.List()
}

// EnsureLoadBalancer creates a new load balancer 'name', or updates the existing one. Returns the status of the balancer
Expand Down Expand Up @@ -253,7 +258,7 @@ func (az *Cloud) EnsureLoadBalancerDeleted(ctx context.Context, clusterName stri
}

klog.V(2).Infof("EnsureLoadBalancerDeleted: reconciling security group for service %q with IP %q, wantLb = false", serviceName, serviceIPToCleanup)
_, err = az.reconcileSecurityGroup(clusterName, service, &serviceIPToCleanup, &[]string{}, false /* wantLb */)
_, err = az.reconcileSecurityGroup(clusterName, service, &serviceIPToCleanup, nil, false /* wantLb */)
if err != nil {
return err
}
Expand Down Expand Up @@ -2522,7 +2527,7 @@ func (az *Cloud) getExpectedHAModeLoadBalancingRuleProperties(

// This reconciles the Network Security Group similar to how the LB is reconciled.
// This entails adding required, missing SecurityRules and removing stale rules.
func (az *Cloud) reconcileSecurityGroup(clusterName string, service *v1.Service, lbIP *string, backendIPAddresses *[]string, wantLb bool) (*network.SecurityGroup, error) {
func (az *Cloud) reconcileSecurityGroup(clusterName string, service *v1.Service, lbIP *string, lbName *string, wantLb bool) (*network.SecurityGroup, error) {
serviceName := getServiceName(service)
klog.V(5).Infof("reconcileSecurityGroup(%s): START clusterName=%q", serviceName, clusterName)

Expand All @@ -2545,6 +2550,22 @@ func (az *Cloud) reconcileSecurityGroup(clusterName string, service *v1.Service,
disableFloatingIP = true
}

backendIPAddresses := make([]string, 0)
if disableFloatingIP {
lb, exist, err := az.getAzureLoadBalancer(to.String(lbName), azcache.CacheReadTypeDefault)
if err != nil {
return nil, err
}
if !exist {
return nil, fmt.Errorf("unable to get lb %s", to.String(lbName))
}
backendPrivateIPv4s, backendPrivateIPv6s := az.GetBackendPrivateIPs(clusterName, service, &lb)
backendIPAddresses = backendPrivateIPv4s
if utilnet.IsIPv6String(*lbIP) {
backendIPAddresses = backendPrivateIPv6s
}
}

destinationIPAddress := ""
if wantLb && lbIP == nil {
return nil, fmt.Errorf("no load balancer IP for setting up security rules for service %s", service.Name)
Expand Down Expand Up @@ -2588,7 +2609,7 @@ func (az *Cloud) reconcileSecurityGroup(clusterName string, service *v1.Service,
sourceAddressPrefixes = append(sourceAddressPrefixes, serviceTags...)
}

expectedSecurityRules, err := az.getExpectedSecurityRules(wantLb, ports, sourceAddressPrefixes, service, destinationIPAddresses, sourceRanges, *backendIPAddresses, disableFloatingIP)
expectedSecurityRules, err := az.getExpectedSecurityRules(wantLb, ports, sourceAddressPrefixes, service, destinationIPAddresses, sourceRanges, backendIPAddresses, disableFloatingIP)
if err != nil {
return nil, err
}
Expand Down
65 changes: 62 additions & 3 deletions pkg/provider/azure_loadbalancer_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3087,6 +3087,7 @@ func TestReconcileSecurityGroup(t *testing.T) {
testCases := []struct {
desc string
lbIP *string
lbName *string
service v1.Service
existingSgs map[string]network.SecurityGroup
expectedSg *network.SecurityGroup
Expand Down Expand Up @@ -3304,6 +3305,7 @@ func TestReconcileSecurityGroup(t *testing.T) {
SecurityGroupPropertiesFormat: &network.SecurityGroupPropertiesFormat{},
}},
lbIP: to.StringPtr("1.2.3.4"),
lbName: to.StringPtr("lb"),
wantLb: true,
expectedSg: &network.SecurityGroup{
Name: to.StringPtr("nsg"),
Expand All @@ -3316,7 +3318,38 @@ func TestReconcileSecurityGroup(t *testing.T) {
SourcePortRange: to.StringPtr("*"),
DestinationPortRange: to.StringPtr(strconv.Itoa(int(getBackendPort(80)))),
SourceAddressPrefix: to.StringPtr("Internet"),
DestinationAddressPrefixes: to.StringSlicePtr([]string{}),
DestinationAddressPrefixes: to.StringSlicePtr([]string{"1.2.3.4", "5.6.7.8"}),
Access: network.SecurityRuleAccess("Allow"),
Priority: to.Int32Ptr(500),
Direction: network.SecurityRuleDirection("Inbound"),
},
},
},
},
},
},
{
desc: "reconcileSecurityGroup shall create sgs with only IPv6 destination addresses for IPv6 services with floating IP disabled",
service: getTestService("test1", v1.ProtocolTCP, map[string]string{consts.ServiceAnnotationDisableLoadBalancerFloatingIP: "true"}, false, 80),
existingSgs: map[string]network.SecurityGroup{"nsg": {
Name: to.StringPtr("nsg"),
SecurityGroupPropertiesFormat: &network.SecurityGroupPropertiesFormat{},
}},
lbIP: to.StringPtr("1234::5"),
lbName: to.StringPtr("lb"),
wantLb: true,
expectedSg: &network.SecurityGroup{
Name: to.StringPtr("nsg"),
SecurityGroupPropertiesFormat: &network.SecurityGroupPropertiesFormat{
SecurityRules: &[]network.SecurityRule{
{
Name: to.StringPtr("atest1-TCP-80-Internet"),
SecurityRulePropertiesFormat: &network.SecurityRulePropertiesFormat{
Protocol: network.SecurityRuleProtocol("Tcp"),
SourcePortRange: to.StringPtr("*"),
DestinationPortRange: to.StringPtr(strconv.Itoa(int(getBackendPort(80)))),
SourceAddressPrefix: to.StringPtr("Internet"),
DestinationAddressPrefixes: to.StringSlicePtr([]string{"fc00::1", "fc00::2"}),
Access: network.SecurityRuleAccess("Allow"),
Priority: to.Int32Ptr(500),
Direction: network.SecurityRuleDirection("Inbound"),
Expand All @@ -3342,7 +3375,33 @@ func TestReconcileSecurityGroup(t *testing.T) {
t.Fatalf("TestCase[%d] meets unexpected error: %v", i, err)
}
}
sg, err := az.reconcileSecurityGroup("testCluster", &test.service, test.lbIP, &[]string{}, test.wantLb)
mockLBClient := az.LoadBalancerClient.(*mockloadbalancerclient.MockInterface)
mockVMSet := NewMockVMSet(ctrl)
az.VMSet = mockVMSet
if test.lbName != nil {
mockLBClient.EXPECT().Get(gomock.Any(), "rg", *test.lbName, gomock.Any()).Return(
network.LoadBalancer{
LoadBalancerPropertiesFormat: &network.LoadBalancerPropertiesFormat{
BackendAddressPools: &[]network.BackendAddressPool{
{
Name: to.StringPtr(getBackendPoolName("testCluster", &test.service)),
BackendAddressPoolPropertiesFormat: &network.BackendAddressPoolPropertiesFormat{
BackendIPConfigurations: &[]network.InterfaceIPConfiguration{
{ID: to.StringPtr("ipConfig1")},
{ID: to.StringPtr("ipConfig2")},
},
},
},
},
},
},
nil)
mockVMSet.EXPECT().GetNodeNameByIPConfigurationID("ipConfig1").Return("Node1", "vmss1", nil)
mockVMSet.EXPECT().GetNodeNameByIPConfigurationID("ipConfig2").Return("Node2", "vmss1", nil)
mockVMSet.EXPECT().GetPrivateIPsByNodeName("Node1").Return([]string{"1.2.3.4", "fc00::1"}, nil)
mockVMSet.EXPECT().GetPrivateIPsByNodeName("Node2").Return([]string{"5.6.7.8", "fc00::2"}, nil)
}
sg, err := az.reconcileSecurityGroup("testCluster", &test.service, test.lbIP, test.lbName, test.wantLb)
assert.Equal(t, test.expectedSg, sg, "TestCase[%d]: %s", i, test.desc)
assert.Equal(t, test.expectedError, err != nil, "TestCase[%d]: %s", i, test.desc)
}
Expand Down Expand Up @@ -3398,7 +3457,7 @@ func TestReconcileSecurityGroupLoadBalancerSourceRanges(t *testing.T) {
mockSGClient := az.SecurityGroupsClient.(*mocksecuritygroupclient.MockInterface)
mockSGClient.EXPECT().Get(gomock.Any(), az.ResourceGroup, gomock.Any(), gomock.Any()).Return(existingSg, nil)
mockSGClient.EXPECT().CreateOrUpdate(gomock.Any(), az.ResourceGroup, gomock.Any(), gomock.Any(), gomock.Any()).Return(nil)
sg, err := az.reconcileSecurityGroup("testCluster", &service, lbIP, &[]string{}, true)
sg, err := az.reconcileSecurityGroup("testCluster", &service, lbIP, nil, true)
assert.NoError(t, err)
assert.Equal(t, expectedSg, *sg)
}
Expand Down

0 comments on commit 6e4a39e

Please sign in to comment.