Skip to content

Commit

Permalink
Allow L4 ILB proto to be modified.
Browse files Browse the repository at this point in the history 
Protocol modification results in forwarding rule being delete before
backend service update. Forwarding rule reuses the IP from service
status, to avoid IP address change due to proto change.

Other changes:
Also use GA API for forwarding rule creation always since Global Access is GA.
Remove feature gate logic for Custom Subnet.
Simplified Forwarding Rule Equality Logic. If the IP address in old and new FR do not match,
it is considered to be a change. Special case for empty IP removed.
  • Loading branch information
@prameshj
prameshj committed Aug 2, 2020
1 parent b1e1c03 commit 9856aef
Show file tree
Hide file tree
Showing 3 changed files with 51 additions and 62 deletions.
4 changes: 3 additions & 1 deletion pkg/backends/backends.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -265,6 +265,8 @@ func (b *Backends) EnsureL4BackendService(name, hcLink, protocol, sessionAffinit
}
if protocol == string(api_v1.ProtocolTCP) {
expectedBS.ConnectionDraining = &composite.ConnectionDraining{DrainingTimeoutSec: DefaultConnectionDrainingTimeoutSeconds}
} else {
expectedBS.ConnectionDraining = &composite.ConnectionDraining{DrainingTimeoutSec: 0}
}

// Create backend service if none was found
Expand All @@ -284,7 +286,7 @@ func (b *Backends) EnsureL4BackendService(name, hcLink, protocol, sessionAffinit
if backendSvcEqual(expectedBS, bs) {
return bs, nil
}
if bs.ConnectionDraining != nil && bs.ConnectionDraining.DrainingTimeoutSec > 0 {
if bs.ConnectionDraining != nil && bs.ConnectionDraining.DrainingTimeoutSec > 0 && protocol == string(api_v1.ProtocolTCP) {
// if user overrides this value, continue using that.
expectedBS.ConnectionDraining.DrainingTimeoutSec = bs.ConnectionDraining.DrainingTimeoutSec
}
Expand Down
87 changes: 28 additions & 59 deletions pkg/loadbalancers/forwarding_rules.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -196,61 +196,27 @@ func (l *L4) ensureForwardingRule(loadBalancerName, bsLink string, options gce.I
if err != nil {
return nil, err
}
desc := utils.L4ILBResourceDescription{}
// version used for creating the existing forwarding rule.
existingVersion := meta.VersionGA
// version to use for the new forwarding rule
newVersion := getAPIVersion(options)
version := meta.VersionGA

// Get the GA version forwarding rule, use the description to identify the version it was created with.
existingFwdRule, err := composite.GetForwardingRule(l.cloud, key, meta.VersionGA)
if utils.IgnoreHTTPNotFound(err) != nil {
return nil, err
}
if existingFwdRule != nil {
if err = desc.Unmarshal(existingFwdRule.Description); err != nil {
klog.Warningf("Failed to lookup forwarding rule version from description, err %v. Using GA Version.", err)
} else {
existingVersion = desc.APIVersion
}
}
// Fetch the right forwarding rule in case it is not using GA
if existingVersion != meta.VersionGA {
existingFwdRule, err = composite.GetForwardingRule(l.cloud, key, existingVersion)
if utils.IgnoreHTTPNotFound(err) != nil {
klog.Errorf("Failed to lookup forwarding rule '%s' at version - %s, err %v", key.Name, existingVersion, err)
return nil, err
}
}

if l.cloud.IsLegacyNetwork() {
l.recorder.Event(l.Service, v1.EventTypeWarning, "ILBOptionsIgnored", "Internal LoadBalancer options are not supported with Legacy Networks.")
options = gce.ILBOptions{}
}
subnetworkURL := l.cloud.SubnetworkURL()

if !l.cloud.AlphaFeatureGate.Enabled(gce.AlphaFeatureILBCustomSubnet) {
if options.SubnetName != "" {
l.recorder.Event(l.Service, v1.EventTypeWarning, "ILBCustomSubnetOptionIgnored", "Internal LoadBalancer CustomSubnet options ignored as the feature gate is disabled.")
options.SubnetName = ""
}
}
if l.cloud.AlphaFeatureGate.Enabled(gce.AlphaFeatureILBCustomSubnet) {
// If this feature is enabled, changes to subnet annotation will be
// picked up and reflected in the forwarding rule.
// Removing the annotation will set the forwarding rule to use the default subnet.
if options.SubnetName != "" {
subnetKey := *key
subnetKey.Name = options.SubnetName
subnetworkURL = cloud.SelfLink(meta.VersionGA, l.cloud.ProjectID(), "subnetworks", &subnetKey)
}
} else {
// TODO(84885) remove this once ILBCustomSubnet goes beta.
if existingFwdRule != nil && existingFwdRule.Subnetwork != "" {
// If the ILB already exists, continue using the subnet that it's already using.
// This is to support existing ILBs that were setup using the wrong subnet - https://github.com/kubernetes/kubernetes/pull/57861
subnetworkURL = existingFwdRule.Subnetwork
}
// Custom subnet feature is always enabled when running L4 controller.
// Changes to subnet annotation will be picked up and reflected in the forwarding rule.
// Removing the annotation will set the forwarding rule to use the default subnet.
if options.SubnetName != "" {
subnetKey := *key
subnetKey.Name = options.SubnetName
subnetworkURL = cloud.SelfLink(meta.VersionGA, l.cloud.ProjectID(), "subnetworks", &subnetKey)
}
// Determine IP which will be used for this LB. If no forwarding rule has been established
// or specified in the Service spec, then requestedIP = "".
Expand Down Expand Up @@ -279,7 +245,7 @@ func (l *L4) ensureForwardingRule(loadBalancerName, bsLink string, options gce.I
ports, _, protocol := utils.GetPortsAndProtocol(l.Service.Spec.Ports)
// Create the forwarding rule
frDesc, err := utils.MakeL4ILBServiceDescription(utils.ServiceKeyFunc(l.Service.Namespace, l.Service.Name), ipToUse,
newVersion)
version)
if err != nil {
return nil, fmt.Errorf("Failed to compute description for forwarding rule %s, err: %v", loadBalancerName,
err)
Expand All @@ -293,7 +259,7 @@ func (l *L4) ensureForwardingRule(loadBalancerName, bsLink string, options gce.I
LoadBalancingScheme: string(cloud.SchemeInternal),
Subnetwork: subnetworkURL,
Network: l.cloud.NetworkURL(),
Version: newVersion,
Version: version,
BackendService: bsLink,
AllowGlobalAccess: options.AllowGlobalAccess,
Description: frDesc,
Expand All @@ -308,7 +274,7 @@ func (l *L4) ensureForwardingRule(loadBalancerName, bsLink string, options gce.I
// If the forwarding rule pointed to a backend service which does not match the controller naming scheme,
// that resouce could be leaked. It is not being deleted here because that is a user-managed resource.
klog.V(2).Infof("ensureForwardingRule: Deleting existing forwarding rule - %s, will be recreated", fr.Name)
if err = utils.IgnoreHTTPNotFound(composite.DeleteForwardingRule(l.cloud, key, existingVersion)); err != nil {
if err = utils.IgnoreHTTPNotFound(composite.DeleteForwardingRule(l.cloud, key, version)); err != nil {
return nil, err
}
l.recorder.Eventf(l.Service, corev1.EventTypeNormal, events.SyncIngress, "ForwardingRule %q deleted", key.Name)
Expand All @@ -321,13 +287,19 @@ func (l *L4) ensureForwardingRule(loadBalancerName, bsLink string, options gce.I
return composite.GetForwardingRule(l.cloud, key, fr.Version)
}

func (l *L4) deleteForwardingRule(name string, version meta.Version) {
key, err := l.CreateKey(name)
if err != nil {
klog.Errorf("Failed to create key for deleting forwarding rule %s, err: %v", name, err)
return
}
if err := utils.IgnoreHTTPNotFound(composite.DeleteForwardingRule(l.cloud, key, version)); err != nil {
klog.Errorf("Failed to delete forwarding rule %s, err: %v", name, err)
}
}

func Equal(fr1, fr2 *composite.ForwardingRule) bool {
// If one of the IP addresses is empty, do not consider it as an inequality.
// If the IP address drops from a valid IP to empty, we do not want to apply
// the change if it is the only change in the forwarding rule. Similarly, if
// the forwarding rule changes from an empty IP to an allocated IP address, the
// subnetwork will change as well.
return (fr1.IPAddress == "" || fr2.IPAddress == "" || fr1.IPAddress == fr2.IPAddress) &&
return fr1.IPAddress == fr2.IPAddress &&
fr1.IPProtocol == fr2.IPProtocol &&
fr1.LoadBalancingScheme == fr2.LoadBalancingScheme &&
utils.EqualStringSets(fr1.Ports, fr2.Ports) &&
Expand All @@ -344,6 +316,11 @@ func ilbIPToUse(svc *v1.Service, fwdRule *composite.ForwardingRule, requestedSub
return svc.Spec.LoadBalancerIP
}
if fwdRule == nil {
// Reuse the already assigned IP address for this ILB. This is most likely the case
// where ILB protocol changed and the forwarding rule got deleted.
if len(svc.Status.LoadBalancer.Ingress) > 0 {
return svc.Status.LoadBalancer.Ingress[0].IP
}
return ""
}
if requestedSubnet != fwdRule.Subnetwork {
Expand All @@ -352,11 +329,3 @@ func ilbIPToUse(svc *v1.Service, fwdRule *composite.ForwardingRule, requestedSub
}
return fwdRule.IPAddress
}

// getAPIVersion returns the API version to use for CRUD of Forwarding rules, given the options enabled.
func getAPIVersion(options gce.ILBOptions) meta.Version {
if options.AllowGlobalAccess {
return meta.VersionBeta
}
return meta.VersionGA
}
22 changes: 20 additions & 2 deletions pkg/loadbalancers/l4.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -90,7 +90,7 @@ func (l *L4) EnsureInternalLoadBalancerDeleted(svc *corev1.Service) error {
}
retErr := err
// If any resource deletion fails, log the error and continue cleanup.
if err = utils.IgnoreHTTPNotFound(composite.DeleteForwardingRule(l.cloud, key, getAPIVersion(getILBOptions(l.Service)))); err != nil {
if err = utils.IgnoreHTTPNotFound(composite.DeleteForwardingRule(l.cloud, key, meta.VersionGA)); err != nil {
klog.Errorf("Failed to delete forwarding rule for internal loadbalancer service %s, err %v", l.NamespacedName.String(), err)
retErr = err
}
Expand Down Expand Up @@ -158,8 +158,12 @@ func (l *L4) EnsureInternalLoadBalancerDeleted(svc *corev1.Service) error {
// service.
func (l *L4) GetFRName() string {
_, _, protocol := utils.GetPortsAndProtocol(l.Service.Spec.Ports)
return l.getFRNameWithProtocol(string(protocol))
}

func (l *L4) getFRNameWithProtocol(protocol string) string {
lbName := l.namer.VMIPNEG(l.Service.Namespace, l.Service.Name)
return lbName + "-" + strings.ToLower(string(protocol))
return lbName + "-" + strings.ToLower(protocol)
}

// EnsureInternalLoadBalancer ensures that all GCE resources for the given loadbalancer service have
Expand Down Expand Up @@ -220,6 +224,20 @@ func (l *L4) EnsureInternalLoadBalancer(nodeNames []string, svc *corev1.Service)
if err != nil {
return nil, err
}

// Check if protocol has changed for this service. In this case, forwarding rule should be deleted before
// the backend service can be updated.
existingBS, err := l.backendPool.Get(name, meta.VersionGA, l.scope)
err = utils.IgnoreHTTPNotFound(err)
if err != nil {
klog.Errorf("Failed to lookup existing backend service, ignoring err: %v", err)
}
if existingBS != nil && existingBS.Protocol != string(protocol) {
klog.Infof("Protocol changed from %q to %q for service %s", existingBS.Protocol, string(protocol), l.NamespacedName)
// Delete forwarding rule if it exists
l.deleteForwardingRule(l.getFRNameWithProtocol(existingBS.Protocol), meta.VersionGA)
}

// ensure backend service
bs, err := l.backendPool.EnsureL4BackendService(name, hcLink, string(protocol), string(l.Service.Spec.SessionAffinity),
string(cloud.SchemeInternal), l.NamespacedName, meta.VersionGA)
Expand Down

0 comments on commit 9856aef

Please sign in to comment.