Skip to content

K
k3s
Commit cfdde6f7 authored by Alex Robinson's avatar Alex Robinson
Browse files
Options

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.
parent 0e71ea12
Hide whitespace changes
Inline Side-by-side
Showing with 37 additions and 37 deletions
pkg/cloudprovider/providers/gce/gce.go
View file @ cfdde6f7
...@@ -387,32 +387,32 @@ func (gce *GCECloud) EnsureTCPLoadBalancer(name, region string, loadBalancerIP n ...@@ -387,32 +387,32 @@ func (gce *GCECloud) EnsureTCPLoadBalancer(name, region string, loadBalancerIP n
} }
if !exists { if !exists {
// Note, though static addresses that _aren't_ in use cost money, ones that _are_ in use don't. // 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() op, err := gce.service.Addresses.Insert(gce.projectID, region, &compute.Address{Name: name}).Do()
if err != nil { 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 { 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) address, exists, err = gce.getAddress(name, region)
if err != nil { 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 { 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 { 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 { } else {
addresses, err := gce.service.Addresses.List(gce.projectID, region).Do() addresses, err := gce.service.Addresses.List(gce.projectID, region).Do()
if err != nil { 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) { 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())
} }
} }
...@@ -459,9 +459,6 @@ func (gce *GCECloud) EnsureTCPLoadBalancer(name, region string, loadBalancerIP n ...@@ -459,9 +459,6 @@ func (gce *GCECloud) EnsureTCPLoadBalancer(name, region string, loadBalancerIP n
PortRange: fmt.Sprintf("%d-%d", minPort, maxPort), PortRange: fmt.Sprintf("%d-%d", minPort, maxPort),
Target: gce.targetPoolURL(name, region), Target: gce.targetPoolURL(name, region),
} }
if loadBalancerIP != nil {
req.IPAddress = loadBalancerIP.String()
}
op, err := gce.service.ForwardingRules.Insert(gce.projectID, region, req).Do() op, err := gce.service.ForwardingRules.Insert(gce.projectID, region, req).Do()
if err != nil && !isHTTPErrorCode(err, http.StatusConflict) { if err != nil && !isHTTPErrorCode(err, http.StatusConflict) {
...@@ -616,58 +613,61 @@ func (gce *GCECloud) UpdateTCPLoadBalancer(name, region string, hosts []string) ...@@ -616,58 +613,61 @@ func (gce *GCECloud) UpdateTCPLoadBalancer(name, region string, hosts []string)
// EnsureTCPLoadBalancerDeleted is an implementation of TCPLoadBalancer.EnsureTCPLoadBalancerDeleted. // EnsureTCPLoadBalancerDeleted is an implementation of TCPLoadBalancer.EnsureTCPLoadBalancerDeleted.
func (gce *GCECloud) EnsureTCPLoadBalancerDeleted(name, region string) error { 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) { 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 { } else if err != nil {
glog.Warningf("Failed to delete Forwarding Rules %s: got error %s.", name, err.Error()) glog.Warningf("Failed to delete firewall %s, got error %v", fwName, err)
return err return err
} else { } else {
err = gce.waitForRegionOp(op, region) if err = gce.waitForGlobalOp(op); err != nil {
if err != nil { glog.Warningf("Failed waiting for Firewall %s to be deleted. Got error: %v", fwName, err)
glog.Warningf("Failed waiting for Forwarding Rule %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() op, err = gce.service.ForwardingRules.Delete(gce.projectID, region, name).Do()
if err != nil && isHTTPErrorCode(err, http.StatusNotFound) { if err != nil && isHTTPErrorCode(err, http.StatusNotFound) {
glog.Infof("Address %s is already deleted.", name) glog.Infof("Forwarding rule %s already deleted. Moving on to delete target pool.", name)
} else if err != nil { } else if err != nil {
glog.Warningf("Failed to delete Address %s, got error %v", name, err) glog.Warningf("Failed to delete Forwarding Rules %s: got error %s.", name, err.Error())
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 return err
} else {
err = gce.waitForRegionOp(op, region)
if err != nil {
glog.Warningf("Failed waiting for Forwarding Rule %s to be deleted: got error %s.", name, err.Error())
return err
}
} }
op, err = gce.service.TargetPools.Delete(gce.projectID, region, name).Do() op, err = gce.service.TargetPools.Delete(gce.projectID, region, name).Do()
if err != nil && isHTTPErrorCode(err, http.StatusNotFound) { if err != nil && isHTTPErrorCode(err, http.StatusNotFound) {
glog.Infof("Target pool %s already deleted.", name) glog.Infof("Target pool %s already deleted. Moving on to delete static IP address.", name)
return nil
} else if err != nil { } else if err != nil {
glog.Warningf("Failed to delete Target Pool %s, got error %s.", name, err.Error()) glog.Warningf("Failed to delete Target Pool %s, got error %s.", name, err.Error())
return err 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)
if err != nil { op, err = gce.service.Addresses.Delete(gce.projectID, region, name).Do()
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) { if err != nil && isHTTPErrorCode(err, http.StatusNotFound) {
glog.Infof("Firewall doesn't exist, moving on to deleting target pool.") glog.Infof("Static IP address %s already deleted. Done deleting load balancer.", name)
} else if err != nil { } else if err != nil {
glog.Warningf("Failed to delete firewall %s, got error %v", fwName, err) glog.Warningf("Failed to delete static IP Address %s, got error %v", name, err)
return err return err
} else { } else {
if err = gce.waitForGlobalOp(op); err != nil { if err := gce.waitForRegionOp(op, region); err != nil {
glog.Warningf("Failed waiting for Firewall %s to be deleted. Got error: %v", fwName, err) glog.Warningf("Failed waiting for address %s to be deleted, got error: %v", name, err)
return err return err
} }
} }
return err
return nil
} }
// UrlMap management // UrlMap management
......
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment