diff --git a/pkg/cloudprovider/providers/azure/azure_loadbalancer.go b/pkg/cloudprovider/providers/azure/azure_loadbalancer.go index a4eee6e7d6e..33ce1770f7f 100644 --- a/pkg/cloudprovider/providers/azure/azure_loadbalancer.go +++ b/pkg/cloudprovider/providers/azure/azure_loadbalancer.go @@ -1205,11 +1205,39 @@ func findRule(rules []network.LoadBalancingRule, rule network.LoadBalancingRule) return false } +// This compares rule's Name, Protocol, SourcePortRange, DestinationPortRange, SourceAddressPrefix, Access, and Direction. +// Note that it compares rule's DestinationAddressPrefix only when it's not consolidated rule as such rule does not have DestinationAddressPrefix defined. +// We intentionally do not compare DestinationAddressPrefixes in consolidated case because reconcileSecurityRule has to consider the two rules equal, +// despite different DestinationAddressPrefixes, in order to give it a chance to consolidate the two rules. func findSecurityRule(rules []network.SecurityRule, rule network.SecurityRule) bool { for _, existingRule := range rules { - if strings.EqualFold(*existingRule.Name, *rule.Name) { - return true + if !strings.EqualFold(*existingRule.Name, *rule.Name) { + continue } + if existingRule.Protocol != rule.Protocol { + continue + } + if !strings.EqualFold(*existingRule.SourcePortRange, *rule.SourcePortRange) { + continue + } + if !strings.EqualFold(*existingRule.DestinationPortRange, *rule.DestinationPortRange) { + continue + } + if !strings.EqualFold(*existingRule.SourceAddressPrefix, *rule.SourceAddressPrefix) { + continue + } + if !allowsConsolidation(existingRule) && !allowsConsolidation(rule) { + if !strings.EqualFold(*existingRule.DestinationAddressPrefix, *rule.DestinationAddressPrefix) { + continue + } + } + if existingRule.Access != rule.Access { + continue + } + if existingRule.Direction != rule.Direction { + continue + } + return true } return false } diff --git a/pkg/cloudprovider/providers/azure/azure_test.go b/pkg/cloudprovider/providers/azure/azure_test.go index f22182ab1b6..073a82e6b36 100644 --- a/pkg/cloudprovider/providers/azure/azure_test.go +++ b/pkg/cloudprovider/providers/azure/azure_test.go @@ -392,6 +392,36 @@ func TestReconcileLoadBalancerAddServiceOnInternalSubnet(t *testing.T) { validateLoadBalancer(t, lb, svc) } +func TestReconcileSecurityGroupFromAnyDestinationAddressPrefixToLoadBalancerIP(t *testing.T) { + az := getTestCloud() + svc1 := getTestService("serviceea", v1.ProtocolTCP, 80) + svc1.Spec.LoadBalancerIP = "192.168.0.0" + sg := getTestSecurityGroup(az) + // Simulate a pre-Kubernetes 1.8 NSG, where we do not specify the destination address prefix + sg, err := az.reconcileSecurityGroup(testClusterName, &svc1, to.StringPtr(""), true) + if err != nil { + t.Errorf("Unexpected error: %q", err) + } + sg, err = az.reconcileSecurityGroup(testClusterName, &svc1, to.StringPtr(svc1.Spec.LoadBalancerIP), true) + if err != nil { + t.Errorf("Unexpected error: %q", err) + } + validateSecurityGroup(t, sg, svc1) +} + +func TestReconcileSecurityGroupDynamicLoadBalancerIP(t *testing.T) { + az := getTestCloud() + svc1 := getTestService("servicea", v1.ProtocolTCP, 80) + svc1.Spec.LoadBalancerIP = "" + sg := getTestSecurityGroup(az) + dynamicallyAssignedIP := "192.168.0.0" + sg, err := az.reconcileSecurityGroup(testClusterName, &svc1, to.StringPtr(dynamicallyAssignedIP), true) + if err != nil { + t.Errorf("unexpected error: %q", err) + } + validateSecurityGroup(t, sg, svc1) +} + // Test addition of services on an internal LB using both default and explicit subnets. func TestReconcileLoadBalancerAddServicesOnMultipleSubnets(t *testing.T) { az := getTestCloud()