Azure cloud provider: expose services on non-default subnets #51757

itowlson · 2017-09-01T02:34:44Z

What this PR does / why we need it: The Azure cloud provider allows users to specify that a service should be exposed on an internal load balancer instead of the default external load balancer. However, in a VNet environment, such services are currently always exposed on the master subnet. Where there are multiple subnets in the VNet, it's desirable to be able to expose an internal service on any subnet. This PR allows this via a new annotation, service.beta.kubernetes.io/azure-load-balancer-internal-subnet.

Which issue this PR fixes: fixes Azure/acs-engine#1296 (no corresponding issue has been raised in the k8s core repo)

Special notes for your reviewer: None

Release note:

A new service annotation has been added for services of type LoadBalancer on Azure, 
to specify the subnet on which the service's front end IP should be provisioned. The 
annotation is service.beta.kubernetes.io/azure-load-balancer-internal-subnet and its 
value is the subnet name (not the subnet ARM ID).  If omitted, the default is the 
master subnet.  It is ignored if the service is not on Azure, if the type is not 
LoadBalancer, or if the load balancer is not internal.

k8s-ci-robot · 2017-09-01T02:34:51Z

Hi @itowlson. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

itowlson · 2017-09-01T02:35:50Z

/assign jdumars

jdumars · 2017-09-01T14:33:47Z

/ok-to-test

itowlson · 2017-09-02T01:29:32Z

/retest

itowlson · 2017-09-02T04:40:34Z

/retest

itowlson · 2017-09-02T06:07:50Z

/retest

itowlson · 2017-09-02T08:11:44Z

/retest

jdumars · 2017-09-02T13:08:57Z

/test pull-kubernetes-kubemark-e2e-gce

itowlson · 2017-09-03T20:42:07Z

/test pull-kubernetes-kubemark-e2e-gce

karataliu · 2017-09-05T05:14:58Z

pkg/cloudprovider/providers/azure/azure_loadbalancer.go

@@ -366,7 +374,7 @@ func (az *Cloud) cleanupLoadBalancer(clusterName string, service *v1.Service, is

 		if !isInternalLb {
 			// Find public ip resource to clean up from IP configuration
-			lbFrontendIPConfigName := getFrontendIPConfigName(service)
+			lbFrontendIPConfigName := getFrontendIPConfigName(service, subnet(service))


Should use nil for subnet name.

Consider the case user switching from 'external loadbalancer (no subnet)' to 'internal loadbalancer with specified subnet'.
During external loadbalancer creating, it uses name where subnet is empty.

When switching to 'internal loadbalancer', it will first clean corresponding entry on the external loadblancer. But this time 'subnet' value is not empty.

The 'subnet-unset' value is compared with 'subnet-set' value, resulting a unmatch. Thus later the public IP for external loadbalancer will remain undeleted.

karataliu · 2017-09-05T05:26:07Z

pkg/cloudprovider/providers/azure/azure_loadbalancer.go

@@ -523,7 +531,7 @@ func (az *Cloud) reconcileLoadBalancer(lb network.LoadBalancer, fipConfiguration
 	isInternal := requiresInternalLoadBalancer(service)
 	lbName := getLoadBalancerName(clusterName, isInternal)
 	serviceName := getServiceName(service)
-	lbFrontendIPConfigName := getFrontendIPConfigName(service)
+	lbFrontendIPConfigName := getFrontendIPConfigName(service, subnet(service))


Should only make use of 'ServiceAnnotationLoadBalancerInternalSubnet, when isInternal is true?

If user expects external loadbalancer, but also specifies ServiceAnnotationLoadBalancerInternalSubnet, we should ignore the annotation since it does not make sense, or just leave a warning.

Fixed by making subnet check that the Internal annotation is also present.

karataliu · 2017-09-05T05:42:10Z

pkg/cloudprovider/providers/azure/azure_loadbalancer.go

@@ -575,6 +583,14 @@ func (az *Cloud) reconcileLoadBalancer(lb network.LoadBalancer, fipConfiguration
 			}


This is for 'if !wantLb {' section in 'https://github.com/kubernetes/kubernetes/pull/51757/files#diff-c901394068476b4ccb003a6c6efad57cR579', since github doesn't allow comment on unmodified code.

strings.EqualFold(*config.Name, lbFrontendIPConfigName) also needs update to 'serviceOwnsFrontEndIP'.

For a corner case:
Switching from 'internal LB with subnetA' to 'external LB with subnetB', the frontendIP won't get deleted.

Though this does not sound a good user input, but we need to ensure previous created frontendIP entry get deleted.

karataliu · 2017-09-05T05:53:50Z

pkg/cloudprovider/providers/azure/azure_loadbalancer.go

@@ -575,6 +583,14 @@ func (az *Cloud) reconcileLoadBalancer(lb network.LoadBalancer, fipConfiguration
 			}
 		}
 	} else {
+		for i := len(newConfigs) - 1; i >= 0; i-- {


Should only do this for internal LB

karataliu · 2017-09-05T06:01:12Z

pkg/cloudprovider/providers/azure/azure_util.go

 }

-func getSecurityRuleName(service *v1.Service, port v1.ServicePort, sourceAddrPrefix string) string {
+func getSecurityRuleName(service *v1.Service, port v1.ServicePort, subnetName *string, sourceAddrPrefix string) string {


Any reason to update this?

securityRuleName embeds information for service id, port, sourceAddrePrefix, which is part of security group rule. subnetName does not play a role here.

You're right, this isn't needed. I thought it was needed so that old security rules got replaced with new ones during an edit that changed the subnet, but looking at the properties, you're correct that we can just keep the existing rule. Thanks!

karataliu · 2017-09-05T07:54:43Z

pkg/cloudprovider/providers/azure/azure_test.go

+	lb := getTestLoadBalancer()
+	nodes := []*v1.Node{}
+
+	svc.Spec.Ports = append(svc.Spec.Ports, v1.ServicePort{


Why add port here?

karataliu · 2017-09-05T08:03:49Z

pkg/cloudprovider/providers/azure/azure_test.go

@@ -887,3 +985,8 @@ func TestMetadataParsing(t *testing.T) {
 		t.Errorf("Unexpected inequality:\n%#v\nvs\n%#v", network, networkJSON)
 	}
 }
+
+func addTestSubnet(svc *v1.Service) {
+	svc.Annotations[ServiceAnnotationLoadBalancerInternal] = "true"


consider make use of getInternalTestService, and leave this func add subnet only.

karataliu · 2017-09-05T08:04:38Z

pkg/cloudprovider/providers/azure/azure_util.go

@@ -224,8 +230,17 @@ func serviceOwnsRule(service *v1.Service, rule string) bool {
 	return strings.HasPrefix(strings.ToUpper(rule), strings.ToUpper(prefix))
 }

-func getFrontendIPConfigName(service *v1.Service) string {
-	return cloudprovider.GetLoadBalancerName(service)
+func serviceOwnsFrontEndIP(fip network.FrontendIPConfiguration, service *v1.Service) bool {


karataliu · 2017-09-05T08:06:17Z

pkg/cloudprovider/providers/azure/azure_test.go

+	if err != nil {
+		t.Errorf("Unexpected error reconciling initial svc: %q", err)
+	}
+


Also validate FrontendIPConfigurations before editing subnet

karataliu · 2017-09-05T08:08:01Z

pkg/cloudprovider/providers/azure/azure_test.go

+	az := getTestCloud()
+	svc := getTestService("servicea", v1.ProtocolTCP, 80)
+	addTestSubnet(&svc)
+	configProperties := getTestPublicFipConfigurationProperties()


This is logically incorrect, the property is for public IP case, though not validated in reconcileLoadBalancer.

itowlson · 2017-09-07T03:07:21Z

@karataliu Thanks for the detailed review - I appreciate your time. I think I've addressed the issues you've identified; please let me know if there are still outstanding problems.

itowlson · 2017-09-07T04:14:20Z

/retest

karataliu

LGTM

karataliu · 2017-09-07T06:54:29Z

pkg/cloudprovider/providers/azure/azure_test.go

@@ -67,6 +67,101 @@ func TestReconcileLoadBalancerAddPort(t *testing.T) {
 	validateLoadBalancer(t, lb, svc)
 }

+// Test additional of a new service/port on an internal LB with a subnet.


"a new service", since no 'adding port' operation now.

karataliu · 2017-09-07T07:00:29Z

@itowlson Just added a note for one line in test comment. Others look good.

itowlson · 2017-09-08T02:39:45Z

/retest

brendandburns · 2017-09-12T23:24:45Z

/lgtm
/approve no-isssue

k8s-github-robot · 2017-09-12T23:25:06Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: brendandburns, itowlson

No associated issue. Update pull-request body to add a reference to an issue, or get approval with /approve no-issue

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these OWNERS Files:

~~pkg/cloudprovider/providers/azure/OWNERS~~ [brendandburns]

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

fejta-bot · 2017-09-17T13:42:00Z