Skip to content

Commit

Permalink
Merge pull request #1204 from prameshj/fr-proto
Browse files Browse the repository at this point in the history 
Allow modifying protocol in L4 ILB service
  • Loading branch information
@k8s-ci-robot
k8s-ci-robot committed Aug 6, 2020
2 parents 8c7bfe2 + e713946 commit 2333839
Show file tree
Hide file tree
Showing 4 changed files with 151 additions and 139 deletions.
7 changes: 5 additions & 2 deletions pkg/backends/backends.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -265,6 +265,9 @@ func (b *Backends) EnsureL4BackendService(name, hcLink, protocol, sessionAffinit
}
if protocol == string(api_v1.ProtocolTCP) {
expectedBS.ConnectionDraining = &composite.ConnectionDraining{DrainingTimeoutSec: DefaultConnectionDrainingTimeoutSeconds}
} else {
// This config is not supported in UDP mode, explicitly set to 0 to reset, if proto was TCP previously.
expectedBS.ConnectionDraining = &composite.ConnectionDraining{DrainingTimeoutSec: 0}
}

// Create backend service if none was found
Expand All @@ -284,8 +287,8 @@ func (b *Backends) EnsureL4BackendService(name, hcLink, protocol, sessionAffinit
if backendSvcEqual(expectedBS, bs) {
return bs, nil
}
if bs.ConnectionDraining != nil && bs.ConnectionDraining.DrainingTimeoutSec > 0 {
// if user overrides this value, continue using that.
if bs.ConnectionDraining != nil && bs.ConnectionDraining.DrainingTimeoutSec > 0 && protocol == string(api_v1.ProtocolTCP) {
// only preserves user overridden timeout value when the protocol is TCP
expectedBS.ConnectionDraining.DrainingTimeoutSec = bs.ConnectionDraining.DrainingTimeoutSec
}
klog.V(2).Infof("EnsureL4BackendService: updating backend service %v", name)
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
Loading

0 comments on commit 2333839

Please sign in to comment.