Merge pull request #104925 from prameshj/ilbracefix

Process GCE ILB services with the v1 annotation in the service controller

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

@@ -53,16 +53,20 @@ 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) && existingFwdRule == nil {
+	if existingFwdRule == nil && !hasFinalizer(svc, ILBFinalizerV1) {
-		// When ILBSubsets is enabled, new ILB services will not be processed here.
+		// Neither the forwarding rule nor the V1 finalizer exists. This is most likely a new service.
-		// Services that have existing GCE resources created by this controller will continue to update.
+		if g.AlphaFeatureGate.Enabled(AlphaFeatureILBSubsets) {
-		klog.V(2).Infof("Skipped ensureInternalLoadBalancer for service %s/%s, since %s feature is enabled.", svc.Namespace, svc.Name, AlphaFeatureILBSubsets)
+			// When ILBSubsets is enabled, new ILB services will not be processed here.
-		return nil, cloudprovider.ImplementedElsewhere
+			// Services that have existing GCE resources created by this controller or the v1 finalizer
-	}
+			// will continue to update.
-	if hasFinalizer(svc, ILBFinalizerV2) {
+			klog.V(2).Infof("Skipped ensureInternalLoadBalancer for service %s/%s, since %s feature is enabled.", svc.Namespace, svc.Name, AlphaFeatureILBSubsets)
-		// Another controller is handling the resources for this service.
+			return nil, cloudprovider.ImplementedElsewhere
-		klog.V(2).Infof("Skipped ensureInternalLoadBalancer for service %s/%s, as service contains %q finalizer.", svc.Namespace, svc.Name, ILBFinalizerV2)
+		}
-		return nil, cloudprovider.ImplementedElsewhere
+		if hasFinalizer(svc, ILBFinalizerV2) {
+			// No V1 resources present - Another controller is handling the resources for this service.
+			klog.V(2).Infof("Skipped ensureInternalLoadBalancer for service %s/%s, as service contains %q finalizer.", svc.Namespace, svc.Name, ILBFinalizerV2)
+			return nil, cloudprovider.ImplementedElsewhere
+		}
 	}
 
 	nm := types.NamespacedName{Name: svc.Name, Namespace: svc.Namespace}

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

@@ -1044,36 +1044,67 @@ func TestCompareHealthChecks(t *testing.T) {
 // Test creation of InternalLoadBalancer with ILB Subsets featuregate enabled.
 func TestEnsureInternalLoadBalancerSubsetting(t *testing.T) {
 	t.Parallel()
+	for _, tc := range []struct {
+		desc                 string
+		finalizers           []string
+		createForwardingRule bool
+		expectErrorMsg       string
+	}{
+		{desc: "New service creation fails with Implemented Elsewhere", expectErrorMsg: cloudprovider.ImplementedElsewhere.Error()},
+		{desc: "Service with existing ForwardingRule is processed", createForwardingRule: true},
+		{desc: "Service with v1 finalizer is processed", finalizers: []string{ILBFinalizerV1}},
+		{desc: "Service with v2 finalizer is skipped", finalizers: []string{ILBFinalizerV2}, expectErrorMsg: cloudprovider.ImplementedElsewhere.Error()},
+		{desc: "Service with v2 finalizer and existing ForwardingRule is processed", finalizers: []string{ILBFinalizerV2}, createForwardingRule: true},
+		{desc: "Service with v1 and v2 finalizers is processed", finalizers: []string{ILBFinalizerV1, ILBFinalizerV2}},
+	} {
+		t.Run(tc.desc, func(t *testing.T) {
+			vals := DefaultTestClusterValues()
+			gce, err := fakeGCECloud(vals)
+			require.NoError(t, err)
+			gce.AlphaFeatureGate = NewAlphaFeatureGate([]string{AlphaFeatureILBSubsets})
+			recorder := record.NewFakeRecorder(1024)
+			gce.eventRecorder = recorder
 
-	vals := DefaultTestClusterValues()
+			nodeNames := []string{"test-node-1"}
-	gce, err := fakeGCECloud(vals)
+			_, err = createAndInsertNodes(gce, nodeNames, vals.ZoneName)
-	require.NoError(t, err)
+			require.NoError(t, err)
-	gce.AlphaFeatureGate = NewAlphaFeatureGate([]string{AlphaFeatureILBSubsets})
+			svc := fakeLoadbalancerService(string(LBTypeInternal))
-	recorder := record.NewFakeRecorder(1024)
+			svc.Finalizers = tc.finalizers
-	gce.eventRecorder = recorder
+			svc, err = gce.client.CoreV1().Services(svc.Namespace).Create(context.TODO(), svc, metav1.CreateOptions{})
-
+			require.NoError(t, err)
-	nodeNames := []string{"test-node-1"}
+			var existingFwdRule *compute.ForwardingRule
-	nodes, err := createAndInsertNodes(gce, nodeNames, vals.ZoneName)
+			if tc.createForwardingRule {
-	require.NoError(t, err)
+				// Create a ForwardingRule with the expected name
-
+				existingFwdRule = &compute.ForwardingRule{
-	svc := fakeLoadbalancerService(string(LBTypeInternal))
+					Name:                gce.GetLoadBalancerName(context.TODO(), "", svc),
-	svc, err = gce.client.CoreV1().Services(svc.Namespace).Create(context.TODO(), svc, metav1.CreateOptions{})
+					IPAddress:           "5.6.7.8",
-	require.NoError(t, err)
+					Ports:               []string{"123"},
-	status, err := createInternalLoadBalancer(gce, svc, nil, nodeNames, vals.ClusterName, vals.ClusterID, vals.ZoneName)
+					IPProtocol:          "TCP",
-	assert.EqualError(t, err, cloudprovider.ImplementedElsewhere.Error())
+					LoadBalancingScheme: string(cloud.SchemeInternal),
-	// No loadbalancer resources will be created due to the ILB Feature Gate
+				}
-	assert.Empty(t, status)
+				gce.CreateRegionForwardingRule(existingFwdRule, gce.region)
-	assertInternalLbResourcesDeleted(t, gce, svc, vals, true)
+			}
-	// Invoking delete should be a no-op
+			gotErrorMsg := ""
-	err = gce.EnsureLoadBalancerDeleted(context.Background(), vals.ClusterName, svc)
+			status, err := createInternalLoadBalancer(gce, svc, existingFwdRule, nodeNames, vals.ClusterName, vals.ClusterID, vals.ZoneName)
-	assert.NoError(t, err)
+			if err != nil {
-	assertInternalLbResourcesDeleted(t, gce, svc, vals, true)
+				gotErrorMsg = err.Error()
-	// Now remove the feature gate so that lb resources are created
+			}
-	gce.AlphaFeatureGate = NewAlphaFeatureGate([]string{})
+			if gotErrorMsg != tc.expectErrorMsg {
-	status, err = gce.EnsureLoadBalancer(context.Background(), vals.ClusterName, svc, nodes)
+				t.Errorf("createInternalLoadBalancer() = %q, want error %q", err, tc.expectErrorMsg)
-	assert.NoError(t, err)
+			}
-	assert.NotEmpty(t, status.Ingress)
+			if err != nil {
-	assertInternalLbResources(t, gce, svc, vals, nodeNames)
+				assert.Empty(t, status)
+				assertInternalLbResourcesDeleted(t, gce, svc, vals, true)
+			} else {
+				assert.NotEmpty(t, status.Ingress)
+				assertInternalLbResources(t, gce, svc, vals, nodeNames)
+			}
+			// Ensure that cleanup is successful, if applicable.
+			err = gce.EnsureLoadBalancerDeleted(context.Background(), vals.ClusterName, svc)
+			assert.NoError(t, err)
+			assertInternalLbResourcesDeleted(t, gce, svc, vals, true)
+		})
+	}
 }
 
 // TestEnsureInternalLoadBalancerDeletedSubsetting verifies that updates and deletion of existing ILB resources