Revert "Maintain an IP address independent of the forwarding rule for GCE plus bug fixes"

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.ForwardingRules.Delete(gce.projectID, region, name).Do()
-	op, err := gce.service.Firewalls.Delete(gce.projectID, fwName).Do()
 	if err != nil && isHTTPErrorCode(err, http.StatusNotFound) {
-		glog.Infof("Firewall %s already deleted. Moving on to delete forwarding rule.", name)
+		glog.Infof("Forwarding rule %s already deleted. Continuing to delete target pool.", 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
@@ -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
-		}
 	}
-
+	err = gce.waitForRegionOp(op, region)
-	op, err = gce.service.Addresses.Delete(gce.projectID, region, name).Do()
+	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 {
+		if err = gce.waitForGlobalOp(op); err != nil {
-			glog.Warningf("Failed waiting for address %s to be deleted, got error: %v", name, err)
+			glog.Warningf("Failed waiting for Firewall %s to be deleted.  Got error: %v", fwName, err)
 			return err
 		}
 	}
-
+	return err
-	return nil
 }
 
 // UrlMap management

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",