Implement support for Internal/Regional Static IP for L7-ILB #1174

spencerhance · 2020-07-06T20:33:20Z

This is implemented via an annotation "kubernetes.io/ingress.regional-static-ip-name". This uses the same prefix as the existing annotation, "global-static-ip-name."

This PR also depends on the update of k8s-cloud-provider in #1173

k8s-ci-robot · 2020-07-06T20:33:28Z

Hi @spencerhance. 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 · 2020-07-06T21:53:26Z

/ok-to-test

spencerhance · 2020-07-13T21:10:22Z

Rebased since #1173 has merged. The tests should pass now.

freehan · 2020-07-14T21:19:56Z

pkg/loadbalancers/addresses.go

 	"k8s.io/ingress-gce/pkg/flags"
 	"k8s.io/ingress-gce/pkg/utils"
 	"k8s.io/ingress-gce/pkg/utils/namer"
 	"k8s.io/klog"
 )

-// checkStaticIP reserves a static IP allocated to the Forwarding Rule.
+// checkStaticIP reserves a regional or global static IP allocated to the Forwarding Rule.
 func (l *L7) checkStaticIP() (err error) {


ingress-gce/pkg/loadbalancers/loadbalancers_test.go

Line 204 in d6080dd

func TestCreateHTTPILBLoadBalancerStaticIp(t *testing.T) {

The tests for checkStaticIP() are sort of included in loadbalancers_test.go right now since it also creates/gets resources from the GCE API, and piggybacks off of those mocks

freehan · 2020-07-20T22:24:28Z

pkg/annotations/ingress.go

+	globalIp := ing.GlobalStaticIPName()
+	regionalIp := ing.RegionalStaticIPName()
+
+	if globalIp != "" && regionalIp != "" {


I guess you need to validate if this is a L7 ILB then just return regionalStaticIP and ignore GlobalStaticIP?

vice versa for L7 XLB?

True, but I wanted to have a check for if the user specified both by accident

freehan · 2020-07-20T22:30:25Z

pkg/loadbalancers/forwarding_rules.go

-			return "", false, fmt.Errorf("the given static IP name %v doesn't translate to an existing global static IP.",
+		// TODO(shance): Replace version
+		if ip, err := composite.GetAddress(l.cloud, key, meta.VersionGA); err != nil || ip == nil {
+			return "", false, fmt.Errorf("The given static IP name %v doesn't translate to an existing static IP.",


lower case for the initial character for error

freehan · 2020-07-20T22:34:15Z

pkg/loadbalancers/forwarding_rules.go

@@ -163,10 +163,16 @@ func (l *L7) getEffectiveIP() (string, bool, error) {
 	// TODO: Handle the last case better.

 	if l.runtimeInfo.StaticIPName != "" {
+		key, err := l.CreateKey(l.runtimeInfo.StaticIPName)


don't you need to handle the L7 ILB for this case as well?

oh I see. CreateKey handles it already.

freehan · 2020-07-20T22:41:35Z

pkg/loadbalancers/addresses.go

 	if ip == nil {
 		klog.V(3).Infof("Creating static ip %v", managedStaticIPName)
-		err = l.cloud.ReserveGlobalAddress(&compute.Address{Name: managedStaticIPName, Address: l.fw.IPAddress})
+		address := &composite.Address{Name: managedStaticIPName, Address: l.fw.IPAddress, Version: meta.VersionGA}


Probably worth having a l.newStaticAddress() function to output a composite.Address based on the L7 config.

Hm, I'm hesitant to split this out into a function since it's fairly short and we only use it once

having a separate function makes this a bit more self contained. Which means the special handling for ILB is invisible to the main flow.

if isInternal { // Used for L7 ILB address.AddressType = "INTERNAL" }

Next time when changes are needed, it can just piggy back to the new function and do not touch the main flow.

Okay done, also added a unit test

This is done via a regional-static-ip annotation on the ingress

freehan

/lgtm
/approve

k8s-ci-robot · 2020-07-29T00:47:22Z

[APPROVALNOTIFIER] This PR is APPROVED

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

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

Cherry pick #1174 [Implement support for Internal/Regional Static IP for L7-ILB] into release-1.9

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 Jul 6, 2020

k8s-ci-robot added the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label Jul 6, 2020

k8s-ci-robot requested review from freehan and thockin July 6, 2020 20:33

spencerhance mentioned this pull request Jul 6, 2020

Static ip on internal ingress #1171

Closed

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 Jul 6, 2020

spencerhance force-pushed the internal-static-ip branch from 8a134d5 to d5344ec Compare July 13, 2020 21:09

Add GCE Addresses to Composites

27139dd

spencerhance force-pushed the internal-static-ip branch from d5344ec to 1f8daf7 Compare July 13, 2020 21:09

spencerhance force-pushed the internal-static-ip branch from 1f8daf7 to 3d6d0e3 Compare July 13, 2020 21:31

freehan reviewed Jul 20, 2020

View reviewed changes

Implements regional static IP support for L7-ILB ingress

d6080dd

This is done via a regional-static-ip annotation on the ingress

spencerhance force-pushed the internal-static-ip branch from 3d6d0e3 to d6080dd Compare July 28, 2020 22:07

freehan approved these changes Jul 29, 2020

View reviewed changes

k8s-ci-robot assigned freehan Jul 29, 2020

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 29, 2020

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 29, 2020

k8s-ci-robot merged commit 6c3ddf6 into kubernetes:master Jul 29, 2020

spencerhance deleted the internal-static-ip branch July 29, 2020 04:37

spencerhance mentioned this pull request Jul 31, 2020

Cherry pick #1174 [Implement support for Internal/Regional Static IP for L7-ILB] into release-1.9 #1201

Merged

k8s-ci-robot added a commit that referenced this pull request Jul 31, 2020

Merge pull request #1201 from spencerhance/cherry-pick-1174-1-9

72bc41c

Cherry pick #1174 [Implement support for Internal/Regional Static IP for L7-ILB] into release-1.9

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement support for Internal/Regional Static IP for L7-ILB #1174

Implement support for Internal/Regional Static IP for L7-ILB #1174

spencerhance commented Jul 6, 2020 •

edited

Loading

k8s-ci-robot commented Jul 6, 2020

freehan commented Jul 6, 2020

spencerhance commented Jul 13, 2020

freehan Jul 14, 2020

spencerhance Jul 28, 2020

spencerhance Jul 28, 2020

freehan Jul 20, 2020

spencerhance Jul 22, 2020

freehan Jul 20, 2020

spencerhance Jul 28, 2020

freehan Jul 20, 2020

freehan Jul 20, 2020

freehan Jul 20, 2020

spencerhance Jul 22, 2020

freehan Jul 23, 2020

spencerhance Jul 28, 2020

freehan left a comment

k8s-ci-robot commented Jul 29, 2020

Implement support for Internal/Regional Static IP for L7-ILB #1174

Implement support for Internal/Regional Static IP for L7-ILB #1174

Conversation

spencerhance commented Jul 6, 2020 • edited Loading

k8s-ci-robot commented Jul 6, 2020

freehan commented Jul 6, 2020

spencerhance commented Jul 13, 2020

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

Choose a reason for hiding this comment

freehan left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Jul 29, 2020

spencerhance commented Jul 6, 2020 •

edited

Loading