Merge pull request #80660 from prameshj/ilb-err

Handle NotImplemented error in service_controller.

pkg/controller/service/service_controller.go

@@ -339,6 +339,13 @@ func (s *ServiceController) syncLoadBalancerIfNeeded(service *v1.Service, key st
 		}
 		newStatus, err = s.ensureLoadBalancer(service)
 		if err != nil {
+			if err == cloudprovider.ImplementedElsewhere {
+				// ImplementedElsewhere indicates that the ensureLoadBalancer is a nop and the
+				// functionality is implemented by a different controller.  In this case, we
+				// return immediately without doing anything.
+				klog.V(4).Infof("LoadBalancer for service %s implemented by a different controller %s, Ignoring error", key, s.cloud.ProviderName())
+				return op, nil
+			}
 			return op, fmt.Errorf("failed to ensure load balancer: %v", err)
 		}
 		s.eventRecorder.Event(service, v1.EventTypeNormal, "EnsuredLoadBalancer", "Ensured load balancer")
@@ -703,7 +710,12 @@ func (s *ServiceController) lockedUpdateLoadBalancerHosts(service *v1.Service, h
 		}
 		return nil
 	}
-
+	if err == cloudprovider.ImplementedElsewhere {
+		// ImplementedElsewhere indicates that the UpdateLoadBalancer is a nop and the
+		// functionality is implemented by a different controller.  In this case, we
+		// return immediately without doing anything.
+		return nil
+	}
 	// It's only an actual error if the load balancer still exists.
 	if _, exists, err := s.balancer.GetLoadBalancer(context.TODO(), s.clusterName, service); err != nil {
 		runtime.HandleError(fmt.Errorf("failed to check if load balancer exists for service %s/%s: %v", service.Namespace, service.Name, err))

staging/src/k8s.io/cloud-provider/cloud.go

@@ -104,6 +104,21 @@ func GetInstanceProviderID(ctx context.Context, cloud Interface, nodeName types.
 }
 
 // LoadBalancer is an abstract, pluggable interface for load balancers.
+//
+// Cloud provider may chose to implement the logic for
+// constructing/destroying specific kinds of load balancers in a
+// controller separate from the ServiceController.  If this is the case,
+// then {Ensure,Update}LoadBalancer must return the ImplementedElsewhere error.
+// For the given LB service, the GetLoadBalancer must return "exists=True" if
+// there exists a LoadBalancer instance created by ServiceController.
+// In all other cases, GetLoadBalancer must return a NotFound error.
+// EnsureLoadBalancerDeleted must not return ImplementedElsewhere to ensure
+// proper teardown of resources that were allocated by the ServiceController.
+// This can happen if a user changes the type of LB via an update to the resource
+// or when migrating from ServiceController to alternate implementation.
+// The finalizer on the service will be added and removed by ServiceController
+// irrespective of the ImplementedElsewhere error. Additional finalizers for
+// LB services must be managed in the alternate implementation.
 type LoadBalancer interface {
 	// TODO: Break this up into different interfaces (LB, etc) when we have more than one type of service
 	// GetLoadBalancer returns whether the specified load balancer exists, and
@@ -199,9 +214,10 @@ type Routes interface {
 }
 
 var (
-	InstanceNotFound = errors.New("instance not found")
-	DiskNotFound     = errors.New("disk is not found")
-	NotImplemented   = errors.New("unimplemented")
+	DiskNotFound         = errors.New("disk is not found")
+	ImplementedElsewhere = errors.New("implemented by alternate to cloud provider")
+	InstanceNotFound     = errors.New("instance not found")
+	NotImplemented       = errors.New("unimplemented")
 )
 
 // Zone represents the location of a particular machine.

staging/src/k8s.io/legacy-cloud-providers/gce/gce_alpha.go

@@ -26,6 +26,9 @@ const (
 	//
 	// alpha: v1.8 (for Services)
 	AlphaFeatureNetworkTiers = "NetworkTiers"
+	// AlphaFeatureILBSubsets allows InternalLoadBalancer services to include a subset
+	// of cluster nodes as backends instead of all nodes.
+	AlphaFeatureILBSubsets = "ILBSubsets"
 )
 
 // AlphaFeatureGate contains a mapping of alpha features to whether they are enabled
@@ -35,6 +38,9 @@ type AlphaFeatureGate struct {
 
 // Enabled returns true if the provided alpha feature is enabled
 func (af *AlphaFeatureGate) Enabled(key string) bool {
+	if af == nil || af.features == nil {
+		return false
+	}
 	return af.features[key]
 }
 

staging/src/k8s.io/legacy-cloud-providers/gce/gce_loadbalancer_internal.go

@@ -27,6 +27,7 @@ import (
 	"k8s.io/api/core/v1"
 	"k8s.io/apimachinery/pkg/types"
 	"k8s.io/apimachinery/pkg/util/sets"
+	cloudprovider "k8s.io/cloud-provider"
 	servicehelpers "k8s.io/cloud-provider/service/helpers"
 	"k8s.io/klog"
 )
@@ -36,6 +37,10 @@ const (
 )
 
 func (g *Cloud) ensureInternalLoadBalancer(clusterName, clusterID string, svc *v1.Service, existingFwdRule *compute.ForwardingRule, nodes []*v1.Node) (*v1.LoadBalancerStatus, error) {
+	if g.AlphaFeatureGate.Enabled(AlphaFeatureILBSubsets) {
+		return nil, cloudprovider.ImplementedElsewhere
+	}
+
 	nm := types.NamespacedName{Name: svc.Name, Namespace: svc.Namespace}
 	ports, protocol := getPortsAndProtocol(svc.Spec.Ports)
 	if protocol != v1.ProtocolTCP && protocol != v1.ProtocolUDP {
@@ -201,6 +206,9 @@ func (g *Cloud) clearPreviousInternalResources(svc *v1.Service, loadBalancerName
 // updateInternalLoadBalancer is called when the list of nodes has changed. Therefore, only the instance groups
 // and possibly the backend service need to be updated.
 func (g *Cloud) updateInternalLoadBalancer(clusterName, clusterID string, svc *v1.Service, nodes []*v1.Node) error {
+	if g.AlphaFeatureGate.Enabled(AlphaFeatureILBSubsets) {
+		return cloudprovider.ImplementedElsewhere
+	}
 	g.sharedResourceLock.Lock()
 	defer g.sharedResourceLock.Unlock()
 

staging/src/k8s.io/legacy-cloud-providers/gce/gce_loadbalancer_internal_test.go

@@ -31,6 +31,7 @@ import (
 	"k8s.io/api/core/v1"
 	"k8s.io/apimachinery/pkg/types"
 	"k8s.io/client-go/tools/record"
+	cloudprovider "k8s.io/cloud-provider"
 	servicehelper "k8s.io/cloud-provider/service/helpers"
 )
 
@@ -847,3 +848,65 @@ func TestCompareHealthChecks(t *testing.T) {
 		})
 	}
 }
+
+// Test creation of InternalLoadBalancer with ILB Subsets featuregate enabled.
+func TestEnsureInternalLoadBalancerSubsetting(t *testing.T) {
+	t.Parallel()
+
+	vals := DefaultTestClusterValues()
+	gce, err := fakeGCECloud(vals)
+	require.NoError(t, err)
+	gce.AlphaFeatureGate = NewAlphaFeatureGate([]string{AlphaFeatureILBSubsets})
+
+	nodeNames := []string{"test-node-1"}
+	nodes, err := createAndInsertNodes(gce, nodeNames, vals.ZoneName)
+	require.NoError(t, err)
+
+	svc := fakeLoadbalancerService(string(LBTypeInternal))
+	status, err := createInternalLoadBalancer(gce, svc, nil, nodeNames, vals.ClusterName, vals.ClusterID, vals.ZoneName)
+	assert.EqualError(t, err, cloudprovider.ImplementedElsewhere.Error())
+	// No loadbalancer resources will be created due to the ILB Feature Gate
+	assert.Empty(t, status)
+	assertInternalLbResourcesDeleted(t, gce, svc, vals, true)
+	// Invoking delete should be a no-op
+	err = gce.EnsureLoadBalancerDeleted(context.Background(), vals.ClusterName, svc)
+	assert.NoError(t, err)
+	assertInternalLbResourcesDeleted(t, gce, svc, vals, true)
+	// Now remove the feature gate so that lb resources are created
+	gce.AlphaFeatureGate = NewAlphaFeatureGate([]string{})
+	status, err = gce.EnsureLoadBalancer(context.Background(), vals.ClusterName, svc, nodes)
+	assert.NoError(t, err)
+	assert.NotEmpty(t, status.Ingress)
+	assertInternalLbResources(t, gce, svc, vals, nodeNames)
+}
+
+func TestEnsureInternalLoadBalancerDeletedSubsetting(t *testing.T) {
+	t.Parallel()
+
+	vals := DefaultTestClusterValues()
+	gce, err := fakeGCECloud(vals)
+	require.NoError(t, err)
+
+	nodeNames := []string{"test-node-1"}
+	nodes, err := createAndInsertNodes(gce, nodeNames, vals.ZoneName)
+	require.NoError(t, err)
+	svc := fakeLoadbalancerService(string(LBTypeInternal))
+	status, err := createInternalLoadBalancer(gce, svc, nil, nodeNames, vals.ClusterName, vals.ClusterID, vals.ZoneName)
+
+	assert.NoError(t, err)
+	assert.NotEmpty(t, status.Ingress)
+	// Enable FeatureGatee
+	gce.AlphaFeatureGate = NewAlphaFeatureGate([]string{AlphaFeatureILBSubsets})
+	newLBStatus := v1.LoadBalancerStatus{Ingress: []v1.LoadBalancerIngress{{IP: "1.2.3.4"}}}
+	// mock scenario where a different controller modifies status.
+	svc.Status.LoadBalancer = newLBStatus
+	status, err = gce.EnsureLoadBalancer(context.Background(), vals.ClusterName, svc, nodes)
+	assert.EqualError(t, err, cloudprovider.ImplementedElsewhere.Error())
+	// ensure that the status is empty
+	assert.Empty(t, status)
+	assert.Equal(t, svc.Status.LoadBalancer, newLBStatus)
+	// Invoked when service is deleted.
+	err = gce.EnsureLoadBalancerDeleted(context.Background(), vals.ClusterName, svc)
+	assert.NoError(t, err)
+	assertInternalLbResourcesDeleted(t, gce, svc, vals, true)
+}