Adds annotation kubernetes.io/ingress.reserve-global-static-ip-name to signal GLBC to manage static IP #1182

retpolanne · 2020-07-15T12:55:39Z

This PR is a follow-up of #1151

This PR adds the kubernetes.io/ingress.reserve-global-static-ip-name annotation, which is a string that specifies a static IP name and manages it, differing from the behaviour of kubernetes.io/ingress.global-static-ip-name.

Things taken in consideration

The behaviour for kubernetes.io/ingress.global-static-ip-name is still the same: if the static IP doesn't exist on GCE, it will fail. However, kubernetes.io/ingress.reserve-global-static-ip-name will bypass the failure and reserve the static IP with the name provided.

This is different from the default behaviour seen in #1080: even if kubernetes.io/ingress.global-static-ip-name was provided, the static IP was reserved with a namer (that follows the pattern k8s-resource--uid) and if the user reserved the static IP with the name provided, GLBC would flip to using that, causing downtime.

In the approach of this PR, however, if kubernetes.io/ingress.global-static-ip-name is provided, we use its value from the beginning.

To avoid flipping to a managed static IP when already using a static IP, we check if there is already a static IP for an Ingress. If it exists, the kubernetes.io/ingress.global-static-ip-name behaviour is ignored.

Things not taken in consideration

~~If the user supplies kubernetes.io/ingress.reserve-global-static-ip-name at a later time, the regression will be triggered, causing the IP flip and downtime.~~

k8s-ci-robot · 2020-07-15T12:55:47Z

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

k8s-ci-robot · 2020-07-15T12:56:03Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: vinicyusmacedo
To complete the pull request process, please assign mrhohn
You can assign the PR to them by writing /assign @mrhohn in a comment when ready.

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

Needs approval from an approver in each of these files:

OWNERS

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

retpolanne · 2020-07-15T12:59:37Z

/cc @prameshj

rramkumar1 · 2020-07-15T17:24:01Z

@vinicyusmacedo We need to think about how to make sure the regression is not triggered again (as you mention in the "Things not taken in consideration" section).

Can you please provide some options? Ideally, we are able to introduce an API without leaving the user the opportunity to shoot themselves in the foot.

retpolanne · 2020-07-16T16:36:04Z

@rramkumar1 I was thinking about checking if the Ingress already has forwarding rules. If it has one already, the ReserveGlobalStaticIP logic is skipped. This way, we will be sure that ReserveGlobalStaticIP only has effect when the ingress is being created. The problem with this approach: it will probably fail during syncs, since the expected static IP name will differ.

Another approach is checking with the cloud provider if the static IP name points to the same forwarding rules as the ones on the Ingress. If it doesn't, we can fallback to using the default static IP namer.

pkg/annotations/ingress.go

retpolanne · 2020-07-18T01:09:57Z

@rramkumar1 I had an idea for ensuring a flip doesn't happen. Please check the last commit.

When l.ip exists (whether it was created when a TLS pre-shared cert or a StaticIPName was specified), we simply ignore ReserveGlobalStaticIPNameKey. This way, we will never flip to its value when it is specified later.

It won't work for Ingresses without TLS/Static IP though, as the IP will be ephemeral.

rramkumar1 · 2020-07-19T21:38:41Z

/ok-to-test

…, the annotation kubernetes.io/ingress.reserve-global-static-ip becomes available, but does nothing - Adds ReserveGlobalStaticIP to L7's runtime info and populates it on controller - Adds test cases for ReserveGlobalStaticIP, whether with existing and non-existent static ip - TestReserveGlobalStaticIPExistingValue changed - If the IP already exists and ReserveGlobalStaticIP is set, the behavior shouldn't be different. - Changes getEffectiveIP behavior to return true for managed if ReserveGlobalStaticIP is provided, changes checkStaticIP to use the provided static IP name - Changes the annotation ReserveGlobalStaticIPKey – instead of being a boolean, it is now a string specifing a static ip name - Checks if l.ip is null before using ReserveGlobalStaticIPName, ensuring that if the ingress has a static IP already, it shouldn't flip to using ReserveGlobalStaticIPName

prameshj · 2020-07-31T15:10:04Z

pkg/annotations/ingress.go

+	// GCE static ip for its forwarding rules. If specified, the Ingress controller
+	// reserves and assigns the static ip by this name to the forwarding rules of the
+	// given ingress. It differs from StaticIPNameKey, since the controller does manage
+	// this ip.


Add that it is the user's responsibility to not use the same value for both "kubernetes.io/ingress.reserve-global-static-ip-name" and "kubernetes.io/ingress.global-static-ip-name" in the same or different ingresses.

Does my comment make it clear? Do not use the same value for both this annotation and GlobalStaticIPNameKey.. Or should I change it to It's the user responsibility not to use the same value for both this annotation and GlobalStaticIPNameKey?

prameshj · 2020-07-31T15:10:40Z

pkg/annotations/ingress.go

@@ -117,17 +124,12 @@ func FromIngress(ing *v1beta1.Ingress) *Ingress {
 	return &Ingress{ing.Annotations}
 }



Add a comment

prameshj · 2020-08-26T22:29:42Z

@spencerhance Can you take a look if this can be supported with ILB staticIP config as well?

fejta-bot · 2020-11-24T23:35:36Z

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle stale

fejta-bot · 2020-12-25T00:20:00Z

Stale issues rot after 30d of inactivity.
Mark the issue as fresh with /remove-lifecycle rotten.
Rotten issues close after an additional 30d of inactivity.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle rotten

fejta-bot · 2021-01-24T01:03:45Z

Rotten issues close after 30d of inactivity.
Reopen the issue with /reopen.
Mark the issue as fresh with /remove-lifecycle rotten.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/close

k8s-ci-robot · 2021-01-24T01:03:57Z

@fejta-bot: Closed this PR.

In response to this:

Rotten issues close after 30d of inactivity.
Reopen the issue with /reopen.
Mark the issue as fresh with /remove-lifecycle rotten.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/close

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.

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Jul 15, 2020

k8s-ci-robot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jul 15, 2020

k8s-ci-robot requested review from bowei and MrHohn July 15, 2020 12:56

k8s-ci-robot requested a review from prameshj July 15, 2020 12:59

prameshj reviewed Jul 17, 2020

View reviewed changes

pkg/annotations/ingress.go Outdated Show resolved Hide resolved

retpolanne changed the title ~~Adds annotation kubernetes.io/ingress.reserve-global-static-ip to signal GLBC to manage static IP~~ Adds annotation kubernetes.io/ingress.reserve-global-static-ip-name to signal GLBC to manage static IP Jul 18, 2020

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 19, 2020

k8s-ci-robot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Jul 30, 2020

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 24, 2020

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 24, 2020

Added translator.TLSCerts to StaticIP tests

46aa836

prameshj reviewed Aug 26, 2020

View reviewed changes

Added comments for ReserveGlobalStaticIPName annotations

c0740e9

k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Nov 24, 2020

k8s-ci-robot removed the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Dec 25, 2020

k8s-ci-robot added the lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. label Dec 25, 2020

k8s-ci-robot closed this Jan 24, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adds annotation kubernetes.io/ingress.reserve-global-static-ip-name to signal GLBC to manage static IP #1182

Adds annotation kubernetes.io/ingress.reserve-global-static-ip-name to signal GLBC to manage static IP #1182

retpolanne commented Jul 15, 2020 •

edited

Loading

k8s-ci-robot commented Jul 15, 2020

k8s-ci-robot commented Jul 15, 2020

retpolanne commented Jul 15, 2020

rramkumar1 commented Jul 15, 2020

retpolanne commented Jul 16, 2020

retpolanne commented Jul 18, 2020 •

edited

Loading

rramkumar1 commented Jul 19, 2020

prameshj Jul 31, 2020

retpolanne Aug 26, 2020

prameshj Jul 31, 2020

prameshj commented Aug 26, 2020

fejta-bot commented Nov 24, 2020

fejta-bot commented Dec 25, 2020

fejta-bot commented Jan 24, 2021

k8s-ci-robot commented Jan 24, 2021

		@@ -117,17 +124,12 @@ func FromIngress(ing v1beta1.Ingress) Ingress {
		return &Ingress{ing.Annotations}
		}

Adds annotation kubernetes.io/ingress.reserve-global-static-ip-name to signal GLBC to manage static IP #1182

Adds annotation kubernetes.io/ingress.reserve-global-static-ip-name to signal GLBC to manage static IP #1182

Conversation

retpolanne commented Jul 15, 2020 • edited Loading

Things taken in consideration

Things not taken in consideration

k8s-ci-robot commented Jul 15, 2020

k8s-ci-robot commented Jul 15, 2020

retpolanne commented Jul 15, 2020

rramkumar1 commented Jul 15, 2020

retpolanne commented Jul 16, 2020

retpolanne commented Jul 18, 2020 • edited Loading

rramkumar1 commented Jul 19, 2020

prameshj Jul 31, 2020

Choose a reason for hiding this comment

retpolanne Aug 26, 2020

Choose a reason for hiding this comment

prameshj Jul 31, 2020

Choose a reason for hiding this comment

prameshj commented Aug 26, 2020

fejta-bot commented Nov 24, 2020

fejta-bot commented Dec 25, 2020

fejta-bot commented Jan 24, 2021

k8s-ci-robot commented Jan 24, 2021

retpolanne commented Jul 15, 2020 •

edited

Loading

retpolanne commented Jul 18, 2020 •

edited

Loading