Skip to content

Commit 8bac30d

Browse files
committed
fixup! refactor into designed declarative approach
1 parent 9327597 commit 8bac30d

File tree

1 file changed

+93
-49
lines changed

1 file changed

+93
-49
lines changed

providers/gce/gce_loadbalancer_external.go

Lines changed: 93 additions & 49 deletions
Original file line numberDiff line numberDiff line change
@@ -121,6 +121,11 @@ func (g *Cloud) ensureExternalLoadBalancer(clusterName string, clusterID string,
121121
return nil, fmt.Errorf("load balancer with multiple protocols (%s) is not supported when AlphaFeatureMultiProtocolLB is disabled", strings.Join(protocols, ","))
122122
}
123123

124+
existingFRs, err := g.getExistingForwardingRules(loadBalancerName)
125+
if err != nil {
126+
return nil, fmt.Errorf("error getting existing forwarding rules for %s: %v", loadBalancerName, err)
127+
}
128+
124129
var fwdRuleIP string
125130
// We need to determine the IP address for all forwarding rules.
126131
// We check if any of the forwarding rules already exist and use their IP.
@@ -148,10 +153,30 @@ func (g *Cloud) ensureExternalLoadBalancer(clusterName string, clusterID string,
148153

149154
// Make sure we know which IP address will be used and have properly reserved
150155
// it as static before moving forward with the rest of our operations.
156+
//
157+
// We use static IP addresses when updating a load balancer to ensure that we
158+
// can replace the load balancer's other components without changing the
159+
// address its service is reachable on. We do it this way rather than always
160+
// keeping the static IP around even though this is more complicated because
161+
// it makes it less likely that we'll run into quota issues. Only 7 static
162+
// IP addresses are allowed per region by default.
163+
//
164+
// We could let an IP be allocated for us when the forwarding rule is created,
165+
// but we need the IP to set up the firewall rule, and we want to keep the
166+
// forwarding rule creation as the last thing that needs to be done in this
167+
// function in order to maintain the invariant that "if the forwarding rule
168+
// exists, the LB has been fully created".
151169
ipAddressToUse := ""
152-
isUserOwnedIP := false
153-
isSafeToReleaseIP := false
154170

171+
// Through this process we try to keep track of whether it is safe to
172+
// release the IP that was allocated. If the user specifically asked for
173+
// an IP, we assume they are managing it themselves. Otherwise, we will
174+
// release the IP in case of early-terminating failure or upon successful
175+
// creating of the LB.
176+
// TODO(#36535): boil this logic down into a set of component functions
177+
// and key the flag values off of errors returned.
178+
isUserOwnedIP := false // if this is set, we never release the IP
179+
isSafeToReleaseIP := false
155180
defer func() {
156181
if isUserOwnedIP {
157182
return
@@ -195,6 +220,12 @@ func (g *Cloud) ensureExternalLoadBalancer(clusterName string, clusterID string,
195220
ipAddressToUse = ipAddr
196221
}
197222

223+
// Now we have the IP address we can build the desired forwarding rules.
224+
desiredFRs, err := g.buildDesiredForwardingRules(loadBalancerName, serviceName.String(), ipAddressToUse, g.targetPoolURL(loadBalancerName), apiService, netTier, existingFRs)
225+
if err != nil {
226+
return nil, fmt.Errorf("error building desired forwarding rules for %s: %v", loadBalancerName, err)
227+
}
228+
198229
// Deal with the firewall next. The reason we do this here rather than last
199230
// is because the forwarding rule is used as the indicator that the load
200231
// balancer is fully created - it's what getLoadBalancer checks for.
@@ -257,33 +288,42 @@ func (g *Cloud) ensureExternalLoadBalancer(clusterName string, clusterID string,
257288
} else {
258289
klog.V(4).Infof("ensureExternalLoadBalancer(%s): Service needs nodes health checks.", lbRefStr)
259290
if hcLocalTrafficExisting != nil {
291+
// This logic exists to detect a transition from OnlyLocal to non-OnlyLocal service
292+
// and turn on the tpNeedsRecreation flag to delete/recreate fwdrule/tpool updating the
293+
// target pool to use nodes health check.
260294
klog.V(2).Infof("ensureExternalLoadBalancer(%s): Updating from local traffic health checks to nodes health checks.", lbRefStr)
261295
hcToDelete = hcLocalTrafficExisting
262296
tpNeedsRecreation = true
263297
}
264298
hcToCreate = makeHTTPHealthCheck(MakeNodesHealthCheckName(clusterID), GetNodesHealthCheckPath(), GetNodesHealthCheckPort())
265299
}
266300

267-
if err := g.ensureTargetPoolAndHealthCheck(tpExists, tpNeedsRecreation, apiService, loadBalancerName, clusterID, ipAddressToUse, hosts, hcToCreate, hcToDelete); err != nil {
268-
return nil, err
269-
}
270-
271-
// Forwarding rule logic
272-
// We build the desired forwarding rules and then sync them with what exists.
273-
existingFRs, err := g.getExistingForwardingRules(loadBalancerName)
274-
if err != nil {
275-
return nil, fmt.Errorf("error getting existing forwarding rules for %s: %v", loadBalancerName, err)
276-
}
301+
// Now we get to some slightly more interesting logic.
302+
// First, neither target pools nor forwarding rules can be updated in place -
303+
// they have to be deleted and recreated.
304+
// Second, forwarding rules are layered on top of target pools in that you
305+
// can't delete a target pool that's currently in use by a forwarding rule.
306+
// Thus, we have to tear down a forwarding rule if either it or the target
307+
// pool needs to be updated.
308+
if len(existingFRs) > 0 && tpNeedsRecreation {
309+
// Begin critical section. If we have to delete the forwarding rule,
310+
// and something should fail before we recreate it, don't release the
311+
// IP. That way we can come back to it later.
312+
isSafeToReleaseIP = false
313+
314+
for _, fr := range existingFRs {
315+
if err := g.DeleteRegionForwardingRule(fr.Name, g.region); err != nil && !isNotFound(err) {
316+
return nil, fmt.Errorf("failed to delete existing forwarding rule for load balancer (%s) update: %v", lbRefStr, err)
317+
}
318+
}
319+
klog.Infof("ensureExternalLoadBalancer(%s): Deleted forwarding rule(s).", lbRefStr)
277320

278-
// Get the old forwarding rule for backward compatibility.
279-
var oldFwdRule *compute.ForwardingRule
280-
if fwd, ok := existingFRs[loadBalancerName]; ok {
281-
oldFwdRule = fwd
321+
// Clear the existing forwarding rules so we don't try to delete them again.
322+
existingFRs = nil
282323
}
283324

284-
desiredFRs, err := g.buildDesiredForwardingRules(loadBalancerName, serviceName.String(), ipAddressToUse, g.targetPoolURL(loadBalancerName), apiService, netTier, oldFwdRule)
285-
if err != nil {
286-
return nil, fmt.Errorf("error building desired forwarding rules for %s: %v", loadBalancerName, err)
325+
if err := g.ensureTargetPoolAndHealthCheck(tpExists, tpNeedsRecreation, apiService, loadBalancerName, clusterID, ipAddressToUse, hosts, hcToCreate, hcToDelete); err != nil {
326+
return nil, err
287327
}
288328

289329
// Delete unwanted forwarding rules.
@@ -299,28 +339,25 @@ func (g *Cloud) ensureExternalLoadBalancer(clusterName string, clusterID string,
299339
// Create or update forwarding rules.
300340
for _, desiredFR := range desiredFRs {
301341
existingFR, exists := existingFRs[desiredFR.Name]
302-
303-
needsUpdate := false
304342
if exists {
305-
if !g.forwardingRulesEqual(existingFR, desiredFR) {
306-
needsUpdate = true
343+
if g.forwardingRulesEqual(existingFR, desiredFR) {
344+
continue
307345
}
308346
}
309347

310-
if !exists || needsUpdate || tpNeedsRecreation {
311-
if exists {
312-
klog.Infof("ensureExternalLoadBalancer(%s): Deleting forwarding rule %s to update.", lbRefStr, desiredFR.Name)
313-
if err := g.DeleteRegionForwardingRule(desiredFR.Name, g.region); err != nil && !isNotFound(err) {
314-
return nil, err
315-
}
316-
}
317-
klog.Infof("ensureExternalLoadBalancer(%s): Creating forwarding rule %s for protocol %s, IP %s (tier: %s).", lbRefStr, desiredFR.Name, desiredFR.IPProtocol, ipAddressToUse, netTier)
318-
// The desiredFR is a complete spec, so we can just pass it to CreateRegionForwardingRule.
319-
if err := g.CreateRegionForwardingRule(desiredFR, g.region); err != nil && !isHTTPErrorCode(err, http.StatusConflict) {
320-
return nil, fmt.Errorf("failed to create forwarding rule for load balancer (%s): %v", lbRefStr, err)
348+
// We can't update a forwarding rule in place, so we have to delete it and recreate it.
349+
if exists {
350+
klog.Infof("ensureExternalLoadBalancer(%s): Deleting forwarding rule %s to update.", lbRefStr, desiredFR.Name)
351+
if err := g.DeleteRegionForwardingRule(desiredFR.Name, g.region); err != nil && !isNotFound(err) {
352+
return nil, err
321353
}
322-
klog.Infof("ensureExternalLoadBalancer(%s): Created forwarding rule %s.", lbRefStr, desiredFR.Name)
323354
}
355+
klog.Infof("ensureExternalLoadBalancer(%s): Creating forwarding rule %s for protocol %s, IP %s (tier: %s).", lbRefStr, desiredFR.Name, desiredFR.IPProtocol, ipAddressToUse, netTier)
356+
// The desiredFR is a complete spec, so we can just pass it to CreateRegionForwardingRule.
357+
if err := g.CreateRegionForwardingRule(desiredFR, g.region); err != nil && !isHTTPErrorCode(err, http.StatusConflict) {
358+
return nil, fmt.Errorf("failed to create forwarding rule for load balancer (%s): %v", lbRefStr, err)
359+
}
360+
klog.Infof("ensureExternalLoadBalancer(%s): Created forwarding rule %s.", lbRefStr, desiredFR.Name)
324361
}
325362

326363
isSafeToReleaseIP = true
@@ -339,19 +376,32 @@ func (g *Cloud) getExistingForwardingRules(loadBalancerName string) (map[string]
339376

340377
existingFRs := make(map[string]*compute.ForwardingRule)
341378
for _, fr := range frs {
342-
if strings.HasPrefix(fr.Name, loadBalancerName) {
379+
isMatch := false
380+
if fr.Name == loadBalancerName {
381+
isMatch = true
382+
} else if fr.Name == g.getProtocolForwardingRuleName(loadBalancerName, v1.Protocol(fr.IPProtocol)) {
383+
isMatch = true
384+
}
385+
386+
if isMatch {
343387
existingFRs[fr.Name] = fr
344388
}
345389
}
346390
return existingFRs, nil
347391
}
348392

349393
// buildDesiredForwardingRules builds the desired forwarding rules for the given load balancer.
350-
func (g *Cloud) buildDesiredForwardingRules(loadBalancerName, serviceName, ipAddress, targetPoolURL string, apiService *v1.Service, netTier cloud.NetworkTier, oldFwdRule *compute.ForwardingRule) (map[string]*compute.ForwardingRule, error) {
394+
func (g *Cloud) buildDesiredForwardingRules(loadBalancerName, serviceName, ipAddress, targetPoolURL string, apiService *v1.Service, netTier cloud.NetworkTier, existingFRs map[string]*compute.ForwardingRule) (map[string]*compute.ForwardingRule, error) {
351395
desiredFRs := make(map[string]*compute.ForwardingRule)
352396
groupedPorts := groupPortsByProtocol(apiService.Spec.Ports)
353397
desc := makeServiceDescription(serviceName)
354398

399+
// Get the old forwarding rule for backward compatibility.
400+
var oldFwdRule *compute.ForwardingRule
401+
if fwd, ok := existingFRs[loadBalancerName]; ok {
402+
oldFwdRule = fwd
403+
}
404+
355405
for protocol, protocolPorts := range groupedPorts {
356406
var frName string
357407
if g.AlphaFeatureGate.Enabled(AlphaFeatureMultiProtocolLB) {
@@ -415,7 +465,6 @@ func (g *Cloud) forwardingRulesEqual(existing, desired *compute.ForwardingRule)
415465
return true
416466
}
417467

418-
419468
// updateExternalLoadBalancer is the external implementation of LoadBalancer.UpdateLoadBalancer.
420469
func (g *Cloud) updateExternalLoadBalancer(clusterName string, service *v1.Service, nodes []*v1.Node) error {
421470
// Process services with LoadBalancerClass "networking.gke.io/l4-regional-external-legacy" used for this controller.
@@ -500,17 +549,16 @@ func (g *Cloud) ensureExternalLoadBalancerDeleted(clusterName, clusterID string,
500549
}
501550

502551
// TODO: Always or just with alpha feature flag?
503-
frs, err := g.ListRegionForwardingRules(g.region)
552+
existingFRs, err := g.getExistingForwardingRules(loadBalancerName)
504553
if err != nil {
505554
return err
506555
}
556+
507557
var deleteErrs []error
508-
for _, fr := range frs {
509-
if strings.HasPrefix(fr.Name, loadBalancerName) {
510-
klog.Infof("ensureExternalLoadBalancerDeleted(%s): Deleting forwarding rule %s.", lbRefStr, fr.Name)
511-
if err := ignoreNotFound(g.DeleteRegionForwardingRule(fr.Name, g.region)); err != nil {
512-
deleteErrs = append(deleteErrs, err)
513-
}
558+
for _, fr := range existingFRs {
559+
klog.Infof("ensureExternalLoadBalancerDeleted(%s): Deleting forwarding rule %s.", lbRefStr, fr.Name)
560+
if err := ignoreNotFound(g.DeleteRegionForwardingRule(fr.Name, g.region)); err != nil {
561+
deleteErrs = append(deleteErrs, err)
514562
}
515563
}
516564
if len(deleteErrs) > 0 {
@@ -908,8 +956,6 @@ func (g *Cloud) ensureHTTPHealthCheck(name, path string, port int32) (hc *comput
908956
return hc, nil
909957
}
910958

911-
912-
913959
// Doesn't check whether the hosts have changed, since host updating is handled
914960
// separately.
915961
func (g *Cloud) targetPoolNeedsRecreation(name, region string, affinityType v1.ServiceAffinity) (exists bool, needsRecreation bool, err error) {
@@ -1145,8 +1191,6 @@ func (g *Cloud) ensureHTTPHealthCheckFirewall(svc *v1.Service, serviceName, ipAd
11451191
return nil
11461192
}
11471193

1148-
1149-
11501194
func (g *Cloud) createFirewall(svc *v1.Service, name, desc, destinationIP string, sourceRanges utilnet.IPNetSet, ports []v1.ServicePort, hosts []*gceInstance) error {
11511195
firewall, err := g.firewallObject(name, desc, destinationIP, sourceRanges, ports, hosts)
11521196
if err != nil {

0 commit comments

Comments
 (0)