Allow use resource ID to specify public IP address in azure_loadbalancer #53557

yolocs · 2017-10-07T06:52:29Z

What this PR does / why we need it: Currently the Azure load balancer assumes that a Public IP address is in the same resource group as the cluster. This is not necessarily true in all environments, in addition to accepting a Public IP, we should allow an annotation to the Service object that indicates what resource group the IP is present in.

Which issue this PR fixes: fixes #53274 #52129

Special notes for your reviewer: first time golang user, please forgive the amateurness

Release note

Allow use resource ID to specify public IP address in azure_loadbalancer

dims · 2017-10-09T01:47:46Z

/ok-to-test

itowlson

Welcome! This looks pretty good to me though I have a couple of suggestions and caveats. Main thing is I would suggest that the annotation be a resource group name rather than the resource ID of a Public IP, but I may well be missing something (and Brendan's original issue can be read either way... grin). Open to being talked round though!

itowlson · 2017-10-10T00:15:33Z

pkg/cloudprovider/providers/azure/azure_loadbalancer.go

+// ServiceAnnotationLoadBalancerPublicIPAddressID is the annotation used on the service
+// to specify the resource ID of the public IP address,
+// which can be used to specify public IP address that's not in the same resource group as the cluster
+const ServiceAnnotationLoadBalancerPublicIPAddressID = "service.beta.kubernetes.io/azure-load-public-ip-id"


It seems like this is used only to derive a resource group name: the actual Public IP resource is still located by looking up the loadBalancerIP value in the public IPs in that resource group. The code doesn't require that the Public IP resource in the annotation is the one that matches the loadBalancerIP address. Given that, perhaps we should just have the annotation be a resource group name here instead?

Annotating with just resource group actually makes sense to me. I was carried away by the issue description 'resource id'.

itowlson · 2017-10-10T00:18:23Z

pkg/cloudprovider/providers/azure/azure_loadbalancer.go

 			}
 		}
 	}
 	// TODO: follow next link here? Will there really ever be that many public IPs?

-	return "", fmt.Errorf("user supplied IP Address %s was not found", loadBalancerIP)
+	return "", resourceGroup, fmt.Errorf("user supplied IP Address %s was not found", loadBalancerIP)


I'm not sure of the Go idiom here (I'm pretty new to this too) but I don't think consumers are going to use the resource group in an error return, so would it be more idiomatic to return "" for the resource group here?

itowlson · 2017-10-10T00:29:32Z

pkg/cloudprovider/providers/azure/azure_loadbalancer.go

-func (az *Cloud) ensurePublicIPExists(serviceName, pipName string) (*network.PublicIPAddress, error) {
-	pip, existsPip, err := az.getPublicIPAddress(pipName)
+func (az *Cloud) ensurePublicIPExists(serviceName, pipResourceGroup string, pipName string) (*network.PublicIPAddress, error) {
+	pip, existsPip, err := az.getPublicIPAddress(pipResourceGroup, pipName)


I think you might have a problem here if the Public IP in the annotation doesn't exist. If that happens, won't it attempt to create the Public IP in the cluster resource group? I'm guessing the desired behaviour would be either to fail or to create it in the resource group from the annotation. If the latter should it use the ID from the annotation too? Again the story may be simpler if the annotation specifies only the RG and uses address lookup to locate the PIP resource...

Since I'm going to change the annotation to resource group, here I can try to create the pip in the given resource group. If the creation fails, it will return error anyway.

itowlson · 2017-10-10T00:34:25Z

pkg/cloudprovider/providers/azure/azure_util.go

+
+// parseResourceGroupNameFromID parses the resource group name from a resource ID
+func parseResourceGroupNameFromID(resourceID string) (resourceGroupName string, err error) {
+	reg, err := regexp.Compile(`(?i)(.*?)/resourceGroups/(?P<rgname>\S+)/providers/(.*?)`)


Consider using a regexp.MustCompile at module level (cf. providerIDRE on line 49 of this file).

itowlson · 2017-10-10T02:21:59Z

@yolo3301 I think this also fixes #52129 - if so could you add that to the "fixes" list in the PR description please? Thanks!

yolocs · 2017-10-10T07:12:39Z

@itowlson thanks for the review!

itowlson

I think there is an asymmetry where we can create Public IPs but not delete them. I'm not quite sure what the preferred behaviour is so it would be good to get feedback from other reviewers on this. Otherwise looks good, just minor cosmetic suggestions.

itowlson · 2017-10-13T18:03:18Z

pkg/cloudprovider/providers/azure/azure_backoff.go

@@ -102,14 +102,14 @@ func (az *Cloud) CreateOrUpdateLBWithRetry(lb network.LoadBalancer) error {
 }

 // CreateOrUpdatePIPWithRetry invokes az.PublicIPAddressesClient.CreateOrUpdate with exponential backoff retry
-func (az *Cloud) CreateOrUpdatePIPWithRetry(pip network.PublicIPAddress) error {
+func (az *Cloud) CreateOrUpdatePIPWithRetry(resourceGroup string, pip network.PublicIPAddress) error {


Do we need to handle deletion if we create the PIP? I believe we would delete it if we created it in the cluster RG, so probably we should delete if we create (but leave alone if the user created the PIP). Would be good to get input from other reviewers on the preferred behaviour (I'm new to this too!).

Good point. I added the code to handle deletion. Basically by matching both name and resource group.

itowlson · 2017-10-13T18:07:46Z

pkg/cloudprovider/providers/azure/azure_loadbalancer.go

+	// Override the cluster resource group if public IP resource ID presents
+	resourceGroup := az.ResourceGroup
+	if publicIPAddressResourceGroup(service) != "" {
+		resourceGroup = publicIPAddressResourceGroup(service)


It's harmless to evaluate twice, but also unnecessary. Can you do something like if pipResourceGroup := publicIPAddressResourceGroup(service); pipResourceGroup != "" { resourceGroup = pipResourceGroup }?

Another possibility could be to make publicIPAddressResourceGroup handle the defaulting to az.ResourceGroup itself.

itowlson · 2017-10-13T18:10:50Z

pkg/cloudprovider/providers/azure/azure_loadbalancer.go

 			}
 		}
 	}
 	// TODO: follow next link here? Will there really ever be that many public IPs?

-	return "", fmt.Errorf("user supplied IP Address %s was not found", loadBalancerIP)
+	return "", "", fmt.Errorf("user supplied IP Address %s was not found", loadBalancerIP)


Would it be useful to include the searched RG in the error message e.g. "user supplied IP Address %s was not found in resource group %s"? This might help a user diagnose a "doh, I forgot/mis-spelled the annotation" situation.

itowlson

Many apologies for taking so long to get back to this. A few nitpicks and a couple of things that worry me: first, not being sure when the 'resource group' variable is populated and when not, and second, whether create/delete is going to be safe in the face of edit scenarios. (If we can't easily support edit scenarios then it may be that we can drop create/delete - I think the main use case for this is when the user already has a particular IP they want to use and it's in a different RG, though I could be wrong!)

itowlson · 2017-10-20T16:35:34Z

pkg/cloudprovider/providers/azure/azure_backoff.go

@@ -128,14 +128,14 @@ func (az *Cloud) CreateOrUpdateInterfaceWithRetry(nic network.Interface) error {
 }

 // DeletePublicIPWithRetry invokes az.PublicIPAddressesClient.Delete with exponential backoff retry
-func (az *Cloud) DeletePublicIPWithRetry(pipName string) error {
+func (az *Cloud) DeletePublicIPWithRetry(pipResourceGroup string, pipName string) error {


There's a minor style inconsistency - in CreateOrUpdate this is called resourceGroup, here pipResourceGroup. (I think I slightly prefer the latter but don't feel strongly as long as they're consistent.)

itowlson · 2017-10-20T16:42:04Z

pkg/cloudprovider/providers/azure/azure_loadbalancer.go

 	loadBalancerIP := service.Spec.LoadBalancerIP
 	if len(loadBalancerIP) == 0 {
-		return getPublicIPName(clusterName, service), nil
+		return getPublicIPName(clusterName, service), "", nil


Would it be safer to return az.getPublicIPAddressResourceGroup() here rather than ""? The current design puts the onus on the calling function to fill in the resource group in certain situations - this could lead to more code paths in the calling functions and more opportunities to miss something during testing. (Longer term perhaps we need a better abstraction around this, such as a PublicIPAddressReference, but not sure about that yet!)

I modified it so that I use az.getPublicIPAddressResourceGroup() as much as possible instead of passing the resource group around.

itowlson · 2017-10-20T16:46:55Z

pkg/cloudprovider/providers/azure/azure_loadbalancer.go

@@ -435,18 +442,20 @@ func (az *Cloud) cleanupLoadBalancer(clusterName string, service *v1.Service, is

 		// Public IP can be deleted after frontend ip configuration rule deleted.
 		if publicIPToCleanup != nil {
-			// Only delete an IP address if we created it, deducing by name.
+			// Only delete an IP address if we created it, deducing by name and resource group.


Is the cleanup logic going to work correctly if a user edits a service and changes which resource group its public IP is in?

The existing code cannot handle it too. Mainly because we have no way to know which pip is actually created by k8s. I'm basically restricting the condition to delete public IP by matching name and rg. So in the worse case (the one you described), it didn't cleanup the pip which is not that bad I think. VS. accidentally deleted a pip in the provided rg. Thoughts?

Maybe also add tags to k8s-managed public IPs?

itowlson · 2017-10-20T16:48:46Z

pkg/cloudprovider/providers/azure/azure_loadbalancer.go

-func (az *Cloud) ensurePublicIPExists(serviceName, pipName string) (*network.PublicIPAddress, error) {
-	pip, existsPip, err := az.getPublicIPAddress(pipName)
+func (az *Cloud) ensurePublicIPExists(serviceName string, pipResourceGroup string, pipName string) (*network.PublicIPAddress, error) {
+	pip, existsPip, err := az.getPublicIPAddress(pipResourceGroup, pipName)


With the current design, does this need to handle the case where pipResourceGroup is the empty string, or is there something else that guarantees that will never happen?

Now I changed it accept a service object instead and get the right resource group inside this method.

brendandburns · 2017-11-03T03:39:47Z

pkg/cloudprovider/providers/azure/azure_loadbalancer.go

+// ServiceAnnotationLoadBalancerPublicIPAddressResourceGroup is the annotation used on the service
+// to specify the resource group of the public IP address,
+// which can be used to specify public IP address that's not in the same resource group as the cluster
+const ServiceAnnotationLoadBalancerPublicIPAddressResourceGroup = "service.beta.kubernetes.io/azure-load-balancer-pip-resource-group"


Let's remove the 'pip' part in case we want to re-use this for other load balancer objects...

Removed. Are we assuming other load balancer objects will share the same resource group?

brendandburns · 2017-11-03T03:42:00Z

pkg/cloudprovider/providers/azure/azure_loadbalancer.go

@@ -1005,6 +1018,14 @@ func (az *Cloud) ensureHostInPool(serviceName string, nodeName types.NodeName, b
 	return nil
 }

+func (az *Cloud) getPublicIPAddressResourceGroup(service *v1.Service) string {
+	if resourceGroup, ok := service.Annotations[ServiceAnnotationLoadBalancerPublicIPAddressResourceGroup]; ok {


nit, I'd prefer found vs ok

brendandburns · 2017-11-03T03:42:53Z

This basically LGTM, a few nits... @yolo3301 let's get this merged...

--brendan

yolocs · 2017-11-03T07:44:47Z

/retest

itowlson · 2017-12-03T08:32:33Z

@yolo3301 do you have cycles to rebase this so we can get it merged? If not I can send a PR to your branch I think.

yolocs · 2017-12-03T19:28:58Z

@itowlson Finally got time to update this PR :)

yolocs · 2017-12-17T22:46:35Z

@brendandburns shall we get this merged?

brendandburns · 2017-12-21T18:12:10Z

/lgtm
/approve

k8s-ci-robot · 2017-12-21T18:12:15Z

[APPROVALNOTIFIER] This PR is APPROVED

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

Associated issue: #53274

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

brendandburns · 2017-12-21T18:12:39Z

@yolo3301 sorry for the delay, please ping me on slack/IM/email in the future :)

k8s-github-robot · 2017-12-21T19:23:14Z

/test all [submit-queue is verifying that this PR is safe to merge]

k8s-github-robot · 2017-12-21T20:11:31Z

Automatic merge from submit-queue. If you want to cherry-pick this change to another branch, please follow the instructions here.

tapanhalani · 2018-02-25T10:35:46Z

Is this feature available in 1.8.7 release? If yes, what annotation should I use in my service metadata to make ip's in a different resource group usable by the kubernetes service? Thanks.

feiskyer · 2018-02-26T01:47:03Z

@tapanhalani No, it will be included in v1.10

k8s-github-robot assigned davidopp and lavalamp Oct 7, 2017

k8s-ci-robot removed the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Oct 9, 2017

itowlson suggested changes Oct 10, 2017

View reviewed changes

itowlson mentioned this pull request Oct 10, 2017

Feature: Azure bind existing IP in another resource group to K8 service of type LoadBalancer #52129

Closed

k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Oct 10, 2017

itowlson suggested changes Oct 13, 2017

View reviewed changes

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Oct 15, 2017

itowlson reviewed Oct 20, 2017

View reviewed changes

feiskyer mentioned this pull request Nov 3, 2017

Add support for custom resource groups for public IP addresses. #54952

Closed

brendandburns reviewed Nov 3, 2017

View reviewed changes

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

yolocs force-pushed the az_lb_pipid branch from f2d1b64 to 4d37741 Compare December 3, 2017 19:26

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

PlasmaPower mentioned this pull request Dec 10, 2017

Unable to assign public static IP Azure/AKS#70

Closed

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

yolocs added 6 commits December 17, 2017 11:24

Allow use resource ID to specify public IP address in azure_loadbalancer

540baa9

annotate service with resource group

9054137

delete pip by matching name and rg

03730a3

get rg inside 'ensure' methods

9f26bec

minor fixes

6b8ff76

reapplied the changes after merge

9c83e80

yolocs force-pushed the az_lb_pipid branch from 4d37741 to 9c83e80 Compare December 17, 2017 19:46

kubernetes deleted a comment from k8s-github-robot Dec 17, 2017

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

k8s-ci-robot assigned brendandburns Dec 21, 2017

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

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Dec 21, 2017

k8s-github-robot merged commit 02be3eb into kubernetes:master Dec 21, 2017

Allow use resource ID to specify public IP address in azure_loadbalancer #53557

Allow use resource ID to specify public IP address in azure_loadbalancer #53557

Conversation

yolocs commented Oct 7, 2017 • edited Loading

dims commented Oct 9, 2017

itowlson left a 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

itowlson commented Oct 10, 2017

yolocs commented Oct 10, 2017

itowlson left a 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

itowlson left a 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

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

brendandburns commented Nov 3, 2017

yolocs commented Nov 3, 2017

itowlson commented Dec 3, 2017

yolocs commented Dec 3, 2017

yolocs commented Dec 17, 2017

brendandburns commented Dec 21, 2017

k8s-ci-robot commented Dec 21, 2017

brendandburns commented Dec 21, 2017

k8s-github-robot commented Dec 21, 2017

k8s-github-robot commented Dec 21, 2017

tapanhalani commented Feb 25, 2018

feiskyer commented Feb 26, 2018

yolocs commented Oct 7, 2017 •

edited

Loading