Reserve Static IPv6 address before syncing L4 ILB by panslava · Pull Request #2157 · kubernetes/ingress-gce

panslava · 2023-05-31T00:24:46Z

This will prevent losing address while recreating forwarding rule

Tested running locally with real cluster

panslava · 2023-05-31T00:32:02Z

CI will fail on vendor folder verification for now

I am waiting for vendor (GoogleCloudPlatform) to have a tag that includes addresses mocks with IPv6 GoogleCloudPlatform/k8s-cloud-provider#123

pkg/loadbalancers/address_manager.go

pkg/loadbalancers/forwarding_rules_ipv6.go

panslava · 2023-06-01T22:02:11Z

/hold

cezarygerard · 2023-06-12T15:02:25Z

Is it even supported on GCP public APIs?

panslava · 2023-06-12T19:33:29Z

@cezarygerard yes, it was added recently, and now all of these code api calls are supported (I verified)

panslava · 2023-06-13T22:18:26Z

/unhold

panslava · 2023-06-21T01:27:38Z

/assign cezarygerard

cezarygerard · 2023-06-21T21:54:42Z

are you going to implement the same for L4NET LB?

panslava · 2023-06-21T22:29:48Z

@cezarygerard yes #2165

pkg/loadbalancers/address_manager_test.go

pkg/loadbalancers/l4.go

pkg/loadbalancers/l4ipv6.go

pkg/loadbalancers/l4.go

cezarygerard · 2023-06-21T21:47:11Z

pkg/loadbalancers/l4.go

+				// Release the address that was reserved, in all cases. If the forwarding rule was successfully created,
+				// the ephemeral IP is not needed anymore. If it was not created, the address should be released to prevent leaks.
+				if err := ipv6AddrMgr.ReleaseAddress(); err != nil {
+					klog.Errorf("EnsureInternalLoadBalancer: failed to release IPv6 address reservation, possibly causing an orphan: %v", err)


should be an event?

Not sure..... I am not 100% sure if this does not return false positive errors, which we don't want to return to customers and scare them

cezarygerard · 2023-06-21T21:47:31Z

pkg/loadbalancers/l4.go

 				// the ephemeral IP is not needed anymore. If it was not created, the address should be released to prevent leaks.
 				if err := addrMgr.ReleaseAddress(); err != nil {
-					klog.Errorf("EnsureInternalLoadBalancer: failed to release address reservation, possibly causing an orphan: %v", err)
+					klog.Errorf("EnsureInternalLoadBalancer: failed to release IPv4 address reservation, possibly causing an orphan: %v", err)


should be an event?

Not sure..... I am not 100% sure if this does not return false positive errors, which we don't want to return to customers and scare them

cezarygerard · 2023-06-21T21:49:05Z

pkg/loadbalancers/l4.go

+			defer func() {
+				// Release the address that was reserved, in all cases. If the forwarding rule was successfully created,
+				// the ephemeral IP is not needed anymore. If it was not created, the address should be released to prevent leaks.
+				if err := ipv6AddrMgr.ReleaseAddress(); err != nil {


what if customer reserved ip before and created the service with this ip?

ReleaseAddress() deletes only controller managed address. Controller created address will have the same name as forwarding rule name, so controller will try to delete address with this name. If it was custom static address owned by customer -- controller will not delete it (nit: as long as customer didn't use forwarding rule name for their address)

pkg/loadbalancers/l4.go

cezarygerard · 2023-06-21T21:53:32Z

pkg/loadbalancers/l4ipv6.go

 	return l4.deleteFirewall(ipv6FirewallName)
 }
+
+func (l4 *L4) getOldIPv6ForwardingRule(existingBS *composite.BackendService) (*composite.ForwardingRule, error) {


no tests for this?

There is a test TestDualStackInternalLoadBalancerModifyProtocol which verifies the whole flow where this functions is useful, that we reserve IP address when protocol is changed

Though, if you think it would be useful, I can add a test specifically for this function, but I would say this is more a private helper function, that just abstracts functionality for readability

leave it as is

This will prevent losing address while recreating forwarding rule

cezarygerard · 2023-06-22T00:10:16Z

/lgtm

k8s-ci-robot · 2023-06-22T00:10:26Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: cezarygerard, panslava

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

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~OWNERS~~ [cezarygerard,panslava]

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 added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label May 31, 2023

k8s-ci-robot requested review from aojea and gauravkghildiyal May 31, 2023 00:24

k8s-ci-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels May 31, 2023

aojea reviewed May 31, 2023

View reviewed changes

pkg/loadbalancers/address_manager.go Show resolved Hide resolved

aojea reviewed May 31, 2023

View reviewed changes

pkg/loadbalancers/address_manager.go Outdated Show resolved Hide resolved

aojea reviewed May 31, 2023

View reviewed changes

pkg/loadbalancers/forwarding_rules_ipv6.go Show resolved Hide resolved

panslava force-pushed the reserve-ipv6-on-sync branch 2 times, most recently from 7b1758a to 41458ee Compare June 1, 2023 05:56

k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jun 1, 2023

panslava force-pushed the reserve-ipv6-on-sync branch 6 times, most recently from c7e3307 to 85d19d9 Compare June 7, 2023 21:59

k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jun 7, 2023

panslava force-pushed the reserve-ipv6-on-sync branch 2 times, most recently from 6aee662 to b09b781 Compare June 7, 2023 23:00

k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jun 13, 2023

panslava force-pushed the reserve-ipv6-on-sync branch from b09b781 to bac5dc3 Compare June 21, 2023 01:04

k8s-ci-robot assigned cezarygerard Jun 21, 2023

cezarygerard reviewed Jun 21, 2023

View reviewed changes

panslava added 2 commits June 21, 2023 23:53

Reserve Static IPv6 address before syncing L4 ILB

dd4db7b

This will prevent losing address while recreating forwarding rule

Fix review comments

9e2d9b1

panslava force-pushed the reserve-ipv6-on-sync branch from bac5dc3 to 9e2d9b1 Compare June 21, 2023 23:59

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 22, 2023

k8s-ci-robot merged commit d7770ae into kubernetes:master Jun 22, 2023

Conversation

panslava commented May 31, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

panslava commented May 31, 2023

Uh oh!

Uh oh!

Uh oh!

Uh oh!

panslava commented Jun 1, 2023

Uh oh!

cezarygerard commented Jun 12, 2023

Uh oh!

panslava commented Jun 12, 2023

Uh oh!

panslava commented Jun 13, 2023

Uh oh!

panslava commented Jun 21, 2023

Uh oh!

cezarygerard commented Jun 21, 2023

Uh oh!

panslava commented Jun 21, 2023

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cezarygerard commented Jun 22, 2023

Uh oh!

k8s-ci-robot commented Jun 22, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

panslava commented May 31, 2023 •

edited

Loading