Rename ILB FirewallRules to be consistent with other resource names. #84622

prameshj · 2019-10-31T23:14:38Z

What type of PR is this?

Uncomment only one /kind <> line, hit enter to put that in a new line, and remove leading whitespace from that line:

/kind api-change
/kind bug
/kind cleanup
/kind design
/kind documentation
/kind failing-test
/kind feature
/kind flake

What this PR does / why we need it:
This PR changes ILB firewall names to contain the "k8s-fw-" prefix like the rest of the firewall rules. This is needed for consistency and also for other components to identify the firewall rule as k8s/service-controller managed.
Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

Change GCP ILB firewall names to contain the "k8s-fw-" prefix like the rest of the firewall rules. This is needed for consistency and also for other components to identify the firewall rule as k8s/service-controller managed.

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

prameshj · 2019-10-31T23:14:57Z

/assign @MrHohn

prameshj · 2019-10-31T23:15:23Z

cc @grayluck @bowei @freehan

staging/src/k8s.io/legacy-cloud-providers/gce/gce_loadbalancer_internal.go

MrHohn

LGTM overall and left some comments.

staging/src/k8s.io/legacy-cloud-providers/gce/gce_loadbalancer_internal.go

MrHohn · 2019-10-31T23:36:21Z

staging/src/k8s.io/legacy-cloud-providers/gce/gce_loadbalancer_internal.go

@@ -248,7 +248,17 @@ func (g *Cloud) ensureInternalLoadBalancerDeleted(clusterName, clusterID string,
 		return err
 	}

-	klog.V(2).Infof("ensureInternalLoadBalancerDeleted(%v): deleting firewall for traffic", loadBalancerName)
+	fwName := MakeFirewallName(loadBalancerName)


I wonder would it matter if we share firewall name between external/internal LB. Since we only check for existence on the rule during ensurance, would there be a chance that the firewall for the external one isn't deleted and we mistakenly think that is for the internal one?

I was wondering the same.. even if we share the same prefix, we wouldn't share the same rule, since the loadbalancerName is a unique id? If that collided, it is an issue for all the other resources - backend service, forwarding rule etc?

I was under the impression that the loadbalancerName would be the same if it is for the same service:
https://github.com/kubernetes/kubernetes/blob/d1d663096ba9180bb295b40b6b20f8ceb0e11ddc/staging/src/k8s.io/cloud-provider/cloud.go#L82-L91

But good point that this is already an issue for forwarding rule (not for backend service because ELB uses target pool). I just realize we in fact check whether firewall rule is equal, so this should be fine.

correct. the name will be the same if it is for the same service. But we can only have either ILB or externalLB at a time for a service, so this should be ok. to your second point, that's correct - externalLB uses target pool.

staging/src/k8s.io/legacy-cloud-providers/gce/gce_loadbalancer_internal.go

prameshj · 2019-11-01T21:39:40Z

/assign @freehan @MrHohn

MrHohn

LGTM but will let Minhan take a look as well :)
/approve

MrHohn · 2019-11-01T22:11:26Z

staging/src/k8s.io/legacy-cloud-providers/gce/gce_loadbalancer_internal.go

@@ -248,7 +248,17 @@ func (g *Cloud) ensureInternalLoadBalancerDeleted(clusterName, clusterID string,
 		return err
 	}

-	klog.V(2).Infof("ensureInternalLoadBalancerDeleted(%v): deleting firewall for traffic", loadBalancerName)
+	fwName := MakeFirewallName(loadBalancerName)


I was under the impression that the loadbalancerName would be the same if it is for the same service:
https://github.com/kubernetes/kubernetes/blob/d1d663096ba9180bb295b40b6b20f8ceb0e11ddc/staging/src/k8s.io/cloud-provider/cloud.go#L82-L91

But good point that this is already an issue for forwarding rule (not for backend service because ELB uses target pool). I just realize we in fact check whether firewall rule is equal, so this should be fine.

k8s-ci-robot · 2019-11-01T22:17:41Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: MrHohn, prameshj

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:

~~staging/src/k8s.io/legacy-cloud-providers/gce/OWNERS~~ [MrHohn]

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

bowei · 2019-11-05T21:12:12Z

staging/src/k8s.io/legacy-cloud-providers/gce/gce_loadbalancer_internal.go

@@ -328,6 +338,29 @@ func (g *Cloud) ensureInternalFirewall(svc *v1.Service, fwName, fwDesc string, s
 	if err != nil && !isNotFound(err) {
 		return err
 	}
+	// TODO Remove legacyFwName logic after 3 releases, so there would have been atleast 2 master upgrades that would


TODO needs issue number for tracking

TODO(12345): xxxxxxxxxx

prameshj · 2019-11-06T06:38:27Z

/kind bug
/priority-important-soon

freehan

LGTM

just one nit.

freehan · 2019-11-06T23:45:11Z

staging/src/k8s.io/legacy-cloud-providers/gce/gce_loadbalancer_internal.go

@@ -248,7 +248,17 @@ func (g *Cloud) ensureInternalLoadBalancerDeleted(clusterName, clusterID string,
 		return err
 	}

-	klog.V(2).Infof("ensureInternalLoadBalancerDeleted(%v): deleting firewall for traffic", loadBalancerName)
+	fwName := MakeFirewallName(loadBalancerName)
+	klog.V(2).Infof("ensureInternalLoadBalancerDeleted(%v): deleting firewall %s for traffic", fwName, loadBalancerName)


Just nit:
consider this?

deleteFunc := func(fwName string) { if err := ignoreNotFound(g.DeleteFirewall(fwName)); err != nil { if isForbidden(err) && g.OnXPN() { klog.V(2).Infof("ensureInternalLoadBalancerDeleted(%v): could not delete traffic firewall on XPN cluster. Raising event.", fwName) g.raiseFirewallChangeNeededEvent(svc, FirewallToGCloudDeleteCmd(loadBalancerName, g.NetworkProjectID())) } else { return err } } } klog.V(2).Infof("ensureInternalLoadBalancerDeleted(%v): deleting firewall %s for traffic", fwName, loadBalancerName) deleteFunc(fwName) klog.V(2).Infof("ensureInternalLoadBalancerDeleted(%v): deleting legacy name firewall for traffic", loadBalancerName) deleteFunc(loadBalancerName)

ExternalLB and the healthcheck firewall rules follow this naming convention. addressed review comments.

freehan · 2019-11-07T01:06:08Z

/lgtm

prameshj · 2019-11-07T03:05:54Z

/priority-important-soon

prameshj · 2019-11-07T03:06:52Z

/priority important-soon

…22-upstream-release-1.16 Automated cherry pick of #84622: Create ILB firewall name with prefix "k8s-fw".

…22-upstream-release-1.15 Automated cherry pick of #84622: Create ILB firewall name with prefix "k8s-fw".

k8s-ci-robot assigned MrHohn Oct 31, 2019

k8s-ci-robot added area/cloudprovider sig/cloud-provider Categorizes an issue or PR as relevant to SIG Cloud Provider. and removed needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Oct 31, 2019

k8s-ci-robot requested review from dims and mcrute October 31, 2019 23:15

freehan reviewed Oct 31, 2019

View reviewed changes

staging/src/k8s.io/legacy-cloud-providers/gce/gce_loadbalancer_internal.go Show resolved Hide resolved

MrHohn reviewed Oct 31, 2019

View reviewed changes

prameshj force-pushed the rename-fwrules branch from 629d799 to 7bbd13d Compare November 1, 2019 20:05

k8s-ci-robot assigned freehan Nov 1, 2019

MrHohn approved these changes Nov 1, 2019

View reviewed changes

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

bowei reviewed Nov 5, 2019

View reviewed changes

prameshj mentioned this pull request Nov 5, 2019

Remove handling of legacy firewall rule names in GCE ILB code #84821

Open

prameshj force-pushed the rename-fwrules branch from 7bbd13d to b13ce69 Compare November 5, 2019 21:40

k8s-ci-robot added kind/bug Categorizes issue or PR as related to a bug. and removed needs-kind Indicates a PR lacks a `kind/foo` label and requires one. labels Nov 6, 2019

freehan reviewed Nov 7, 2019

View reviewed changes

prameshj force-pushed the rename-fwrules branch from b13ce69 to 4ae88e7 Compare November 7, 2019 00:49

prameshj force-pushed the rename-fwrules branch from 4ae88e7 to f6e2a3c Compare November 7, 2019 00:54

prameshj added 2 commits November 6, 2019 17:04

Create ILB firewall name with prefix "k8s-fw".

0d0d7ec

ExternalLB and the healthcheck firewall rules follow this naming convention. addressed review comments.

Added new test, fixed existing tests.

a52125c

prameshj force-pushed the rename-fwrules branch from f6e2a3c to a52125c Compare November 7, 2019 01:04

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

k8s-ci-robot added priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. and removed needs-priority Indicates a PR lacks a `priority/foo` label and requires one. labels Nov 7, 2019

k8s-ci-robot merged commit 303de85 into kubernetes:master Nov 7, 2019

k8s-ci-robot added this to the v1.17 milestone Nov 7, 2019

k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed release-note-none Denotes a PR that doesn't merit a release note. labels Nov 24, 2019

k8s-ci-robot added a commit that referenced this pull request Nov 24, 2019

Merge pull request #85102 from prameshj/automated-cherry-pick-of-#846…

53755d6

…22-upstream-release-1.16 Automated cherry pick of #84622: Create ILB firewall name with prefix "k8s-fw".

k8s-ci-robot added a commit that referenced this pull request Dec 6, 2019

Merge pull request #85245 from prameshj/automated-cherry-pick-of-#846…

388f59d

…22-upstream-release-1.15 Automated cherry pick of #84622: Create ILB firewall name with prefix "k8s-fw".

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rename ILB FirewallRules to be consistent with other resource names. #84622

Rename ILB FirewallRules to be consistent with other resource names. #84622

prameshj commented Oct 31, 2019 •

edited by justaugustus

Loading

prameshj commented Oct 31, 2019

prameshj commented Oct 31, 2019

MrHohn left a comment

MrHohn Oct 31, 2019

prameshj Nov 1, 2019

MrHohn Nov 1, 2019

prameshj Nov 1, 2019

prameshj commented Nov 1, 2019

MrHohn left a comment

MrHohn Nov 1, 2019

k8s-ci-robot commented Nov 1, 2019

bowei Nov 5, 2019

prameshj Nov 5, 2019

prameshj commented Nov 6, 2019

freehan left a comment

freehan Nov 6, 2019

freehan commented Nov 7, 2019

prameshj commented Nov 7, 2019

prameshj commented Nov 7, 2019

Rename ILB FirewallRules to be consistent with other resource names. #84622

Rename ILB FirewallRules to be consistent with other resource names. #84622

Conversation

prameshj commented Oct 31, 2019 • edited by justaugustus Loading

prameshj commented Oct 31, 2019

prameshj commented Oct 31, 2019

MrHohn 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

prameshj commented Nov 1, 2019

MrHohn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

k8s-ci-robot commented Nov 1, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

prameshj commented Nov 6, 2019

freehan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

freehan commented Nov 7, 2019

prameshj commented Nov 7, 2019

prameshj commented Nov 7, 2019

prameshj commented Oct 31, 2019 •

edited by justaugustus

Loading