Undesired operation detection for L4 ILB

pkg/l4lb/l4controller.go

@@ -480,6 +480,12 @@ func (l4c *L4Controller) sync(key string, svcLogger klog.Logger) error {
 			// result will be nil if the service was ignored(due to presence of service controller finalizer).
 			return nil
 		}
+		svcLogger.V(3).Info("Resources modified in the sync", "modifiedResources", result.ResourceUpdates.String(), "wasResync", isResync)
+		if isResync {
+			if result.ResourceUpdates.WereAnyResourcesModified() {
+				svcLogger.V(3).Error(nil, "Resources were modified but this was not expected for a resync.", "modifiedResources", result.ResourceUpdates.String())
+			}
+		}
 		l4c.publishMetrics(result, namespacedName, isResync, svcLogger)
 		l4c.serviceVersions.SetProcessed(key, svc.ResourceVersion, result.Error == nil, isResync, svcLogger)
 		return skipUserError(result.Error, svcLogger)
@@ -600,6 +606,7 @@ func (l4c *L4Controller) publishMetrics(result *loadbalancers.L4ILBSyncResult, n
 		if result.MetricsState.Multinetwork {
 			l4metrics.PublishL4ILBMultiNetSyncLatency(result.Error == nil, result.SyncType, result.StartTime, isResync)
 		}
+		l4metrics.PublishL4SyncDetails(l4ILBControllerName, result.Error == nil, isResync, result.ResourceUpdates.WereAnyResourcesModified())
 
 	case loadbalancers.SyncTypeDelete:
 		// if service is successfully deleted, remove it from cache

pkg/l4lb/metrics/metrics.go

@@ -41,7 +41,7 @@ const (
 	l4LastSyncTimeName                             = "l4_last_sync_time"
 	l4LBRemovedFinalizerMetricName                 = "l4_removed_finalizer_count"
 	l4LBControllerPanicsMetricName                 = "l4_controllers_panics_count"
-	L4netlbSyncDetailsMetricName                   = "l4_netlb_sync_details_count"
+	L4SyncDetailsMetricName                        = "l4_sync_details_count"
 	l4WeightedLBPodsPerNodeMetricName              = "l4_weighted_lb_pods_per_node"
 )
 
@@ -177,8 +177,8 @@ var (
 
 	l4LBSyncDetails = prometheus.NewCounterVec(
 		prometheus.CounterOpts{
-			Name: L4netlbSyncDetailsMetricName,
-			Help: "Details of updates done during a resync",
+			Name: L4SyncDetailsMetricName,
+			Help: "Details of updates done during L4 LB ensure operations",
 		},
 		[]string{"controller_name", "success", "predicted_periodic_resync", "was_update"},
 	)

pkg/loadbalancers/forwarding_rules.go

@@ -211,7 +211,7 @@ func (l7 *L7) getEffectiveIP() (string, bool, error) {
 
 // ensureIPv4ForwardingRule creates a forwarding rule with the given name, if it does not exist. It updates the existing
 // forwarding rule if needed.
-func (l4 *L4) ensureIPv4ForwardingRule(bsLink string, options gce.ILBOptions, existingFwdRule *composite.ForwardingRule, subnetworkURL, ipToUse string) (*composite.ForwardingRule, error) {
+func (l4 *L4) ensureIPv4ForwardingRule(bsLink string, options gce.ILBOptions, existingFwdRule *composite.ForwardingRule, subnetworkURL, ipToUse string) (*composite.ForwardingRule, utils.ResourceSyncStatus, error) {
 	start := time.Now()
 
 	// version used for creating the existing forwarding rule.
@@ -231,7 +231,7 @@ func (l4 *L4) ensureIPv4ForwardingRule(bsLink string, options gce.ILBOptions, ex
 	frDesc, err := utils.MakeL4LBServiceDescription(utils.ServiceKeyFunc(l4.Service.Namespace, l4.Service.Name), ipToUse,
 		version, false, utils.ILB)
 	if err != nil {
-		return nil, fmt.Errorf("Failed to compute description for forwarding rule %s, err: %w", frName,
+		return nil, utils.ResourceResync, fmt.Errorf("Failed to compute description for forwarding rule %s, err: %w", frName,
 			err)
 	}
 
@@ -257,12 +257,12 @@ func (l4 *L4) ensureIPv4ForwardingRule(bsLink string, options gce.ILBOptions, ex
 	if existingFwdRule != nil {
 		equal, err := Equal(existingFwdRule, newFwdRule)
 		if err != nil {
-			return nil, err
+			return nil, utils.ResourceResync, err
 		}
 		if equal {
 			// nothing to do
 			frLogger.V(2).Info("ensureIPv4ForwardingRule: Skipping update of unchanged forwarding rule")
-			return existingFwdRule, nil
+			return existingFwdRule, utils.ResourceResync, nil
 		}
 		frDiff := cmp.Diff(existingFwdRule, newFwdRule)
 		frLogger.V(2).Info("ensureIPv4ForwardingRule: forwarding rule changed.",
@@ -271,29 +271,29 @@ func (l4 *L4) ensureIPv4ForwardingRule(bsLink string, options gce.ILBOptions, ex
 		filtered, patchable := filterPatchableFields(existingFwdRule, newFwdRule)
 		if patchable {
 			if err = l4.forwardingRules.Patch(filtered); err != nil {
-				return nil, err
+				return nil, utils.ResourceUpdate, err
 			}
 			l4.recorder.Eventf(l4.Service, corev1.EventTypeNormal, events.SyncIngress, "ForwardingRule %s patched", existingFwdRule.Name)
 		} else {
 			if err := l4.updateForwardingRule(existingFwdRule, newFwdRule, frLogger); err != nil {
-				return nil, err
+				return nil, utils.ResourceUpdate, err
 			}
 		}
 	} else {
 		if err = l4.createFwdRule(newFwdRule, frLogger); err != nil {
-			return nil, err
+			return nil, utils.ResourceUpdate, err
 		}
 		l4.recorder.Eventf(l4.Service, corev1.EventTypeNormal, events.SyncIngress, "ForwardingRule %s created", newFwdRule.Name)
 	}
 
 	readFwdRule, err := l4.forwardingRules.Get(newFwdRule.Name)
 	if err != nil {
-		return nil, err
+		return nil, utils.ResourceUpdate, err
 	}
 	if readFwdRule == nil {
-		return nil, fmt.Errorf("Forwarding Rule %s not found", frName)
+		return nil, utils.ResourceUpdate, fmt.Errorf("Forwarding Rule %s not found", frName)
 	}
-	return readFwdRule, nil
+	return readFwdRule, utils.ResourceUpdate, nil
 }
 
 func (l4 *L4) updateForwardingRule(existingFwdRule, newFr *composite.ForwardingRule, frLogger klog.Logger) error {

pkg/loadbalancers/forwarding_rules_ipv6.go

@@ -40,12 +40,12 @@ const (
 	prefix96range = "/96"
 )
 
-func (l4 *L4) ensureIPv6ForwardingRule(bsLink string, options gce.ILBOptions, existingIPv6FwdRule *composite.ForwardingRule, ipv6AddressToUse string) (*composite.ForwardingRule, error) {
+func (l4 *L4) ensureIPv6ForwardingRule(bsLink string, options gce.ILBOptions, existingIPv6FwdRule *composite.ForwardingRule, ipv6AddressToUse string) (*composite.ForwardingRule, utils.ResourceSyncStatus, error) {
 	start := time.Now()
 
 	expectedIPv6FwdRule, err := l4.buildExpectedIPv6ForwardingRule(bsLink, options, ipv6AddressToUse)
 	if err != nil {
-		return nil, fmt.Errorf("l4.buildExpectedIPv6ForwardingRule(%s, %v, %s) returned error %w, want nil", bsLink, options, ipv6AddressToUse, err)
+		return nil, utils.ResourceResync, fmt.Errorf("l4.buildExpectedIPv6ForwardingRule(%s, %v, %s) returned error %w, want nil", bsLink, options, ipv6AddressToUse, err)
 	}
 
 	frLogger := l4.svcLogger.WithValues("forwardingRuleName", expectedIPv6FwdRule.Name)
@@ -57,26 +57,26 @@ func (l4 *L4) ensureIPv6ForwardingRule(bsLink string, options gce.ILBOptions, ex
 	if existingIPv6FwdRule != nil {
 		equal, err := EqualIPv6ForwardingRules(existingIPv6FwdRule, expectedIPv6FwdRule)
 		if err != nil {
-			return existingIPv6FwdRule, err
+			return existingIPv6FwdRule, utils.ResourceResync, err
 		}
 		if equal {
 			frLogger.V(2).Info("ensureIPv6ForwardingRule: Skipping update of unchanged ipv6 forwarding rule")
-			return existingIPv6FwdRule, nil
+			return existingIPv6FwdRule, utils.ResourceResync, nil
 		}
 		err = l4.deleteChangedIPv6ForwardingRule(existingIPv6FwdRule, expectedIPv6FwdRule)
 		if err != nil {
-			return nil, err
+			return nil, utils.ResourceUpdate, err
 		}
 	}
 
 	frLogger.V(2).Info("ensureIPv6ForwardingRule: Creating/Recreating forwarding rule")
 	err = l4.forwardingRules.Create(expectedIPv6FwdRule)
 	if err != nil {
-		return nil, err
+		return nil, utils.ResourceUpdate, err
 	}
 
 	createdFr, err := l4.forwardingRules.Get(expectedIPv6FwdRule.Name)
-	return createdFr, err
+	return createdFr, utils.ResourceUpdate, err
 }
 
 func (l4 *L4) buildExpectedIPv6ForwardingRule(bsLink string, options gce.ILBOptions, ipv6AddressToUse string) (*composite.ForwardingRule, error) {

pkg/loadbalancers/forwarding_rules_ipv6_test.go

@@ -17,10 +17,25 @@ limitations under the License.
 package loadbalancers
 
 import (
+	"strings"
 	"testing"
 
 	"github.com/GoogleCloudPlatform/k8s-cloud-provider/pkg/cloud"
+	"github.com/GoogleCloudPlatform/k8s-cloud-provider/pkg/cloud/meta"
+	"github.com/google/go-cmp/cmp"
+	"github.com/google/go-cmp/cmp/cmpopts"
+	"google.golang.org/api/compute/v1"
+	corev1 "k8s.io/api/core/v1"
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"k8s.io/apimachinery/pkg/types"
+	"k8s.io/client-go/tools/record"
+	"k8s.io/cloud-provider-gcp/providers/gce"
 	"k8s.io/ingress-gce/pkg/composite"
+	"k8s.io/ingress-gce/pkg/forwardingrules"
+	"k8s.io/ingress-gce/pkg/network"
+	"k8s.io/ingress-gce/pkg/utils"
+	"k8s.io/ingress-gce/pkg/utils/namer"
+	"k8s.io/klog/v2"
 )
 
 func TestIPv6ForwardingRulesEqual(t *testing.T) {
@@ -180,3 +195,182 @@ func TestIPv6ForwardingRulesEqual(t *testing.T) {
 		})
 	}
 }
+
+func TestL4EnsureIPv6ForwardingRuleUpdate(t *testing.T) {
+	serviceNamespace := "testNs"
+	serviceName := "testSvc"
+	l4namer := namer.NewL4Namer("test", namer.NewNamer("testCluster", "testFirewall", klog.TODO()))
+
+	bsLink := "http://www.googleapis.com/projects/test/regions/us-central1/backendServices/bs1"
+	networkURL := "https://www.googleapis.com/compute/v1/projects/test-poject/global/networks/test-vpc"
+	subnetworkURL := "https://www.googleapis.com/compute/v1/projects/test-poject/regions/us-central1/subnetworks/default-subnet"
+
+	testCases := []struct {
+		desc         string
+		svc          *corev1.Service
+		namedAddress *compute.Address
+		existingRule *composite.ForwardingRule
+		wantRule     *composite.ForwardingRule
+		wantUpdate   utils.ResourceSyncStatus
+		wantErrMsg   string
+	}{
+		{
+			desc: "create",
+			svc: &corev1.Service{
+				ObjectMeta: metav1.ObjectMeta{Name: serviceName, Namespace: serviceNamespace, UID: types.UID("1")},
+				Spec: corev1.ServiceSpec{
+					Ports: []corev1.ServicePort{
+						{
+							Port:     8080,
+							Protocol: corev1.ProtocolTCP,
+						},
+					},
+					Type: "LoadBalancer",
+				},
+			},
+			existingRule: nil,
+			wantRule: &composite.ForwardingRule{
+				Ports:               []string{"8080"},
+				IPProtocol:          "TCP",
+				IpVersion:           IPVersionIPv6,
+				LoadBalancingScheme: string(cloud.SchemeInternal),
+				NetworkTier:         cloud.NetworkTierDefault.ToGCEValue(),
+				Version:             meta.VersionGA,
+				BackendService:      bsLink,
+				Description:         ipV6ForwardingRuleDescription(t, serviceNamespace, serviceName),
+			},
+			wantUpdate: utils.ResourceUpdate,
+		},
+		{
+			desc: "no update",
+			svc: &corev1.Service{
+				ObjectMeta: metav1.ObjectMeta{Name: serviceName, Namespace: serviceNamespace, UID: types.UID("1")},
+				Spec: corev1.ServiceSpec{
+					Ports: []corev1.ServicePort{
+						{
+							Port:     8080,
+							Protocol: corev1.ProtocolTCP,
+						},
+					},
+					Type: "LoadBalancer",
+				},
+			},
+			existingRule: &composite.ForwardingRule{
+				Ports:               []string{"8080"},
+				IPProtocol:          "TCP",
+				IpVersion:           IPVersionIPv6,
+				LoadBalancingScheme: string(cloud.SchemeInternal),
+				NetworkTier:         cloud.NetworkTierDefault.ToGCEValue(),
+				Version:             meta.VersionGA,
+				BackendService:      bsLink,
+				Description:         ipV6ForwardingRuleDescription(t, serviceNamespace, serviceName),
+			},
+			wantRule: &composite.ForwardingRule{
+				Ports:               []string{"8080"},
+				IPProtocol:          "TCP",
+				IpVersion:           IPVersionIPv6,
+				LoadBalancingScheme: string(cloud.SchemeInternal),
+				NetworkTier:         cloud.NetworkTierDefault.ToGCEValue(),
+				Version:             meta.VersionGA,
+				BackendService:      bsLink,
+				Description:         ipV6ForwardingRuleDescription(t, serviceNamespace, serviceName),
+			},
+			wantUpdate: utils.ResourceResync,
+		},
+		{
+			desc: "update ports",
+			svc: &corev1.Service{
+				ObjectMeta: metav1.ObjectMeta{Name: serviceName, Namespace: serviceNamespace, UID: types.UID("1")},
+				Spec: corev1.ServiceSpec{
+					Ports: []corev1.ServicePort{
+						{
+							Port:     8080,
+							Protocol: corev1.ProtocolTCP,
+						},
+						{
+							Port:     8082,
+							Protocol: corev1.ProtocolTCP,
+						},
+					},
+					Type: "LoadBalancer",
+				},
+			},
+			existingRule: &composite.ForwardingRule{
+				Ports:               []string{"8080"},
+				IPProtocol:          "TCP",
+				IpVersion:           IPVersionIPv6,
+				LoadBalancingScheme: string(cloud.SchemeInternal),
+				NetworkTier:         cloud.NetworkTierDefault.ToGCEValue(),
+				Version:             meta.VersionGA,
+				BackendService:      bsLink,
+				Description:         ipV6ForwardingRuleDescription(t, serviceNamespace, serviceName),
+			},
+			wantRule: &composite.ForwardingRule{
+				Ports:               []string{"8080", "8082"},
+				IPProtocol:          "TCP",
+				IpVersion:           IPVersionIPv6,
+				LoadBalancingScheme: string(cloud.SchemeInternal),
+				NetworkTier:         cloud.NetworkTierDefault.ToGCEValue(),
+				Version:             meta.VersionGA,
+				BackendService:      bsLink,
+				Description:         ipV6ForwardingRuleDescription(t, serviceNamespace, serviceName),
+			},
+			wantUpdate: utils.ResourceUpdate,
+		},
+	}
+
+	for _, tc := range testCases {
+		t.Run(tc.desc, func(t *testing.T) {
+			fakeGCE := gce.NewFakeGCECloud(gce.DefaultTestClusterValues())
+			l4 := &L4{
+				cloud:           fakeGCE,
+				forwardingRules: forwardingrules.New(fakeGCE, meta.VersionGA, meta.Regional, klog.TODO()),
+				namer:           l4namer,
+				Service:         tc.svc,
+				network: network.NetworkInfo{
+					IsDefault:     true,
+					NetworkURL:    networkURL,
+					SubnetworkURL: subnetworkURL,
+				},
+				recorder: &record.FakeRecorder{},
+			}
+			tc.wantRule.Name = l4.getIPv6FRName()
+			if tc.existingRule != nil {
+				tc.existingRule.Name = l4.getIPv6FRName()
+			}
+			if tc.namedAddress != nil {
+				fakeGCE.ReserveRegionAddress(tc.namedAddress, fakeGCE.Region())
+			}
+			fr, updated, err := l4.ensureIPv6ForwardingRule(bsLink, gce.ILBOptions{}, tc.existingRule, "")
+
+			if err != nil && tc.wantErrMsg == "" {
+				t.Errorf("ensureIPv4ForwardingRule() err=%v", err)
+			}
+			if tc.wantErrMsg != "" {
+				if err == nil {
+					t.Errorf("ensureIPv4ForwardingRule() wanted error with msg=%q but got none", tc.wantErrMsg)
+				} else if !strings.Contains(err.Error(), tc.wantErrMsg) {
+					t.Errorf("ensureIPv4ForwardingRule() wanted error with msg=%q but got err=%v", tc.wantErrMsg, err)
+				}
+				return
+			}
+			if updated != tc.wantUpdate {
+				t.Errorf("ensureIPv4ForwardingRule() wanted updated=%v but got=%v", tc.wantUpdate, updated)
+			}
+
+			if diff := cmp.Diff(tc.wantRule, fr, cmpopts.IgnoreFields(composite.ForwardingRule{}, "SelfLink", "Region", "Scope")); diff != "" {
+				t.Errorf("ensureIPv4ForwardingRule() diff -want +got\n%v\n", diff)
+			}
+		})
+	}
+}
+
+func ipV6ForwardingRuleDescription(t *testing.T, namespace, name string) string {
+	t.Helper()
+	description, err := (&utils.L4LBResourceDescription{ServiceName: utils.ServiceKeyFunc(namespace, name)}).Marshal()
+	if err != nil {
+		t.Errorf("failed to create forwarding rule description for service %s/%s", namespace, name)
+	}
+	return description
+
+}