diff --git a/pkg/l4lb/l4controller.go b/pkg/l4lb/l4controller.go index 13557250a..1d187b3fd 100644 --- a/pkg/l4lb/l4controller.go +++ b/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 diff --git a/pkg/l4lb/metrics/metrics.go b/pkg/l4lb/metrics/metrics.go index 9c3b494a3..96bf063e3 100644 --- a/pkg/l4lb/metrics/metrics.go +++ b/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"}, ) diff --git a/pkg/loadbalancers/forwarding_rules.go b/pkg/loadbalancers/forwarding_rules.go index 7634e84c3..1b51ebfad 100644 --- a/pkg/loadbalancers/forwarding_rules.go +++ b/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 { diff --git a/pkg/loadbalancers/forwarding_rules_ipv6.go b/pkg/loadbalancers/forwarding_rules_ipv6.go index 10090ee80..6301a91ff 100644 --- a/pkg/loadbalancers/forwarding_rules_ipv6.go +++ b/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) { diff --git a/pkg/loadbalancers/forwarding_rules_ipv6_test.go b/pkg/loadbalancers/forwarding_rules_ipv6_test.go index d9f7df019..432ccb736 100644 --- a/pkg/loadbalancers/forwarding_rules_ipv6_test.go +++ b/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 + +} diff --git a/pkg/loadbalancers/forwarding_rules_test.go b/pkg/loadbalancers/forwarding_rules_test.go index 79970c861..c6b8f80ec 100644 --- a/pkg/loadbalancers/forwarding_rules_test.go +++ b/pkg/loadbalancers/forwarding_rules_test.go @@ -656,41 +656,6 @@ func TestL4CreateExternalForwardingRuleUpdate(t *testing.T) { }, wantUpdate: utils.ResourceUpdate, }, - { - desc: "network mismatch error", - svc: &corev1.Service{ - ObjectMeta: metav1.ObjectMeta{Name: serviceName, Namespace: serviceNamespace, UID: types.UID("1"), Annotations: map[string]string{annotations.NetworkTierAnnotationKey: string(cloud.NetworkTierStandard)}}, - Spec: corev1.ServiceSpec{ - Ports: []corev1.ServicePort{ - { - Port: 8080, - Protocol: corev1.ProtocolTCP, - }, - }, - Type: "LoadBalancer", - }, - }, - existingRule: &composite.ForwardingRule{ - PortRange: "8080-8080", - IPProtocol: "TCP", - LoadBalancingScheme: string(cloud.SchemeExternal), - NetworkTier: cloud.NetworkTierDefault.ToGCEValue(), - Version: meta.VersionGA, - BackendService: bsLink, - Description: l4ServiceDescription(t, serviceName, serviceNamespace, "", utils.XLB), - }, - wantRule: &composite.ForwardingRule{ - PortRange: "8080-8080", - IPProtocol: "TCP", - LoadBalancingScheme: string(cloud.SchemeExternal), - NetworkTier: cloud.NetworkTierDefault.ToGCEValue(), - Version: meta.VersionGA, - BackendService: bsLink, - Description: l4ServiceDescription(t, serviceName, serviceNamespace, "", utils.XLB), - }, - wantUpdate: utils.ResourceResync, - wantErrMsg: "Network tier mismatch for resource", - }, } for _, tc := range testCases { @@ -759,7 +724,7 @@ func TestL4EnsureIPv4ForwardingRuleAddressAlreadyInUse(t *testing.T) { fakeGCE.ReserveRegionAddress(addr, fakeGCE.Region()) insertError := &googleapi.Error{Code: http.StatusConflict, Message: "IP_IN_USE_BY_ANOTHER_RESOURCE - IP '10.107.116.14' is already being used by another resource."} fakeGCE.Compute().(*cloud.MockGCE).MockForwardingRules.InsertHook = test.InsertForwardingRuleErrorHook(insertError) - _, err := l4.ensureIPv4ForwardingRule("link", gce.ILBOptions{}, nil, "subnetworkX", "1.1.1.1") + _, _, err := l4.ensureIPv4ForwardingRule("link", gce.ILBOptions{}, nil, "subnetworkX", "1.1.1.1") require.Error(t, err) assert.True(t, utils.IsIPConfigurationError(err)) @@ -801,8 +766,9 @@ func TestL4EnsureIPv4ForwardingRule(t *testing.T) { ipToUse := "1.1.1.1" bsLink := "http://www.googleapis.com/projects/test/regions/us-central1/backendServices/bs1" - forwardingRule, err := l4.ensureIPv4ForwardingRule(bsLink, gce.ILBOptions{}, nil, subnetworkURL, ipToUse) + forwardingRule, syncStatus, err := l4.ensureIPv4ForwardingRule(bsLink, gce.ILBOptions{}, nil, subnetworkURL, ipToUse) require.NoError(t, err) + require.Equal(t, utils.ResourceUpdate, syncStatus) wantForwardingRule := &composite.ForwardingRule{ Name: l4namer.L4ForwardingRule(serviceNamespace, serviceName, "tcp"), @@ -832,3 +798,147 @@ func l4ServiceDescription(t *testing.T, svcName, svcNamespace, ipToUse string, l } return description } + +func TestL4EnsureInternalForwardingRuleUpdate(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: "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", + LoadBalancingScheme: string(cloud.SchemeInternal), + NetworkTier: cloud.NetworkTierDefault.ToGCEValue(), + Version: meta.VersionGA, + Network: networkURL, + Subnetwork: subnetworkURL, + BackendService: bsLink, + Description: l4ServiceDescription(t, serviceName, serviceNamespace, "", utils.ILB), + }, + wantRule: &composite.ForwardingRule{ + Ports: []string{"8080"}, + IPProtocol: "TCP", + LoadBalancingScheme: string(cloud.SchemeInternal), + NetworkTier: cloud.NetworkTierDefault.ToGCEValue(), + Version: meta.VersionGA, + Network: networkURL, + Subnetwork: subnetworkURL, + BackendService: bsLink, + Description: l4ServiceDescription(t, serviceName, serviceNamespace, "", utils.ILB), + }, + 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", + LoadBalancingScheme: string(cloud.SchemeInternal), + NetworkTier: cloud.NetworkTierDefault.ToGCEValue(), + Version: meta.VersionGA, + Network: networkURL, + Subnetwork: subnetworkURL, + BackendService: bsLink, + Description: l4ServiceDescription(t, serviceName, serviceNamespace, "", utils.ILB), + }, + wantRule: &composite.ForwardingRule{ + Ports: []string{"8080", "8082"}, + IPProtocol: "TCP", + LoadBalancingScheme: string(cloud.SchemeInternal), + NetworkTier: cloud.NetworkTierDefault.ToGCEValue(), + Version: meta.VersionGA, + Network: networkURL, + Subnetwork: subnetworkURL, + BackendService: bsLink, + Description: l4ServiceDescription(t, serviceName, serviceNamespace, "", utils.ILB), + }, + 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.GetFRName() + tc.existingRule.Name = l4.GetFRName() + if tc.namedAddress != nil { + fakeGCE.ReserveRegionAddress(tc.namedAddress, fakeGCE.Region()) + } + fr, updated, err := l4.ensureIPv4ForwardingRule(bsLink, gce.ILBOptions{}, tc.existingRule, subnetworkURL, "") + + 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) + } + }) + } +} diff --git a/pkg/loadbalancers/l4.go b/pkg/loadbalancers/l4.go index 22b7436c2..e5208332e 100644 --- a/pkg/loadbalancers/l4.go +++ b/pkg/loadbalancers/l4.go @@ -85,6 +85,7 @@ type L4ILBSyncResult struct { MetricsState metrics.L4ServiceState SyncType string StartTime time.Time + ResourceUpdates ResourceUpdates } func NewL4ILBSyncResult(syncType string, startTime time.Time, svc *corev1.Service, isMultinetService bool, isWeightedLBPodsPerNode bool) *L4ILBSyncResult { @@ -497,7 +498,8 @@ func (l4 *L4) EnsureInternalLoadBalancer(nodeNames []string, svc *corev1.Service ConnectionTrackingPolicy: noConnectionTrackingPolicy, LocalityLbPolicy: localityLbPolicy, } - bs, _, err := l4.backendPool.EnsureL4BackendService(backendParams, l4.svcLogger) + bs, bsSyncStatus, err := l4.backendPool.EnsureL4BackendService(backendParams, l4.svcLogger) + result.ResourceUpdates.SetBackendService(bsSyncStatus) if err != nil { if utils.IsUnsupportedFeatureError(err, string(backends.LocalityLBPolicyMaglev)) { result.GCEResourceInError = annotations.BackendServiceResource @@ -564,6 +566,8 @@ func (l4 *L4) provideDualStackHealthChecks(nodeNames []string, result *L4ILBSync func (l4 *L4) provideIPv4HealthChecks(nodeNames []string, result *L4ILBSyncResult) string { sharedHC := !helpers.RequestsOnlyLocalTraffic(l4.Service) hcResult := l4.healthChecks.EnsureHealthCheckWithFirewall(l4.Service, l4.namer, sharedHC, meta.Global, utils.ILB, nodeNames, l4.network, l4.svcLogger) + result.ResourceUpdates.SetHealthCheck(hcResult.WasUpdated) + result.ResourceUpdates.SetFirewallForHealthCheck(hcResult.WasFirewallUpdated) if hcResult.Err != nil { result.GCEResourceInError = hcResult.GceResourceInError result.Error = hcResult.Err @@ -591,7 +595,8 @@ func (l4 *L4) ensureDualStackResources(result *L4ILBSyncResult, nodeNames []stri // - IPv4 Forwarding Rule // - IPv4 Firewall func (l4 *L4) ensureIPv4Resources(result *L4ILBSyncResult, nodeNames []string, options gce.ILBOptions, bs *composite.BackendService, existingFR *composite.ForwardingRule, subnetworkURL, ipToUse string) { - fr, err := l4.ensureIPv4ForwardingRule(bs.SelfLink, options, existingFR, subnetworkURL, ipToUse) + fr, fwdRuleSyncStatus, err := l4.ensureIPv4ForwardingRule(bs.SelfLink, options, existingFR, subnetworkURL, ipToUse) + result.ResourceUpdates.SetForwardingRule(fwdRuleSyncStatus) if err != nil { l4.svcLogger.Error(err, "ensureIPv4Resources: Failed to ensure forwarding rule for L4 ILB Service") result.GCEResourceInError = annotations.ForwardingRuleResource @@ -651,7 +656,8 @@ func (l4 *L4) ensureIPv4NodesFirewall(nodeNames []string, ipAddress string, resu Network: l4.network, } - _, err = firewalls.EnsureL4LBFirewallForNodes(l4.Service, &nodesFWRParams, l4.cloud, l4.recorder, fwLogger) + fwSyncStatus, err := firewalls.EnsureL4LBFirewallForNodes(l4.Service, &nodesFWRParams, l4.cloud, l4.recorder, fwLogger) + result.ResourceUpdates.SetFirewallForNodes(fwSyncStatus) if err != nil { result.GCEResourceInError = annotations.FirewallRuleResource result.Error = err diff --git a/pkg/loadbalancers/l4ipv6.go b/pkg/loadbalancers/l4ipv6.go index ccc2ba62d..e8c057f5c 100644 --- a/pkg/loadbalancers/l4ipv6.go +++ b/pkg/loadbalancers/l4ipv6.go @@ -34,7 +34,8 @@ import ( // - IPv6 Firewall // it also adds IPv6 address to LB status func (l4 *L4) ensureIPv6Resources(syncResult *L4ILBSyncResult, nodeNames []string, options gce.ILBOptions, bsLink string, existingIPv6FwdRule *composite.ForwardingRule, ipv6AddressToUse string) { - ipv6fr, err := l4.ensureIPv6ForwardingRule(bsLink, options, existingIPv6FwdRule, ipv6AddressToUse) + ipv6fr, fwdRuleSyncStatus, err := l4.ensureIPv6ForwardingRule(bsLink, options, existingIPv6FwdRule, ipv6AddressToUse) + syncResult.ResourceUpdates.SetForwardingRule(fwdRuleSyncStatus) if err != nil { l4.svcLogger.Error(err, "ensureIPv6Resources: Failed to ensure ipv6 forwarding rule") syncResult.GCEResourceInError = annotations.ForwardingRuleIPv6Resource @@ -156,7 +157,8 @@ func (l4 *L4) ensureIPv6NodesFirewall(ipAddress string, nodeNames []string, resu Network: l4.network, } - _, err = firewalls.EnsureL4LBFirewallForNodes(l4.Service, &ipv6nodesFWRParams, l4.cloud, l4.recorder, fwLogger) + fwSyncStatus, err := firewalls.EnsureL4LBFirewallForNodes(l4.Service, &ipv6nodesFWRParams, l4.cloud, l4.recorder, fwLogger) + result.ResourceUpdates.SetFirewallForNodes(fwSyncStatus) if err != nil { result.GCEResourceInError = annotations.FirewallRuleIPv6Resource result.Error = err diff --git a/pkg/loadbalancers/l4netlb.go b/pkg/loadbalancers/l4netlb.go index dcefffa40..b3945d32e 100644 --- a/pkg/loadbalancers/l4netlb.go +++ b/pkg/loadbalancers/l4netlb.go @@ -18,7 +18,6 @@ package loadbalancers import ( "fmt" - "strings" "time" "github.com/GoogleCloudPlatform/k8s-cloud-provider/pkg/cloud" @@ -90,45 +89,6 @@ type L4NetLBSyncResult struct { GCEResourceUpdate ResourceUpdates } -type ResourceUpdates struct { - backendServiceUpdate utils.ResourceSyncStatus - forwardingRuleUpdate utils.ResourceSyncStatus - healthCheckUpdate utils.ResourceSyncStatus - firewallForNodesUpdate utils.ResourceSyncStatus - firewallForHCUpdate utils.ResourceSyncStatus -} - -func (ru *ResourceUpdates) WereAnyResourcesModified() bool { - return ru.forwardingRuleUpdate == utils.ResourceUpdate || - ru.backendServiceUpdate == utils.ResourceUpdate || - ru.healthCheckUpdate == utils.ResourceUpdate || - ru.firewallForNodesUpdate == utils.ResourceUpdate || - ru.firewallForHCUpdate == utils.ResourceUpdate -} - -func (ru *ResourceUpdates) String() string { - if ru.WereAnyResourcesModified() { - var modifiedResources []string - if ru.forwardingRuleUpdate == utils.ResourceUpdate { - modifiedResources = append(modifiedResources, "forwarding rule") - } - if ru.backendServiceUpdate == utils.ResourceUpdate { - modifiedResources = append(modifiedResources, "backend service") - } - if ru.healthCheckUpdate == utils.ResourceUpdate { - modifiedResources = append(modifiedResources, "health check") - } - if ru.firewallForNodesUpdate == utils.ResourceUpdate { - modifiedResources = append(modifiedResources, "nodes firewall") - } - if ru.firewallForHCUpdate == utils.ResourceUpdate { - modifiedResources = append(modifiedResources, "health check firewall") - } - return strings.Join(modifiedResources, ",") - } - return "-" -} - func NewL4SyncResult(syncType string, svc *corev1.Service, isMultinet bool, enabledStrongSessionAffinity bool, isWeightedLBPodsPerNode bool) *L4NetLBSyncResult { startTime := time.Now() result := &L4NetLBSyncResult{ @@ -310,8 +270,8 @@ func (l4netlb *L4NetLB) provideHealthChecks(nodeNames []string, result *L4NetLBS func (l4netlb *L4NetLB) provideDualStackHealthChecks(nodeNames []string, result *L4NetLBSyncResult) string { sharedHC := !helpers.RequestsOnlyLocalTraffic(l4netlb.Service) hcResult := l4netlb.healthChecks.EnsureHealthCheckWithDualStackFirewalls(l4netlb.Service, l4netlb.namer, sharedHC, l4netlb.scope, utils.XLB, nodeNames, utils.NeedsIPv4(l4netlb.Service), utils.NeedsIPv6(l4netlb.Service), l4netlb.networkInfo, l4netlb.svcLogger) - result.GCEResourceUpdate.healthCheckUpdate = hcResult.WasUpdated - result.GCEResourceUpdate.firewallForHCUpdate = hcResult.WasFirewallUpdated + result.GCEResourceUpdate.SetHealthCheck(hcResult.WasUpdated) + result.GCEResourceUpdate.SetFirewallForHealthCheck(hcResult.WasFirewallUpdated) if hcResult.Err != nil { result.GCEResourceInError = hcResult.GceResourceInError result.Error = hcResult.Err @@ -331,8 +291,8 @@ func (l4netlb *L4NetLB) provideDualStackHealthChecks(nodeNames []string, result func (l4netlb *L4NetLB) provideIPv4HealthChecks(nodeNames []string, result *L4NetLBSyncResult) string { sharedHC := !helpers.RequestsOnlyLocalTraffic(l4netlb.Service) hcResult := l4netlb.healthChecks.EnsureHealthCheckWithFirewall(l4netlb.Service, l4netlb.namer, sharedHC, l4netlb.scope, utils.XLB, nodeNames, l4netlb.networkInfo, l4netlb.svcLogger) - result.GCEResourceUpdate.healthCheckUpdate = hcResult.WasUpdated - result.GCEResourceUpdate.firewallForHCUpdate = hcResult.WasFirewallUpdated + result.GCEResourceUpdate.SetHealthCheck(hcResult.WasUpdated) + result.GCEResourceUpdate.SetFirewallForHealthCheck(hcResult.WasFirewallUpdated) if hcResult.Err != nil { result.GCEResourceInError = hcResult.GceResourceInError result.Error = hcResult.Err @@ -377,7 +337,7 @@ func (l4netlb *L4NetLB) provideBackendService(syncResult *L4NetLBSyncResult, hcL } bs, wasUpdate, err := l4netlb.backendPool.EnsureL4BackendService(backendParams, l4netlb.svcLogger) - syncResult.GCEResourceUpdate.backendServiceUpdate = wasUpdate + syncResult.GCEResourceUpdate.SetBackendService(wasUpdate) if err != nil { if utils.IsUnsupportedFeatureError(err, strongSessionAffinityFeatureName) { syncResult.GCEResourceInError = annotations.BackendServiceResource @@ -413,7 +373,7 @@ func (l4netlb *L4NetLB) ensureDualStackResources(result *L4NetLBSyncResult, node // - IPv4 Firewall func (l4netlb *L4NetLB) ensureIPv4Resources(result *L4NetLBSyncResult, nodeNames []string, bsLink string) { fr, ipAddrType, wasUpdate, err := l4netlb.ensureIPv4ForwardingRule(bsLink) - result.GCEResourceUpdate.forwardingRuleUpdate = wasUpdate + result.GCEResourceUpdate.SetForwardingRule(wasUpdate) if err != nil { // User can misconfigure the forwarding rule if Network Tier will not match service level Network Tier. result.GCEResourceInError = annotations.ForwardingRuleResource @@ -475,7 +435,9 @@ func (l4netlb *L4NetLB) ensureIPv4NodesFirewall(nodeNames []string, ipAddress st NodeNames: nodeNames, Network: l4netlb.networkInfo, } - result.GCEResourceUpdate.firewallForNodesUpdate, result.Error = firewalls.EnsureL4LBFirewallForNodes(l4netlb.Service, &nodesFWRParams, l4netlb.cloud, l4netlb.recorder, fwLogger) + var firewallForNodesUpdateStatus utils.ResourceSyncStatus + firewallForNodesUpdateStatus, result.Error = firewalls.EnsureL4LBFirewallForNodes(l4netlb.Service, &nodesFWRParams, l4netlb.cloud, l4netlb.recorder, fwLogger) + result.GCEResourceUpdate.SetFirewallForNodes(firewallForNodesUpdateStatus) if result.Error != nil { result.GCEResourceInError = annotations.FirewallRuleResource result.Error = err diff --git a/pkg/loadbalancers/l4netlbipv6.go b/pkg/loadbalancers/l4netlbipv6.go index a991677f4..1b2f8bbc8 100644 --- a/pkg/loadbalancers/l4netlbipv6.go +++ b/pkg/loadbalancers/l4netlbipv6.go @@ -41,7 +41,7 @@ const ( // it also adds IPv6 address to LB status func (l4netlb *L4NetLB) ensureIPv6Resources(syncResult *L4NetLBSyncResult, nodeNames []string, bsLink string) { ipv6fr, wasUpdate, err := l4netlb.ensureIPv6ForwardingRule(bsLink) - syncResult.GCEResourceUpdate.forwardingRuleUpdate = wasUpdate + syncResult.GCEResourceUpdate.SetForwardingRule(wasUpdate) if err != nil { l4netlb.svcLogger.Error(err, "ensureIPv6Resources: Failed to create ipv6 forwarding rule") syncResult.GCEResourceInError = annotations.ForwardingRuleIPv6Resource @@ -145,7 +145,7 @@ func (l4netlb *L4NetLB) ensureIPv6NodesFirewall(ipAddress string, nodeNames []st } wasUpdate, err := firewalls.EnsureL4LBFirewallForNodes(l4netlb.Service, &ipv6nodesFWRParams, l4netlb.cloud, l4netlb.recorder, fwLogger) - syncResult.GCEResourceUpdate.firewallForNodesUpdate = wasUpdate + syncResult.GCEResourceUpdate.SetFirewallForNodes(wasUpdate) if err != nil { fwLogger.Error(err, "Failed to ensure ipv6 nodes firewall for L4 NetLB") syncResult.GCEResourceInError = annotations.FirewallRuleIPv6Resource diff --git a/pkg/loadbalancers/resourceupdate.go b/pkg/loadbalancers/resourceupdate.go new file mode 100644 index 000000000..3ac22e68b --- /dev/null +++ b/pkg/loadbalancers/resourceupdate.go @@ -0,0 +1,95 @@ +package loadbalancers + +import ( + "strings" + + "k8s.io/ingress-gce/pkg/utils" +) + +// ResourceUpdates tracks the updates to the GCE resources that were done during ensuring the LB. +// Ensuring of a resource follows this pattern: +// - get the existing resource +// - compare it with the expected state +// - if the resource is already in the expected state - do nothing +// - if the resource differs perform an update +// This struct will track if nothing was done (resync) or if an update was performed. +// It usually should be added to the SyncResult struct of L4 controllers and updated +// with sync results of GCE resources ensure operations. +// It is part of the effort to add more transparency to what the controller +// is doing and also to detect situations where resources are unexpectedly updated. +type ResourceUpdates struct { + backendServiceUpdate utils.ResourceSyncStatus + forwardingRuleUpdate utils.ResourceSyncStatus + healthCheckUpdate utils.ResourceSyncStatus + firewallForNodesUpdate utils.ResourceSyncStatus + firewallForHCUpdate utils.ResourceSyncStatus +} + +// WereAnyResourcesModified returns true if any of the LB resources were updated. +func (ru *ResourceUpdates) WereAnyResourcesModified() bool { + return ru.forwardingRuleUpdate == utils.ResourceUpdate || + ru.backendServiceUpdate == utils.ResourceUpdate || + ru.healthCheckUpdate == utils.ResourceUpdate || + ru.firewallForNodesUpdate == utils.ResourceUpdate || + ru.firewallForHCUpdate == utils.ResourceUpdate +} + +func (ru *ResourceUpdates) String() string { + if ru.WereAnyResourcesModified() { + var modifiedResources []string + if ru.forwardingRuleUpdate == utils.ResourceUpdate { + modifiedResources = append(modifiedResources, "forwarding rule") + } + if ru.backendServiceUpdate == utils.ResourceUpdate { + modifiedResources = append(modifiedResources, "backend service") + } + if ru.healthCheckUpdate == utils.ResourceUpdate { + modifiedResources = append(modifiedResources, "health check") + } + if ru.firewallForNodesUpdate == utils.ResourceUpdate { + modifiedResources = append(modifiedResources, "nodes firewall") + } + if ru.firewallForHCUpdate == utils.ResourceUpdate { + modifiedResources = append(modifiedResources, "health check firewall") + } + return strings.Join(modifiedResources, ",") + } + return "-" +} + +func (ru *ResourceUpdates) set(field *utils.ResourceSyncStatus, new utils.ResourceSyncStatus) { + if *field == utils.ResourceUpdate { + return + } + *field = new +} + +// SetBackendService sets the status of the Backend Service update. +// When this function is invoked multiple times with at least one UPDATE status then the result will be UPDATE. +func (ru *ResourceUpdates) SetBackendService(status utils.ResourceSyncStatus) { + ru.set(&ru.backendServiceUpdate, status) +} + +// SetForwardingRule sets the status of the Forwarding Rule update. +// When this function is invoked multiple times with at least one UPDATE status then the result will be UPDATE. +func (ru *ResourceUpdates) SetForwardingRule(status utils.ResourceSyncStatus) { + ru.set(&ru.forwardingRuleUpdate, status) +} + +// SetHealthCheck sets the status of the Health Check update. +// When this function is invoked multiple times with at least one UPDATE status then the result will be UPDATE. +func (ru *ResourceUpdates) SetHealthCheck(status utils.ResourceSyncStatus) { + ru.set(&ru.healthCheckUpdate, status) +} + +// SetFirewallForNodes sets the status of the Firewall for nodes update. +// When this function is invoked multiple times with at least one UPDATE status then the result will be UPDATE. +func (ru *ResourceUpdates) SetFirewallForNodes(status utils.ResourceSyncStatus) { + ru.set(&ru.firewallForNodesUpdate, status) +} + +// SetFirewallForHealthCheck sets the status of the Firewall for Health Check update. +// When this function is invoked multiple times with at least one UPDATE status then the result will be UPDATE. +func (ru *ResourceUpdates) SetFirewallForHealthCheck(status utils.ResourceSyncStatus) { + ru.set(&ru.firewallForHCUpdate, status) +}