Add support for Azure internal load balancer #43510

karataliu · 2017-03-22T09:55:51Z

Which issue this PR fixes
Fixes #38901

What this PR does / why we need it:
This PR is to add support for Azure internal load balancer

Currently when exposing a serivce with LoadBalancer type, Azure provider would assume that it requires a public load balancer.
Thus it will request a public IP address resource, and expose the service via that public IP.
In this case we're not able to apply private IP addresses (within the cluster virtual network) for the service.

Special notes for your reviewer:

Clarification:
a. 'LoadBalancer' refers to an option for 'type' field under ServiceSpec. See https://kubernetes.io/docs/resources-reference/v1.5/#servicespec-v1
b. 'Azure LoadBalancer' refers a type of Azure resource. See https://docs.microsoft.com/en-us/azure/load-balancer/load-balancer-overview
For a single Azure LoadBalancer, all frontend ip should reference either a subnet or publicIpAddress, which means that it could be either an Internet facing load balancer or an internal one.
For current provider, it would create an Azure LoadBalancer with generated '${loadBalancerName}' for all services with 'LoadBalancer' type.
This PR introduces name '${loadBalancerName}-internal' for a separate Azure Load Balancer resource, used by all the service that requires internal load balancers.
This PR introduces a new annotation for the internal load balancer type behaviour:
a. When the annotaion value is set to 'false' or not set, it falls back to the original behaviour, assuming that user is requesting a public load balancer;
b. When the annotaion value is set to 'true', the following rule applies depending on 'loadBalancerIP' field on ServiceSpec:
- If 'loadBalancerIP' is not set, it will create a load balancer rule with dynamic assigned frontend IP under the cluster subnet;
- If 'loadBalancerIP' is set, it will create a load balancer rule with the frontend IP set to the given value. If the given value is not valid, that is, it does not falls into the cluster subnet range, then the creation will fail.
Users may change the load balancer type by applying the annotation to the service at runtime.
In this case, the load balancer rule would need to be 'switched' between the internal one and external one.
For example, it we have a service with internal load balancer, and then user removes the annotation, making it to a public one. Before we creating rules in the public Azure LoadBalancer, we'll need to clean up rules in the internal Azure LoadBalancer.

Release note:

k8s-ci-robot · 2017-03-22T09:55:58Z

Hi @karataliu. 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 @k8s-bot 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.

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.

k8s-reviewable · 2017-03-22T09:55:58Z

This change is

khenidak · 2017-03-22T19:00:55Z

CC @colemickens @brendandburns

khenidak · 2017-03-22T19:13:27Z

@karataliu Hi! just a QQ ensureLoadBalancerDeleted() does not do anything when delete is happening on ilb? is this expected? wouldn't that leave dangling load balancing rules on the lib?

colemickens · 2017-03-22T19:39:51Z

Few questions, haven't looked at code yet:

Is the value of the annotation a bool, or is the desired private subnet IP? Seems like you describe it as both?
What happens if the user pre-supposes the private ip and it is already taken? Is the failure surfaced well to the user?

colemickens · 2017-03-22T19:50:02Z

There's a number of edge-cases here to consider too. For example, what happens if you create two services (one ILB, one ELB) w/ frontend port = 80 and backend port = 80?

When they're on the same LB object, Azure will allow this if DSR is enabled. If they're on separate LB objects, even with DSR enabled on both, it's very possible that we will reject it. Can you confirm?

karataliu · 2017-03-23T02:45:48Z

@khenidak

Thanks for the review.

For 'ensureLoadBalancerDeleted', please check the new file in the diff panel.
https://github.com/kubernetes/kubernetes/pull/43510/files#diff-c901394068476b4ccb003a6c6efad57cR281

At Line272 it figures out the loadbalancer name (based on internal or not), and then at Line281~Line302 it performs the same update logic taken from original code.

Basically the call to 'reconcileLoadBalancer' will calculate the rule to be updated, and later code will apply the update.

karataliu · 2017-03-23T02:55:50Z

@colemickens

Thanks for the review.

Q: Is the value of the annotation a bool, or is the desired private subnet IP? Seems like you describe it as both?

A: The annotation value is a boolean only. We'll then look up 'loadBalancerIP' for the desired private subnet IP. Please check Note#3.

Q: What happens if the user pre-supposes the private ip and it is already taken? Is the failure surfaced well to the user?

A: This is a similar case to that of proposing a private IP which does not fall into the subnet IP range.
It will trigger following entry in the service's events.

Type:Warning
Reason:CreatingLoadBalancerFailed
Message: ... Private IP address ... from the subnet can be assigned to only one IP configuration. ...

Which comes from

kubernetes/pkg/controller/service/servicecontroller.go

Line 249 in 58ec5cc

    
           s.eventRecorder.Event(service, v1.EventTypeWarning, "CreatingLoadBalancerFailed", message)

The controller will keep retrying. And if later at some time the assigned IP is freed, the service creation will eventually complete successfully.

Q: What happens if you create two services (one ILB, one ELB) w/ frontend port = 80 and backend port = 80?
When they're on the same LB object, Azure will allow this if DSR is enabled. If they're on separate LB objects, even with DSR enabled on both, it's very possible that we will reject it. Can you confirm?

A: Azure will allow ILB and ELB based on identical backend IPConfiguration to reuse the same backend port, as long as 'EnableFloatingIP' (DSR) option is set to true.
In my test I created 2 services with same port configuration, only difference is that one of them has set the internal load balancer annotaion.
Since we've enabled EnableFloatingIP option in code, both services are created successfully. The loadbalancer rules and connectivity for services also appear ok.

jinmiaoplaygrd · 2017-03-24T08:55:01Z

pkg/cloudprovider/providers/azure/azure_loadbalancer.go

-	service.Spec.Ports = []v1.ServicePort{}
+	az.ensureLoadBalancerDeleted(clusterName, service, isInternal)
+
+	sg, existsSg, err := az.getSecurityGroup()


Shouldn't the following be moved to the overloaded func?

We've got two Azure LoadBalancer Resource (internal, external), but only one Network Security Group (bound to vnet).

'ensureLoadBalancerDeleted' is for reconciliation of rules under certain Azure LoadBalancer resource.

If the service requests an external LoadBalancer:
Upon creation, controller calls it (isInternal=true) on internal load balancer resource to ensure the related internal rules deleted (in case the service was switched from internal load balancer type).
Then controller calls 'reconcileSecurityGroup' (wantLb=true) to ensure new rules under Network Security Group.

Upon deletion, controller calls it (isInternal=false) on external load balancer resource to ensure the related external rules deleted.
Then controller calls 'reconcileSecurityGroup' (wantLb=false) to cleanup related rules under Network Security Group.

For internal LoadBalancer, it'll be. creation: 'isInternal=false', 'wantLb=true'. Deletion: 'isInternal=true', 'wantLb=false'.

That is, the parameter value of 'isInternal' on 'ensureLoadBalancerDeleted', is orthogonal to 'wantLb' on 'reconcileSecurityGroup'. In this case, they may be not well suited to combine.

I think we may do some refactor later to wrap the similar code logic around 'reconcileSecurityGroup' calls, however.

How about passing in another flag, say, "cleanupSecurityGroup" which defaults to true? Another option is to pull the IP removal part out the original func also, and rename the overloaded one to something like "deleteLoadBalancerOnly" to avoid confusion.

Looks good, the name 'ensureLoadBalancerDeleted' is somewhat confusing.
Will rename 'ensureLoadBalancerDeleted' to 'deleteLoadBalancerResource', and create a separate func for 'deletePublicIpResource'.

Current PR branch is conflicting with a newly merged change. Will update after rebasing.

Added 'cleanupLoadBalancer' and 'cleanupPublicIP' in new commit.

jinmiaoplaygrd · 2017-03-24T08:55:12Z

pkg/cloudprovider/providers/azure/azure_loadbalancer.go

@@ -105,7 +127,50 @@ func (az *Cloud) EnsureLoadBalancer(clusterName string, service *v1.Service, nod
 		}
 	}

-	lb, lbNeedsUpdate, err := az.reconcileLoadBalancer(lb, pip, clusterName, service, nodes)
+	var lbIP *string


Would it be better to move the added portion to a new func? This one is getting so much longer with this.

Yeah, the function gets a bit long. But we'll also need to pass down the external parameters to the spited func.

I've got a try for the change: karataliu@3848fb4 , and could be evaluated later.

jinmiaoplaygrd · 2017-03-24T09:40:31Z

pkg/cloudprovider/providers/azure/azure_loadbalancer.go


 	sourceRanges, err := serviceapi.GetLoadBalancerSourceRanges(service)
 	if err != nil {
 		return sg, false, err
 	}
 	var sourceAddressPrefixes []string
 	if sourceRanges == nil || serviceapi.IsAllowAll(sourceRanges) {
-		sourceAddressPrefixes = []string{"Internet"}
+		if !requiresInternalLoadBalancer(service) {


So sourceAddressPrefixes is nil for the internal case?

Yes, and it would function like an empty array.
Traffics from vnet is allowed by default rule, thus there is no need to add an explicit one.

gmarek · 2017-04-03T12:46:39Z

/unassign @gmarek
/assign @mikedanese

karataliu · 2017-04-05T05:05:17Z

@colemickens, @khenidak Can you help take a look? Thanks.

brendandburns · 2017-04-06T06:44:21Z

A couple of nits, but this LGTM.

Can you add an e2e test for this?

Thanks!
--brendan

karataliu · 2017-04-07T07:10:26Z

@brendandburns
Thanks for the comments. I've pushed the change for log level update.

As for e2e test, I did a search around and find the following two related test cases:
https://github.com/kubernetes/kubernetes/blob/dea98cb/test/e2e/service.go#L469
https://github.com/kubernetes/kubernetes/blob/dea98cb/test/e2e/service.go#L1168

They are both testing cloud Load Balancer, and against certain cloud providers (gce, gke, aws).

The change in this PR is limited to Azure provider only. But seems for now 'azure' as an E2E cloud provider is not ready. Do we need to add that support first? If so, shall we include it in the same PR? Could you please kindly suggest?

Related: #22715

karataliu · 2017-04-10T10:17:45Z

Regarding the failed test (Jenkins GCE e2e), the test history indicates it passed at first round. The newly added commit only changes log level, which doesn't seem to cause the break. Any idea how to get it rerun, or to deal with such a situation? Thanks.

karataliu · 2017-04-18T07:40:19Z

Update the links on previous comment:
https://github.com/kubernetes/kubernetes/blob/877dc56/test/e2e/service.go#L469
https://github.com/kubernetes/kubernetes/blob/877dc56/test/e2e/service.go#L1168

Adding E2E tests might require some extra work in test-infra project.

Any suggestions on how to proceed with this PR? Thank you.

colemickens · 2017-04-18T21:54:17Z

@brendandburns Do you want to gate this on adding e2e tests, given there's no groundwork for e2e for Azure right now? (Not sure how much that entails)

@karataliu We have the e2e tests running in Azure right now, without doing work in test-infra. Do you have to do work in test-infra just to add a new golang-based e2e test that we can then execute in our Jenkins system on Azure?

karataliu · 2017-04-19T01:46:43Z

@colemickens I just did some search around and found there's cloud testing related code lies in test-infra. It is fine to use other approaches. Thanks.

colemickens · 2017-04-19T01:50:04Z

@karataliu From my time in that repo, I believe that's more about describing the jobs that will run in various Jenkins setups to run the actual e2e tests. But the e2e tests all just live in the main Kubernetes code base. I don't think there's any changes needed, but it might be hard for you to run the tests without having access to a Jenkins box that is already running the tests in/against Azure. If that becomes a road-block, we can share details of the Jenkins jobs that we use so that others can create similar jobs to run the e2e ginkgo tests.

brendandburns · 2017-04-19T03:54:23Z

fwiw, the tests are here in the main repo:

https://github.com/kubernetes/kubernetes/tree/master/test/e2e

I'm ok with merging this as is, and then adding a test as a follow on, but I'd really like to make sure we get some coverage on this code.

/lgtm

brendandburns · 2017-04-19T03:54:42Z

@k8s-bot cvm gce e2e test this

brendandburns · 2017-04-19T03:54:45Z

/approve

k8s-github-robot · 2017-04-19T04:37:04Z

[APPROVALNOTIFIER] This PR is APPROVED

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

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

k8s-github-robot · 2017-04-19T06:22:02Z

Automatic merge from submit-queue (batch tested with PRs 44645, 44639, 43510)

karataliu · 2017-04-19T09:19:46Z

Thanks for the update. I'll keep a note on adding test back for the change. Thank you.

nathanrobb · 2017-04-26T05:32:36Z

pkg/cloudprovider/providers/azure/azure_loadbalancer.go

+	var fipConfigurationProperties *network.FrontendIPConfigurationPropertiesFormat
+
+	if isInternal {
+		subnet, existsSubnet, err := az.getSubnet(az.VnetName, az.SubnetName)


Hey guys, just a quick question regarding this.

Does the subnet need to be in the same resource group (in Azure) as the cluster?
This should not be the case given we can create the kubernetes cluster with an existing VNet in different resource groups.

Getting the following error (the subnet (internal-network/kube-machines) is in a different resource group, it is the same subnet that the agent machines are on):
Error creating load balancer (will retry): Failed to create load balancer for service default/rabbitmq: ensure(default/rabbitmq): lb(XXXXXX-internal) - failed to get subnet: internal-network/kube-machines

The cloudprovider really isn't setup well to handle multi-resource-group things right now unfortunately. There's a larger task to revamp and use resource IDs everywhere instead of resource "names" that are assumed to be under a single RG.

dcieslak19973 · 2017-05-04T13:40:50Z

Will this be added in 1.7?

calebamiles · 2017-05-11T22:12:04Z

@dcieslak19973 this should land in 1.7 and is in the process of being backported to 1.6

…dbalancer Automatic merge from submit-queue Cherry pick azure internal loadbalancer Backports fixes for Azure load balancer: - [Add support for Azure internal load balancer](#43510) - [Add support for bring-your-own ip address for Services on Azure](#42034) **Release note**: ```release-note Add support for Azure internal load balancer. ```

@brendandburns

Automatic merge from submit-queue Add E2E tests for Azure internal loadbalancer support, fix an issue for public IP resource deletion. **What this PR does / why we need it**: - Add E2E tests for Azure internal loadbalancer support: #43510 - Fix an issue that public IP resource not get deleted when switching from external loadbalancer to internal static loadbalancer. **Special notes for your reviewer**: 1. Add new Azure resource tag to Public IP resources to indicate kubernetes managed resources. Currently we determine whether the public IP resource should be deleted by looking at LoadBalancerIp property on spec. In the scenario 'Switching from external loadbalancer to internal loadbalancer with static IP', that value might have been updated for internal loadbalancer. So here we're to add an explicit tag for kubernetes managed resources. 2. Merge cleanupPublicIP logic into cleanupLoadBalancer **Release note**: NONE CC @brendandburns @colemickens

djsly · 2017-11-20T19:16:29Z

It was discovered that the Azure Internal Load Balancer doesn't open up the NSG associated with the Subnet where the Internal LB is created, unlike the Public LB.

#56086

@brendandburns

Automatic merge from submit-queue Add E2E tests for Azure internal loadbalancer support, fix an issue for public IP resource deletion. **What this PR does / why we need it**: - Add E2E tests for Azure internal loadbalancer support: kubernetes/kubernetes#43510 - Fix an issue that public IP resource not get deleted when switching from external loadbalancer to internal static loadbalancer. **Special notes for your reviewer**: 1. Add new Azure resource tag to Public IP resources to indicate kubernetes managed resources. Currently we determine whether the public IP resource should be deleted by looking at LoadBalancerIp property on spec. In the scenario 'Switching from external loadbalancer to internal loadbalancer with static IP', that value might have been updated for internal loadbalancer. So here we're to add an explicit tag for kubernetes managed resources. 2. Merge cleanupPublicIP logic into cleanupLoadBalancer **Release note**: NONE CC @brendandburns @colemickens

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Mar 22, 2017

k8s-github-robot assigned gmarek Mar 22, 2017

k8s-github-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. release-note-label-needed labels Mar 22, 2017

jinmiaoplaygrd reviewed Mar 24, 2017

View reviewed changes

karataliu added 3 commits March 27, 2017 12:39

Add annotation for internal load balancer type in Azure.

7bf15f6

Update EnsureLoadBalancer, EnsureLoadBalancerDeleted for azure.

4f44bf5

Update reconcileSecurityGroup logic for Azure, add tests.

54664d0

k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 27, 2017

karataliu force-pushed the azurelb branch from 31ab593 to 54664d0 Compare March 27, 2017 06:12

k8s-github-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 27, 2017

Add separate func 'cleanupLoadBalancer' and 'cleanupPublicIP' for Azure.

ed36aba

rocketraman mentioned this pull request Mar 28, 2017

Doc update: distinguish between custom VNET *within* 10.0.0.0/8 and otherwise Azure/acs-engine#437

Closed

k8s-ci-robot assigned mikedanese and unassigned gmarek Apr 3, 2017

mikedanese assigned colemickens and brendandburns and unassigned mikedanese Apr 5, 2017

Update message log level for azure_loadbalancer.

f20e9bf

dcieslak19973 mentioned this pull request Apr 17, 2017

azure Feature Request: Services with LoadBalancer should allow us to configure the azure lb with a private IP #38901

Closed

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 19, 2017

k8s-github-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 19, 2017

k8s-github-robot merged commit d2060ad into kubernetes:master Apr 19, 2017

dcieslak19973 mentioned this pull request Apr 21, 2017

Clusters without public IPs Azure/acs-engine#221

Closed

karataliu deleted the azurelb branch April 25, 2017 07:41

nathanrobb reviewed Apr 26, 2017

View reviewed changes

karataliu mentioned this pull request May 8, 2017

Add E2E tests for Azure internal loadbalancer support, fix an issue for public IP resource deletion. #45473

Merged

karataliu mentioned this pull request May 12, 2017

Cherry pick azure internal loadbalancer #45690

Merged

karataliu mentioned this pull request Jun 14, 2017

Update document for Azure LoadBalancer support. kubernetes/website#4082

Merged

Add support for Azure internal load balancer #43510

Add support for Azure internal load balancer #43510

Conversation

karataliu commented Mar 22, 2017

k8s-ci-robot commented Mar 22, 2017

k8s-reviewable commented Mar 22, 2017

khenidak commented Mar 22, 2017

khenidak commented Mar 22, 2017

colemickens commented Mar 22, 2017

colemickens commented Mar 22, 2017

karataliu commented Mar 23, 2017

karataliu commented Mar 23, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gmarek commented Apr 3, 2017

karataliu commented Apr 5, 2017

brendandburns commented Apr 6, 2017

karataliu commented Apr 7, 2017

karataliu commented Apr 10, 2017

karataliu commented Apr 18, 2017

colemickens commented Apr 18, 2017

karataliu commented Apr 19, 2017

colemickens commented Apr 19, 2017

brendandburns commented Apr 19, 2017

brendandburns commented Apr 19, 2017

brendandburns commented Apr 19, 2017

k8s-github-robot commented Apr 19, 2017

k8s-github-robot commented Apr 19, 2017

karataliu commented Apr 19, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dcieslak19973 commented May 4, 2017

calebamiles commented May 11, 2017

djsly commented Nov 20, 2017

karataliu commented Mar 23, 2017 •

edited

Loading