GCE: Reserve address for ILBs during sync #51055

nicksardo · 2017-08-21T23:01:05Z

What this PR does / why we need it:
This PR adds the ability for the service controller to hold the ILB's IP during sync which may delete/recreate the forwarding rule.

Fixes: #47531

Release note:

GCE: Internal load balancer IPs are now reserved during service sync to prevent losing the address to another service.

nicksardo · 2017-08-21T23:02:49Z

FYI @yujuhong. This PR isn't done, but showing you what the ILB address logic will look like.

nicksardo · 2017-08-25T17:48:02Z

/unassign
/assign @yujuhong @bowei

yujuhong · 2017-08-25T20:51:33Z

pkg/cloudprovider/providers/gce/gce_address_manager.go

+type addressManager struct {
+	logPrefix   string
+	svc         CloudAddressService
+	lbName      string


I'd make this name or addrName. The fact that the controller chooses to use the lbNamd for all related resources seemed irrelevant to this manager.

I think we can make it clear that this manager would only manage (promote, delete) the address with this name

yujuhong · 2017-08-25T21:59:01Z

pkg/cloudprovider/providers/gce/gce_address_manager.go

+	}
+
+	// If the retrieved address is not named with the loadbalancer name, then the controller does not own it.
+	if addr.Name != am.lbName {


I'd probably make this clear by adding a method. It'd be easier to change in the future too.

func (am *addressManager) isManagedAddress(addr *computebeta.Address) { return addr.Name == am.Name }

yujuhong · 2017-08-25T22:09:11Z

pkg/cloudprovider/providers/gce/gce_address_manager.go

+		glog.V(3).Infof("%v: not attempting release of address %v.", am.logPrefix, am.targetIP)
+		return nil
+	}
+


If we are willing to spend an extra GET call, we can check whether the address is IN_USE. If yes, it's safe release it. If not, this IP may be lost forever. WDYT?

And we don't have to keep the tryRelease field. Just GET and return if not found.

yujuhong · 2017-08-25T22:11:00Z

pkg/cloudprovider/providers/gce/gce_address_manager.go

+	err := am.svc.DeleteRegionAddress(am.lbName, am.region)
+	if err != nil {
+		if isNotFound(err) {
+			glog.Warningf("%v: address %q was not found. Ignoring.", am.logPrefix, am.targetIP, am.lbName)


Remove am.targetIP.

Good catch, thanks

yujuhong · 2017-08-25T22:15:57Z

pkg/cloudprovider/providers/gce/gce_address_manager.go

+		Subnetwork:  am.subnetURL,
+	}
+
+	err := am.svc.ReserveBetaRegionAddress(newAddr, am.region)


nit (optional). I like less indents.

if err == nil { return nil } if !isHTTPErrorCode(err, http.StatusConflict) { return err }

yujuhong · 2017-08-25T23:14:20Z

pkg/cloudprovider/providers/gce/gce_address_manager_test.go

+
+	computebeta "google.golang.org/api/compute/v0.beta"
+	compute "google.golang.org/api/compute/v1"
+)


Would you consider using github.com/stretchr/testify/assert (for Errorf) and github.com/stretchr/testify/require (for Fatalf). They tend to make the code more succinct .

I'm a fan of testify/assert for personal projects, but haven't seen enormous use of it here in K8s. Happy to use it unless someone says otherwise.

yujuhong · 2017-08-25T23:19:48Z

pkg/cloudprovider/providers/gce/gce_interfaces.go

 	GetRegionAddressByIP(region, ipAddress string) (*compute.Address, error)
-	// TODO: Mock `DeleteRegionAddress(name, region string) endpoint
-	// TODO: Mock Global endpoints
+	DeleteRegionAddress(name, region string) error


oops... I added the same function in another PR. More rebasing...

yujuhong · 2017-08-25T23:27:05Z

pkg/cloudprovider/providers/gce/gce_loadbalancer_internal.go

+	targetIP := determineTargetIP(svc, existingFwdRule)
+
+	var addrMgr *addressManager
+	if targetIP != "" {


Just curious. The external LB ensures a static IP (i.e., creating a new one) even if targetIP == "" because many other components (e.g., firewall, targetpool) needs a target IP. I guess this not the case for ILB?

The firewall and target pool only use the IP for providing some extra info in the description. On first sync the description IP would be empty; but on second sync, it would be filled. I didn't see a need to perform the extra 2 API calls when the forwarding rule will just generate an IP itself, but I'll change the logic to have the description set right from the first sync.

yujuhong · 2017-08-25T23:34:28Z

pkg/cloudprovider/providers/gce/gce_address_manager.go

+
+	// Reserving the address failed due to a conflict. The address manager just checked that no address
+	// exists with the lbName, so it may belong to the user.
+	addr, err := am.svc.GetBetaRegionAddressByIP(am.region, am.targetIP)


I read the controller change but didn't see where you verify the AddressType of user-owned address.

yujuhong · 2017-08-25T23:37:45Z

pkg/cloudprovider/providers/gce/gce_loadbalancer_internal.go

+	// The addrMgr does not exist if no targetIP was specified. This function was not deferred
+	// because the code should continue to hold the targetIP if the sync fails for any reason.
+	// At this point, the existing rule is known to exist; therefore, releasing the address is fine.
+	if addrMgr != nil {


Did not read the entire ILB file, just want to ask whether you need to defer the ReleaseAddress to reduce the time the controller holds the static IP. Perhaps the IP quota is not a concern for internal IPs?

I'm confused, we discussed this case offline. If the service sync fails for any reason, I thought you said we should hold onto the IP for the next sync, right?

We should hold on to the IP if the forwarding rule is not using it. If the forwarding rule IS using the IP, it's safe to release the it since we can always get it back. This is what the external LB do today for optimization. If there is no need for ILB to do so, we don't have to add more complexity.

yujuhong · 2017-08-26T00:00:11Z

pkg/cloudprovider/providers/gce/gce_address_manager.go

+
+	err := am.svc.ReserveBetaRegionAddress(newAddr, am.region)
+	if err != nil {
+		if !isHTTPErrorCode(err, http.StatusConflict) {


Actually, this is not correct. I just found out that if you try to reserve an address with an IP that's already static (but not using the same name), you get a 400 error.

Error 400: Invalid value for field 'resource.address': '1.2.3.4'. Specified IP address is already reserved., invalid

FWIW, I don't think the current external LB controller handle some corner cases because of this....
E.g., if the fwd rule is using an IP that's not specified in the Service object, but is actually owned by the user (e.g., not using the lbName), the controller would just fail to ensure the IP.

Weird, I just tested this again and got 409. Wonder if there's an alpha vs beta difference

{ "kind": "compute#operation", "id": "1599788789319664192", "name": "operation-1503706287098-5579ce716c890-3262ae10-677fa2b7", "operationType": "insert", "targetLink": "https://www.googleapis.com/compute/beta/projects/XYZ/regions/us-central1/addresses/myip2", "targetId": "5223319394054898240", "status": "DONE", "progress": 100, "insertTime": "2017-08-25T17:11:27.558-07:00", "startTime": "2017-08-25T17:11:28.371-07:00", "endTime": "2017-08-25T17:11:31.465-07:00", "error": { "errors": [ { "code": "IP_IN_USE_BY_ANOTHER_RESOURCE", "message": "IP '10.0.0.10' is already being used by another resource. " } ] }, "httpErrorStatusCode": 409, "httpErrorMessage": "CONFLICT", "selfLink": "https://www.googleapis.com/compute/beta/projects/XYZ/regions/us-central1/operations/operation-1503706287098-5579ce716c890-3262ae10-677fa2b7", "region": "https://www.googleapis.com/compute/beta/projects/XYZ/regions/us-central1" }

I tested more and it's not about alpha/beta/v1. The function returns 409 if AddressType == INTERNAL. Otherwise, it always returns 400. (sigh...)

We should take this up with the appropriate team - it should be consistent.

Handling both

yujuhong

Looks good overall with mainly some nits.

FWIW, I tried with my GCP project and reserved internal address doesn't seem to take up any quota of static IP addresses.

yujuhong · 2017-08-29T22:21:35Z

pkg/cloudprovider/providers/gce/gce_address_manager.go

+}
+
+// HoldAddress will ensure that the IP is reserved with an address - either owned by the controller
+// or by a user. If the address is not the loadBalancerName, then it's assumed to be a user's address.


nit: s/loadBalancerName/addressManager.name

nit: document that the returned string is the IP address reserved.

yujuhong · 2017-08-29T22:25:00Z

pkg/cloudprovider/providers/gce/gce_address_manager.go

+			return addr.Address, nil
+		}
+
+		glog.V(3).Infof("%v: existing address %q has IP %q Type %q which does not match targeted IP %q Type %q. Attemping to delete.", am.logPrefix, addr.Name, addr.Address, addr.AddressType, am.targetIP, am.addressType)


Can we make this V(2). Would be useful for debugging GKE issues.

yujuhong · 2017-08-29T22:26:34Z

pkg/cloudprovider/providers/gce/gce_address_manager.go

+	// could be reserving another address; therefore, it would need to be deleted. In the normal
+	// case of using a controller address, retrieving the address by name results in the fewest API
+	// calls since it indicates whether a Delete is necessary before Reserve.
+	glog.V(3).Infof("%v: attempting hold of IP %q with type %q", am.logPrefix, am.targetIP, am.addressType)


Question: Why using V(3) for the majority of the logs? Just curious because I've seen mostly V(2) and V(4) logs in this package

yujuhong · 2017-08-29T22:50:27Z

pkg/cloudprovider/providers/gce/gce_address_manager.go

+	}
+
+	// Check that the address attributes are as required.
+	if addr.AddressType != string(am.addressType) {


suggestion: factor this out to a function since the manager checks essentially the same thing in line 71. This ensures that they stay consistent. E.g.,

func (am *addressManager) validateAddress(addr *computebeta.Address) error { ... }

yujuhong · 2017-08-29T22:58:55Z

pkg/cloudprovider/providers/gce/gce_address_manager_test.go

+	loadBalancerName := "a111111111111111"
+	forwardingRuleAddr := "1.1.1.1"
+	loadBalancerIP := ""
+	targetIP := getTargetIP(forwardingRuleAddr, loadBalancerIP)


Why using this function? AddressManager doesn't care what targetIP you choose.

nit (optional): the only difference between this test and the previous test is that the targetIP is non-empty. Could consider merging the two.

Another option is moving the code after HoldAddress to a separate function to reuse across tests.

yujuhong · 2017-08-29T23:06:15Z

pkg/cloudprovider/providers/gce/gce_address_manager_test.go

+	assert.NotEmpty(t, ipToUse)
+
+	addr, err = svc.GetBetaRegionAddress(loadBalancerName, region)
+	assert.NoError(t, err)


Use require.NoError(t, err)

yujuhong · 2017-08-29T23:09:31Z

pkg/cloudprovider/providers/gce/gce_address_manager_test.go

+	assert.NoError(t, err)
+	assert.NotEmpty(t, ipToUse)
+
+	addr, err = svc.GetBetaRegionAddress(loadBalancerName, region)


You use GetRegionAddress in other tests, why using Beta here?

yujuhong · 2017-08-29T23:17:33Z

pkg/cloudprovider/providers/gce/gce_addresses_fakes.go

+	return &addr
+}
+
+func (cas *FakeCloudAddressService) Print() {


Is this used anywhere?

This is good for debugging, but I would suggest returning a string so that the caller can dump the information wherever it deems appropriate.

yujuhong · 2017-08-29T23:22:17Z

pkg/cloudprovider/providers/gce/gce_loadbalancer_internal.go

@@ -79,19 +79,29 @@ func (gce *GCECloud) ensureInternalLoadBalancer(clusterName, clusterID string, s
 		return nil, err
 	}

+	// Determine IP which will be used for this LB. If no forwarding rule has been established
+	// or specified in the Service spec, then requestedIP = "".
+	requestedIP := determineRequestedIP(svc, existingFwdRule)


Before reaching this point, I assume it's not possible to have an existing forwarding rule with the wrong type of address, e.g., using an EXTERNAL IP. I am relying on the similar logic in external LB.

In the future, if we ever combine external and internal LB code, we'd need to ensure we still check whether the forwarding rule has all the expected attributes (and delete it if not).

We could check to see if the address is RFC 1918? (totally breaks down for IPv6...)

yujuhong · 2017-08-29T23:25:57Z

pkg/cloudprovider/providers/gce/gce_address_manager.go

+	return nil
+}
+
+func (am *addressManager) ensureAddressReservation() (string, error) {


Logic looks correct, and I like the useful log messages.

I do wish we could care (a bit) less about the QPS can make the code simpler.

nicksardo · 2017-08-30T00:15:28Z

@yujuhong I asked around and found that there is a hard quota of 200. I'm going to stick with releasing the address.

yujuhong

New commits look good. Just minor nits.

yujuhong · 2017-08-30T02:23:13Z

pkg/cloudprovider/providers/gce/gce_address_manager.go

+
+	if addr != nil {
+		// If address exists, check if the address had the expected attributes.
+		if (am.targetIP == "" || am.targetIP == addr.Address) && addr.AddressType == string(am.addressType) {


call validateAddress here as well.

yujuhong · 2017-08-30T02:23:49Z

pkg/cloudprovider/providers/gce/gce_address_manager.go

+}
+
+func (am *addressManager) validateAddress(addr *computebeta.Address) error {
+	if addr.AddressType != string(am.addressType) {


also check the IP if the targetIP is not empty.

nicksardo · 2017-08-30T16:51:40Z

/retest

nicksardo · 2017-08-30T19:27:03Z

/test pull-kubernetes-e2e-gce-gpu

yujuhong · 2017-08-30T23:45:02Z

LGTM.

bowei · 2017-08-30T23:46:38Z

pkg/cloudprovider/providers/gce/gce_address_manager.go

+
+		glog.V(4).Infof("%v: successfully created address %q which reserved IP %q", am.logPrefix, addr.Name, addr.Address)
+		return addr.Address, nil
+	} else if !isHTTPErrorCode(err, http.StatusConflict) && !isHTTPErrorCode(err, http.StatusBadRequest) {


can also get rid of this indent

bowei · 2017-08-30T23:47:30Z

pkg/cloudprovider/providers/gce/gce_addresses.go

@@ -82,6 +84,16 @@ func (gce *GCECloud) ReserveAlphaRegionAddress(addr *computealpha.Address, regio
 	return gce.waitForRegionOp(op, region, mc)
 }

+// ReserveBetaRegionAddress creates a beta region address


Curious why the Beta keyword is in the middle? Shouldn't it be a prefix or suffix?

bowei · 2017-08-30T23:48:24Z

pkg/cloudprovider/providers/gce/gce_loadbalancer_internal.go

@@ -79,19 +79,29 @@ func (gce *GCECloud) ensureInternalLoadBalancer(clusterName, clusterID string, s
 		return nil, err
 	}

+	// Determine IP which will be used for this LB. If no forwarding rule has been established
+	// or specified in the Service spec, then requestedIP = "".
+	requestedIP := determineRequestedIP(svc, existingFwdRule)


We could check to see if the address is RFC 1918? (totally breaks down for IPv6...)

bowei · 2017-08-30T23:49:02Z

/lgtm

k8s-github-robot · 2017-08-30T23:49:25Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: bowei, nicksardo

No associated issue. Update pull-request body to add a reference to an issue, or get approval with /approve no-issue

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

Needs approval from an approver in each of these OWNERS Files:

~~pkg/cloudprovider/providers/gce/OWNERS~~ [bowei,nicksardo]

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

k8s-github-robot · 2017-09-01T15:27:11Z

Automatic merge from submit-queue (batch tested with PRs 51632, 51055, 51676, 51560, 50007)

k8s-ci-robot assigned nicksardo Aug 21, 2017

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. sig/network Categorizes an issue or PR as relevant to SIG Network. area/platform/gce labels Aug 21, 2017

k8s-github-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. release-note Denotes a PR that will be considered when it comes time to generate release notes. labels Aug 21, 2017

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

nicksardo force-pushed the gce-ilb-ip branch from 4b80d04 to 511ca10 Compare August 22, 2017 21:19

k8s-github-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 Aug 22, 2017

nicksardo force-pushed the gce-ilb-ip branch from 90a163e to c86686f Compare August 24, 2017 16:13

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

nicksardo force-pushed the gce-ilb-ip branch from c86686f to 8ed34b9 Compare August 25, 2017 01:25

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

nicksardo force-pushed the gce-ilb-ip branch from 8ed34b9 to cf63aea Compare August 25, 2017 16:54

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 Aug 25, 2017

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

k8s-ci-robot assigned bowei and yujuhong and unassigned nicksardo Aug 25, 2017

yujuhong reviewed Aug 25, 2017

View reviewed changes

yujuhong reviewed Aug 26, 2017

View reviewed changes

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

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

nicksardo force-pushed the gce-ilb-ip branch from bc4b54f to 73a5fe8 Compare August 29, 2017 18:02

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

nicksardo force-pushed the gce-ilb-ip branch from 73a5fe8 to d3d386c Compare August 29, 2017 19:13

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 Aug 29, 2017

nicksardo force-pushed the gce-ilb-ip branch from d3d386c to 90c8eb6 Compare August 29, 2017 20:34

freehan added this to the v1.8 milestone Aug 29, 2017

yujuhong reviewed Aug 29, 2017

View reviewed changes

yujuhong reviewed Aug 30, 2017

View reviewed changes

nicksardo force-pushed the gce-ilb-ip branch 5 times, most recently from f1c301c to e1d1a12 Compare August 30, 2017 06:39

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

nicksardo force-pushed the gce-ilb-ip branch from e1d1a12 to a869ac8 Compare August 30, 2017 17:14

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

nicksardo force-pushed the gce-ilb-ip branch from a869ac8 to 1c8361a Compare August 30, 2017 17:22

Reserve internal address for ILBs

644aa69

nicksardo force-pushed the gce-ilb-ip branch from 1c8361a to 644aa69 Compare August 30, 2017 18:46

bowei reviewed Aug 30, 2017

View reviewed changes

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 30, 2017

k8s-github-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 30, 2017

k8s-github-robot merged commit 324a3bf into kubernetes:master Sep 1, 2017

nicksardo deleted the gce-ilb-ip branch January 12, 2018 07:00

GCE: Reserve address for ILBs during sync #51055

GCE: Reserve address for ILBs during sync #51055

Conversation

nicksardo commented Aug 21, 2017 • edited Loading

nicksardo commented Aug 21, 2017

nicksardo commented Aug 25, 2017

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

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

nicksardo Aug 26, 2017 • edited Loading

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

yujuhong 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

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

nicksardo commented Aug 30, 2017

yujuhong left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nicksardo commented Aug 30, 2017

nicksardo commented Aug 30, 2017

yujuhong commented Aug 30, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bowei commented Aug 30, 2017

k8s-github-robot commented Aug 30, 2017

k8s-github-robot commented Sep 1, 2017

nicksardo commented Aug 21, 2017 •

edited

Loading

nicksardo Aug 26, 2017 •

edited

Loading