diff --git a/staging/src/k8s.io/legacy-cloud-providers/azure/azure_loadbalancer.go b/staging/src/k8s.io/legacy-cloud-providers/azure/azure_loadbalancer.go index a2e95d1f744..251f8e5e1ce 100644 --- a/staging/src/k8s.io/legacy-cloud-providers/azure/azure_loadbalancer.go +++ b/staging/src/k8s.io/legacy-cloud-providers/azure/azure_loadbalancer.go @@ -23,6 +23,7 @@ import ( "fmt" "math" "reflect" + "sort" "strconv" "strings" @@ -82,6 +83,9 @@ const ( // ServiceAnnotationPIPName specifies the pip that will be applied to load balancer ServiceAnnotationPIPName = "service.beta.kubernetes.io/azure-pip-name" + // ServiceAnnotationIPTagsForPublicIP specifies the iptags used when dynamically creating a public ip + ServiceAnnotationIPTagsForPublicIP = "service.beta.kubernetes.io/azure-pip-ip-tags" + // ServiceAnnotationAllowedServiceTag is the annotation used on the service // to specify a list of allowed service tags separated by comma // Refer https://docs.microsoft.com/en-us/azure/virtual-network/security-overview#service-tags for all supported service tags. @@ -546,6 +550,7 @@ func (az *Cloud) ensurePublicIPExists(service *v1.Service, pipName string, domai pip.Location = to.StringPtr(az.Location) pip.PublicIPAddressPropertiesFormat = &network.PublicIPAddressPropertiesFormat{ PublicIPAllocationMethod: network.Static, + IPTags: getServiceIPTagRequestForPublicIP(service).IPTags, } pip.Tags = map[string]*string{ serviceTagKey: &serviceName, @@ -604,6 +609,93 @@ func (az *Cloud) ensurePublicIPExists(service *v1.Service, pipName string, domai return &pip, nil } +type serviceIPTagRequest struct { + IPTagsRequestedByAnnotation bool + IPTags *[]network.IPTag +} + +// Get the ip tag Request for the public ip from service annotations. +func getServiceIPTagRequestForPublicIP(service *v1.Service) serviceIPTagRequest { + if service != nil { + if ipTagString, found := service.Annotations[ServiceAnnotationIPTagsForPublicIP]; found { + return serviceIPTagRequest{ + IPTagsRequestedByAnnotation: true, + IPTags: convertIPTagMapToSlice(getIPTagMap(ipTagString)), + } + } + } + + return serviceIPTagRequest{ + IPTagsRequestedByAnnotation: false, + IPTags: nil, + } +} + +func getIPTagMap(ipTagString string) map[string]string { + outputMap := make(map[string]string) + commaDelimitedPairs := strings.Split(strings.TrimSpace(ipTagString), ",") + for _, commaDelimitedPair := range commaDelimitedPairs { + splitKeyValue := strings.Split(commaDelimitedPair, "=") + + // Include only valid pairs in the return value + // Last Write wins. + if len(splitKeyValue) == 2 { + tagKey := strings.TrimSpace(splitKeyValue[0]) + tagValue := strings.TrimSpace(splitKeyValue[1]) + + outputMap[tagKey] = tagValue + } + } + + return outputMap +} + +func sortIPTags(ipTags *[]network.IPTag) { + if ipTags != nil { + sort.Slice(*ipTags, func(i, j int) bool { + ipTag := *ipTags + return to.String(ipTag[i].IPTagType) < to.String(ipTag[j].IPTagType) || + to.String(ipTag[i].Tag) < to.String(ipTag[j].Tag) + }) + } +} + +func areIPTagsEquivalent(ipTags1 *[]network.IPTag, ipTags2 *[]network.IPTag) bool { + sortIPTags(ipTags1) + sortIPTags(ipTags2) + + if ipTags1 == nil { + ipTags1 = &[]network.IPTag{} + } + + if ipTags2 == nil { + ipTags2 = &[]network.IPTag{} + } + + return reflect.DeepEqual(ipTags1, ipTags2) +} + +func convertIPTagMapToSlice(ipTagMap map[string]string) *[]network.IPTag { + if ipTagMap == nil { + return nil + } + + if len(ipTagMap) == 0 { + return &[]network.IPTag{} + } + + outputTags := []network.IPTag{} + for k, v := range ipTagMap { + ipTag := network.IPTag{ + IPTagType: to.StringPtr(k), + Tag: to.StringPtr(v), + } + outputTags = append(outputTags, ipTag) + } + + return &outputTags +} + func getDomainNameLabel(pip *network.PublicIPAddress) string { if pip == nil || pip.PublicIPAddressPropertiesFormat == nil || pip.PublicIPAddressPropertiesFormat.DNSSettings == nil { return "" @@ -1468,10 +1560,34 @@ func deduplicate(collection *[]string) *[]string { return &result } +// Determine if we should release existing owned public IPs +func shouldReleaseExistingOwnedPublicIP(existingPip *network.PublicIPAddress, lbShouldExist bool, lbIsInternal bool, desiredPipName string, ipTagRequest serviceIPTagRequest) bool { + // Latch some variables for readability purposes. + pipName := *(*existingPip).Name + + // Assume the current IP Tags are empty by default unless properties specify otherwise. + currentIPTags := &[]network.IPTag{} + pipPropertiesFormat := (*existingPip).PublicIPAddressPropertiesFormat + if pipPropertiesFormat != nil { + currentIPTags = (*pipPropertiesFormat).IPTags + } + + // Release the ip under the following criteria - + // #1 - If we don't actually want a load balancer, + return !lbShouldExist || + // #2 - If the load balancer is internal, and thus doesn't require public exposure + lbIsInternal || + // #3 - If the name of this public ip does not match the desired name, + (pipName != desiredPipName) || + // #4 If the service annotations have specified the ip tags that the public ip must have, but they do not match the ip tags of the existing instance + (ipTagRequest.IPTagsRequestedByAnnotation && !areIPTagsEquivalent(currentIPTags, ipTagRequest.IPTags)) +} + // This reconciles the PublicIP resources similar to how the LB is reconciled. func (az *Cloud) reconcilePublicIP(clusterName string, service *v1.Service, lbName string, wantLb bool) (*network.PublicIPAddress, error) { isInternal := requiresInternalLoadBalancer(service) serviceName := getServiceName(service) + serviceIPTagRequest := getServiceIPTagRequestForPublicIP(service) var lb *network.LoadBalancer var desiredPipName string var err error @@ -1498,27 +1614,39 @@ func (az *Cloud) reconcilePublicIP(clusterName string, service *v1.Service, lbNa return nil, err } - var found bool + var serviceAnnotationRequestsNamedPublicIP bool = shouldPIPExisted + var discoveredDesiredPublicIP bool + var deletedDesiredPublicIP bool var pipsToBeDeleted []*network.PublicIPAddress for i := range pips { pip := pips[i] pipName := *pip.Name - if serviceOwnsPublicIP(&pip, clusterName, serviceName) { - // We need to process for pips belong to this service - if wantLb && !isInternal && pipName == desiredPipName { - // This is the only case we should preserve the - // Public ip resource with match service tag - found = true - } else { - pipsToBeDeleted = append(pipsToBeDeleted, &pip) - } - } else if wantLb && !isInternal && pipName == desiredPipName { - found = true + + // If we've been told to use a specific public ip by the client, let's track whether or not it actually existed + // when we inspect the set in Azure. + discoveredDesiredPublicIP = discoveredDesiredPublicIP || wantLb && !isInternal && pipName == desiredPipName + + // Now, let's perform additional analysis to determine if we should release the public ips we have found. + // We can only let them go if (a) they are owned by this service and (b) they meet the criteria for deletion. + if serviceOwnsPublicIP(&pip, clusterName, serviceName) && + shouldReleaseExistingOwnedPublicIP(&pip, wantLb, isInternal, desiredPipName, serviceIPTagRequest) { + + // Then, release the public ip + pipsToBeDeleted = append(pipsToBeDeleted, &pip) + + // Flag if we deleted the desired public ip + deletedDesiredPublicIP = deletedDesiredPublicIP || pipName == desiredPipName + + // An aside: It would be unusual, but possible, for us to delete a public ip referred to explicitly by name + // in Service annotations (which is usually reserved for non-service-owned externals), if that IP is tagged as + // having been owned by a particular Kubernetes cluster. } } - if !isInternal && shouldPIPExisted && !found && wantLb { + + if !isInternal && serviceAnnotationRequestsNamedPublicIP && !discoveredDesiredPublicIP && wantLb { return nil, fmt.Errorf("reconcilePublicIP for service(%s): pip(%s) not found", serviceName, desiredPipName) } + var deleteFuncs []func() error for _, pip := range pipsToBeDeleted { pipCopy := *pip @@ -1536,7 +1664,8 @@ func (az *Cloud) reconcilePublicIP(clusterName string, service *v1.Service, lbNa // Confirm desired public ip resource exists var pip *network.PublicIPAddress domainNameLabel, found := getPublicIPDomainNameLabel(service) - if pip, err = az.ensurePublicIPExists(service, desiredPipName, domainNameLabel, clusterName, shouldPIPExisted, found); err != nil { + errorIfPublicIPDoesNotExist := serviceAnnotationRequestsNamedPublicIP && discoveredDesiredPublicIP && !deletedDesiredPublicIP + if pip, err = az.ensurePublicIPExists(service, desiredPipName, domainNameLabel, clusterName, errorIfPublicIPDoesNotExist, found); err != nil { return nil, err } return pip, nil diff --git a/staging/src/k8s.io/legacy-cloud-providers/azure/azure_loadbalancer_test.go b/staging/src/k8s.io/legacy-cloud-providers/azure/azure_loadbalancer_test.go index a465a10e3c5..a5a0223376b 100644 --- a/staging/src/k8s.io/legacy-cloud-providers/azure/azure_loadbalancer_test.go +++ b/staging/src/k8s.io/legacy-cloud-providers/azure/azure_loadbalancer_test.go @@ -23,8 +23,10 @@ import ( "fmt" "net/http" "reflect" + "sort" "strconv" "strings" + "sync" "testing" "time" @@ -515,8 +517,8 @@ func TestServiceOwnsPublicIP(t *testing.T) { } for i, c := range tests { - owns := serviceOwnsPublicIP(c.pip, c.clusterName, c.serviceName) - assert.Equal(t, owns, c.expected, "TestCase[%d]: %s", i, c.desc) + actual := serviceOwnsPublicIP(c.pip, c.clusterName, c.serviceName) + assert.Equal(t, c.expected, actual, "TestCase[%d]: %s", i, c.desc) } } @@ -554,6 +556,478 @@ func TestGetPublicIPAddressResourceGroup(t *testing.T) { } } +func TestShouldReleaseExistingOwnedPublicIP(t *testing.T) { + existingPipWithTag := network.PublicIPAddress{ + ID: to.StringPtr("/subscriptions/subscription/resourceGroups/rg/providers/Microsoft.Network/publicIPAddresses/testPIP"), + Name: to.StringPtr("testPIP"), + Tags: map[string]*string{"service": to.StringPtr("default/test1")}, + PublicIPAddressPropertiesFormat: &network.PublicIPAddressPropertiesFormat{ + PublicIPAddressVersion: network.IPv4, + PublicIPAllocationMethod: network.Static, + IPTags: &[]network.IPTag{ + { + IPTagType: to.StringPtr("tag1"), + Tag: to.StringPtr("tag1value"), + }, + }, + }, + } + + existingPipWithNoPublicIPAddressFormatProperties := network.PublicIPAddress{ + ID: to.StringPtr("/subscriptions/subscription/resourceGroups/rg/providers/Microsoft.Network/publicIPAddresses/testPIP"), + Name: to.StringPtr("testPIP"), + Tags: map[string]*string{"service": to.StringPtr("default/test1")}, + PublicIPAddressPropertiesFormat: nil, + } + + tests := []struct { + desc string + existingPip network.PublicIPAddress + lbShouldExist bool + lbIsInternal bool + desiredPipName string + ipTagRequest serviceIPTagRequest + expectedShouldRelease bool + }{ + { + desc: "Everything matches, no release", + existingPip: existingPipWithTag, + lbShouldExist: true, + lbIsInternal: false, + desiredPipName: *existingPipWithTag.Name, + ipTagRequest: serviceIPTagRequest{ + IPTagsRequestedByAnnotation: true, + IPTags: existingPipWithTag.PublicIPAddressPropertiesFormat.IPTags, + }, + expectedShouldRelease: false, + }, + { + desc: "nil tags (none-specified by annotation, some are present on object), no release", + existingPip: existingPipWithTag, + lbShouldExist: true, + lbIsInternal: false, + desiredPipName: *existingPipWithTag.Name, + ipTagRequest: serviceIPTagRequest{ + IPTagsRequestedByAnnotation: false, + IPTags: nil, + }, + expectedShouldRelease: false, + }, + { + desc: "existing public ip with no format properties (unit test only?), tags required by annotation, no release", + existingPip: existingPipWithNoPublicIPAddressFormatProperties, + lbShouldExist: true, + lbIsInternal: false, + desiredPipName: *existingPipWithTag.Name, + ipTagRequest: serviceIPTagRequest{ + IPTagsRequestedByAnnotation: true, + IPTags: existingPipWithTag.PublicIPAddressPropertiesFormat.IPTags, + }, + expectedShouldRelease: true, + }, + { + desc: "LB no longer desired, expect release", + existingPip: existingPipWithTag, + lbShouldExist: false, + lbIsInternal: false, + desiredPipName: *existingPipWithTag.Name, + ipTagRequest: serviceIPTagRequest{ + IPTagsRequestedByAnnotation: true, + IPTags: existingPipWithTag.PublicIPAddressPropertiesFormat.IPTags, + }, + expectedShouldRelease: true, + }, + { + desc: "LB now internal, expect release", + existingPip: existingPipWithTag, + lbShouldExist: true, + lbIsInternal: true, + desiredPipName: *existingPipWithTag.Name, + ipTagRequest: serviceIPTagRequest{ + IPTagsRequestedByAnnotation: true, + IPTags: existingPipWithTag.PublicIPAddressPropertiesFormat.IPTags, + }, + expectedShouldRelease: true, + }, + { + desc: "Alternate desired name, expect release", + existingPip: existingPipWithTag, + lbShouldExist: true, + lbIsInternal: false, + desiredPipName: "otherName", + ipTagRequest: serviceIPTagRequest{ + IPTagsRequestedByAnnotation: true, + IPTags: existingPipWithTag.PublicIPAddressPropertiesFormat.IPTags, + }, + expectedShouldRelease: true, + }, + { + desc: "mismatching, expect release", + existingPip: existingPipWithTag, + lbShouldExist: true, + lbIsInternal: false, + desiredPipName: *existingPipWithTag.Name, + ipTagRequest: serviceIPTagRequest{ + IPTagsRequestedByAnnotation: true, + IPTags: &[]network.IPTag{ + { + IPTagType: to.StringPtr("tag2"), + Tag: to.StringPtr("tag2value"), + }, + }, + }, + expectedShouldRelease: true, + }, + } + + for i, c := range tests { + + actualShouldRelease := shouldReleaseExistingOwnedPublicIP(&c.existingPip, c.lbShouldExist, c.lbIsInternal, c.desiredPipName, c.ipTagRequest) + assert.Equal(t, c.expectedShouldRelease, actualShouldRelease, "TestCase[%d]: %s", i, c.desc) + } +} + +func TestgetIPTagMap(t *testing.T) { + tests := []struct { + desc string + input string + expected map[string]string + }{ + { + desc: "empty map should be returned when service has blank annotations", + input: "", + expected: map[string]string{}, + }, + { + desc: "a single tag should be returned when service has set one tag pair in the annotation", + input: "tag1=tagvalue1", + expected: map[string]string{ + "tag1": "tagvalue1", + }, + }, + { + desc: "a single tag should be returned when service has set one tag pair in the annotation (and spaces are trimmed)", + input: " tag1 = tagvalue1 ", + expected: map[string]string{ + "tag1": "tagvalue1", + }, + }, + { + desc: "a single tag should be returned when service has set two tag pairs in the annotation with the same key (last write wins - according to appearance order in the string)", + input: "tag1=tagvalue1,tag1=tagvalue1new", + expected: map[string]string{ + "tag1": "tagvalue1new", + }, + }, + { + desc: "two tags should be returned when service has set two tag pairs in the annotation", + input: "tag1=tagvalue1,tag2=tagvalue2", + expected: map[string]string{ + "tag1": "tagvalue1", + "tag2": "tagvalue2", + }, + }, + { + desc: "two tags should be returned when service has set two tag pairs (and one malformation) in the annotation", + input: "tag1=tagvalue1,tag2=tagvalue2,tag3malformed", + expected: map[string]string{ + "tag1": "tagvalue1", + "tag2": "tagvalue2", + }, + }, + { + // We may later decide not to support blank values. The Azure contract is not entirely clear here. + desc: "two tags should be returned when service has set two tag pairs (and one has a blank value) in the annotation", + input: "tag1=tagvalue1,tag2=", + expected: map[string]string{ + "tag1": "tagvalue1", + "tag2": "", + }, + }, + { + // We may later decide not to support blank keys. The Azure contract is not entirely clear here. + desc: "two tags should be returned when service has set two tag pairs (and one has a blank key) in the annotation", + input: "tag1=tagvalue1,=tag2value", + expected: map[string]string{ + "tag1": "tagvalue1", + "": "tag2value", + }, + }, + } + + for i, c := range tests { + actual := getIPTagMap(c.input) + assert.Equal(t, c.expected, actual, "TestCase[%d]: %s", i, c.desc) + } +} + +func TestConvertIPTagMapToSlice(t *testing.T) { + tests := []struct { + desc string + input map[string]string + expected *[]network.IPTag + }{ + { + desc: "nil slice should be returned when the map is nil", + input: nil, + expected: nil, + }, + { + desc: "empty slice should be returned when the map is empty", + input: map[string]string{}, + expected: &[]network.IPTag{}, + }, + { + desc: "one tag should be returned when the map has one tag", + input: map[string]string{ + "tag1": "tag1value", + }, + expected: &[]network.IPTag{ + { + IPTagType: to.StringPtr("tag1"), + Tag: to.StringPtr("tag1value"), + }, + }, + }, + { + desc: "two tags should be returned when the map has two tags", + input: map[string]string{ + "tag1": "tag1value", + "tag2": "tag2value", + }, + expected: &[]network.IPTag{ + { + IPTagType: to.StringPtr("tag1"), + Tag: to.StringPtr("tag1value"), + }, + { + IPTagType: to.StringPtr("tag2"), + Tag: to.StringPtr("tag2value"), + }, + }, + }, + } + + for i, c := range tests { + actual := convertIPTagMapToSlice(c.input) + + // Sort output to provide stability of return from map for test comparison + // The order doesn't matter at runtime. + if actual != nil { + sort.Slice(*actual, func(i, j int) bool { + ipTagSlice := *actual + return to.String(ipTagSlice[i].IPTagType) < to.String(ipTagSlice[j].IPTagType) + }) + } + if c.expected != nil { + sort.Slice(*c.expected, func(i, j int) bool { + ipTagSlice := *c.expected + return to.String(ipTagSlice[i].IPTagType) < to.String(ipTagSlice[j].IPTagType) + }) + + } + + assert.Equal(t, c.expected, actual, "TestCase[%d]: %s", i, c.desc) + } +} + +func TestGetserviceIPTagRequestForPublicIP(t *testing.T) { + tests := []struct { + desc string + input *v1.Service + expected serviceIPTagRequest + }{ + { + desc: "Annotation should be false when service is absent", + input: nil, + expected: serviceIPTagRequest{ + IPTagsRequestedByAnnotation: false, + IPTags: nil, + }, + }, + { + desc: "Annotation should be false when service is present, without annotation", + input: &v1.Service{ + ObjectMeta: metav1.ObjectMeta{ + Annotations: map[string]string{}, + }, + }, + expected: serviceIPTagRequest{ + IPTagsRequestedByAnnotation: false, + IPTags: nil, + }, + }, + { + desc: "Annotation should be true, tags slice empty, when annotation blank", + input: &v1.Service{ + ObjectMeta: metav1.ObjectMeta{ + Annotations: map[string]string{ + ServiceAnnotationIPTagsForPublicIP: "", + }, + }, + }, + expected: serviceIPTagRequest{ + IPTagsRequestedByAnnotation: true, + IPTags: &[]network.IPTag{}, + }, + }, + { + desc: "two tags should be returned when service has set two tag pairs (and one malformation) in the annotation", + input: &v1.Service{ + ObjectMeta: metav1.ObjectMeta{ + Annotations: map[string]string{ + ServiceAnnotationIPTagsForPublicIP: "tag1=tag1value,tag2=tag2value,tag3malformed", + }, + }, + }, + expected: serviceIPTagRequest{ + IPTagsRequestedByAnnotation: true, + IPTags: &[]network.IPTag{ + { + IPTagType: to.StringPtr("tag1"), + Tag: to.StringPtr("tag1value"), + }, + { + IPTagType: to.StringPtr("tag2"), + Tag: to.StringPtr("tag2value"), + }, + }, + }, + }, + } + for i, c := range tests { + actual := getServiceIPTagRequestForPublicIP(c.input) + + // Sort output to provide stability of return from map for test comparison + // The order doesn't matter at runtime. + if actual.IPTags != nil { + sort.Slice(*actual.IPTags, func(i, j int) bool { + ipTagSlice := *actual.IPTags + return to.String(ipTagSlice[i].IPTagType) < to.String(ipTagSlice[j].IPTagType) + }) + } + if c.expected.IPTags != nil { + sort.Slice(*c.expected.IPTags, func(i, j int) bool { + ipTagSlice := *c.expected.IPTags + return to.String(ipTagSlice[i].IPTagType) < to.String(ipTagSlice[j].IPTagType) + }) + + } + + assert.Equal(t, actual, c.expected, "TestCase[%d]: %s", i, c.desc) + } +} + +func TestAreIpTagsEquivalent(t *testing.T) { + tests := []struct { + desc string + input1 *[]network.IPTag + input2 *[]network.IPTag + expected bool + }{ + { + desc: "nils should be considered equal", + input1: nil, + input2: nil, + expected: true, + }, + { + desc: "nils should be considered to empty arrays (case 1)", + input1: nil, + input2: &[]network.IPTag{}, + expected: true, + }, + { + desc: "nils should be considered to empty arrays (case 1)", + input1: &[]network.IPTag{}, + input2: nil, + expected: true, + }, + { + desc: "nil should not be considered equal to anything (case 1)", + input1: &[]network.IPTag{ + { + IPTagType: to.StringPtr("tag1"), + Tag: to.StringPtr("tag1value"), + }, + { + IPTagType: to.StringPtr("tag2"), + Tag: to.StringPtr("tag2value"), + }, + }, + input2: nil, + expected: false, + }, + { + desc: "nil should not be considered equal to anything (case 2)", + input2: &[]network.IPTag{ + { + IPTagType: to.StringPtr("tag1"), + Tag: to.StringPtr("tag1value"), + }, + { + IPTagType: to.StringPtr("tag2"), + Tag: to.StringPtr("tag2value"), + }, + }, + input1: nil, + expected: false, + }, + { + desc: "exactly equal should be treated as equal", + input1: &[]network.IPTag{ + { + IPTagType: to.StringPtr("tag1"), + Tag: to.StringPtr("tag1value"), + }, + { + IPTagType: to.StringPtr("tag2"), + Tag: to.StringPtr("tag2value"), + }, + }, + input2: &[]network.IPTag{ + { + IPTagType: to.StringPtr("tag1"), + Tag: to.StringPtr("tag1value"), + }, + { + IPTagType: to.StringPtr("tag2"), + Tag: to.StringPtr("tag2value"), + }, + }, + expected: true, + }, + { + desc: "equal but out of order should be treated as equal", + input1: &[]network.IPTag{ + { + IPTagType: to.StringPtr("tag1"), + Tag: to.StringPtr("tag1value"), + }, + { + IPTagType: to.StringPtr("tag2"), + Tag: to.StringPtr("tag2value"), + }, + }, + input2: &[]network.IPTag{ + { + IPTagType: to.StringPtr("tag2"), + Tag: to.StringPtr("tag2value"), + }, + { + IPTagType: to.StringPtr("tag1"), + Tag: to.StringPtr("tag1value"), + }, + }, + expected: true, + }, + } + for i, c := range tests { + actual := areIPTagsEquivalent(c.input1, c.input2) + assert.Equal(t, actual, c.expected, "TestCase[%d]: %s", i, c.desc) + } +} + func TestGetServiceTags(t *testing.T) { tests := []struct { desc string @@ -2035,17 +2509,19 @@ func TestReconcilePublicIP(t *testing.T) { defer ctrl.Finish() testCases := []struct { - desc string - wantLb bool - annotations map[string]string - existingPIPs []network.PublicIPAddress - expectedID string - expectedPIP *network.PublicIPAddress - expectedError bool + desc string + wantLb bool + annotations map[string]string + existingPIPs []network.PublicIPAddress + expectedID string + expectedPIP *network.PublicIPAddress + expectedError bool + expectedCreateOrUpdateCount int }{ { - desc: "reconcilePublicIP shall return nil if there's no pip in service", - wantLb: false, + desc: "reconcilePublicIP shall return nil if there's no pip in service", + wantLb: false, + expectedCreateOrUpdateCount: 0, }, { desc: "reconcilePublicIP shall return nil if no pip is owned by service", @@ -2055,6 +2531,7 @@ func TestReconcilePublicIP(t *testing.T) { Name: to.StringPtr("pip1"), }, }, + expectedCreateOrUpdateCount: 0, }, { desc: "reconcilePublicIP shall delete unwanted pips and create a new one", @@ -2067,6 +2544,7 @@ func TestReconcilePublicIP(t *testing.T) { }, expectedID: "/subscriptions/subscription/resourceGroups/rg/providers/" + "Microsoft.Network/publicIPAddresses/testCluster-atest1", + expectedCreateOrUpdateCount: 1, }, { desc: "reconcilePublicIP shall report error if the given PIP name doesn't exist in the resource group", @@ -2082,7 +2560,8 @@ func TestReconcilePublicIP(t *testing.T) { Tags: map[string]*string{"service": to.StringPtr("default/test1")}, }, }, - expectedError: true, + expectedError: true, + expectedCreateOrUpdateCount: 0, }, { desc: "reconcilePublicIP shall delete unwanted PIP when given the name of desired PIP", @@ -2107,6 +2586,85 @@ func TestReconcilePublicIP(t *testing.T) { Name: to.StringPtr("testPIP"), Tags: map[string]*string{"service": to.StringPtr("default/test1")}, }, + expectedCreateOrUpdateCount: 0, + }, + { + desc: "reconcilePublicIP shall delete unwanted pips and existing pips, when the existing pips IP tags do not match", + wantLb: true, + annotations: map[string]string{ + ServiceAnnotationPIPName: "testPIP", + ServiceAnnotationIPTagsForPublicIP: "tag1=tag1value", + }, + existingPIPs: []network.PublicIPAddress{ + { + Name: to.StringPtr("pip1"), + Tags: map[string]*string{"service": to.StringPtr("default/test1")}, + }, + { + Name: to.StringPtr("pip2"), + Tags: map[string]*string{"service": to.StringPtr("default/test1")}, + }, + { + Name: to.StringPtr("testPIP"), + Tags: map[string]*string{"service": to.StringPtr("default/test1")}, + }, + }, + expectedPIP: &network.PublicIPAddress{ + ID: to.StringPtr("/subscriptions/subscription/resourceGroups/rg/providers/Microsoft.Network/publicIPAddresses/testPIP"), + Name: to.StringPtr("testPIP"), + Tags: map[string]*string{"service": to.StringPtr("default/test1")}, + PublicIPAddressPropertiesFormat: &network.PublicIPAddressPropertiesFormat{ + PublicIPAddressVersion: network.IPv4, + PublicIPAllocationMethod: network.Static, + IPTags: &[]network.IPTag{ + { + IPTagType: to.StringPtr("tag1"), + Tag: to.StringPtr("tag1value"), + }, + }, + }, + }, + expectedCreateOrUpdateCount: 1, + }, + { + desc: "reconcilePublicIP shall preserve existing pips, when the existing pips IP tags do match", + wantLb: true, + annotations: map[string]string{ + ServiceAnnotationPIPName: "testPIP", + ServiceAnnotationIPTagsForPublicIP: "tag1=tag1value", + }, + existingPIPs: []network.PublicIPAddress{ + { + Name: to.StringPtr("testPIP"), + Tags: map[string]*string{"service": to.StringPtr("default/test1")}, + PublicIPAddressPropertiesFormat: &network.PublicIPAddressPropertiesFormat{ + PublicIPAddressVersion: network.IPv4, + PublicIPAllocationMethod: network.Static, + IPTags: &[]network.IPTag{ + { + IPTagType: to.StringPtr("tag1"), + Tag: to.StringPtr("tag1value"), + }, + }, + }, + }, + }, + expectedPIP: &network.PublicIPAddress{ + ID: to.StringPtr("/subscriptions/subscription/resourceGroups/rg/providers/Microsoft.Network/publicIPAddresses/testPIP"), + Name: to.StringPtr("testPIP"), + Tags: map[string]*string{"service": to.StringPtr("default/test1")}, + PublicIPAddressPropertiesFormat: &network.PublicIPAddressPropertiesFormat{ + PublicIPAddressVersion: network.IPv4, + PublicIPAllocationMethod: network.Static, + IPTags: &[]network.IPTag{ + { + IPTagType: to.StringPtr("tag1"), + Tag: to.StringPtr("tag1value"), + }, + }, + }, + }, + expectedCreateOrUpdateCount: 0, }, { desc: "reconcilePublicIP shall find the PIP by given name and shall not delete the PIP which is not owned by service", @@ -2128,13 +2686,27 @@ func TestReconcilePublicIP(t *testing.T) { ID: to.StringPtr("/subscriptions/subscription/resourceGroups/rg/providers/Microsoft.Network/publicIPAddresses/testPIP"), Name: to.StringPtr("testPIP"), }, + expectedCreateOrUpdateCount: 0, }, } for i, test := range testCases { + deletedPips := make(map[string]bool) + savedPips := make(map[string]network.PublicIPAddress) + createOrUpdateCount := 0 + var m sync.Mutex az := GetTestCloud(ctrl) mockPIPsClient := az.PublicIPAddressesClient.(*mockpublicipclient.MockInterface) - mockPIPsClient.EXPECT().CreateOrUpdate(gomock.Any(), "rg", gomock.Any(), gomock.Any()).Return(nil).AnyTimes() + creator := mockPIPsClient.EXPECT().CreateOrUpdate(gomock.Any(), "rg", gomock.Any(), gomock.Any()).AnyTimes() + creator.DoAndReturn(func(ctx context.Context, resourceGroupName string, publicIPAddressName string, parameters network.PublicIPAddress) *retry.Error { + m.Lock() + deletedPips[publicIPAddressName] = false + savedPips[publicIPAddressName] = parameters + createOrUpdateCount++ + m.Unlock() + return nil + }) + mockPIPsClient.EXPECT().List(gomock.Any(), "rg").Return(test.existingPIPs, nil).AnyTimes() if i == 2 { mockPIPsClient.EXPECT().Get(gomock.Any(), "rg", "testCluster-atest1", gomock.Any()).Return(network.PublicIPAddress{}, &retry.Error{HTTPStatusCode: http.StatusNotFound}).Times(1) @@ -2143,19 +2715,58 @@ func TestReconcilePublicIP(t *testing.T) { service := getTestService("test1", v1.ProtocolTCP, nil, false, 80) service.Annotations = test.annotations for _, pip := range test.existingPIPs { - mockPIPsClient.EXPECT().Get(gomock.Any(), "rg", *pip.Name, gomock.Any()).Return(pip, nil).AnyTimes() - mockPIPsClient.EXPECT().Delete(gomock.Any(), "rg", *pip.Name).Return(nil).AnyTimes() + savedPips[*pip.Name] = pip + getter := mockPIPsClient.EXPECT().Get(gomock.Any(), "rg", *pip.Name, gomock.Any()).AnyTimes() + getter.DoAndReturn(func(ctx context.Context, resourceGroupName string, publicIPAddressName string, expand string) (result network.PublicIPAddress, rerr *retry.Error) { + m.Lock() + deletedValue, deletedContains := deletedPips[publicIPAddressName] + savedPipValue, savedPipContains := savedPips[publicIPAddressName] + m.Unlock() + + if (!deletedContains || !deletedValue) && savedPipContains { + return savedPipValue, nil + } + + return network.PublicIPAddress{}, &retry.Error{HTTPStatusCode: http.StatusNotFound} + + }) + deleter := mockPIPsClient.EXPECT().Delete(gomock.Any(), "rg", *pip.Name).Return(nil).AnyTimes() + deleter.Do(func(ctx context.Context, resourceGroupName string, publicIPAddressName string) *retry.Error { + m.Lock() + deletedPips[publicIPAddressName] = true + m.Unlock() + return nil + }) + err := az.PublicIPAddressesClient.CreateOrUpdate(context.TODO(), "rg", to.String(pip.Name), pip) if err != nil { t.Fatalf("TestCase[%d] meets unexpected error: %v", i, err) } + + // Clear create or update count to prepare for main execution + createOrUpdateCount = 0 } pip, err := az.reconcilePublicIP("testCluster", &service, "", test.wantLb) + if !test.expectedError { + assert.Equal(t, nil, err, "TestCase[%d]: %s", i, test.desc) + } if test.expectedID != "" { assert.Equal(t, test.expectedID, to.String(pip.ID), "TestCase[%d]: %s", i, test.desc) } else if test.expectedPIP != nil && test.expectedPIP.Name != nil { assert.Equal(t, *test.expectedPIP.Name, *pip.Name, "TestCase[%d]: %s", i, test.desc) + + if test.expectedPIP.PublicIPAddressPropertiesFormat != nil { + sortIPTags(test.expectedPIP.PublicIPAddressPropertiesFormat.IPTags) + } + + if pip.PublicIPAddressPropertiesFormat != nil { + sortIPTags(pip.PublicIPAddressPropertiesFormat.IPTags) + } + + assert.Equal(t, test.expectedPIP.PublicIPAddressPropertiesFormat, pip.PublicIPAddressPropertiesFormat, "TestCase[%d]: %s", i, test.desc) + } + assert.Equal(t, test.expectedCreateOrUpdateCount, createOrUpdateCount, "TestCase[%d]: %s", i, test.desc) assert.Equal(t, test.expectedError, err != nil, "TestCase[%d]: %s", i, test.desc) } } @@ -2169,6 +2780,7 @@ func TestEnsurePublicIPExists(t *testing.T) { existingPIPs []network.PublicIPAddress inputDNSLabel string foundDNSLabelAnnotation bool + additionalAnnotations map[string]string expectedPIP *network.PublicIPAddress expectedID string isIPv6 bool @@ -2287,6 +2899,7 @@ func TestEnsurePublicIPExists(t *testing.T) { for i, test := range testCases { az := GetTestCloud(ctrl) service := getTestService("test1", v1.ProtocolTCP, nil, test.isIPv6, 80) + service.ObjectMeta.Annotations = test.additionalAnnotations mockPIPsClient := az.PublicIPAddressesClient.(*mockpublicipclient.MockInterface) mockPIPsClient.EXPECT().CreateOrUpdate(gomock.Any(), "rg", gomock.Any(), gomock.Any()).Return(nil).AnyTimes() mockPIPsClient.EXPECT().Get(gomock.Any(), "rg", "pip1", gomock.Any()).DoAndReturn(func(ctx context.Context, resourceGroupName string, publicIPAddressName string, expand string) (network.PublicIPAddress, *retry.Error) {