From 9f01981863be724610917f36da43e2a4178e9f7c Mon Sep 17 00:00:00 2001 From: Brian Grant Date: Fri, 25 Sep 2015 22:04:35 -0700 Subject: [PATCH] Revert "Maintain an IP address independent of the forwarding rule for GCE plus bug fixes" --- pkg/cloudprovider/providers/gce/gce.go | 106 +++----------------- pkg/cloudprovider/providers/gce/gce_test.go | 59 ----------- 2 files changed, 15 insertions(+), 150 deletions(-) diff --git a/pkg/cloudprovider/providers/gce/gce.go b/pkg/cloudprovider/providers/gce/gce.go index bcfd3d7c860..d88255e6807 100644 --- a/pkg/cloudprovider/providers/gce/gce.go +++ b/pkg/cloudprovider/providers/gce/gce.go @@ -349,27 +349,6 @@ 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. @@ -379,44 +358,6 @@ 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 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 static IP address: %v", err) - } - if err := gce.waitForRegionOp(op, region); err != nil { - 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 static IP address: %v", err) - } - if !exists { - 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 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 IP addresses: %v", err) - } - if !ownsAddress(loadBalancerIP, addresses.Items) { - return nil, fmt.Errorf("this gce project don't own the IP 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) @@ -454,7 +395,6 @@ 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), @@ -585,23 +525,9 @@ func (gce *GCECloud) UpdateTCPLoadBalancer(name, region string, hosts []string) // EnsureTCPLoadBalancerDeleted is an implementation of TCPLoadBalancer.EnsureTCPLoadBalancerDeleted. func (gce *GCECloud) EnsureTCPLoadBalancerDeleted(name, region string) error { - fwName := makeFirewallName(name) - op, err := gce.service.Firewalls.Delete(gce.projectID, fwName).Do() + op, err := gce.service.ForwardingRules.Delete(gce.projectID, region, name).Do() if err != nil && isHTTPErrorCode(err, http.StatusNotFound) { - 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) + glog.Infof("Forwarding rule %s already deleted. Continuing 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 @@ -612,34 +538,32 @@ func (gce *GCECloud) EnsureTCPLoadBalancerDeleted(name, region string) error { 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. Moving on to delete static IP address.", name) + glog.Infof("Target pool %s already deleted.", name) + return nil } else if err != nil { glog.Warningf("Failed to delete Target Pool %s, got error %s.", name, err.Error()) return err - } else { - 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 - } } - - op, err = gce.service.Addresses.Delete(gce.projectID, region, name).Do() + 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("Static IP address %s already deleted. Done deleting load balancer.", name) + glog.Infof("Firewall doesn't exist, moving on to deleting target pool.") } else if err != nil { - glog.Warningf("Failed to delete static IP Address %s, got error %v", name, err) + glog.Warningf("Failed to delete firewall %s, got error %v", fwName, 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) + if err = gce.waitForGlobalOp(op); err != nil { + glog.Warningf("Failed waiting for Firewall %s to be deleted. Got error: %v", fwName, err) return err } } - - return nil + return err } // UrlMap management diff --git a/pkg/cloudprovider/providers/gce/gce_test.go b/pkg/cloudprovider/providers/gce/gce_test.go index 4a6157d132b..3f916848135 100644 --- a/pkg/cloudprovider/providers/gce/gce_test.go +++ b/pkg/cloudprovider/providers/gce/gce_test.go @@ -17,68 +17,9 @@ 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",