Bugfix: Delete unused GCE load-balancer resources on ingress spec change #894

skmatti · 2019-10-16T01:24:00Z

The Ensure workflow of Loadbalancer pool is modified in order to delete unused GCE resources.
This looks at user specified ingress options/annotations to determine if we need to delete specific frontend resources.

AllowHTTP is set to false:
if existing ingress annotations has http resources, then those will be deleted.
SSL is not configured:
if existing ingress annotations has https resources, then those will be deleted.
If User specifies an IP:
We try to delete Ingress managed static IP if exists. This is performed only when both http and https are enabled.

Note that the existing behavior of load-balancer pool does not update frontend resource annotations for the above mentioned cases. This modifies this behavior to update these annotations which will be used for determining whether we need to delete frontend resources.

k8s-ci-robot · 2019-10-16T01:24:08Z

Hi @skmatti. 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.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

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.

freehan · 2019-10-18T23:30:42Z

/ok-to-test

skmatti · 2019-10-21T17:45:11Z

/assign @MrHohn

freehan · 2019-11-12T01:19:17Z

pkg/loadbalancers/l7.go

+	} else {
+		delete(existing, annotations.TargetHttpsProxyKey)
+	}
+	if l.ip != nil {


why not delete it?

The IP is retained for use after it's promoted to static. This will be deleted when load-balancer is deleted or when use specifies a different IP. Added a comment to describe that.

skmatti · 2019-11-12T18:03:07Z

/assign @rramkumar1

rramkumar1

First round of comments.

rramkumar1 · 2019-11-12T19:08:16Z

pkg/annotations/ingress.go

@@ -77,6 +77,25 @@ const (
 	// - annotations:
 	//     networking.gke.io/v1beta1.FrontendConfig: 'my-frontendconfig'
 	FrontendConfigKey = "networking.gke.io/v1beta1.FrontendConfig"
+
+	// UrlMapKey is the annotation key used by controller to record GCP URL map.
+	UrlMapKey = StatusPrefix + "/url-map"


does StatusPrefix need to be exported anymore?

There are couple of other places where this is referenced.

ingress-gce/pkg/loadbalancers/l7.go

Line 378 in ba81191

resourceName, _ = ingAnnotations[fmt.Sprintf("%v/%v", annotations.StatusPrefix, resourceName)]

pkg/loadbalancers/l7.go

rramkumar1 · 2019-11-12T19:17:11Z

pkg/loadbalancers/l7.go

+			annotations.TargetHttpProxyKey,
+		}...)
+	default:
+		klog.Fatalf("Invalid frontend resource protocol %v", protocol)


It would technically be a programming error to ever get to this point so ideally unit tests should catch this but if not, do we want the behavior to be klog.Fatalf? I wonder if we should log this but not do anything since I'm not sure we want the controller to crash because of this?. Worth thinking about alternatives like returning an error.

Makes sense, modified this to surface an error instead.

rramkumar1 · 2019-11-12T19:19:17Z

pkg/loadbalancers/addresses.go

 		return nil
 	}
-	staticIPName := l.namer.ForwardingRule(namer.HTTPProtocol)


What was the motivation for changing the name of this var?

I had to move this line to the top of the loop. At that point, we are still nor sure if this is the static IP name. getEffectiveIP may return a different name for static IP.

rramkumar1 · 2019-11-12T19:20:03Z

pkg/loadbalancers/l7.go

-	if key, err = l.CreateKey(fwsName); err != nil {
+// deleteHttp deletes http forwarding rule and target http proxy.
+func (l *L7) deleteHttp(versions *features.ResourceVersions) error {
+	frName := l.namer.ForwardingRule(namer.HTTPProtocol)


The first couple lines of this func and deleteHttps are the same. Maybe consider wrapping that logic in a func with unit tests?

rramkumar1 · 2019-11-12T19:21:56Z

pkg/loadbalancers/loadbalancers_test.go

+	versions := features.GAResourceVersions
+	certName1 := feNamer.SSLCertName(GetCertHash("cert1"))
+
+	testCases := []struct {


I would add a description to the test case struct itself. That is the typical way it is done.

freehan · 2019-11-12T19:55:58Z

Discussed offline, put it here for reference:

The code added here is quite embedded into the workflow. My main concern was that there is no easy way to flag gate this behavior. I would recommend gathering all the logic and move it into a trim function and call it after edgeHop with flag gating.

rramkumar1 · 2019-11-12T21:28:26Z

Discussed offline, put it here for reference:

The code added here is quite embedded into the workflow. My main concern was that there is no easy way to flag gate this behavior. I would recommend gathering all the logic and move it into a trim function and call it after edgeHop with flag gating.

+1

skmatti · 2019-11-12T21:55:43Z

Discussed offline, put it here for reference:
The code added here is quite embedded into the workflow. My main concern was that there is no easy way to flag gate this behavior. I would recommend gathering all the logic and move it into a trim function and call it after edgeHop with flag gating.

+1

Done

pkg/loadbalancers/l7.go

rramkumar1 · 2019-11-12T23:16:37Z

pkg/loadbalancers/l7.go

-		if certErr != nil {
-			return certErr
+		if err := utils.IgnoreHTTPNotFound(composite.DeleteSslCertificate(l.cloud, key, versions.SslCertificate)); err != nil {
+			klog.Errorf("Old cert delete failed - %v", err)


Is it necessary to log here if the error is returned? Is the error logged higher up in the stack?

This is not logged higher up in the stack. Also, this was just refactored from the existing workflow. May be we want to keep this?

rramkumar1 · 2019-11-13T01:33:47Z

Looks good to me. Will leave final approval to @freehan

freehan

/lgtm
/approve

k8s-ci-robot · 2019-11-13T19:04:35Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: freehan, skmatti

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

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [freehan]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

k8s-ci-robot assigned bowei and freehan Oct 16, 2019

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Oct 16, 2019

k8s-ci-robot requested review from freehan and rramkumar1 October 16, 2019 01:24

k8s-ci-robot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Oct 16, 2019

skmatti force-pushed the resource-leak-fix branch 2 times, most recently from 12d20d1 to e91b460 Compare October 16, 2019 01:28

skmatti changed the title ~~Delete unused GCE load-balancer resources on ingress spec change~~ Bugfix: Delete unused GCE load-balancer resources on ingress spec change Oct 16, 2019

skmatti force-pushed the resource-leak-fix branch 4 times, most recently from 57438f3 to 44ce7f8 Compare October 18, 2019 20:53

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Oct 18, 2019

skmatti force-pushed the resource-leak-fix branch from 44ce7f8 to 00e45af Compare October 21, 2019 17:05

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Oct 21, 2019

skmatti force-pushed the resource-leak-fix branch from 00e45af to 089c41f Compare October 21, 2019 17:33

skmatti mentioned this pull request Oct 21, 2019

Add e2e tests for frontend resource leak fix #905

Merged

k8s-ci-robot assigned MrHohn Oct 21, 2019

skmatti force-pushed the resource-leak-fix branch 2 times, most recently from 604ffda to 39471f2 Compare November 4, 2019 18:23

freehan reviewed Nov 12, 2019

View reviewed changes

k8s-ci-robot assigned rramkumar1 Nov 12, 2019

skmatti force-pushed the resource-leak-fix branch 2 times, most recently from 18f6887 to 29c25b4 Compare November 12, 2019 18:15

rramkumar1 reviewed Nov 12, 2019

View reviewed changes

skmatti force-pushed the resource-leak-fix branch from 29c25b4 to 64dfe18 Compare November 12, 2019 20:00

skmatti force-pushed the resource-leak-fix branch from 64dfe18 to 29ac11b Compare November 12, 2019 21:53

rramkumar1 reviewed Nov 12, 2019

View reviewed changes

Delete unused GCE loadbalancer resources on updating ingress spec

3d188a0

skmatti force-pushed the resource-leak-fix branch from 29ac11b to 3d188a0 Compare November 12, 2019 23:38

freehan reviewed Nov 13, 2019

View reviewed changes

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 13, 2019

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 13, 2019

k8s-ci-robot merged commit 4741042 into kubernetes:master Nov 13, 2019

skmatti deleted the resource-leak-fix branch November 19, 2019 19:51

brianpursley mentioned this pull request Feb 14, 2020

"get ingress" still shows port 80 even though it is HTTPS only. kubernetes/kubectl#500

Closed

skmatti mentioned this pull request Apr 29, 2020

Cannot Disable HTTP when using managed-certificates. HTTPS + HTTP are both provisioned #764

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bugfix: Delete unused GCE load-balancer resources on ingress spec change #894

Bugfix: Delete unused GCE load-balancer resources on ingress spec change #894

skmatti commented Oct 16, 2019 •

edited

Loading

k8s-ci-robot commented Oct 16, 2019

freehan commented Oct 18, 2019

skmatti commented Oct 21, 2019

freehan Nov 12, 2019

skmatti Nov 12, 2019

skmatti commented Nov 12, 2019

rramkumar1 left a comment

rramkumar1 Nov 12, 2019

skmatti Nov 12, 2019

rramkumar1 Nov 12, 2019

skmatti Nov 12, 2019

rramkumar1 Nov 12, 2019

skmatti Nov 12, 2019

rramkumar1 Nov 12, 2019

skmatti Nov 12, 2019

rramkumar1 Nov 12, 2019

skmatti Nov 12, 2019

freehan commented Nov 12, 2019

rramkumar1 commented Nov 12, 2019

skmatti commented Nov 12, 2019

rramkumar1 Nov 12, 2019

skmatti Nov 12, 2019

rramkumar1 commented Nov 13, 2019

freehan left a comment

k8s-ci-robot commented Nov 13, 2019

Bugfix: Delete unused GCE load-balancer resources on ingress spec change #894

Bugfix: Delete unused GCE load-balancer resources on ingress spec change #894

Conversation

skmatti commented Oct 16, 2019 • edited Loading

k8s-ci-robot commented Oct 16, 2019

freehan commented Oct 18, 2019

skmatti commented Oct 21, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

skmatti commented Nov 12, 2019

rramkumar1 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

freehan commented Nov 12, 2019

rramkumar1 commented Nov 12, 2019

skmatti commented Nov 12, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rramkumar1 commented Nov 13, 2019

freehan left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Nov 13, 2019

skmatti commented Oct 16, 2019 •

edited

Loading