From a5f4484f24a3c4f7c109a16258a20700c48c0764 Mon Sep 17 00:00:00 2001 From: Brendan Burns Date: Fri, 28 Aug 2015 15:40:43 -0700 Subject: [PATCH 1/2] Maintain an IP address independent of the forwarding rule for GCE --- pkg/cloudprovider/providers/gce/gce.go | 73 +++++++++++++++++++++ pkg/cloudprovider/providers/gce/gce_test.go | 59 +++++++++++++++++ 2 files changed, 132 insertions(+) diff --git a/pkg/cloudprovider/providers/gce/gce.go b/pkg/cloudprovider/providers/gce/gce.go index d88255e6807..59aaa9b2746 100644 --- a/pkg/cloudprovider/providers/gce/gce.go +++ b/pkg/cloudprovider/providers/gce/gce.go @@ -349,6 +349,27 @@ func makeFirewallName(name string) string { return fmt.Sprintf("k8s-fw-%s", name) } +func (gce *GCECloud) getAddress(name, region string) (string, bool, error) { + address, err := gce.service.Addresses.Get(gce.projectID, region, name).Do() + if err == nil { + return address.Address, true, nil + } + if isHTTPErrorCode(err, http.StatusNotFound) { + return "", false, nil + } + return "", false, err +} + +func ownsAddress(ip net.IP, addrs []*compute.Address) bool { + ipStr := ip.String() + for _, addr := range addrs { + if addr.Address == ipStr { + return true + } + } + return false +} + // EnsureTCPLoadBalancer is an implementation of TCPLoadBalancer.EnsureTCPLoadBalancer. // TODO(a-robinson): Don't just ignore specified IP addresses. Check if they're // owned by the project and available to be used, and use them if they are. @@ -358,6 +379,44 @@ func (gce *GCECloud) EnsureTCPLoadBalancer(name, region string, loadBalancerIP n } glog.V(2).Infof("Checking if load balancer already exists: %s", name) + if loadBalancerIP == nil { + glog.V(2).Info("Checking if the external ip address already exists: %s", name) + address, exists, err := gce.getAddress(name, region) + if err != nil { + return nil, fmt.Errorf("error looking for gce address: %v", err) + } + if !exists { + // Note, though static addresses that _aren't_ in use cost money, ones that _are_ in use don't. + // However, quota is limited. TODO: determine quota here + op, err := gce.service.Addresses.Insert(gce.projectID, region, &compute.Address{Name: name}).Do() + if err != nil { + return nil, fmt.Errorf("error creating gce address: %v", err) + } + if err := gce.waitForRegionOp(op, region); err != nil { + return nil, fmt.Errorf("error waiting for gce address to complete: %v", err) + } + address, exists, err = gce.getAddress(name, region) + if err != nil { + return nil, fmt.Errorf("error re-getting gce address: %v", err) + } + if !exists { + return nil, fmt.Errorf("failed to re-get gce address for %s", name) + } + } + if loadBalancerIP = net.ParseIP(address); loadBalancerIP == nil { + return nil, fmt.Errorf("error parsing external IP: %s", address) + } + } else { + addresses, err := gce.service.Addresses.List(gce.projectID, region).Do() + if err != nil { + return nil, fmt.Errorf("failed to list gce addresses: %v", err) + } + if !ownsAddress(loadBalancerIP, addresses.Items) { + return nil, fmt.Errorf("don't own the address: %s", loadBalancerIP.String()) + } + } + + glog.V(2).Info("Checking if load balancer already exists: %s", name) _, exists, err := gce.GetTCPLoadBalancer(name, region) if err != nil { return nil, fmt.Errorf("error checking if GCE load balancer already exists: %v", err) @@ -395,6 +454,7 @@ func (gce *GCECloud) EnsureTCPLoadBalancer(name, region string, loadBalancerIP n } req := &compute.ForwardingRule{ Name: name, + IPAddress: loadBalancerIP.String(), IPProtocol: "TCP", PortRange: fmt.Sprintf("%d-%d", minPort, maxPort), Target: gce.targetPoolURL(name, region), @@ -538,6 +598,19 @@ func (gce *GCECloud) EnsureTCPLoadBalancerDeleted(name, region string) error { return err } } + + op, err = gce.service.Addresses.Delete(gce.projectID, region, name).Do() + if err != nil && isHTTPErrorCode(err, http.StatusNotFound) { + glog.Infof("Address %s is already deleted.", name) + } else if err != nil { + glog.Warningf("Failed to delete Address %s, got error %v", name, err) + return err + } + if err := gce.waitForRegionOp(op, region); err != nil { + glog.Warningf("Failed waiting for address %s to be deleted, got error: %v", name, err) + return err + } + op, err = gce.service.TargetPools.Delete(gce.projectID, region, name).Do() if err != nil && isHTTPErrorCode(err, http.StatusNotFound) { glog.Infof("Target pool %s already deleted.", name) diff --git a/pkg/cloudprovider/providers/gce/gce_test.go b/pkg/cloudprovider/providers/gce/gce_test.go index 3f916848135..4a6157d132b 100644 --- a/pkg/cloudprovider/providers/gce/gce_test.go +++ b/pkg/cloudprovider/providers/gce/gce_test.go @@ -17,9 +17,68 @@ limitations under the License. package gce_cloud import ( + "net" "testing" + + compute "google.golang.org/api/compute/v1" ) +func TestOwnsAddress(t *testing.T) { + tests := []struct { + ip net.IP + addrs []*compute.Address + expectOwn bool + }{ + { + ip: net.ParseIP("1.2.3.4"), + addrs: []*compute.Address{}, + expectOwn: false, + }, + { + ip: net.ParseIP("1.2.3.4"), + addrs: []*compute.Address{ + {Address: "2.3.4.5"}, + {Address: "2.3.4.6"}, + {Address: "2.3.4.7"}, + }, + expectOwn: false, + }, + { + ip: net.ParseIP("2.3.4.5"), + addrs: []*compute.Address{ + {Address: "2.3.4.5"}, + {Address: "2.3.4.6"}, + {Address: "2.3.4.7"}, + }, + expectOwn: true, + }, + { + ip: net.ParseIP("2.3.4.6"), + addrs: []*compute.Address{ + {Address: "2.3.4.5"}, + {Address: "2.3.4.6"}, + {Address: "2.3.4.7"}, + }, + expectOwn: true, + }, + { + ip: net.ParseIP("2.3.4.7"), + addrs: []*compute.Address{ + {Address: "2.3.4.5"}, + {Address: "2.3.4.6"}, + {Address: "2.3.4.7"}, + }, + expectOwn: true, + }, + } + for _, test := range tests { + own := ownsAddress(test.ip, test.addrs) + if own != test.expectOwn { + t.Errorf("expected: %v, got %v for %v", test.expectOwn, own, test) + } + } +} + func TestGetRegion(t *testing.T) { gce := &GCECloud{ zone: "us-central1-b", From 242d28cf681a0369f956ee550ba63fa267d77a77 Mon Sep 17 00:00:00 2001 From: Alex Robinson Date: Fri, 25 Sep 2015 18:11:52 +0000 Subject: [PATCH 2/2] Fix PR for maintaining a GCE IP independently of the forwarding rule. This code was in rough shape, so I've fixed the issues with the original PR as well as a few other changes: 1. Clarify the error messages related to the "gce Addresses" to make it clear we're talking about static IP addresses 2. Fix the bug in the original PR, which was a nil pointer dereference from passing op to waitForRegionOp when the address doesn't exist. 3. Rearrange the steps of EnsureTCPLoadBalancerDeleted to be the reverse of EnsureCreated, which mostly just seems like good practice to me. This is also supported by the following two bugs I found :( 4. Fix an independent bug of returning too early if the target pool doesn't exist, effectively stranding the firewall. This was likely introduced because target pools used to be the last thing deleted, so it was previously safe to return there. 5. Fix an independent bug of not returning an error waiting for the target pool to be deleted failed. This was very possibly causing target pool leaks in our e2e tests. This was similarly due to assuming that the target pool was the last thing deleted in the function, then having the firewall deletion stuck in after it. --- pkg/cloudprovider/providers/gce/gce.go | 81 +++++++++++++------------- 1 file changed, 42 insertions(+), 39 deletions(-) diff --git a/pkg/cloudprovider/providers/gce/gce.go b/pkg/cloudprovider/providers/gce/gce.go index 59aaa9b2746..bcfd3d7c860 100644 --- a/pkg/cloudprovider/providers/gce/gce.go +++ b/pkg/cloudprovider/providers/gce/gce.go @@ -387,32 +387,32 @@ func (gce *GCECloud) EnsureTCPLoadBalancer(name, region string, loadBalancerIP n } if !exists { // Note, though static addresses that _aren't_ in use cost money, ones that _are_ in use don't. - // However, quota is limited. TODO: determine quota here + // However, quota is limited to only 7 addresses per region by default. op, err := gce.service.Addresses.Insert(gce.projectID, region, &compute.Address{Name: name}).Do() if err != nil { - return nil, fmt.Errorf("error creating gce address: %v", err) + return nil, fmt.Errorf("error creating gce static IP address: %v", err) } if err := gce.waitForRegionOp(op, region); err != nil { - return nil, fmt.Errorf("error waiting for gce address to complete: %v", err) + return nil, fmt.Errorf("error waiting for gce static IP address to complete: %v", err) } address, exists, err = gce.getAddress(name, region) if err != nil { - return nil, fmt.Errorf("error re-getting gce address: %v", err) + return nil, fmt.Errorf("error re-getting gce static IP address: %v", err) } if !exists { - return nil, fmt.Errorf("failed to re-get gce address for %s", name) + return nil, fmt.Errorf("failed to re-get gce static IP address for %s", name) } } if loadBalancerIP = net.ParseIP(address); loadBalancerIP == nil { - return nil, fmt.Errorf("error parsing external IP: %s", address) + return nil, fmt.Errorf("error parsing gce static IP address: %s", address) } } else { addresses, err := gce.service.Addresses.List(gce.projectID, region).Do() if err != nil { - return nil, fmt.Errorf("failed to list gce addresses: %v", err) + return nil, fmt.Errorf("failed to list gce IP addresses: %v", err) } if !ownsAddress(loadBalancerIP, addresses.Items) { - return nil, fmt.Errorf("don't own the address: %s", loadBalancerIP.String()) + return nil, fmt.Errorf("this gce project don't own the IP address: %s", loadBalancerIP.String()) } } @@ -585,9 +585,23 @@ func (gce *GCECloud) UpdateTCPLoadBalancer(name, region string, hosts []string) // EnsureTCPLoadBalancerDeleted is an implementation of TCPLoadBalancer.EnsureTCPLoadBalancerDeleted. func (gce *GCECloud) EnsureTCPLoadBalancerDeleted(name, region string) error { - op, err := gce.service.ForwardingRules.Delete(gce.projectID, region, name).Do() + fwName := makeFirewallName(name) + op, err := gce.service.Firewalls.Delete(gce.projectID, fwName).Do() if err != nil && isHTTPErrorCode(err, http.StatusNotFound) { - glog.Infof("Forwarding rule %s already deleted. Continuing to delete target pool.", name) + glog.Infof("Firewall %s already deleted. Moving on to delete forwarding rule.", name) + } else if err != nil { + glog.Warningf("Failed to delete firewall %s, got error %v", fwName, err) + return err + } else { + if err = gce.waitForGlobalOp(op); err != nil { + glog.Warningf("Failed waiting for Firewall %s to be deleted. Got error: %v", fwName, err) + return err + } + } + + op, err = gce.service.ForwardingRules.Delete(gce.projectID, region, name).Do() + if err != nil && isHTTPErrorCode(err, http.StatusNotFound) { + glog.Infof("Forwarding rule %s already deleted. Moving on to delete target pool.", name) } else if err != nil { glog.Warningf("Failed to delete Forwarding Rules %s: got error %s.", name, err.Error()) return err @@ -599,44 +613,33 @@ func (gce *GCECloud) EnsureTCPLoadBalancerDeleted(name, region string) error { } } - op, err = gce.service.Addresses.Delete(gce.projectID, region, name).Do() - if err != nil && isHTTPErrorCode(err, http.StatusNotFound) { - glog.Infof("Address %s is already deleted.", name) - } else if err != nil { - glog.Warningf("Failed to delete Address %s, got error %v", name, err) - return err - } - if err := gce.waitForRegionOp(op, region); err != nil { - glog.Warningf("Failed waiting for address %s to be deleted, got error: %v", name, err) - return err - } - op, err = gce.service.TargetPools.Delete(gce.projectID, region, name).Do() if err != nil && isHTTPErrorCode(err, http.StatusNotFound) { - glog.Infof("Target pool %s already deleted.", name) - return nil + glog.Infof("Target pool %s already deleted. Moving on to delete static IP address.", name) } else if err != nil { glog.Warningf("Failed to delete Target Pool %s, got error %s.", name, err.Error()) return err - } - err = gce.waitForRegionOp(op, region) - if err != nil { - glog.Warningf("Failed waiting for Target Pool %s to be deleted: got error %s.", name, err.Error()) - } - fwName := makeFirewallName(name) - op, err = gce.service.Firewalls.Delete(gce.projectID, fwName).Do() - if err != nil && isHTTPErrorCode(err, http.StatusNotFound) { - glog.Infof("Firewall doesn't exist, moving on to deleting target pool.") - } else if err != nil { - glog.Warningf("Failed to delete firewall %s, got error %v", fwName, err) - return err } else { - if err = gce.waitForGlobalOp(op); err != nil { - glog.Warningf("Failed waiting for Firewall %s to be deleted. Got error: %v", fwName, err) + if err := gce.waitForRegionOp(op, region); err != nil { + glog.Warningf("Failed waiting for Target Pool %s to be deleted: got error %s.", name, err.Error()) return err } } - return err + + op, err = gce.service.Addresses.Delete(gce.projectID, region, name).Do() + if err != nil && isHTTPErrorCode(err, http.StatusNotFound) { + glog.Infof("Static IP address %s already deleted. Done deleting load balancer.", name) + } else if err != nil { + glog.Warningf("Failed to delete static IP Address %s, got error %v", name, err) + return err + } else { + if err := gce.waitForRegionOp(op, region); err != nil { + glog.Warningf("Failed waiting for address %s to be deleted, got error: %v", name, err) + return err + } + } + + return nil } // UrlMap management