Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Allow modifying protocol in L4 ILB service #1204

Merged
merged 2 commits into from
Aug 6, 2020
Merged

Allow modifying protocol in L4 ILB service #1204

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
19dd066
Allow L4 ILB proto to be modified.
prameshj Jul 31, 2020
e713946
fix unit test and remove duplicate test case.
prameshj Jul 31, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 5 additions & 2 deletions pkg/backends/backends.go
View file
Open in desktop
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}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

add a comment here to explain why timeout config for UDP is 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) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

add a comment:

only preserves user config timeout value when the protocol is TCP

// 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
Open in desktop
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
Open in desktop
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