From c3d53f1bab6e50ac14364c5e32ecb1ceb8d2f89b Mon Sep 17 00:00:00 2001 From: Slavik Panasovets Date: Mon, 19 Sep 2022 23:19:59 +0000 Subject: [PATCH 1/4] Add Dual-Stack support to L4 NetLB --- cmd/glbc/main.go | 27 +- pkg/context/context.go | 19 +- pkg/flags/flags.go | 2 + pkg/healthchecksl4/healthchecksl4.go | 14 +- pkg/l4lb/l4lbcommon.go | 10 + pkg/l4lb/l4netlbcontroller.go | 80 ++- pkg/l4lb/l4netlbcontroller_test.go | 166 ++++- pkg/loadbalancers/forwarding_rules.go | 35 +- pkg/loadbalancers/forwarding_rules_ipv6.go | 73 ++ pkg/loadbalancers/forwarding_rules_test.go | 2 +- pkg/loadbalancers/l4_test.go | 3 +- pkg/loadbalancers/l4ipv6.go | 1 - pkg/loadbalancers/l4netlb.go | 334 ++++++--- pkg/loadbalancers/l4netlb_test.go | 785 +++++++++++++++++---- pkg/loadbalancers/l4netlbipv6.go | 176 +++++ pkg/loadbalancers/l4netlbipv6_test.go | 61 ++ pkg/loadbalancers/l4syncresult.go | 1 + pkg/test/utils.go | 12 + pkg/utils/namer/l4_namer.go | 8 +- 19 files changed, 1501 insertions(+), 308 deletions(-) create mode 100644 pkg/loadbalancers/l4netlbipv6.go create mode 100644 pkg/loadbalancers/l4netlbipv6_test.go diff --git a/cmd/glbc/main.go b/cmd/glbc/main.go index fc221d2fed..0e6634fa0c 100644 --- a/cmd/glbc/main.go +++ b/cmd/glbc/main.go @@ -188,19 +188,20 @@ func main() { cloud := app.NewGCEClient() defaultBackendServicePort := app.DefaultBackendServicePort(kubeClient) ctxConfig := ingctx.ControllerContextConfig{ - Namespace: flags.F.WatchNamespace, - ResyncPeriod: flags.F.ResyncPeriod, - NumL4Workers: flags.F.NumL4Workers, - NumL4NetLBWorkers: flags.F.NumL4NetLBWorkers, - DefaultBackendSvcPort: defaultBackendServicePort, - HealthCheckPath: flags.F.HealthCheckPath, - FrontendConfigEnabled: flags.F.EnableFrontendConfig, - EnableASMConfigMap: flags.F.EnableASMConfigMapBasedConfig, - ASMConfigMapNamespace: flags.F.ASMConfigMapBasedConfigNamespace, - ASMConfigMapName: flags.F.ASMConfigMapBasedConfigCMName, - EndpointSlicesEnabled: flags.F.EnableEndpointSlices, - MaxIGSize: flags.F.MaxIGSize, - EnableL4ILBDualStack: flags.F.EnableL4ILBDualStack, + Namespace: flags.F.WatchNamespace, + ResyncPeriod: flags.F.ResyncPeriod, + NumL4Workers: flags.F.NumL4Workers, + NumL4NetLBWorkers: flags.F.NumL4NetLBWorkers, + DefaultBackendSvcPort: defaultBackendServicePort, + HealthCheckPath: flags.F.HealthCheckPath, + FrontendConfigEnabled: flags.F.EnableFrontendConfig, + EnableASMConfigMap: flags.F.EnableASMConfigMapBasedConfig, + ASMConfigMapNamespace: flags.F.ASMConfigMapBasedConfigNamespace, + ASMConfigMapName: flags.F.ASMConfigMapBasedConfigCMName, + EndpointSlicesEnabled: flags.F.EnableEndpointSlices, + MaxIGSize: flags.F.MaxIGSize, + EnableL4ILBDualStack: flags.F.EnableL4ILBDualStack, + EnableL4NetLBDualStack: flags.F.EnableL4NetLBDualStack, } ctx := ingctx.NewControllerContext(kubeConfig, kubeClient, backendConfigClient, frontendConfigClient, svcNegClient, ingParamsClient, svcAttachmentClient, cloud, namer, kubeSystemUID, ctxConfig) go app.RunHTTPServer(ctx.HealthCheck) diff --git a/pkg/context/context.go b/pkg/context/context.go index c46288c5c2..225edce1af 100644 --- a/pkg/context/context.go +++ b/pkg/context/context.go @@ -118,15 +118,16 @@ type ControllerContextConfig struct { NumL4Workers int NumL4NetLBWorkers int // DefaultBackendSvcPortID is the ServicePort for the system default backend. - DefaultBackendSvcPort utils.ServicePort - HealthCheckPath string - FrontendConfigEnabled bool - EnableASMConfigMap bool - ASMConfigMapNamespace string - ASMConfigMapName string - EndpointSlicesEnabled bool - MaxIGSize int - EnableL4ILBDualStack bool + DefaultBackendSvcPort utils.ServicePort + HealthCheckPath string + FrontendConfigEnabled bool + EnableASMConfigMap bool + ASMConfigMapNamespace string + ASMConfigMapName string + EndpointSlicesEnabled bool + MaxIGSize int + EnableL4ILBDualStack bool + EnableL4NetLBDualStack bool } // NewControllerContext returns a new shared set of informers. diff --git a/pkg/flags/flags.go b/pkg/flags/flags.go index 5c1e340831..d3ffa8a46a 100644 --- a/pkg/flags/flags.go +++ b/pkg/flags/flags.go @@ -110,6 +110,7 @@ var ( EnableEndpointSlices bool EnablePinhole bool EnableL4ILBDualStack bool + EnableL4NetLBDualStack bool EnableMultipleIGs bool MaxIGSize int }{ @@ -254,6 +255,7 @@ L7 load balancing. CSV values accepted. Example: -node-port-ranges=80,8080,400-5 flag.BoolVar(&F.EnableEndpointSlices, "enable-endpoint-slices", false, "Enable using Endpoint Slices API instead of Endpoints API") flag.BoolVar(&F.EnablePinhole, "enable-pinhole", false, "Enable Pinhole firewall feature") flag.BoolVar(&F.EnableL4ILBDualStack, "enable-l4ilb-dual-stack", false, "Enable Dual-Stack handling for L4 Internal Load Balancers") + flag.BoolVar(&F.EnableL4NetLBDualStack, "enable-l4netlb-dual-stack", false, "Enable Dual-Stack handling for L4 External Load Balancers") flag.BoolVar(&F.EnableMultipleIGs, "enable-multiple-igs", false, "Enable using multiple unmanaged instance groups") flag.IntVar(&F.MaxIGSize, "max-ig-size", 1000, "Max number of instances in Instance Group") flag.DurationVar(&F.MetricsExportInterval, "metrics-export-interval", 10*time.Minute, `Period for calculating and exporting metrics related to state of managed objects.`) diff --git a/pkg/healthchecksl4/healthchecksl4.go b/pkg/healthchecksl4/healthchecksl4.go index 2084e7a952..176ef1e7a3 100644 --- a/pkg/healthchecksl4/healthchecksl4.go +++ b/pkg/healthchecksl4/healthchecksl4.go @@ -47,6 +47,7 @@ const ( gceSharedHcUnhealthyThreshold = int64(3) // 3 * 8 = 24 seconds before the LB will steer traffic away gceLocalHcUnhealthyThreshold = int64(2) // 2 * 3 = 6 seconds before the LB will steer traffic away L4ILBIPv6HCRange = "2600:2d00:1:b029::/64" + L4NetLBIPv6HCRange = "2600:1901:8001::/48" ) var ( @@ -150,7 +151,7 @@ func (l4hc *l4HealthChecks) EnsureHealthCheckWithDualStackFirewalls(svc *corev1. if needsIPv6 { klog.V(3).Infof("Ensuring IPv6 firewall rule for health check %s for service %s", hcName, namespacedName.String()) - l4hc.ensureIPv6Firewall(svc, namer, hcPort, sharedHC, nodeNames, hcResult) + l4hc.ensureIPv6Firewall(svc, namer, hcPort, sharedHC, nodeNames, l4Type, hcResult) } return hcResult @@ -232,7 +233,7 @@ func (l4hc *l4HealthChecks) ensureIPv4Firewall(svc *corev1.Service, namer namer. hcResult.HCFirewallRuleName = hcFwName } -func (l4hc *l4HealthChecks) ensureIPv6Firewall(svc *corev1.Service, namer namer.L4ResourcesNamer, hcPort int32, isSharedHC bool, nodeNames []string, hcResult *EnsureHealthCheckResult) { +func (l4hc *l4HealthChecks) ensureIPv6Firewall(svc *corev1.Service, namer namer.L4ResourcesNamer, hcPort int32, isSharedHC bool, nodeNames []string, l4Type utils.L4LBType, hcResult *EnsureHealthCheckResult) { ipv6HCFWName := namer.L4IPv6HealthCheckFirewall(svc.Namespace, svc.Name, isSharedHC) start := time.Now() @@ -243,7 +244,7 @@ func (l4hc *l4HealthChecks) ensureIPv6Firewall(svc *corev1.Service, namer namer. hcFWRParams := firewalls.FirewallParams{ PortRanges: []string{strconv.Itoa(int(hcPort))}, - SourceRanges: []string{L4ILBIPv6HCRange}, + SourceRanges: getIPv6HCFirewallSourceRanges(l4Type), Protocol: string(corev1.ProtocolTCP), Name: ipv6HCFWName, NodeNames: nodeNames, @@ -463,3 +464,10 @@ func needToUpdateHealthChecks(hc, newHC *composite.HealthCheck) bool { hc.UnhealthyThreshold < newHC.UnhealthyThreshold || hc.HealthyThreshold < newHC.HealthyThreshold } + +func getIPv6HCFirewallSourceRanges(l4Type utils.L4LBType) []string { + if l4Type == utils.XLB { + return []string{L4NetLBIPv6HCRange} + } + return []string{L4ILBIPv6HCRange} +} diff --git a/pkg/l4lb/l4lbcommon.go b/pkg/l4lb/l4lbcommon.go index 0a1b93abfd..fbd3b9e859 100644 --- a/pkg/l4lb/l4lbcommon.go +++ b/pkg/l4lb/l4lbcommon.go @@ -95,6 +95,16 @@ func deleteL4RBSAnnotations(ctx *context.ControllerContext, svc *v1.Service) err return patch.PatchServiceObjectMetadata(ctx.KubeClient.CoreV1(), svc, *newObjectMeta) } +// deleteL4RBSDualStackAnnotations deletes all annotations which could be added by L4 ELB RBS controller +func deleteL4RBSDualStackAnnotations(ctx *context.ControllerContext, svc *v1.Service) error { + newObjectMeta := computeNewAnnotationsIfNeeded(svc, nil, loadbalancers.L4DualStackResourceRBSAnnotationKeys) + if newObjectMeta == nil { + return nil + } + klog.V(3).Infof("Deleting all DualStack annotations for L4 ELB RBS service %v/%v", svc.Namespace, svc.Name) + return patch.PatchServiceObjectMetadata(ctx.KubeClient.CoreV1(), svc, *newObjectMeta) +} + func deleteAnnotation(ctx *context.ControllerContext, svc *v1.Service, annotationKey string) error { newObjectMeta := svc.ObjectMeta.DeepCopy() if _, ok := newObjectMeta.Annotations[annotationKey]; !ok { diff --git a/pkg/l4lb/l4netlbcontroller.go b/pkg/l4lb/l4netlbcontroller.go index a7a769b946..89af3ad505 100644 --- a/pkg/l4lb/l4netlbcontroller.go +++ b/pkg/l4lb/l4netlbcontroller.go @@ -19,6 +19,7 @@ package l4lb import ( "fmt" "reflect" + "strings" "time" "github.com/GoogleCloudPlatform/k8s-cloud-provider/pkg/cloud" @@ -61,6 +62,7 @@ type L4NetLBController struct { instancePool instancegroups.Manager igLinker *backends.RegionalInstanceGroupLinker forwardingRules ForwardingRulesGetter + enableDualStack bool } // NewL4NetLBController creates a controller for l4 external loadbalancer. @@ -84,6 +86,7 @@ func NewL4NetLBController( instancePool: ctx.InstancePool, igLinker: backends.NewRegionalInstanceGroupLinker(ctx.InstancePool, backendPool), forwardingRules: forwardingrules.New(ctx.Cloud, meta.VersionGA, meta.Regional), + enableDualStack: ctx.EnableL4NetLBDualStack, } l4netLBc.svcQueue = utils.NewPeriodicTaskQueueWithMultipleWorkers("l4netLB", "services", ctx.NumL4NetLBWorkers, l4netLBc.sync) @@ -213,6 +216,11 @@ func (lc *L4NetLBController) needsUpdate(newSvc, oldSvc *v1.Service) bool { oldSvc.Spec.HealthCheckNodePort, newSvc.Spec.HealthCheckNodePort) return true } + if lc.enableDualStack && !reflect.DeepEqual(oldSvc.Spec.IPFamilies, newSvc.Spec.IPFamilies) { + recorder.Eventf(newSvc, v1.EventTypeNormal, "IPFamilies", "%v -> %v", + oldSvc.Spec.IPFamilies, newSvc.Spec.IPFamilies) + return true + } return false } @@ -417,10 +425,11 @@ func (lc *L4NetLBController) sync(key string) error { // Returns an error if processing the service update failed. func (lc *L4NetLBController) syncInternal(service *v1.Service) *loadbalancers.L4NetLBSyncResult { l4NetLBParams := &loadbalancers.L4NetLBParams{ - Service: service, - Cloud: lc.ctx.Cloud, - Namer: lc.namer, - Recorder: lc.ctx.Recorder(service.Namespace), + Service: service, + Cloud: lc.ctx.Cloud, + Namer: lc.namer, + Recorder: lc.ctx.Recorder(service.Namespace), + DualStackEnabled: lc.enableDualStack, } l4netlb := loadbalancers.NewL4NetLB(l4NetLBParams) // check again that rbs is enabled. @@ -473,18 +482,39 @@ func (lc *L4NetLBController) syncInternal(service *v1.Service) *loadbalancers.L4 syncResult.Error = err return syncResult } - lc.ctx.Recorder(service.Namespace).Eventf(service, v1.EventTypeNormal, "SyncLoadBalancerSuccessful", - "Successfully ensured L4 External LoadBalancer resources") - if err = updateL4ResourcesAnnotations(lc.ctx, service, syncResult.Annotations); err != nil { - lc.ctx.Recorder(service.Namespace).Eventf(service, v1.EventTypeWarning, "SyncExternalLoadBalancerFailed", - "Failed to update annotations for load balancer, err: %v", err) - syncResult.Error = fmt.Errorf("failed to set resource annotations, err: %w", err) - return syncResult + if lc.enableDualStack { + lc.emitEnsuredDualStackEvent(service) + + if err = updateL4DualStackResourcesAnnotations(lc.ctx, service, syncResult.Annotations); err != nil { + lc.ctx.Recorder(service.Namespace).Eventf(service, v1.EventTypeWarning, "SyncExternalLoadBalancerFailed", + "Failed to update annotations for load balancer, err: %v", err) + syncResult.Error = fmt.Errorf("failed to set resource annotations, err: %w", err) + return syncResult + } + } else { + lc.ctx.Recorder(service.Namespace).Eventf(service, v1.EventTypeNormal, "SyncLoadBalancerSuccessful", + "Successfully ensured L4 External LoadBalancer resources") + + if err = updateL4ResourcesAnnotations(lc.ctx, service, syncResult.Annotations); err != nil { + lc.ctx.Recorder(service.Namespace).Eventf(service, v1.EventTypeWarning, "SyncExternalLoadBalancerFailed", + "Failed to update annotations for load balancer, err: %v", err) + syncResult.Error = fmt.Errorf("failed to set resource annotations, err: %w", err) + return syncResult + } } syncResult.SetMetricsForSuccessfulServiceSync() return syncResult } +func (lc *L4NetLBController) emitEnsuredDualStackEvent(service *v1.Service) { + var ipFamilies []string + for _, ipFamily := range service.Spec.IPFamilies { + ipFamilies = append(ipFamilies, string(ipFamily)) + } + lc.ctx.Recorder(service.Namespace).Eventf(service, v1.EventTypeNormal, "SyncLoadBalancerSuccessful", + "Successfully ensured %v External LoadBalancer resources", strings.Join(ipFamilies, " ")) +} + func (lc *L4NetLBController) ensureBackendLinking(service *v1.Service) error { start := time.Now() klog.V(2).Infof("Linking backend service with instance groups for service %s/%s", service.Namespace, service.Name) @@ -537,10 +567,11 @@ func (lc *L4NetLBController) garbageCollectRBSNetLB(key string, svc *v1.Service) }() l4NetLBParams := &loadbalancers.L4NetLBParams{ - Service: svc, - Cloud: lc.ctx.Cloud, - Namer: lc.namer, - Recorder: lc.ctx.Recorder(svc.Namespace), + Service: svc, + Cloud: lc.ctx.Cloud, + Namer: lc.namer, + Recorder: lc.ctx.Recorder(svc.Namespace), + DualStackEnabled: lc.enableDualStack, } l4netLB := loadbalancers.NewL4NetLB(l4NetLBParams) lc.ctx.Recorder(svc.Namespace).Eventf(svc, v1.EventTypeNormal, "DeletingLoadBalancer", @@ -577,11 +608,20 @@ func (lc *L4NetLBController) garbageCollectRBSNetLB(key string, svc *v1.Service) } // IMPORTANT: Remove LB annotations from the Service LAST, cause changing service fields are emitted as service update to other controllers - if err := deleteL4RBSAnnotations(lc.ctx, svc); err != nil { - lc.ctx.Recorder(svc.Namespace).Eventf(svc, v1.EventTypeWarning, "DeleteLoadBalancer", - "Error removing resource annotations: %v: %v", err) - result.Error = fmt.Errorf("Failed to reset resource annotations, err: %w", err) - return result + if lc.enableDualStack { + if err := deleteL4RBSDualStackAnnotations(lc.ctx, svc); err != nil { + lc.ctx.Recorder(svc.Namespace).Eventf(svc, v1.EventTypeWarning, "DeleteLoadBalancer", + "Error removing Dual Stack resource annotations: %v: %v", err) + result.Error = fmt.Errorf("failed to reset Dual Stack resource annotations, err: %w", err) + return result + } + } else { + if err := deleteL4RBSAnnotations(lc.ctx, svc); err != nil { + lc.ctx.Recorder(svc.Namespace).Eventf(svc, v1.EventTypeWarning, "DeleteLoadBalancer", + "Error removing resource annotations: %v: %v", err) + result.Error = fmt.Errorf("failed to reset resource annotations, err: %w", err) + return result + } } lc.ctx.Recorder(svc.Namespace).Eventf(svc, v1.EventTypeNormal, "DeletedLoadBalancer", "Deleted L4 External LoadBalancer") diff --git a/pkg/l4lb/l4netlbcontroller_test.go b/pkg/l4lb/l4netlbcontroller_test.go index fd803453a6..ceb0f24a9f 100644 --- a/pkg/l4lb/l4netlbcontroller_test.go +++ b/pkg/l4lb/l4netlbcontroller_test.go @@ -62,13 +62,20 @@ const ( ) var ( - serviceAnnotationKeys = []string{ - annotations.FirewallRuleKey, + netLBCommonAnnotationKeys = []string{ annotations.BackendServiceKey, annotations.HealthcheckKey, + } + netLBIPv4AnnotationKeys = []string{ + annotations.FirewallRuleKey, annotations.TCPForwardingRuleKey, annotations.FirewallRuleForHealthcheckKey, } + netLBIPv6AnnotationKeys = []string{ + annotations.FirewallRuleIPv6Key, + annotations.TCPForwardingRuleIPv6Key, + annotations.FirewallRuleForHealthcheckIPv6Key, + } ) func getExternalIPS() []string { @@ -155,7 +162,6 @@ func createAndSyncLegacyNetLBSvc(t *testing.T) (svc *v1.Service, lc *L4NetLBCont } func checkBackendService(lc *L4NetLBController, svc *v1.Service) error { - backendServiceLink, bs, err := getBackend(lc, svc) if err != nil { return fmt.Errorf("Failed to fetch backend service, err %v", err) @@ -249,9 +255,22 @@ func validateNetLBSvcStatus(svc *v1.Service, t *testing.T) { } } +func calculateNetLBExpectedAnnotationsKeys(svc *v1.Service) []string { + expectedAnnotations := netLBCommonAnnotationKeys + if utils.NeedsIPv4(svc) { + expectedAnnotations = append(expectedAnnotations, netLBIPv4AnnotationKeys...) + } + if utils.NeedsIPv6(svc) { + expectedAnnotations = append(expectedAnnotations, netLBIPv6AnnotationKeys...) + } + return expectedAnnotations +} + func validateAnnotations(svc *v1.Service) error { - missingKeys := []string{} - for _, key := range serviceAnnotationKeys { + expectedAnnotationsKeys := calculateNetLBExpectedAnnotationsKeys(svc) + + var missingKeys []string + for _, key := range expectedAnnotationsKeys { if _, ok := svc.Annotations[key]; !ok { missingKeys = append(missingKeys, key) } @@ -263,8 +282,10 @@ func validateAnnotations(svc *v1.Service) error { } func validateAnnotationsDeleted(svc *v1.Service) error { - unexpectedKeys := []string{} - for _, key := range serviceAnnotationKeys { + expectedAnnotationsKeys := calculateNetLBExpectedAnnotationsKeys(svc) + + var unexpectedKeys []string + for _, key := range expectedAnnotationsKeys { if _, exists := svc.Annotations[key]; exists { unexpectedKeys = append(unexpectedKeys, key) } @@ -1185,11 +1206,69 @@ func TestShouldProcessService(t *testing.T) { } { result := l4netController.shouldProcessService(testCase.newSvc, testCase.oldSvc) if result != testCase.shouldProcess { - t.Errorf("Old service %v. New service %v. Expected shouldProcess: %t, got: %t", testCase.oldSvc, testCase.newSvc, testCase.shouldProcess, result) + t.Errorf("Old service %v. New service %v. Expected needsUpdate: %t, got: %t", testCase.oldSvc, testCase.newSvc, testCase.shouldProcess, result) } } } +func TestDualStackServiceNeedsUpdate(t *testing.T) { + testCases := []struct { + desc string + initialIPFamilies []v1.IPFamily + finalIPFamilies []v1.IPFamily + needsUpdate bool + }{ + { + desc: "Should update NetLB on ipv4 -> ipv4, ipv6", + initialIPFamilies: []v1.IPFamily{v1.IPv4Protocol}, + finalIPFamilies: []v1.IPFamily{v1.IPv4Protocol, v1.IPv6Protocol}, + needsUpdate: true, + }, + { + desc: "Should update NetLB on ipv4, ipv6 -> ipv4", + initialIPFamilies: []v1.IPFamily{v1.IPv4Protocol, v1.IPv6Protocol}, + finalIPFamilies: []v1.IPFamily{v1.IPv4Protocol}, + needsUpdate: true, + }, + { + desc: "Should update NetLB on ipv6 -> ipv6, ipv4", + initialIPFamilies: []v1.IPFamily{v1.IPv6Protocol}, + finalIPFamilies: []v1.IPFamily{v1.IPv6Protocol, v1.IPv4Protocol}, + needsUpdate: true, + }, + { + desc: "Should update NetLB on ipv6, ipv4 -> ipv6", + initialIPFamilies: []v1.IPFamily{v1.IPv6Protocol, v1.IPv4Protocol}, + finalIPFamilies: []v1.IPFamily{v1.IPv6Protocol}, + needsUpdate: true, + }, + { + desc: "Should not update NetLB on same IP families update", + initialIPFamilies: []v1.IPFamily{v1.IPv6Protocol, v1.IPv4Protocol}, + finalIPFamilies: []v1.IPFamily{v1.IPv6Protocol, v1.IPv4Protocol}, + needsUpdate: false, + }, + } + for _, tc := range testCases { + tc := tc + t.Run(tc.desc, func(t *testing.T) { + t.Parallel() + + controller := newL4NetLBServiceController() + controller.enableDualStack = true + oldSvc := test.NewL4NetLBRBSService(8080) + oldSvc.Spec.IPFamilies = tc.initialIPFamilies + newSvc := test.NewL4NetLBRBSService(8080) + newSvc.Spec.IPFamilies = tc.finalIPFamilies + + result := controller.needsUpdate(oldSvc, newSvc) + if result != tc.needsUpdate { + t.Errorf("Old service %v. New service %v. Expected needsUpdate: %t, got: %t", oldSvc, newSvc, tc.needsUpdate, result) + } + }) + } +} + func TestPreventTargetPoolToRBSMigration(t *testing.T) { testCases := []struct { desc string @@ -1328,3 +1407,74 @@ func TestIsRBSBasedServiceForNonLoadBalancersType(t *testing.T) { }) } } + +func TestCreateDeleteDualStackNetLBService(t *testing.T) { + testCases := []struct { + desc string + ipFamilies []v1.IPFamily + }{ + { + desc: "Create and delete IPv4 NetLB", + ipFamilies: []v1.IPFamily{v1.IPv4Protocol}, + }, + { + desc: "Create and delete IPv4 IPv6 NetLB", + ipFamilies: []v1.IPFamily{v1.IPv4Protocol, v1.IPv6Protocol}, + }, + { + desc: "Create and delete IPv6 NetLB", + ipFamilies: []v1.IPFamily{v1.IPv6Protocol}, + }, + { + desc: "Create and delete IPv6 IPv4 NetLB", + ipFamilies: []v1.IPFamily{v1.IPv6Protocol, v1.IPv4Protocol}, + }, + { + desc: "Create and delete NetLB with empty IP families", + ipFamilies: []v1.IPFamily{}, + }, + } + + for _, tc := range testCases { + t.Run(tc.desc, func(t *testing.T) { + controller := newL4NetLBServiceController() + controller.enableDualStack = true + svc := test.NewL4NetLBRBSService(8080) + svc.Spec.IPFamilies = tc.ipFamilies + addNetLBService(controller, svc) + + prevMetrics, err := test.GetL4NetLBLatencyMetric() + if err != nil { + t.Errorf("Error getting L4 NetLB latency metrics err: %v", err) + } + if prevMetrics == nil { + t.Fatalf("Cannot get prometheus metrics for L4NetLB latency") + } + + key, _ := common.KeyFunc(svc) + err = controller.sync(key) + if err != nil { + t.Errorf("Failed to sync newly added service %s, err %v", svc.Name, err) + } + svc, err = controller.ctx.KubeClient.CoreV1().Services(svc.Namespace).Get(context.TODO(), svc.Name, metav1.GetOptions{}) + if err != nil { + t.Errorf("Failed to lookup service %s, err %v", svc.Name, err) + } + + expectedIngressLength := len(tc.ipFamilies) + // For empty IP Families we should provide ipv4 address + if expectedIngressLength == 0 { + expectedIngressLength = 1 + } + if len(svc.Status.LoadBalancer.Ingress) != expectedIngressLength { + t.Errorf("expectedIngressLength = %d, got %d", expectedIngressLength, len(svc.Status.LoadBalancer.Ingress)) + } + + err = validateAnnotations(svc) + if err != nil { + t.Errorf("validateAnnotations(%+v) returned error %v, want nil", svc, err) + } + deleteNetLBService(controller, svc) + }) + } +} diff --git a/pkg/loadbalancers/forwarding_rules.go b/pkg/loadbalancers/forwarding_rules.go index adc424ac4e..9d697214a4 100644 --- a/pkg/loadbalancers/forwarding_rules.go +++ b/pkg/loadbalancers/forwarding_rules.go @@ -284,10 +284,10 @@ func (l4 *L4) ensureIPv4ForwardingRule(bsLink string, options gce.ILBOptions, ex return fr, nil } -// ensureExternalForwardingRule creates a forwarding rule with the given name for L4NetLB, +// ensureIPv4ForwardingRule creates a forwarding rule with the given name for L4NetLB, // if it does not exist. It updates the existing forwarding rule if needed. -func (l4netlb *L4NetLB) ensureExternalForwardingRule(bsLink string) (*composite.ForwardingRule, IPAddressType, error) { - frName := l4netlb.GetFRName() +func (l4netlb *L4NetLB) ensureIPv4ForwardingRule(bsLink string) (*composite.ForwardingRule, IPAddressType, error) { + frName := l4netlb.frName() start := time.Now() klog.V(2).Infof("Ensuring external forwarding rule %s for L4 NetLB Service %s/%s, backend service link: %s", l4netlb.Service.Namespace, l4netlb.Service.Name, frName, bsLink) @@ -306,7 +306,7 @@ func (l4netlb *L4NetLB) ensureExternalForwardingRule(bsLink string) (*composite. // Determine IP which will be used for this LB. If no forwarding rule has been established // or specified in the Service spec, then requestedIP = "". ipToUse := l4lbIPToUse(l4netlb.Service, existingFwdRule, "") - klog.V(2).Infof("ensureExternalForwardingRule(%s): LoadBalancer IP %s", frName, ipToUse) + klog.V(2).Infof("ensureIPv4ForwardingRule(%s): LoadBalancer IP %s", frName, ipToUse) netTier, isFromAnnotation := utils.GetNetworkTier(l4netlb.Service) var isIPManaged IPAddressType @@ -328,12 +328,12 @@ func (l4netlb *L4NetLB) ensureExternalForwardingRule(bsLink string) (*composite. if err != nil { return nil, IPAddrUndefined, err } - klog.V(2).Infof("ensureExternalForwardingRule(%v): reserved IP %q for the forwarding rule", frName, ipToUse) + klog.V(2).Infof("ensureIPv4ForwardingRule(%v): reserved IP %q for the forwarding rule", frName, ipToUse) defer func() { // Release the address that was reserved, in all cases. If the forwarding rule was successfully created, // the ephemeral IP is not needed anymore. If it was not created, the address should be released to prevent leaks. if err := addrMgr.ReleaseAddress(); err != nil { - klog.Errorf("ensureExternalForwardingRule: failed to release address reservation, possibly causing an orphan: %v", err) + klog.Errorf("ensureIPv4ForwardingRule: failed to release address reservation, possibly causing an orphan: %v", err) } }() } @@ -369,19 +369,19 @@ func (l4netlb *L4NetLB) ensureExternalForwardingRule(bsLink string) (*composite. } if equal { // nothing to do - klog.V(2).Infof("ensureExternalForwardingRule: Skipping update of unchanged forwarding rule - %s", fr.Name) + klog.V(2).Infof("ensureIPv4ForwardingRule: Skipping update of unchanged forwarding rule - %s", fr.Name) return existingFwdRule, isIPManaged, nil } frDiff := cmp.Diff(existingFwdRule, fr) // If the forwarding rule pointed to a backend service which does not match the controller naming scheme, // that resource could be leaked. It is not being deleted here because that is a user-managed resource. - klog.V(2).Infof("ensureExternalForwardingRule: forwarding rule changed - Existing - %+v\n, New - %+v\n, Diff(-existing, +new) - %s\n. Deleting existing forwarding rule.", existingFwdRule, fr, frDiff) + klog.V(2).Infof("ensureIPv4ForwardingRule: forwarding rule changed - Existing - %+v\n, New - %+v\n, Diff(-existing, +new) - %s\n. Deleting existing forwarding rule.", existingFwdRule, fr, frDiff) if err = l4netlb.forwardingRules.Delete(existingFwdRule.Name); err != nil { return nil, IPAddrUndefined, err } l4netlb.recorder.Eventf(l4netlb.Service, corev1.EventTypeNormal, events.SyncIngress, "ForwardingRule %q deleted", existingFwdRule.Name) } - klog.V(2).Infof("ensureExternalForwardingRule: Creating/Recreating forwarding rule - %s", fr.Name) + klog.V(2).Infof("ensureIPv4ForwardingRule: Creating/Recreating forwarding rule - %s", fr.Name) if err = l4netlb.forwardingRules.Create(fr); err != nil { if isAddressAlreadyInUseError(err) { return nil, IPAddrUndefined, utils.NewIPConfigurationError(fr.IPAddress, addressAlreadyInUseMessageExternal) @@ -398,23 +398,6 @@ func (l4netlb *L4NetLB) ensureExternalForwardingRule(bsLink string) (*composite. return createdFr, isIPManaged, err } -func (l4netlb *L4NetLB) deleteExternalForwardingRule(result *L4NetLBSyncResult) { - frName := l4netlb.GetFRName() - - start := time.Now() - klog.V(2).Infof("Deleting external forwarding rule %s for service %s/%s", frName, l4netlb.Service.Namespace, l4netlb.Service.Name) - defer func() { - klog.V(2).Infof("Finished deleting external forwarding rule %s for service %s/%s, time taken: %v", frName, l4netlb.Service.Namespace, l4netlb.Service.Name, time.Since(start)) - }() - - err := l4netlb.forwardingRules.Delete(frName) - if err != nil { - klog.Errorf("Failed to delete forwarding rule %s for service %s - %v", frName, l4netlb.NamespacedName.String(), err) - result.Error = err - result.GCEResourceInError = annotations.ForwardingRuleResource - } -} - // tearDownResourcesWithWrongNetworkTier removes forwarding rule or IP address if its Network Tier differs from desired. func (l4netlb *L4NetLB) tearDownResourcesWithWrongNetworkTier(existingFwdRule *composite.ForwardingRule, svcNetTier cloud.NetworkTier, am *addressManager) error { if existingFwdRule != nil && existingFwdRule.NetworkTier != svcNetTier.ToGCEValue() { diff --git a/pkg/loadbalancers/forwarding_rules_ipv6.go b/pkg/loadbalancers/forwarding_rules_ipv6.go index 1b9f104212..46591b59d3 100644 --- a/pkg/loadbalancers/forwarding_rules_ipv6.go +++ b/pkg/loadbalancers/forwarding_rules_ipv6.go @@ -127,6 +127,79 @@ func (l4 *L4) deleteChangedIPv6ForwardingRule(existingFwdRule *composite.Forward return nil } +func (l4netlb *L4NetLB) ensureIPv6ForwardingRule(bsLink string) (*composite.ForwardingRule, error) { + expectedIPv6FwdRule, err := l4netlb.buildExpectedIPv6ForwardingRule(bsLink) + if err != nil { + return nil, fmt.Errorf("l4netlb.buildExpectedIPv6ForwardingRule(%s) returned error %w, want nil", bsLink, err) + } + + existingIPv6FwdRule, err := l4netlb.forwardingRules.Get(expectedIPv6FwdRule.Name) + if err != nil { + return nil, fmt.Errorf("l4netlb.forwardingRules.GetForwardingRule(%s) returned error %w, want nil", expectedIPv6FwdRule.Name, err) + } + + if existingIPv6FwdRule != nil { + equal, err := EqualIPv6ForwardingRules(existingIPv6FwdRule, expectedIPv6FwdRule) + if err != nil { + return existingIPv6FwdRule, err + } + if equal { + klog.V(2).Infof("ensureIPv6ForwardingRule: Skipping update of unchanged ipv6 forwarding rule - %s", expectedIPv6FwdRule.Name) + return existingIPv6FwdRule, nil + } + err = l4netlb.deleteChangedIPv6ForwardingRule(existingIPv6FwdRule, expectedIPv6FwdRule) + if err != nil { + return nil, err + } + } + klog.V(2).Infof("ensureIPv6ForwardingRule: Creating/Recreating forwarding rule - %s", expectedIPv6FwdRule.Name) + err = l4netlb.forwardingRules.Create(expectedIPv6FwdRule) + if err != nil { + return nil, err + } + + createdFr, err := l4netlb.forwardingRules.Get(expectedIPv6FwdRule.Name) + return createdFr, err +} + +func (l4netlb *L4NetLB) buildExpectedIPv6ForwardingRule(bsLink string) (*composite.ForwardingRule, error) { + frName := l4netlb.ipv6FRName() + + frDesc, err := utils.MakeL4IPv6ForwardingRuleDescription(l4netlb.Service) + if err != nil { + return nil, fmt.Errorf("failed to compute description for forwarding rule %s, err: %w", frName, err) + } + + svcPorts := l4netlb.Service.Spec.Ports + portRange, protocol := utils.MinMaxPortRangeAndProtocol(svcPorts) + + fr := &composite.ForwardingRule{ + Name: frName, + Description: frDesc, + IPProtocol: protocol, + PortRange: portRange, + LoadBalancingScheme: string(cloud.SchemeExternal), + BackendService: bsLink, + IpVersion: "IPV6", + NetworkTier: cloud.NetworkTierPremium.ToGCEValue(), + Subnetwork: l4netlb.cloud.SubnetworkURL(), + } + + return fr, nil +} + +func (l4netlb *L4NetLB) deleteChangedIPv6ForwardingRule(existingFwdRule *composite.ForwardingRule, expectedFwdRule *composite.ForwardingRule) error { + frDiff := cmp.Diff(existingFwdRule, expectedFwdRule, cmpopts.IgnoreFields(composite.ForwardingRule{}, "IPAddress")) + klog.V(2).Infof("IPv6 External forwarding rule changed - Existing - %+v\n, New - %+v\n, Diff(-existing, +new) - %s\n. Deleting existing ipv6 forwarding rule.", existingFwdRule, expectedFwdRule, frDiff) + + err := l4netlb.forwardingRules.Delete(existingFwdRule.Name) + if err != nil { + return err + } + l4netlb.recorder.Eventf(l4netlb.Service, corev1.EventTypeNormal, events.SyncIngress, "External ForwardingRule %q deleted", existingFwdRule.Name) + return nil +} + func EqualIPv6ForwardingRules(fr1, fr2 *composite.ForwardingRule) (bool, error) { id1, err := cloud.ParseResourceURL(fr1.BackendService) if err != nil { diff --git a/pkg/loadbalancers/forwarding_rules_test.go b/pkg/loadbalancers/forwarding_rules_test.go index 91ecbd8df8..f27aa8daec 100644 --- a/pkg/loadbalancers/forwarding_rules_test.go +++ b/pkg/loadbalancers/forwarding_rules_test.go @@ -264,7 +264,7 @@ func TestL4CreateExternalForwardingRuleAddressAlreadyInUse(t *testing.T) { fakeGCE.ReserveRegionAddress(addr, fakeGCE.Region()) insertError := &googleapi.Error{Code: http.StatusBadRequest, Message: "Invalid value for field 'resource.IPAddress': '1.1.1.1'. Specified IP address is in-use and would result in a conflict., invalid"} fakeGCE.Compute().(*cloud.MockGCE).MockForwardingRules.InsertHook = test.InsertForwardingRuleErrorHook(insertError) - _, _, err := l4.ensureExternalForwardingRule("link") + _, _, err := l4.ensureIPv4ForwardingRule("link") require.Error(t, err) assert.True(t, utils.IsIPConfigurationError(err)) diff --git a/pkg/loadbalancers/l4_test.go b/pkg/loadbalancers/l4_test.go index 403ba68e44..026989b110 100644 --- a/pkg/loadbalancers/l4_test.go +++ b/pkg/loadbalancers/l4_test.go @@ -680,6 +680,7 @@ func TestHealthCheckFirewallDeletionWithNetLB(t *testing.T) { if len(xlbResult.Status.Ingress) == 0 { t.Errorf("Got empty loadBalancer status using handler %v", l4NetLB) } + l4NetLB.Service.Annotations = xlbResult.Annotations assertNetLBResources(t, l4NetLB, nodeNames) // Delete the ILB loadbalancer @@ -1801,7 +1802,7 @@ func TestDualStackLoadBalancerTransitions(t *testing.T) { } } -func TestDualStackLBCleansOnlyAnnotationResources(t *testing.T) { +func TestDualStackILBCleansOnlyAnnotationResources(t *testing.T) { t.Parallel() nodeNames := []string{"test-node-1"} vals := gce.DefaultTestClusterValues() diff --git a/pkg/loadbalancers/l4ipv6.go b/pkg/loadbalancers/l4ipv6.go index e5267ec94a..156b4cc455 100644 --- a/pkg/loadbalancers/l4ipv6.go +++ b/pkg/loadbalancers/l4ipv6.go @@ -83,7 +83,6 @@ func (l4 *L4) deleteIPv6ResourcesOnDelete(syncResult *L4ILBSyncResult) { // if resource exists in Service annotation, if shouldIgnoreAnnotations not set to true // IPv6 Specific resources: // - IPv6 Forwarding Rule -// - IPv6 Address // - IPv6 Firewall // This function does not delete Backend Service and Health Check, because they are shared between IPv4 and IPv6. // IPv6 Firewall Rule for Health Check also will not be deleted here, and will be left till the Service Deletion. diff --git a/pkg/loadbalancers/l4netlb.go b/pkg/loadbalancers/l4netlb.go index 9d8e3e7ce4..ef1537cf4f 100644 --- a/pkg/loadbalancers/l4netlb.go +++ b/pkg/loadbalancers/l4netlb.go @@ -51,6 +51,7 @@ type L4NetLB struct { NamespacedName types.NamespacedName healthChecks healthchecksl4.L4HealthChecks forwardingRules ForwardingRulesProvider + enableDualStack bool } // L4NetLBSyncResult contains information about the outcome of an L4 NetLB sync. It stores the list of resource name annotations, @@ -82,10 +83,11 @@ func (r *L4NetLBSyncResult) SetMetricsForSuccessfulServiceSync() { } type L4NetLBParams struct { - Service *corev1.Service - Cloud *gce.Cloud - Namer namer.L4ResourcesNamer - Recorder record.EventRecorder + Service *corev1.Service + Cloud *gce.Cloud + Namer namer.L4ResourcesNamer + Recorder record.EventRecorder + DualStackEnabled bool } // NewL4NetLB creates a new Handler for the given L4NetLB service. @@ -100,6 +102,7 @@ func NewL4NetLB(params *L4NetLBParams) *L4NetLB { backendPool: backends.NewPool(params.Cloud, params.Namer), healthChecks: healthchecksl4.NewL4HealthChecks(params.Cloud, params.Recorder), forwardingRules: forwardingrules.New(params.Cloud, meta.VersionGA, meta.Regional), + enableDualStack: params.DualStackEnabled, } return l4netlb } @@ -115,7 +118,6 @@ func (l4netlb *L4NetLB) createKey(name string) (*meta.Key, error) { // This function does not link instances to Backend Service. func (l4netlb *L4NetLB) EnsureFrontend(nodeNames []string, svc *corev1.Service) *L4NetLBSyncResult { result := NewL4SyncResult(SyncTypeCreate) - // If service already has an IP assigned, treat it as an update instead of a new Loadbalancer. if len(svc.Status.LoadBalancer.Ingress) > 0 { result.SyncType = SyncTypeUpdate @@ -124,54 +126,156 @@ func (l4netlb *L4NetLB) EnsureFrontend(nodeNames []string, svc *corev1.Service) l4netlb.Service = svc - sharedHC := !helpers.RequestsOnlyLocalTraffic(svc) + hcLink := l4netlb.provideHealthChecks(nodeNames, result) + if result.Error != nil { + return result + } + + bsLink := l4netlb.provideBackendService(result, hcLink) + if result.Error != nil { + return result + } + + if l4netlb.enableDualStack { + l4netlb.ensureDualStackResources(result, nodeNames, bsLink) + } else { + l4netlb.ensureIPv4Resources(result, nodeNames, bsLink) + } + + return result +} + +func (l4netlb *L4NetLB) provideHealthChecks(nodeNames []string, result *L4NetLBSyncResult) string { + if l4netlb.enableDualStack { + return l4netlb.provideDualStackHealthChecks(nodeNames, result) + } + return l4netlb.provideIPv4HealthChecks(nodeNames, result) +} + +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)) + if hcResult.Err != nil { + result.GCEResourceInError = hcResult.GceResourceInError + result.Error = hcResult.Err + return "" + } + + if hcResult.HCFirewallRuleName != "" { + result.Annotations[annotations.FirewallRuleForHealthcheckKey] = hcResult.HCFirewallRuleName + } + if hcResult.HCFirewallRuleIPv6Name != "" { + result.Annotations[annotations.FirewallRuleForHealthcheckIPv6Key] = hcResult.HCFirewallRuleIPv6Name + } + result.Annotations[annotations.HealthcheckKey] = hcResult.HCName + return hcResult.HCLink +} + +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) if hcResult.Err != nil { result.GCEResourceInError = hcResult.GceResourceInError - result.Error = fmt.Errorf("Failed to ensure health check %s - %w", hcResult.HCName, hcResult.Err) - return result + result.Error = hcResult.Err + return "" } result.Annotations[annotations.HealthcheckKey] = hcResult.HCName result.Annotations[annotations.FirewallRuleForHealthcheckKey] = hcResult.HCFirewallRuleName + return hcResult.HCLink +} +func (l4netlb *L4NetLB) provideBackendService(syncResult *L4NetLBSyncResult, hcLink string) string { bsName := l4netlb.namer.L4Backend(l4netlb.Service.Namespace, l4netlb.Service.Name) servicePorts := l4netlb.Service.Spec.Ports protocol := utils.GetProtocol(servicePorts) - bs, err := l4netlb.backendPool.EnsureL4BackendService(bsName, hcResult.HCLink, string(protocol), string(l4netlb.Service.Spec.SessionAffinity), string(cloud.SchemeExternal), l4netlb.NamespacedName) + bs, err := l4netlb.backendPool.EnsureL4BackendService(bsName, hcLink, string(protocol), string(l4netlb.Service.Spec.SessionAffinity), string(cloud.SchemeExternal), l4netlb.NamespacedName) if err != nil { - result.GCEResourceInError = annotations.BackendServiceResource - result.Error = fmt.Errorf("Failed to ensure backend service %s - %w", bsName, err) - return result + syncResult.GCEResourceInError = annotations.BackendServiceResource + syncResult.Error = fmt.Errorf("Failed to ensure backend service %s - %w", bsName, err) + return "" } - result.Annotations[annotations.BackendServiceKey] = bsName + syncResult.Annotations[annotations.BackendServiceKey] = bsName + return bs.SelfLink +} + +func (l4netlb *L4NetLB) ensureDualStackResources(result *L4NetLBSyncResult, nodeNames []string, bsLink string) { + if utils.NeedsIPv4(l4netlb.Service) { + l4netlb.ensureIPv4Resources(result, nodeNames, bsLink) + } else { + l4netlb.deleteIPv4ResourcesOnSync(result) + } + if utils.NeedsIPv6(l4netlb.Service) { + l4netlb.ensureIPv6Resources(result, nodeNames, bsLink) + } else { + l4netlb.deleteIPv6ResourcesOnSync(result) + } +} - fr, ipAddrType, err := l4netlb.ensureExternalForwardingRule(bs.SelfLink) +// ensureIPv4Resources creates resources specific to IPv4 L4 Load Balancers: +// - IPv4 Forwarding Rule +// - IPv4 Firewall +func (l4netlb *L4NetLB) ensureIPv4Resources(result *L4NetLBSyncResult, nodeNames []string, bsLink string) { + fr, ipAddrType, err := l4netlb.ensureIPv4ForwardingRule(bsLink) if err != nil { // User can misconfigure the forwarding rule if Network Tier will not match service level Network Tier. result.MetricsState.IsUserError = utils.IsUserError(err) result.GCEResourceInError = annotations.ForwardingRuleResource - result.Error = fmt.Errorf("Failed to ensure forwarding rule - %w", err) - return result + result.Error = fmt.Errorf("failed to ensure forwarding rule - %w", err) + return } if fr.IPProtocol == string(corev1.ProtocolTCP) { result.Annotations[annotations.TCPForwardingRuleKey] = fr.Name } else { result.Annotations[annotations.UDPForwardingRuleKey] = fr.Name } + result.MetricsState.IsManagedIP = ipAddrType == IPAddrManaged + result.MetricsState.IsPremiumTier = fr.NetworkTier == cloud.NetworkTierPremium.ToGCEValue() + + l4netlb.ensureIPv4NodesFirewall(nodeNames, fr.IPAddress, result) + if result.Error != nil { + klog.Errorf("ensureIPv4Resources: Failed to ensure nodes firewall for L4 NetLB Service %s/%s, error: %v", l4netlb.Service.Namespace, l4netlb.Service.Name, err) + return + } result.Status = utils.AddIPToLBStatus(result.Status, fr.IPAddress) - result.MetricsState.IsPremiumTier = fr.NetworkTier == cloud.NetworkTierPremium.ToGCEValue() - result.MetricsState.IsManagedIP = ipAddrType == IPAddrManaged +} + +func (l4netlb *L4NetLB) ensureIPv4NodesFirewall(nodeNames []string, ipAddress string, result *L4NetLBSyncResult) { + start := time.Now() firewallName := l4netlb.namer.L4Firewall(l4netlb.Service.Namespace, l4netlb.Service.Name) + servicePorts := l4netlb.Service.Spec.Ports portRanges := utils.GetServicePortRanges(servicePorts) - res := l4netlb.ensureNodesFirewall(firewallName, nodeNames, fr.IPAddress, portRanges, string(protocol)) - if res.Error != nil { - return res + protocol := utils.GetProtocol(servicePorts) + + klog.V(2).Infof("Ensuring nodes firewall %s for L4 NetLB Service %s/%s, ipAddress: %s, protocol: %s, len(nodeNames): %v, portRanges: %v", firewallName, l4netlb.Service.Namespace, l4netlb.Service.Name, ipAddress, protocol, len(nodeNames), portRanges) + defer func() { + klog.V(2).Infof("Finished ensuring nodes firewall %s for L4 NetLB Service %s/%s, time taken: %v", firewallName, l4netlb.Service.Namespace, l4netlb.Service.Name, time.Since(start)) + }() + + sourceRanges, err := utils.IPv4ServiceSourceRanges(l4netlb.Service) + if err != nil { + result.Error = err + return } - result.Annotations[annotations.FirewallRuleKey] = firewallName - return result + // Add firewall rule for L4 External LoadBalancer traffic to nodes + nodesFWRParams := firewalls.FirewallParams{ + PortRanges: portRanges, + SourceRanges: sourceRanges, + DestinationRanges: []string{ipAddress}, + Protocol: string(protocol), + Name: firewallName, + IP: l4netlb.Service.Spec.LoadBalancerIP, + NodeNames: nodeNames, + } + result.Error = firewalls.EnsureL4LBFirewallForNodes(l4netlb.Service, &nodesFWRParams, l4netlb.cloud, l4netlb.recorder) + if result.Error != nil { + result.GCEResourceInError = annotations.FirewallRuleResource + result.Error = err + return + } + result.Annotations[annotations.FirewallRuleKey] = firewallName } // EnsureLoadBalancerDeleted performs a cleanup of all GCE resources for the given loadbalancer service. @@ -180,53 +284,122 @@ func (l4netlb *L4NetLB) EnsureLoadBalancerDeleted(svc *corev1.Service) *L4NetLBS result := NewL4SyncResult(SyncTypeDelete) l4netlb.Service = svc - // If any resource deletion fails, log the error and continue cleanup. - l4netlb.deleteExternalForwardingRule(result) - l4netlb.deleteAddress(result) - l4netlb.deleteNodesFirewall(result) + l4netlb.deleteIPv4ResourcesOnDelete(result) + if l4netlb.enableDualStack { + l4netlb.deleteIPv6ResourcesOnDelete(result) + } + l4netlb.deleteBackendService(result) l4netlb.deleteHealthChecksWithFirewall(result) return result } -func (l4netlb *L4NetLB) deleteNodesFirewall(result *L4NetLBSyncResult) { - firewallName := l4netlb.namer.L4Firewall(l4netlb.Service.Namespace, l4netlb.Service.Name) +// deleteIPv4ResourcesOnSync deletes resources specific to IPv4, +// only if corresponding resource annotation exist on Service object. +// This function is called only on Service update or periodic sync. +// Checking for annotation saves us from emitting too much error logs "Resource not found". +// If annotation was deleted, but resource still exists, it will be left till the Service deletion, +// where we delete all resources, no matter if they exist in annotations. +func (l4netlb *L4NetLB) deleteIPv4ResourcesOnSync(result *L4NetLBSyncResult) { + klog.Infof("Deleting IPv4 resources for L4 NetLB Service %s/%s on sync, with checking for existence in annotation", l4netlb.Service.Namespace, l4netlb.Service.Name) + l4netlb.deleteIPv4ResourcesAnnotationBased(result, false) +} - start := time.Now() - klog.V(2).Infof("Deleting nodes firewall %s for L4 NetLB Service %s/%s", firewallName, l4netlb.Service.Namespace, l4netlb.Service.Name) - defer func() { - klog.V(2).Infof("Finished deleting nodes firewall %s for L4 NetLB Service %s/%s, time taken: %v", firewallName, l4netlb.Service.Namespace, l4netlb.Service.Name, time.Since(start)) - }() +// deleteIPv4ResourcesOnDelete deletes all resources specific to IPv4. +// This function is called only on Service deletion. +// During sync, we delete resources only that exist in annotations, +// so they could be leaked, if annotation was deleted. +// That's why on service deletion we delete all IPv4 resources, ignoring their existence in annotations +func (l4netlb *L4NetLB) deleteIPv4ResourcesOnDelete(result *L4NetLBSyncResult) { + klog.Infof("Deleting IPv4 resources for L4 NetLB Service %s/%s on delete, without checking for existence in annotation", l4netlb.Service.Namespace, l4netlb.Service.Name) + l4netlb.deleteIPv4ResourcesAnnotationBased(result, true) +} - err := firewalls.EnsureL4FirewallRuleDeleted(l4netlb.cloud, firewallName) - if err != nil { - if fwErr, ok := err.(*firewalls.FirewallXPNError); ok { - l4netlb.recorder.Eventf(l4netlb.Service, corev1.EventTypeNormal, "XPN", fwErr.Message) - return +// deleteIPv4ResourcesAnnotationBased deletes IPv4 only resources with checking, +// if resource exists in Service annotation, if shouldIgnoreAnnotations not set to true +// IPv4 Specific resources: +// - IPv4 Forwarding Rule +// - IPv4 Address +// - IPv4 Firewall +// This function does not delete Backend Service and Health Check, because they are shared between IPv4 and IPv6. +// IPv4 Firewall Rule for Health Check also will not be deleted here, and will be left till the Service Deletion. +func (l4netlb *L4NetLB) deleteIPv4ResourcesAnnotationBased(result *L4NetLBSyncResult, shouldIgnoreAnnotations bool) { + if shouldIgnoreAnnotations || l4netlb.hasAnnotation(annotations.TCPForwardingRuleKey) || l4netlb.hasAnnotation(annotations.UDPForwardingRuleKey) { + err := l4netlb.deleteIPv4ForwardingRule() + if err != nil { + klog.Errorf("Failed to delete forwarding rule for NetLB RBS service %s, error: %v", l4netlb.NamespacedName.String(), err) + result.Error = err + result.GCEResourceInError = annotations.ForwardingRuleResource } + } - klog.Errorf("Failed to delete firewall rule %s for service %s - %v", firewallName, l4netlb.NamespacedName.String(), err) - result.GCEResourceInError = annotations.FirewallRuleResource + // Deleting non-existent address do not print error audit logs, and we don't store address in annotations + // that's why we can delete it without checking annotation + err := l4netlb.deleteIPv4Address() + if err != nil { + klog.Errorf("Failed to delete address for NetLB RBS service %s, error: %v", l4netlb.NamespacedName.String(), err) result.Error = err + result.GCEResourceInError = annotations.AddressResource + } + + // delete firewall rule allowing load balancer source ranges + if shouldIgnoreAnnotations || l4netlb.hasAnnotation(annotations.FirewallRuleKey) { + err = l4netlb.deleteIPv4NodesFirewall() + if err != nil { + klog.Errorf("Failed to delete firewall rule for NetLB RBS service %s, error: %v", l4netlb.NamespacedName.String(), err) + result.GCEResourceInError = annotations.FirewallRuleResource + result.Error = err + } } } -func (l4netlb *L4NetLB) deleteAddress(result *L4NetLBSyncResult) { - addressName := l4netlb.GetFRName() +func (l4netlb *L4NetLB) deleteIPv4ForwardingRule() error { + frName := l4netlb.frName() + + start := time.Now() + klog.V(2).Infof("Deleting IPv4 external forwarding rule %s for L4 NetLB Service %s/%s", frName, l4netlb.Service.Namespace, l4netlb.Service.Name) + defer func() { + klog.V(2).Infof("Finished deleting IPv4 external forwarding rule %s for L4 NetLB Service %s/%s, time taken: %v", frName, l4netlb.Service.Namespace, l4netlb.Service.Name, time.Since(start)) + }() + + return l4netlb.forwardingRules.Delete(frName) +} + +func (l4netlb *L4NetLB) deleteIPv4Address() error { + addressName := l4netlb.frName() start := time.Now() - klog.V(2).Infof("Deleting external static address %s for L4 NetLB service %s/%s", addressName, l4netlb.Service.Namespace, l4netlb.Service.Name) + klog.V(2).Infof("Deleting IPv4 external static address %s for L4 NetLB service %s/%s", addressName, l4netlb.Service.Namespace, l4netlb.Service.Name) defer func() { - klog.V(2).Infof("Finished deleting external static address %s for L4 NetLB service %s/%s, time taken: %v", addressName, l4netlb.Service.Namespace, l4netlb.Service.Name, time.Since(start)) + klog.V(2).Infof("Finished deleting IPv4 external static address %s for L4 NetLB service %s/%s, time taken: %v", addressName, l4netlb.Service.Namespace, l4netlb.Service.Name, time.Since(start)) }() - err := ensureAddressDeleted(l4netlb.cloud, addressName, l4netlb.cloud.Region()) + return ensureAddressDeleted(l4netlb.cloud, addressName, l4netlb.cloud.Region()) +} + +func (l4netlb *L4NetLB) deleteIPv4NodesFirewall() error { + firewallName := l4netlb.namer.L4Firewall(l4netlb.Service.Namespace, l4netlb.Service.Name) + + start := time.Now() + klog.V(2).Infof("Deleting IPv4 nodes firewall %s for L4 NetLB Service %s/%s", firewallName, l4netlb.Service.Namespace, l4netlb.Service.Name) + defer func() { + klog.V(2).Infof("Finished deleting IPv4 nodes firewall %s for L4 NetLB Service %s/%s, time taken: %v", firewallName, l4netlb.Service.Namespace, l4netlb.Service.Name, time.Since(start)) + }() + + return l4netlb.deleteFirewall(firewallName) +} + +func (l4netlb *L4NetLB) deleteFirewall(firewallName string) error { + err := firewalls.EnsureL4FirewallRuleDeleted(l4netlb.cloud, firewallName) if err != nil { - klog.Errorf("Failed to delete address for service %s - %v", l4netlb.NamespacedName.String(), err) - result.Error = err - result.GCEResourceInError = annotations.AddressResource + if fwErr, ok := err.(*firewalls.FirewallXPNError); ok { + l4netlb.recorder.Eventf(l4netlb.Service, corev1.EventTypeNormal, "XPN", fwErr.Message) + return nil + } + return err } + return nil } func (l4netlb *L4NetLB) deleteBackendService(result *L4NetLBSyncResult) { @@ -260,51 +433,34 @@ func (l4netlb *L4NetLB) deleteHealthChecksWithFirewall(result *L4NetLBSyncResult // When service is deleted we need to check both health checks shared and non-shared // and delete them if needed. for _, isShared := range []bool{true, false} { - resourceInError, err := l4netlb.healthChecks.DeleteHealthCheckWithFirewall(l4netlb.Service, l4netlb.namer, isShared, meta.Regional, utils.XLB) - if err != nil { - result.GCEResourceInError = resourceInError - result.Error = err - // continue with deletion of the non-shared Healthcheck regardless of the error, both healthchecks may need to be deleted, + if l4netlb.enableDualStack { + resourceInError, err := l4netlb.healthChecks.DeleteHealthCheckWithDualStackFirewalls(l4netlb.Service, l4netlb.namer, isShared, meta.Regional, utils.XLB) + if err != nil { + result.GCEResourceInError = resourceInError + result.Error = err + // continue with deletion of the non-shared Healthcheck regardless of the error, both healthchecks may need to be deleted, + } + } else { + resourceInError, err := l4netlb.healthChecks.DeleteHealthCheckWithFirewall(l4netlb.Service, l4netlb.namer, isShared, meta.Regional, utils.XLB) + if err != nil { + result.GCEResourceInError = resourceInError + result.Error = err + // continue with deletion of the non-shared Healthcheck regardless of the error, both healthchecks may need to be deleted, + } } } } -// GetFRName returns the name of the forwarding rule for the given L4 External LoadBalancer service. +func (l4netlb *L4NetLB) hasAnnotation(annotationKey string) bool { + if _, ok := l4netlb.Service.Annotations[annotationKey]; ok { + return true + } + return false +} + +// frName returns the name of the forwarding rule for the given L4 External LoadBalancer service. // This name should align with legacy forwarding rule name because we use forwarding rule to determine // which controller should process the service Ingress-GCE or k/k service controller. -func (l4netlb *L4NetLB) GetFRName() string { +func (l4netlb *L4NetLB) frName() string { return utils.LegacyForwardingRuleName(l4netlb.Service) } - -func (l4netlb *L4NetLB) ensureNodesFirewall(name string, nodeNames []string, ipAddress string, portRanges []string, protocol string) *L4NetLBSyncResult { - start := time.Now() - klog.V(2).Infof("Ensuring nodes firewall %s for L4 NetLB Service %s/%s, ipAddress: %s, protocol: %s, len(nodeNames): %v, portRanges: %v", name, l4netlb.Service.Namespace, l4netlb.Service.Name, ipAddress, protocol, len(nodeNames), portRanges) - defer func() { - klog.V(2).Infof("Finished ensuring nodes firewall %s for L4 NetLB Service %s/%s, time taken: %v", name, l4netlb.Service.Namespace, l4netlb.Service.Name, time.Since(start)) - }() - - result := &L4NetLBSyncResult{} - sourceRanges, err := utils.IPv4ServiceSourceRanges(l4netlb.Service) - if err != nil { - result.Error = err - return result - } - - // Add firewall rule for L4 External LoadBalancer traffic to nodes - nodesFWRParams := firewalls.FirewallParams{ - PortRanges: portRanges, - SourceRanges: sourceRanges, - DestinationRanges: []string{ipAddress}, - Protocol: protocol, - Name: name, - IP: l4netlb.Service.Spec.LoadBalancerIP, - NodeNames: nodeNames, - } - result.Error = firewalls.EnsureL4LBFirewallForNodes(l4netlb.Service, &nodesFWRParams, l4netlb.cloud, l4netlb.recorder) - if result.Error != nil { - result.GCEResourceInError = annotations.FirewallRuleResource - result.Error = err - return result - } - return result -} diff --git a/pkg/loadbalancers/l4netlb_test.go b/pkg/loadbalancers/l4netlb_test.go index 30647f375a..7d306cfe19 100644 --- a/pkg/loadbalancers/l4netlb_test.go +++ b/pkg/loadbalancers/l4netlb_test.go @@ -24,6 +24,7 @@ import ( "github.com/GoogleCloudPlatform/k8s-cloud-provider/pkg/cloud" "github.com/GoogleCloudPlatform/k8s-cloud-provider/pkg/cloud/meta" "github.com/GoogleCloudPlatform/k8s-cloud-provider/pkg/cloud/mock" + "github.com/google/go-cmp/cmp" ga "google.golang.org/api/compute/v1" v1 "k8s.io/api/core/v1" "k8s.io/client-go/tools/record" @@ -73,35 +74,13 @@ func TestEnsureL4NetLoadBalancer(t *testing.T) { if len(result.Status.Ingress) == 0 { t.Errorf("Got empty loadBalancer status using handler %v", l4netlb) } - if err := checkAnnotations(result, l4netlb); err != nil { - t.Errorf("Annotations error: %v", err) - } + l4netlb.Service.Annotations = result.Annotations assertNetLBResources(t, l4netlb, nodeNames) if err := checkMetrics(result.MetricsState /*isManaged = */, true /*isPremium = */, true /*isUserError =*/, false); err != nil { t.Errorf("Metrics error: %v", err) } } -func checkAnnotations(result *L4NetLBSyncResult, l4netlb *L4NetLB) error { - expBackendName := l4netlb.namer.L4Backend(l4netlb.Service.Namespace, l4netlb.Service.Name) - if result.Annotations[annotations.BackendServiceKey] != expBackendName { - return fmt.Errorf("BackendServiceKey mismatch %v != %v", expBackendName, result.Annotations[annotations.BackendServiceKey]) - } - expTcpFR := l4netlb.GetFRName() - if result.Annotations[annotations.TCPForwardingRuleKey] != expTcpFR { - return fmt.Errorf("TCPForwardingRuleKey mismatch %v != %v", expTcpFR, result.Annotations[annotations.TCPForwardingRuleKey]) - } - expFwRule := expBackendName - if result.Annotations[annotations.FirewallRuleKey] != expFwRule { - return fmt.Errorf("FirewallRuleKey mismatch %v != %v", expFwRule, result.Annotations[annotations.FirewallRuleKey]) - } - expHcFwName := l4netlb.namer.L4HealthCheckFirewall(l4netlb.Service.Namespace, l4netlb.Service.Name, true) - if result.Annotations[annotations.FirewallRuleForHealthcheckKey] != expHcFwName { - return fmt.Errorf("FirewallRuleForHealthcheckKey mismatch %v != %v", expHcFwName, result.Annotations[annotations.FirewallRuleForHealthcheckKey]) - } - return nil -} - func TestDeleteL4NetLoadBalancer(t *testing.T) { t.Parallel() nodeNames := []string{"test-node-1"} @@ -130,6 +109,7 @@ func TestDeleteL4NetLoadBalancer(t *testing.T) { if len(result.Status.Ingress) == 0 { t.Errorf("Got empty loadBalancer status using handler %v", l4NetLB) } + l4NetLB.Service.Annotations = result.Annotations assertNetLBResources(t, l4NetLB, nodeNames) if err := l4NetLB.EnsureLoadBalancerDeleted(svc); err.Error != nil { @@ -194,6 +174,7 @@ func TestHealthCheckFirewallDeletionWithILB(t *testing.T) { if len(result.Status.Ingress) == 0 { t.Errorf("Got empty loadBalancer status using handler %v", l4NetLB) } + l4NetLB.Service.Annotations = result.Annotations assertNetLBResources(t, l4NetLB, nodeNames) // Delete the NetLB loadbalancer. @@ -217,7 +198,407 @@ func TestHealthCheckFirewallDeletionWithILB(t *testing.T) { if err == nil || !strings.Contains(err.Error(), "not found") { t.Errorf("Healthcheck %s should be deleted", hcName) } +} + +func TestMetricsForStandardNetworkTier(t *testing.T) { + nodeNames := []string{"test-node-1"} + vals := gce.DefaultTestClusterValues() + fakeGCE := getFakeGCECloud(vals) + createUserStaticIPInStandardTier(fakeGCE, vals.Region) + (fakeGCE.Compute().(*cloud.MockGCE)).MockForwardingRules.GetHook = test.GetRBSForwardingRuleInStandardTier + + svc := test.NewL4NetLBRBSService(8080) + svc.Spec.LoadBalancerIP = usersIP + svc.ObjectMeta.Annotations[annotations.NetworkTierAnnotationKey] = string(cloud.NetworkTierStandard) + namer := namer_util.NewL4Namer(kubeSystemUID, namer_util.NewNamer(vals.ClusterName, "cluster-fw")) + + l4NetLBParams := &L4NetLBParams{ + Service: svc, + Cloud: fakeGCE, + Namer: namer, + Recorder: record.NewFakeRecorder(100), + } + l4netlb := NewL4NetLB(l4NetLBParams) + l4netlb.healthChecks = healthchecksl4.Fake(fakeGCE, l4netlb.recorder) + if _, err := test.CreateAndInsertNodes(l4netlb.cloud, nodeNames, vals.ZoneName); err != nil { + t.Errorf("Unexpected error when adding nodes %v", err) + } + result := l4netlb.EnsureFrontend(nodeNames, svc) + if result.Error != nil { + t.Errorf("Failed to ensure loadBalancer, err %v", result.Error) + } + if err := checkMetrics(result.MetricsState /*isManaged = */, false /*isPremium = */, false /*isUserError =*/, false); err != nil { + t.Errorf("Metrics error: %v", err) + } + // Check that service sync will return error if User Address IP Network Tier mismatch with service Network Tier. + svc.ObjectMeta.Annotations[annotations.NetworkTierAnnotationKey] = string(cloud.NetworkTierPremium) + result = l4netlb.EnsureFrontend(nodeNames, svc) + if result.Error == nil || !utils.IsNetworkTierError(result.Error) { + t.Errorf("LoadBalancer sync should return Network Tier error, err %v", result.Error) + } + if err := checkMetrics(result.MetricsState /*isManaged = */, false /*isPremium = */, false /*isUserError =*/, true); err != nil { + t.Errorf("Metrics error: %v", err) + } + // Check that when network tier annotation will be deleted which will change desired service Network Tier to PREMIUM + // service sync will return User Error because we do not support updating forwarding rule. + // Forwarding rule with wrong tier should be tear down and it can be done only via annotation change. + + // Crete new Static IP in Premium Network Tier to match default service Network Tier. + createUserStaticIPInPremiumTier(fakeGCE, vals.Region) + svc.Spec.LoadBalancerIP = usersIPPremium + delete(svc.ObjectMeta.Annotations, annotations.NetworkTierAnnotationKey) + + result = l4netlb.EnsureFrontend(nodeNames, svc) + if result.Error == nil || !utils.IsNetworkTierError(result.Error) { + t.Errorf("LoadBalancer sync should return Network Tier error, err %v", result.Error) + } + if err := checkMetrics(result.MetricsState /*isManaged = */, false /*isPremium = */, false /*isUserError =*/, true); err != nil { + t.Errorf("Metrics error: %v", err) + } +} + +func TestEnsureNetLBFirewallDestinations(t *testing.T) { + nodeNames := []string{"test-node-1"} + vals := gce.DefaultTestClusterValues() + fakeGCE := getFakeGCECloud(vals) + + svc := test.NewL4NetLBRBSService(8080) + namer := namer_util.NewL4Namer(kubeSystemUID, nil) + l4NetLBParams := &L4NetLBParams{ + Service: svc, + Cloud: fakeGCE, + Namer: namer, + Recorder: record.NewFakeRecorder(100), + } + l4netlb := NewL4NetLB(l4NetLBParams) + l4netlb.healthChecks = healthchecksl4.Fake(fakeGCE, l4netlb.recorder) + + if _, err := test.CreateAndInsertNodes(l4netlb.cloud, nodeNames, vals.ZoneName); err != nil { + t.Errorf("Unexpected error when adding nodes %v", err) + } + + flags.F.EnablePinhole = true + fwName := l4netlb.namer.L4Backend(l4netlb.Service.Namespace, l4netlb.Service.Name) + + fwrParams := firewalls.FirewallParams{ + Name: fwName, + SourceRanges: []string{"10.0.0.0/20"}, + DestinationRanges: []string{"20.0.0.0/20"}, + NodeNames: nodeNames, + Protocol: string(v1.ProtocolTCP), + IP: "1.2.3.4", + } + + err := firewalls.EnsureL4FirewallRule(l4netlb.cloud, utils.ServiceKeyFunc(svc.Namespace, svc.Name), &fwrParams /*sharedRule = */, false) + if err != nil { + t.Errorf("Unexpected error %v when ensuring firewall rule %s for svc %+v", err, fwName, svc) + } + existingFirewall, err := l4netlb.cloud.GetFirewall(fwName) + if err != nil || existingFirewall == nil || len(existingFirewall.Allowed) == 0 { + t.Errorf("Unexpected error %v when looking up firewall %s, Got firewall %+v", err, fwName, existingFirewall) + } + oldDestinationRanges := existingFirewall.DestinationRanges + + fwrParams.DestinationRanges = []string{"30.0.0.0/20"} + err = firewalls.EnsureL4FirewallRule(l4netlb.cloud, utils.ServiceKeyFunc(svc.Namespace, svc.Name), &fwrParams /*sharedRule = */, false) + if err != nil { + t.Errorf("Unexpected error %v when ensuring firewall rule %s for svc %+v", err, fwName, svc) + } + + updatedFirewall, err := l4netlb.cloud.GetFirewall(fwName) + if err != nil || updatedFirewall == nil || len(updatedFirewall.Allowed) == 0 { + t.Errorf("Unexpected error %v when looking up firewall %s, Got firewall %+v", err, fwName, updatedFirewall) + } + + if reflect.DeepEqual(oldDestinationRanges, updatedFirewall.DestinationRanges) { + t.Errorf("DestinationRanges is not updated. oldDestinationRanges:%v, updatedFirewall.DestinationRanges:%v", oldDestinationRanges, updatedFirewall.DestinationRanges) + } +} + +func TestEnsureExternalDualStackLoadBalancer(t *testing.T) { + t.Parallel() + nodeNames := []string{"test-node-1"} + vals := gce.DefaultTestClusterValues() + + namer := namer_util.NewL4Namer(kubeSystemUID, nil) + + testCases := []struct { + ipFamilies []v1.IPFamily + trafficPolicy v1.ServiceExternalTrafficPolicyType + desc string + }{ + { + desc: "Test ipv4 ipv6 service", + ipFamilies: []v1.IPFamily{v1.IPv4Protocol, v1.IPv6Protocol}, + trafficPolicy: v1.ServiceExternalTrafficPolicyTypeCluster, + }, + { + desc: "Test ipv4 ipv6 local service", + ipFamilies: []v1.IPFamily{v1.IPv4Protocol, v1.IPv6Protocol}, + trafficPolicy: v1.ServiceExternalTrafficPolicyTypeLocal, + }, + { + desc: "Test ipv6 ipv4 service", + ipFamilies: []v1.IPFamily{v1.IPv6Protocol, v1.IPv4Protocol}, + trafficPolicy: v1.ServiceExternalTrafficPolicyTypeCluster, + }, + { + desc: "Test ipv4 service", + ipFamilies: []v1.IPFamily{v1.IPv4Protocol}, + trafficPolicy: v1.ServiceExternalTrafficPolicyTypeCluster, + }, + { + desc: "Test ipv6 service", + ipFamilies: []v1.IPFamily{v1.IPv6Protocol}, + trafficPolicy: v1.ServiceExternalTrafficPolicyTypeCluster, + }, + } + + for _, tc := range testCases { + tc := tc + + t.Run(tc.desc, func(t *testing.T) { + t.Parallel() + + fakeGCE := getFakeGCECloud(vals) + + svc := test.NewL4NetLBRBSDualStackService(8080, v1.ProtocolTCP, tc.ipFamilies, tc.trafficPolicy) + + l4NetLBParams := &L4NetLBParams{ + Service: svc, + Cloud: fakeGCE, + Namer: namer, + Recorder: record.NewFakeRecorder(100), + DualStackEnabled: true, + } + l4NetLB := NewL4NetLB(l4NetLBParams) + l4NetLB.healthChecks = healthchecksl4.Fake(fakeGCE, l4NetLBParams.Recorder) + + if _, err := test.CreateAndInsertNodes(l4NetLB.cloud, nodeNames, vals.ZoneName); err != nil { + t.Errorf("Unexpected error when adding nodes %v", err) + } + + result := l4NetLB.EnsureFrontend(nodeNames, svc) + if result.Error != nil { + t.Errorf("Failed to ensure loadBalancer, err %v", result.Error) + } + if len(result.Status.Ingress) == 0 { + t.Errorf("Got empty loadBalancer status using handler %v", l4NetLB) + } + l4NetLB.Service.Annotations = result.Annotations + assertDualStackNetLBResources(t, l4NetLB, nodeNames) + + l4NetLB.EnsureLoadBalancerDeleted(l4NetLB.Service) + assertDualStackNetLBResourcesDeleted(t, l4NetLB) + }) + } +} + +// This is exhaustive test that checks for all possible transitions of +// - ServiceExternalTrafficPolicy +// - Protocol +// - IPFamilies +// for dual-stack service. In total 400 combinations +func TestDualStackNetLBLoadBalancerTransitions(t *testing.T) { + t.Parallel() + nodeNames := []string{"test-node-1"} + vals := gce.DefaultTestClusterValues() + + namer := namer_util.NewL4Namer(kubeSystemUID, nil) + + trafficPolicyStates := []v1.ServiceExternalTrafficPolicyType{v1.ServiceExternalTrafficPolicyTypeLocal, v1.ServiceExternalTrafficPolicyTypeCluster} + protocols := []v1.Protocol{v1.ProtocolTCP, v1.ProtocolUDP} + ipFamiliesStates := [][]v1.IPFamily{ + {v1.IPv4Protocol}, + {v1.IPv4Protocol, v1.IPv6Protocol}, + {v1.IPv6Protocol}, + {v1.IPv6Protocol, v1.IPv4Protocol}, + {}, + } + + for _, initialIPFamily := range ipFamiliesStates { + for _, finalIPFamily := range ipFamiliesStates { + for _, initialTrafficPolicy := range trafficPolicyStates { + for _, finalTrafficPolicy := range trafficPolicyStates { + for _, initialProtocol := range protocols { + for _, finalProtocol := range protocols { + initialIPFamily := initialIPFamily + finalIPFamily := finalIPFamily + initialTrafficPolicy := initialTrafficPolicy + finalTrafficPolicy := finalTrafficPolicy + initialProtocol := initialProtocol + finalProtocol := finalProtocol + + var stringInitialIPFamily []string + for _, f := range initialIPFamily { + stringInitialIPFamily = append(stringInitialIPFamily, string(f)) + } + + var stringFinalIPFamily []string + for _, f := range finalIPFamily { + stringFinalIPFamily = append(stringFinalIPFamily, string(f)) + } + desc := struct { + fromIPFamily string + toIPFamily string + fromTrafficPolicy string + toTrafficPolicy string + fromProtocol string + toProtocol string + }{ + strings.Join(stringInitialIPFamily, ","), + strings.Join(stringFinalIPFamily, ","), + string(initialTrafficPolicy), + string(finalTrafficPolicy), + string(initialProtocol), + string(finalProtocol), + } + + t.Run(fmt.Sprintf("+%v", desc), func(t *testing.T) { + t.Parallel() + + fakeGCE := getFakeGCECloud(vals) + + svc := test.NewL4NetLBRBSDualStackService(8080, initialProtocol, initialIPFamily, initialTrafficPolicy) + l4NetLBParams := &L4NetLBParams{ + Service: svc, + Cloud: fakeGCE, + Namer: namer, + Recorder: record.NewFakeRecorder(100), + DualStackEnabled: true, + } + l4NetLB := NewL4NetLB(l4NetLBParams) + l4NetLB.healthChecks = healthchecksl4.Fake(fakeGCE, l4NetLBParams.Recorder) + + if _, err := test.CreateAndInsertNodes(l4NetLB.cloud, nodeNames, vals.ZoneName); err != nil { + t.Errorf("Unexpected error when adding nodes %v", err) + } + + result := l4NetLB.EnsureFrontend(nodeNames, svc) + if result.Error != nil { + t.Errorf("Failed to ensure loadBalancer, err %v", result.Error) + } + if len(result.Status.Ingress) == 0 { + t.Errorf("Got empty loadBalancer status using handler %v", l4NetLB) + } + l4NetLB.Service.Annotations = result.Annotations + assertDualStackNetLBResources(t, l4NetLB, nodeNames) + + l4NetLB.EnsureLoadBalancerDeleted(l4NetLB.Service) + assertDualStackNetLBResourcesDeleted(t, l4NetLB) + }) + } + } + } + } + } + } +} + +func TestDualStackNetLBCleansOnlyAnnotationResources(t *testing.T) { + t.Parallel() + nodeNames := []string{"test-node-1"} + vals := gce.DefaultTestClusterValues() + + testCases := []struct { + desc string + ipFamiliesStates [2][]v1.IPFamily + annotationsToDelete []string + verifyResourcesNotDeleted func(l4netlb *L4NetLB) error + }{ + { + desc: "Should not delete IPv6 resources if they not exist in annotation", + ipFamiliesStates: [2][]v1.IPFamily{{v1.IPv4Protocol, v1.IPv6Protocol}, {v1.IPv4Protocol}}, + annotationsToDelete: []string{annotations.TCPForwardingRuleIPv6Key, annotations.FirewallRuleIPv6Key, annotations.FirewallRuleForHealthcheckIPv6Key}, + verifyResourcesNotDeleted: func(l4netlb *L4NetLB) error { + // Verify IPv6 Firewall was not deleted + ipv6FWName := l4netlb.namer.L4IPv6Firewall(l4netlb.Service.Namespace, l4netlb.Service.Name) + err := verifyFirewallNotExists(l4netlb.cloud, ipv6FWName) + if err == nil { + return fmt.Errorf("firewall rule %s was deleted, expected not", ipv6FWName) + } + + // Verify IPv6 Forwarding Rule was not deleted + ipv6FRName := l4netlb.ipv6FRName() + err = verifyForwardingRuleNotExists(l4netlb.cloud, ipv6FRName) + if err == nil { + return fmt.Errorf("forwarding rule %s was deleted, expected not", ipv6FRName) + } + return nil + }, + }, + { + desc: "Should not delete IPv4 resources if they not exist in annotation", + ipFamiliesStates: [2][]v1.IPFamily{{v1.IPv6Protocol, v1.IPv4Protocol}, {v1.IPv6Protocol}}, + annotationsToDelete: []string{annotations.TCPForwardingRuleKey, annotations.FirewallRuleKey, annotations.FirewallRuleForHealthcheckKey}, + verifyResourcesNotDeleted: func(l4netlb *L4NetLB) error { + // Verify IPv6 Firewall was not deleted + backendServiceName := l4netlb.namer.L4Backend(l4netlb.Service.Namespace, l4netlb.Service.Name) + err := verifyFirewallNotExists(l4netlb.cloud, backendServiceName) + if err == nil { + return fmt.Errorf("firewall rule %s was deleted, expected not", backendServiceName) + } + + // Verify IPv6 Forwarding Rule was not deleted + ipv4FRName := l4netlb.frName() + err = verifyForwardingRuleNotExists(l4netlb.cloud, ipv4FRName) + if err == nil { + return fmt.Errorf("forwarding rule %s was deleted, expected not", ipv4FRName) + } + return nil + }, + }, + } + + for _, tc := range testCases { + tc := tc + t.Run(tc.desc, func(t *testing.T) { + t.Parallel() + + namer := namer_util.NewL4Namer(kubeSystemUID, nil) + fakeGCE := getFakeGCECloud(vals) + + svc := test.NewL4NetLBRBSService(8080) + l4NetLBParams := &L4NetLBParams{ + Service: svc, + Cloud: fakeGCE, + Namer: namer, + Recorder: record.NewFakeRecorder(100), + DualStackEnabled: true, + } + l4NetLB := NewL4NetLB(l4NetLBParams) + l4NetLB.healthChecks = healthchecksl4.Fake(fakeGCE, l4NetLBParams.Recorder) + + if _, err := test.CreateAndInsertNodes(l4NetLB.cloud, nodeNames, vals.ZoneName); err != nil { + t.Errorf("Unexpected error when adding nodes %v", err) + } + + svc.Spec.IPFamilies = tc.ipFamiliesStates[0] + result := l4NetLB.EnsureFrontend(nodeNames, svc) + svc.Annotations = result.Annotations + assertDualStackNetLBResources(t, l4NetLB, nodeNames) + + // Delete resources annotation + for _, annotationToDelete := range tc.annotationsToDelete { + delete(svc.Annotations, annotationToDelete) + } + svc.Spec.IPFamilies = tc.ipFamiliesStates[1] + + // Run new sync. Controller should not delete resources, if they don't exist in annotation + result = l4NetLB.EnsureFrontend(nodeNames, svc) + svc.Annotations = result.Annotations + + err := tc.verifyResourcesNotDeleted(l4NetLB) + if err != nil { + t.Errorf("tc.verifyResourcesNotDeleted(_) returned error %v, want nil", err) + } + + l4NetLB.EnsureLoadBalancerDeleted(l4NetLB.Service) + // After complete deletion, IPv6 and IPv4 resources should be cleaned up, even if the were leaked + assertDualStackNetLBResourcesDeleted(t, l4NetLB) + }) + } } func ensureLoadBalancer(port int, vals gce.TestClusterValues, fakeGCE *gce.Cloud, t *testing.T) (*v1.Service, *L4NetLB) { @@ -232,7 +613,7 @@ func ensureLoadBalancer(port int, vals gce.TestClusterValues, fakeGCE *gce.Cloud Recorder: record.NewFakeRecorder(100), } l4NetLB := NewL4NetLB(l4NetLBParams) - l4NetLB.healthChecks = healthchecksl4.Fake(fakeGCE, l4NetLB.recorder) + l4NetLB.healthChecks = healthchecksl4.Fake(fakeGCE, l4NetLBParams.Recorder) result := l4NetLB.EnsureFrontend(emptyNodes, svc) if result.Error != nil { @@ -241,6 +622,7 @@ func ensureLoadBalancer(port int, vals gce.TestClusterValues, fakeGCE *gce.Cloud if len(result.Status.Ingress) == 0 { t.Errorf("Got empty loadBalancer status using handler %v", l4NetLB) } + l4NetLB.Service.Annotations = result.Annotations assertNetLBResources(t, l4NetLB, emptyNodes) return svc, l4NetLB } @@ -265,7 +647,7 @@ func assertNetLBResourcesDeleted(t *testing.T, l4netlb *L4NetLB) { } } - frName := l4netlb.GetFRName() + frName := l4netlb.frName() err := verifyForwardingRuleNotExists(l4netlb.cloud, frName) if err != nil { t.Errorf("verifyForwardingRuleNotExists(_, %s) returned error %v, want nil", frName, err) @@ -294,17 +676,83 @@ func assertNetLBResourcesDeleted(t *testing.T, l4netlb *L4NetLB) { } } +func assertDualStackNetLBResourcesDeleted(t *testing.T, l4netlb *L4NetLB) { + t.Helper() + + err := verifyNetLBCommonDualStackResourcesDeleted(l4netlb) + if err != nil { + t.Errorf("verifyNetLBCommonDualStackResourcesDeleted(_) returned erorr %v, want nil", err) + } + + err = verifyNetLBIPv4ResourcesDeletedOnSync(l4netlb) + if err != nil { + t.Errorf("verifyNetLBIPv4ResourcesDeletedOnSync(_) returned erorr %v, want nil", err) + } + + err = verifyNetLBIPv6ResourcesDeletedOnSync(l4netlb) + if err != nil { + t.Errorf("verifyNetLBIPv6ResourcesDeletedOnSync(_) returned erorr %v, want nil", err) + } + + // Check health check firewalls separately, because we don't clean them on sync, only on final deletion + ipv4HcFwNameShared := l4netlb.namer.L4HealthCheckFirewall(l4netlb.Service.Namespace, l4netlb.Service.Name, true) + ipv6HcFwNameShared := l4netlb.namer.L4IPv6HealthCheckFirewall(l4netlb.Service.Namespace, l4netlb.Service.Name, true) + ipv4HcFwNameNonShared := l4netlb.namer.L4HealthCheckFirewall(l4netlb.Service.Namespace, l4netlb.Service.Name, false) + ipv6HcFwNameNonShared := l4netlb.namer.L4IPv6HealthCheckFirewall(l4netlb.Service.Namespace, l4netlb.Service.Name, false) + + fwNames := []string{ + ipv4HcFwNameShared, + ipv4HcFwNameNonShared, + ipv6HcFwNameShared, + ipv6HcFwNameNonShared, + } + + for _, fwName := range fwNames { + err = verifyFirewallNotExists(l4netlb.cloud, fwName) + if err != nil { + t.Errorf("verifyFirewallNotExists(_, %s) returned error %v, want nil", fwName, err) + } + } +} + +func verifyNetLBCommonDualStackResourcesDeleted(l4netlb *L4NetLB) error { + backendServiceName := l4netlb.namer.L4Backend(l4netlb.Service.Namespace, l4netlb.Service.Name) + + err := verifyBackendServiceNotExists(l4netlb.cloud, backendServiceName) + if err != nil { + return fmt.Errorf("verifyBackendServiceNotExists(_, %s)", backendServiceName) + } + + hcNameShared := l4netlb.namer.L4HealthCheck(l4netlb.Service.Namespace, l4netlb.Service.Name, true) + err = verifyHealthCheckNotExists(l4netlb.cloud, hcNameShared, meta.Regional) + if err != nil { + return fmt.Errorf("verifyHealthCheckNotExists(_, %s)", hcNameShared) + } + + hcNameNonShared := l4netlb.namer.L4HealthCheck(l4netlb.Service.Namespace, l4netlb.Service.Name, false) + err = verifyHealthCheckNotExists(l4netlb.cloud, hcNameNonShared, meta.Regional) + if err != nil { + return fmt.Errorf("verifyHealthCheckNotExists(_, %s)", hcNameNonShared) + } + + err = verifyAddressNotExists(l4netlb.cloud, backendServiceName) + if err != nil { + return fmt.Errorf("verifyAddressNotExists(_, %s)", backendServiceName) + } + return nil +} + func assertNetLBResources(t *testing.T, l4NetLB *L4NetLB, nodeNames []string) { t.Helper() - err := verifyNetLBNodesFirewall(l4NetLB, nodeNames) + err := verifyNetLBIPv4NodesFirewall(l4NetLB, nodeNames) if err != nil { - t.Errorf("verifyNetLBNodesFirewall(_, %v) returned error %v, want nil", nodeNames, err) + t.Errorf("verifyNetLBIPv4NodesFirewall(_, %v) returned error %v, want nil", nodeNames, err) } - err = verifyNetLBHealthCheckFirewall(l4NetLB, nodeNames) + err = verifyNetLBIPv4HealthCheckFirewall(l4NetLB, nodeNames) if err != nil { - t.Errorf("verifyNetLBHealthCheckFirewall(_, %v) returned error %v, want nil", nodeNames, err) + t.Errorf("verifyNetLBIPv4HealthCheckFirewall(_, %v) returned error %v, want nil", nodeNames, err) } // Check that HealthCheck is created @@ -318,27 +766,108 @@ func assertNetLBResources(t *testing.T, l4NetLB *L4NetLB, nodeNames []string) { t.Errorf("getAndVerifyNetLBBackendService(_, %v) returned error %v, want nil", healthcheck, err) } - err = verifyNetLBForwardingRule(l4NetLB, backendService.SelfLink) + err = verifyNetLBIPv4ForwardingRule(l4NetLB, backendService.SelfLink) if err != nil { - t.Errorf("verifyNetLBForwardingRule(_, %s) returned error %v, want nil", backendService.SelfLink, err) + t.Errorf("verifyNetLBIPv4ForwardingRule(_, %s) returned error %v, want nil", backendService.SelfLink, err) + } + + expectedAnnotations := buildExpectedNetLBAnnotations(l4NetLB) + if !reflect.DeepEqual(expectedAnnotations, l4NetLB.Service.Annotations) { + diff := cmp.Diff(expectedAnnotations, l4NetLB.Service.Annotations) + t.Errorf("Expected annotations %v, got %v, diff %v", expectedAnnotations, l4NetLB.Service.Annotations, diff) } } -func verifyNetLBNodesFirewall(l4netlb *L4NetLB, nodeNames []string) error { +func assertDualStackNetLBResources(t *testing.T, l4NetLB *L4NetLB, nodeNames []string) { + t.Helper() + + // Check that HealthCheck is created + healthCheck, err := getAndVerifyNetLBHealthCheck(l4NetLB) + if err != nil { + t.Errorf("getAndVerifyNetLBHealthCheck(_) returned error %v, want nil", err) + } + + backendService, err := getAndVerifyNetLBBackendService(l4NetLB, healthCheck) + if err != nil { + t.Fatalf("getAndVerifyNetLBBackendService(_, %v) returned error %v, want nil", healthCheck, err) + } + + if utils.NeedsIPv4(l4NetLB.Service) { + err = verifyNetLBIPv4ForwardingRule(l4NetLB, backendService.SelfLink) + if err != nil { + t.Errorf("verifyNetLBIPv4ForwardingRule(_, %s) returned error %v, want nil", backendService.SelfLink, err) + } + + err = verifyNetLBIPv4NodesFirewall(l4NetLB, nodeNames) + if err != nil { + t.Errorf("verifyNetLBIPv4NodesFirewall(_, %v) returned error %v, want nil", nodeNames, err) + } + + err = verifyNetLBIPv4HealthCheckFirewall(l4NetLB, nodeNames) + if err != nil { + t.Errorf("verifyNetLBIPv4HealthCheckFirewall(_, %v) returned error %v, want nil", nodeNames, err) + } + } else { + err = verifyNetLBIPv4ResourcesDeletedOnSync(l4NetLB) + if err != nil { + t.Errorf("verifyNetLBIPv4ResourcesDeletedOnSync(_) returned error %v, want nil", err) + } + } + if utils.NeedsIPv6(l4NetLB.Service) { + err = verifyNetLBIPv6ForwardingRule(l4NetLB, backendService.SelfLink) + if err != nil { + t.Errorf("verifyNetLBIPv6ForwardingRule(_, %s) returned error %v, want nil", backendService.SelfLink, err) + } + + err = verifyNetLBIPv6NodesFirewall(l4NetLB, nodeNames) + if err != nil { + t.Errorf("verifyNetLBIPv6NodesFirewall(_, %v) returned error %v, want nil", nodeNames, err) + } + + err = verifyNetLBIPv6HealthCheckFirewall(l4NetLB, nodeNames) + if err != nil { + t.Errorf("verifyNetLBIPv6HealthCheckFirewall(_, %v) returned error %v, want nil", nodeNames, err) + } + } else { + err = verifyNetLBIPv6ResourcesDeletedOnSync(l4NetLB) + if err != nil { + t.Errorf("verifyNetLBIPv6ResourcesDeletedOnSync(_) returned error %v, want nil", err) + } + } + + expectedAnnotations := buildExpectedNetLBAnnotations(l4NetLB) + if !reflect.DeepEqual(expectedAnnotations, l4NetLB.Service.Annotations) { + diff := cmp.Diff(expectedAnnotations, l4NetLB.Service.Annotations) + t.Errorf("Expected annotations %v, got %v, diff %v", expectedAnnotations, l4NetLB.Service.Annotations, diff) + } +} + +func verifyNetLBIPv4NodesFirewall(l4netlb *L4NetLB, nodeNames []string) error { fwName := l4netlb.namer.L4Firewall(l4netlb.Service.Namespace, l4netlb.Service.Name) fwDesc, err := utils.MakeL4LBServiceDescription(utils.ServiceKeyFunc(l4netlb.Service.Namespace, l4netlb.Service.Name), "", meta.VersionGA, false, utils.XLB) if err != nil { return fmt.Errorf("failed to create description for resources, err %w", err) } - sourceRanges, err := utils.IPv4ServiceSourceRanges(l4netlb.Service) + sourceRanges, err := servicehelper.GetLoadBalancerSourceRanges(l4netlb.Service) if err != nil { return fmt.Errorf("servicehelper.GetLoadBalancerSourceRanges(%+v) returned error %v, want nil", l4netlb.Service, err) } - return verifyFirewall(l4netlb.cloud, nodeNames, fwName, fwDesc, sourceRanges) + return verifyFirewall(l4netlb.cloud, nodeNames, fwName, fwDesc, sourceRanges.StringSlice()) +} + +func verifyNetLBIPv6NodesFirewall(l4netlb *L4NetLB, nodeNames []string) error { + ipv6FirewallName := l4netlb.namer.L4IPv6Firewall(l4netlb.Service.Namespace, l4netlb.Service.Name) + + fwDesc, err := utils.MakeL4LBServiceDescription(utils.ServiceKeyFunc(l4netlb.Service.Namespace, l4netlb.Service.Name), "", meta.VersionGA, false, utils.XLB) + if err != nil { + return fmt.Errorf("failed to create description for resources, err %w", err) + } + + return verifyFirewall(l4netlb.cloud, nodeNames, ipv6FirewallName, fwDesc, []string{"0::0/0"}) } -func verifyNetLBHealthCheckFirewall(l4netlb *L4NetLB, nodeNames []string) error { +func verifyNetLBIPv4HealthCheckFirewall(l4netlb *L4NetLB, nodeNames []string) error { isSharedHC := !servicehelper.RequestsOnlyLocalTraffic(l4netlb.Service) hcFwName := l4netlb.namer.L4HealthCheckFirewall(l4netlb.Service.Namespace, l4netlb.Service.Name, isSharedHC) @@ -350,6 +879,18 @@ func verifyNetLBHealthCheckFirewall(l4netlb *L4NetLB, nodeNames []string) error return verifyFirewall(l4netlb.cloud, nodeNames, hcFwName, hcFwDesc, gce.L4LoadBalancerSrcRanges()) } +func verifyNetLBIPv6HealthCheckFirewall(l4netlb *L4NetLB, nodeNames []string) error { + isSharedHC := !servicehelper.RequestsOnlyLocalTraffic(l4netlb.Service) + + ipv6hcFwName := l4netlb.namer.L4IPv6HealthCheckFirewall(l4netlb.Service.Namespace, l4netlb.Service.Name, isSharedHC) + hcFwDesc, err := utils.MakeL4LBFirewallDescription(utils.ServiceKeyFunc(l4netlb.Service.Namespace, l4netlb.Service.Name), "", meta.VersionGA, isSharedHC) + if err != nil { + return fmt.Errorf("failed to calculate decsription for health check for service %v, error %v", l4netlb.Service, err) + } + + return verifyFirewall(l4netlb.cloud, nodeNames, ipv6hcFwName, hcFwDesc, []string{healthchecksl4.L4NetLBIPv6HCRange}) +} + func getAndVerifyNetLBHealthCheck(l4netlb *L4NetLB) (*composite.HealthCheck, error) { isSharedHC := !servicehelper.RequestsOnlyLocalTraffic(l4netlb.Service) hcName := l4netlb.namer.L4HealthCheck(l4netlb.Service.Namespace, l4netlb.Service.Name, isSharedHC) @@ -386,7 +927,7 @@ func getAndVerifyNetLBBackendService(l4netlb *L4NetLB, healthCheck *composite.He } backendServiceLink := cloud.SelfLink(meta.VersionGA, l4netlb.cloud.ProjectID(), "backendServices", key) if bs.SelfLink != backendServiceLink { - return nil, fmt.Errorf("unexpected self link in backend service with name %s - Expected %s, Got %s", backendServiceName, bs.SelfLink, backendServiceLink) + return nil, fmt.Errorf("unexpected self link in backend service - Expected %s, Got %s", bs.SelfLink, backendServiceLink) } resourceDesc, err := utils.MakeL4LBServiceDescription(utils.ServiceKeyFunc(l4netlb.Service.Namespace, l4netlb.Service.Name), "", meta.VersionGA, false, utils.XLB) @@ -403,19 +944,31 @@ func getAndVerifyNetLBBackendService(l4netlb *L4NetLB, healthCheck *composite.He return bs, nil } -func verifyNetLBForwardingRule(l4netlb *L4NetLB, backendServiceLink string) error { - frName := l4netlb.GetFRName() +func verifyNetLBIPv4ForwardingRule(l4netlb *L4NetLB, backendServiceLink string) error { + frName := l4netlb.frName() + return verifyNetLBForwardingRule(l4netlb, frName, backendServiceLink) +} + +func verifyNetLBIPv6ForwardingRule(l4netlb *L4NetLB, backendServiceLink string) error { + ipv6FrName := l4netlb.ipv6FRName() + return verifyNetLBForwardingRule(l4netlb, ipv6FrName, backendServiceLink) +} + +func verifyNetLBForwardingRule(l4netlb *L4NetLB, frName string, backendServiceLink string) error { fwdRule, err := composite.GetForwardingRule(l4netlb.cloud, meta.RegionalKey(frName, l4netlb.cloud.Region()), meta.VersionGA) if err != nil { return fmt.Errorf("failed to fetch forwarding rule %s - err %w", frName, err) } + if fwdRule.Name != frName { + return fmt.Errorf("unexpected name for forwarding rule '%s' - expected '%s'", fwdRule.Name, frName) + } if fwdRule.LoadBalancingScheme != string(cloud.SchemeExternal) { return fmt.Errorf("unexpected LoadBalancingScheme for forwarding rule '%s' - expected '%s'", fwdRule.LoadBalancingScheme, cloud.SchemeExternal) } proto := utils.GetProtocol(l4netlb.Service.Spec.Ports) if fwdRule.IPProtocol != string(proto) { - return fmt.Errorf("expected protocol '%s', got '%s' for forwarding rule %v", proto, fwdRule.IPProtocol, fwdRule) + return fmt.Errorf("unexpected protocol '%s' for forwarding rule %v", fwdRule.IPProtocol, fwdRule) } if fwdRule.BackendService != backendServiceLink { @@ -429,120 +982,86 @@ func verifyNetLBForwardingRule(l4netlb *L4NetLB, backendServiceLink string) erro return nil } -func TestMetricsForStandardNetworkTier(t *testing.T) { - nodeNames := []string{"test-node-1"} - vals := gce.DefaultTestClusterValues() - fakeGCE := getFakeGCECloud(vals) - createUserStaticIPInStandardTier(fakeGCE, vals.Region) - (fakeGCE.Compute().(*cloud.MockGCE)).MockForwardingRules.GetHook = test.GetRBSForwardingRuleInStandardTier - - svc := test.NewL4NetLBRBSService(8080) - svc.Spec.LoadBalancerIP = usersIP - svc.ObjectMeta.Annotations[annotations.NetworkTierAnnotationKey] = string(cloud.NetworkTierStandard) - namer := namer_util.NewL4Namer(kubeSystemUID, namer_util.NewNamer(vals.ClusterName, "cluster-fw")) - - l4NetLBParams := &L4NetLBParams{ - Service: svc, - Cloud: fakeGCE, - Namer: namer, - Recorder: record.NewFakeRecorder(100), +// we don't delete ipv4 health check firewall on sync +func verifyNetLBIPv4ResourcesDeletedOnSync(l4netlb *L4NetLB) error { + ipv4FwName := l4netlb.namer.L4Backend(l4netlb.Service.Namespace, l4netlb.Service.Name) + err := verifyFirewallNotExists(l4netlb.cloud, ipv4FwName) + if err != nil { + return fmt.Errorf("verifyFirewallNotExists(_, %s) returned error %w, want nil", ipv4FwName, err) } - l4netlb := NewL4NetLB(l4NetLBParams) - l4netlb.healthChecks = healthchecksl4.Fake(fakeGCE, l4netlb.recorder) - if _, err := test.CreateAndInsertNodes(l4netlb.cloud, nodeNames, vals.ZoneName); err != nil { - t.Errorf("Unexpected error when adding nodes %v", err) - } - result := l4netlb.EnsureFrontend(nodeNames, svc) - if result.Error != nil { - t.Errorf("Failed to ensure loadBalancer, err %v", result.Error) - } - if err := checkMetrics(result.MetricsState /*isManaged = */, false /*isPremium = */, false /*isUserError =*/, false); err != nil { - t.Errorf("Metrics error: %v", err) - } - // Check that service sync will return error if User Address IP Network Tier mismatch with service Network Tier. - svc.ObjectMeta.Annotations[annotations.NetworkTierAnnotationKey] = string(cloud.NetworkTierPremium) - result = l4netlb.EnsureFrontend(nodeNames, svc) - if result.Error == nil || !utils.IsNetworkTierError(result.Error) { - t.Errorf("LoadBalancer sync should return Network Tier error, err %v", result.Error) + ipv4FrName := l4netlb.frName() + err = verifyForwardingRuleNotExists(l4netlb.cloud, ipv4FrName) + if err != nil { + return fmt.Errorf("verifyForwardingRuleNotExists(_, %s) returned error %w, want nil", ipv4FrName, err) } - if err := checkMetrics(result.MetricsState /*isManaged = */, false /*isPremium = */, false /*isUserError =*/, true); err != nil { - t.Errorf("Metrics error: %v", err) + + addressName := ipv4FwName + err = verifyAddressNotExists(l4netlb.cloud, addressName) + if err != nil { + return fmt.Errorf("verifyAddressNotExists(_, %s)", addressName) } - // Check that when network tier annotation will be deleted which will change desired service Network Tier to PREMIUM - // service sync will return User Error because we do not support updating forwarding rule. - // Forwarding rule with wrong tier should be tear down and it can be done only via annotation change. - // Crete new Static IP in Premium Network Tier to match default service Network Tier. - createUserStaticIPInPremiumTier(fakeGCE, vals.Region) - svc.Spec.LoadBalancerIP = usersIPPremium - delete(svc.ObjectMeta.Annotations, annotations.NetworkTierAnnotationKey) + return nil +} - result = l4netlb.EnsureFrontend(nodeNames, svc) - if result.Error == nil || !utils.IsNetworkTierError(result.Error) { - t.Errorf("LoadBalancer sync should return Network Tier error, err %v", result.Error) +// we don't delete ipv6 health check firewall on sync +func verifyNetLBIPv6ResourcesDeletedOnSync(l4netlb *L4NetLB) error { + ipv6FwName := l4netlb.namer.L4IPv6Firewall(l4netlb.Service.Namespace, l4netlb.Service.Name) + err := verifyFirewallNotExists(l4netlb.cloud, ipv6FwName) + if err != nil { + return fmt.Errorf("verifyFirewallNotExists(_, %s) returned error %w, want nil", ipv6FwName, err) } - if err := checkMetrics(result.MetricsState /*isManaged = */, false /*isPremium = */, false /*isUserError =*/, true); err != nil { - t.Errorf("Metrics error: %v", err) + + ipv6FrName := l4netlb.ipv6FRName() + err = verifyForwardingRuleNotExists(l4netlb.cloud, ipv6FrName) + if err != nil { + return fmt.Errorf("verifyForwardingRuleNotExists(_, %s) returned error %w, want nil", ipv6FrName, err) } + + return nil } -func TestEnsureNetLBFirewallDestinations(t *testing.T) { - nodeNames := []string{"test-node-1"} - vals := gce.DefaultTestClusterValues() - fakeGCE := getFakeGCECloud(vals) +func buildExpectedNetLBAnnotations(l4netlb *L4NetLB) map[string]string { + isSharedHC := !servicehelper.RequestsOnlyLocalTraffic(l4netlb.Service) + proto := utils.GetProtocol(l4netlb.Service.Spec.Ports) - svc := test.NewL4NetLBRBSService(8080) - namer := namer_util.NewL4Namer(kubeSystemUID, nil) - l4NetLBParams := &L4NetLBParams{ - Service: svc, - Cloud: fakeGCE, - Namer: namer, - Recorder: record.NewFakeRecorder(100), - } - l4netlb := NewL4NetLB(l4NetLBParams) - l4netlb.healthChecks = healthchecksl4.Fake(fakeGCE, l4netlb.recorder) + backendName := l4netlb.namer.L4Backend(l4netlb.Service.Namespace, l4netlb.Service.Name) + hcName := l4netlb.namer.L4HealthCheck(l4netlb.Service.Namespace, l4netlb.Service.Name, isSharedHC) - if _, err := test.CreateAndInsertNodes(l4netlb.cloud, nodeNames, vals.ZoneName); err != nil { - t.Errorf("Unexpected error when adding nodes %v", err) + expectedAnnotations := map[string]string{ + annotations.BackendServiceKey: backendName, + annotations.HealthcheckKey: hcName, } - flags.F.EnablePinhole = true - fwName := l4netlb.namer.L4Backend(l4netlb.Service.Namespace, l4netlb.Service.Name) + if utils.NeedsIPv4(l4netlb.Service) { + hcFwName := l4netlb.namer.L4HealthCheckFirewall(l4netlb.Service.Namespace, l4netlb.Service.Name, isSharedHC) - fwrParams := firewalls.FirewallParams{ - Name: fwName, - SourceRanges: []string{"10.0.0.0/20"}, - DestinationRanges: []string{"20.0.0.0/20"}, - NodeNames: nodeNames, - Protocol: string(v1.ProtocolTCP), - IP: "1.2.3.4", - } + expectedAnnotations[annotations.FirewallRuleForHealthcheckKey] = hcFwName + expectedAnnotations[annotations.FirewallRuleKey] = backendName - err := firewalls.EnsureL4FirewallRule(l4netlb.cloud, utils.ServiceKeyFunc(svc.Namespace, svc.Name), &fwrParams /*sharedRule = */, false) - if err != nil { - t.Errorf("Unexpected error %v when ensuring firewall rule %s for svc %+v", err, fwName, svc) - } - existingFirewall, err := l4netlb.cloud.GetFirewall(fwName) - if err != nil || existingFirewall == nil || len(existingFirewall.Allowed) == 0 { - t.Errorf("Unexpected error %v when looking up firewall %s, Got firewall %+v", err, fwName, existingFirewall) - } - oldDestinationRanges := existingFirewall.DestinationRanges - - fwrParams.DestinationRanges = []string{"30.0.0.0/20"} - err = firewalls.EnsureL4FirewallRule(l4netlb.cloud, utils.ServiceKeyFunc(svc.Namespace, svc.Name), &fwrParams /*sharedRule = */, false) - if err != nil { - t.Errorf("Unexpected error %v when ensuring firewall rule %s for svc %+v", err, fwName, svc) + ipv4FRName := l4netlb.frName() + if proto == v1.ProtocolTCP { + expectedAnnotations[annotations.TCPForwardingRuleKey] = ipv4FRName + } else { + expectedAnnotations[annotations.UDPForwardingRuleKey] = ipv4FRName + } } + if utils.NeedsIPv6(l4netlb.Service) { + ipv6hcFwName := l4netlb.namer.L4IPv6HealthCheckFirewall(l4netlb.Service.Namespace, l4netlb.Service.Name, isSharedHC) + ipv6FirewallName := l4netlb.namer.L4IPv6Firewall(l4netlb.Service.Namespace, l4netlb.Service.Name) - updatedFirewall, err := l4netlb.cloud.GetFirewall(fwName) - if err != nil || updatedFirewall == nil || len(updatedFirewall.Allowed) == 0 { - t.Errorf("Unexpected error %v when looking up firewall %s, Got firewall %+v", err, fwName, updatedFirewall) - } + expectedAnnotations[annotations.FirewallRuleForHealthcheckIPv6Key] = ipv6hcFwName + expectedAnnotations[annotations.FirewallRuleIPv6Key] = ipv6FirewallName - if reflect.DeepEqual(oldDestinationRanges, updatedFirewall.DestinationRanges) { - t.Errorf("DestinationRanges is not updated. oldDestinationRanges:%v, updatedFirewall.DestinationRanges:%v", oldDestinationRanges, updatedFirewall.DestinationRanges) + ipv6FRName := l4netlb.ipv6FRName() + if proto == v1.ProtocolTCP { + expectedAnnotations[annotations.TCPForwardingRuleIPv6Key] = ipv6FRName + } else { + expectedAnnotations[annotations.UDPForwardingRuleIPv6Key] = ipv6FRName + } } + return expectedAnnotations } func createUserStaticIPInStandardTier(fakeGCE *gce.Cloud, region string) { diff --git a/pkg/loadbalancers/l4netlbipv6.go b/pkg/loadbalancers/l4netlbipv6.go new file mode 100644 index 0000000000..d8f0ff1353 --- /dev/null +++ b/pkg/loadbalancers/l4netlbipv6.go @@ -0,0 +1,176 @@ +/* +Copyright 2023 The Kubernetes Authors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package loadbalancers + +import ( + "strings" + "time" + + corev1 "k8s.io/api/core/v1" + "k8s.io/ingress-gce/pkg/annotations" + "k8s.io/ingress-gce/pkg/composite" + "k8s.io/ingress-gce/pkg/firewalls" + "k8s.io/ingress-gce/pkg/utils" + "k8s.io/ingress-gce/pkg/utils/namer" + "k8s.io/klog/v2" +) + +const ( + ipv6Suffix = "-ipv6" +) + +// ensureIPv6Resources creates resources specific to IPv6 L4 NetLB Load Balancers: +// - IPv6 Forwarding Rule +// - IPv6 Firewall +// it also adds IPv6 address to LB status +func (l4netlb *L4NetLB) ensureIPv6Resources(syncResult *L4NetLBSyncResult, nodeNames []string, bsLink string) { + ipv6fr, err := l4netlb.ensureIPv6ForwardingRule(bsLink) + if err != nil { + klog.Errorf("ensureIPv6Resources: Failed to create ipv6 forwarding rule - %v", err) + syncResult.GCEResourceInError = annotations.ForwardingRuleIPv6Resource + syncResult.Error = err + return + } + + if ipv6fr.IPProtocol == string(corev1.ProtocolTCP) { + syncResult.Annotations[annotations.TCPForwardingRuleIPv6Key] = ipv6fr.Name + } else { + syncResult.Annotations[annotations.UDPForwardingRuleIPv6Key] = ipv6fr.Name + } + + l4netlb.ensureIPv6NodesFirewall(ipv6fr, nodeNames, syncResult) + if syncResult.Error != nil { + return + } + + trimmedIPv6Address := strings.Split(ipv6fr.IPAddress, "/")[0] + syncResult.Status = utils.AddIPToLBStatus(syncResult.Status, trimmedIPv6Address) +} + +// deleteIPv6ResourcesOnSync deletes resources specific to IPv6, +// only if corresponding resource annotation exist on Service object. +// This function is called only on Service update or periodic sync. +// Checking for annotation saves us from emitting too much error logs "Resource not found". +// If annotation was deleted, but resource still exists, it will be left till the Service deletion, +// where we delete all resources, no matter if they exist in annotations. +func (l4netlb *L4NetLB) deleteIPv6ResourcesOnSync(syncResult *L4NetLBSyncResult) { + l4netlb.deleteIPv6ResourcesAnnotationBased(syncResult, false) +} + +// deleteIPv6ResourcesOnDelete deletes all resources specific to IPv6. +// This function is called only on Service deletion. +// During sync, we delete resources only that exist in annotations, +// so they could be leaked, if annotation was deleted. +// That's why on service deletion we delete all IPv4 resources, ignoring their existence in annotations +func (l4netlb *L4NetLB) deleteIPv6ResourcesOnDelete(syncResult *L4NetLBSyncResult) { + l4netlb.deleteIPv6ResourcesAnnotationBased(syncResult, true) +} + +// deleteIPv6ResourcesAnnotationBased deletes IPv6 only resources with checking, +// if resource exists in Service annotation, if shouldIgnoreAnnotations not set to true +// IPv6 Specific resources: +// - IPv6 Forwarding Rule +// - IPv6 Firewall +// This function does not delete Backend Service and Health Check, because they are shared between IPv4 and IPv6. +// IPv6 Firewall Rule for Health Check also will not be deleted here, and will be left till the Service Deletion. +func (l4netlb *L4NetLB) deleteIPv6ResourcesAnnotationBased(syncResult *L4NetLBSyncResult, shouldIgnoreAnnotations bool) { + if shouldIgnoreAnnotations || l4netlb.hasAnnotation(annotations.TCPForwardingRuleIPv6Key) || l4netlb.hasAnnotation(annotations.UDPForwardingRuleIPv6Key) { + l4netlb.deleteIPv6ForwardingRule(syncResult) + } + + if shouldIgnoreAnnotations || l4netlb.hasAnnotation(annotations.FirewallRuleIPv6Key) { + l4netlb.deleteIPv6NodesFirewall(syncResult) + } +} + +func (l4netlb *L4NetLB) ipv6FRName() string { + return namer.GetSuffixedName(l4netlb.frName(), ipv6Suffix) +} + +func (l4netlb *L4NetLB) ensureIPv6NodesFirewall(forwardingRule *composite.ForwardingRule, nodeNames []string, syncResult *L4NetLBSyncResult) { + start := time.Now() + + firewallName := l4netlb.namer.L4IPv6Firewall(l4netlb.Service.Namespace, l4netlb.Service.Name) + svcPorts := l4netlb.Service.Spec.Ports + portRanges := utils.GetServicePortRanges(svcPorts) + protocol := utils.GetProtocol(svcPorts) + + klog.V(2).Infof("Ensuring IPv6 nodes firewall %s for L4 ILB Service %s/%s, ipAddress: %s, protocol: %s, len(nodeNames): %v, portRanges: %v", firewallName, l4netlb.Service.Namespace, l4netlb.Service.Name, forwardingRule.IPAddress, protocol, len(nodeNames), portRanges) + defer func() { + klog.V(2).Infof("Finished ensuring IPv6 nodes firewall %s for L4 ILB Service %s/%s, time taken: %v", l4netlb.Service.Namespace, l4netlb.Service.Name, firewallName, time.Since(start)) + }() + + // ensure firewalls + ipv6SourceRanges, err := utils.IPv6ServiceSourceRanges(l4netlb.Service) + if err != nil { + syncResult.Error = err + return + } + + ipv6nodesFWRParams := firewalls.FirewallParams{ + PortRanges: portRanges, + SourceRanges: ipv6SourceRanges, + DestinationRanges: []string{forwardingRule.IPAddress}, + Protocol: string(protocol), + Name: firewallName, + NodeNames: nodeNames, + L4Type: utils.XLB, + } + + err = firewalls.EnsureL4LBFirewallForNodes(l4netlb.Service, &ipv6nodesFWRParams, l4netlb.cloud, l4netlb.recorder) + if err != nil { + klog.Errorf("Failed to ensure ipv6 nodes firewall %s for L4 NetLB - %v", firewallName, err) + syncResult.GCEResourceInError = annotations.FirewallRuleIPv6Resource + syncResult.Error = err + return + } + syncResult.Annotations[annotations.FirewallRuleIPv6Key] = firewallName +} + +func (l4netlb *L4NetLB) deleteIPv6ForwardingRule(syncResult *L4NetLBSyncResult) { + ipv6FrName := l4netlb.ipv6FRName() + + start := time.Now() + klog.V(2).Infof("Deleting IPv6 forwarding rule %s for L4 NetLB Service %s/%s", ipv6FrName, l4netlb.Service.Namespace, l4netlb.Service.Name) + defer func() { + klog.V(2).Infof("Finished deleting IPv6 forwarding rule %s for L4 NetLB Service %s/%s, time taken: %v", ipv6FrName, l4netlb.Service.Namespace, l4netlb.Service.Name, time.Since(start)) + }() + + err := l4netlb.forwardingRules.Delete(ipv6FrName) + if err != nil { + klog.Errorf("Failed to delete ipv6 forwarding rule for external loadbalancer service %s, err %v", l4netlb.NamespacedName.String(), err) + syncResult.Error = err + syncResult.GCEResourceInError = annotations.ForwardingRuleIPv6Resource + } +} + +func (l4netlb *L4NetLB) deleteIPv6NodesFirewall(syncResult *L4NetLBSyncResult) { + ipv6FirewallName := l4netlb.namer.L4IPv6Firewall(l4netlb.Service.Namespace, l4netlb.Service.Name) + + start := time.Now() + klog.V(2).Infof("Deleting IPv6 nodes firewall %s for L4 NetLB Service %s/%s", ipv6FirewallName, l4netlb.Service.Namespace, l4netlb.Service.Name) + defer func() { + klog.V(2).Infof("Finished deleting IPv6 nodes firewall %s for L4 NetLB Service %s/%s, time taken: %v", ipv6FirewallName, l4netlb.Service.Namespace, l4netlb.Service.Name, time.Since(start)) + }() + + err := l4netlb.deleteFirewall(ipv6FirewallName) + if err != nil { + klog.Errorf("Failed to delete ipv6 firewall rule for external loadbalancer service %s, err %v", l4netlb.NamespacedName.String(), err) + syncResult.GCEResourceInError = annotations.FirewallRuleIPv6Resource + syncResult.Error = err + } +} diff --git a/pkg/loadbalancers/l4netlbipv6_test.go b/pkg/loadbalancers/l4netlbipv6_test.go new file mode 100644 index 0000000000..26d4274f86 --- /dev/null +++ b/pkg/loadbalancers/l4netlbipv6_test.go @@ -0,0 +1,61 @@ +/* +Copyright 2023 The Kubernetes Authors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package loadbalancers + +import ( + "testing" + + "k8s.io/apimachinery/pkg/types" + "k8s.io/ingress-gce/pkg/test" +) + +func TestIPv6FRName(t *testing.T) { + testCases := []struct { + desc string + svcUID types.UID + expectedIPv6FRName string + }{ + { + desc: "Should add -ipv6 suffix to normal RBS forwarding rule name", + svcUID: "09e0afadb26f4dccbc25d00f971b289", + expectedIPv6FRName: "a09e0afadb26f4dccbc25d00f971b289-ipv6", + }, + { + // this test checks theoretical situation, in reality, we never have services with such a long UIDs + desc: "Should trim and add -ipv6 suffix to normal RBS forwarding rule name", + svcUID: "09e0afadb26f4dccbc25d00f971b289a09e0afadb26f4dccbc25d00f971b289", + expectedIPv6FRName: "a09e0afadb26f4dccbc25d00f971b289-ipv6", + }, + } + + for _, tc := range testCases { + tc := tc + t.Run(tc.desc, func(t *testing.T) { + svc := test.NewL4NetLBRBSService(8080) + svc.UID = tc.svcUID + l4NetLBParams := &L4NetLBParams{ + Service: svc, + } + l4NetLB := NewL4NetLB(l4NetLBParams) + + ipv6FRName := l4NetLB.ipv6FRName() + if ipv6FRName != tc.expectedIPv6FRName { + t.Errorf("Expecetd ipv6 forwarding rule name: %s, got %s", tc.expectedIPv6FRName, ipv6FRName) + } + }) + } +} diff --git a/pkg/loadbalancers/l4syncresult.go b/pkg/loadbalancers/l4syncresult.go index 7c87aac28d..9dce0ab083 100644 --- a/pkg/loadbalancers/l4syncresult.go +++ b/pkg/loadbalancers/l4syncresult.go @@ -43,3 +43,4 @@ var l4IPv6AnnotationKeys = []string{ annotations.UDPForwardingRuleIPv6Key, } var L4DualStackResourceAnnotationKeys = append(L4LBResourceAnnotationKeys, l4IPv6AnnotationKeys...) +var L4DualStackResourceRBSAnnotationKeys = append(L4DualStackResourceAnnotationKeys, annotations.RBSAnnotationKey) diff --git a/pkg/test/utils.go b/pkg/test/utils.go index ead95efd1d..a7f7638121 100644 --- a/pkg/test/utils.go +++ b/pkg/test/utils.go @@ -153,6 +153,18 @@ func NewL4NetLBRBSService(port int) *api_v1.Service { return svc } +// NewL4NetLBRBSDualStackService creates a Service of type LoadBalancer with RBS Annotation and provided ipFamilies and ipFamilyPolicy +func NewL4NetLBRBSDualStackService(port int, protocol api_v1.Protocol, ipFamilies []api_v1.IPFamily, externalTrafficPolicy api_v1.ServiceExternalTrafficPolicyType) *api_v1.Service { + svc := NewL4LegacyNetLBServiceWithoutPorts() + svc.ObjectMeta.Annotations[annotations.RBSAnnotationKey] = annotations.RBSEnabled + svc.Spec.Ports = []api_v1.ServicePort{ + {Name: "testport", Port: int32(port), Protocol: protocol}, + } + svc.Spec.IPFamilies = ipFamilies + svc.Spec.ExternalTrafficPolicy = externalTrafficPolicy + return svc +} + // NewL4NetLBRBSServiceMultiplePorts creates a Service of type LoadBalancer with multiple named ports. func NewL4NetLBRBSServiceMultiplePorts(name string, ports []int32) *api_v1.Service { svc := NewL4LegacyNetLBServiceWithoutPorts() diff --git a/pkg/utils/namer/l4_namer.go b/pkg/utils/namer/l4_namer.go index 69ea24f7e2..1aaa9e0ac1 100644 --- a/pkg/utils/namer/l4_namer.go +++ b/pkg/utils/namer/l4_namer.go @@ -79,7 +79,7 @@ func (namer *L4Namer) L4Firewall(namespace, name string) string { // // Output name is at most 63 characters. func (namer *L4Namer) L4IPv6Firewall(namespace, name string) string { - return getSuffixedName(namer.L4Backend(namespace, name), "-"+ipv6Suffix) + return GetSuffixedName(namer.L4Backend(namespace, name), "-"+ipv6Suffix) } // L4ForwardingRule returns the name of the L4 forwarding rule name based on the service namespace, name and protocol. @@ -125,7 +125,7 @@ func (namer *L4Namer) L4HealthCheckFirewall(namespace, name string, shared bool) // // Output name is at most 63 characters. func (namer *L4Namer) L4IPv6ForwardingRule(namespace, name, protocol string) string { - return getSuffixedName(namer.L4ForwardingRule(namespace, name, protocol), "-"+ipv6Suffix) + return GetSuffixedName(namer.L4ForwardingRule(namespace, name, protocol), "-"+ipv6Suffix) } // L4IPv6HealthCheckFirewall returns the name of the IPv6 L4 LB health check firewall rule. @@ -152,14 +152,14 @@ func (n *L4Namer) getClusterSuffix(namespace, name string) string { // hcFirewallName generates the firewall name for the given healthcheck. // It ensures that the name is at most 63 chars long. func (n *L4Namer) hcFirewallName(hcName, suffix string) string { - return getSuffixedName(hcName, firewallHcSuffix+suffix) + return GetSuffixedName(hcName, firewallHcSuffix+suffix) } func getTrimmedNamespacedName(namespace, name string, maxLength int) string { return strings.Join(TrimFieldsEvenly(maxLength, namespace, name), "-") } -func getSuffixedName(name string, suffix string) string { +func GetSuffixedName(name string, suffix string) string { return ensureSpaceForSuffix(name, suffix) + suffix } From aac0e6771910ff4644f790ff60c4ae9479d3bf59 Mon Sep 17 00:00:00 2001 From: Slavik Panasovets Date: Mon, 13 Feb 2023 14:03:01 +0000 Subject: [PATCH 2/4] Move all time.Start to the beggining of functions, add measuring ipv6 external forwarding rule time --- pkg/healthchecksl4/healthchecksl4.go | 9 ++++++--- pkg/loadbalancers/forwarding_rules.go | 3 ++- pkg/loadbalancers/forwarding_rules_ipv6.go | 10 +++++++++- pkg/loadbalancers/l4.go | 6 ++++-- pkg/loadbalancers/l4ipv6.go | 3 ++- pkg/loadbalancers/l4netlb.go | 6 ++++-- 6 files changed, 27 insertions(+), 10 deletions(-) diff --git a/pkg/healthchecksl4/healthchecksl4.go b/pkg/healthchecksl4/healthchecksl4.go index 176ef1e7a3..547aebc11f 100644 --- a/pkg/healthchecksl4/healthchecksl4.go +++ b/pkg/healthchecksl4/healthchecksl4.go @@ -208,9 +208,10 @@ func (l4hc *l4HealthChecks) ensureHealthCheck(hcName string, svcName types.Names // L4 ILB and L4 NetLB Services with ExternalTrafficPolicy=Cluster use the same firewall // rule at global scope. func (l4hc *l4HealthChecks) ensureIPv4Firewall(svc *corev1.Service, namer namer.L4ResourcesNamer, hcPort int32, isSharedHC bool, nodeNames []string, hcResult *EnsureHealthCheckResult) { + start := time.Now() + hcFwName := namer.L4HealthCheckFirewall(svc.Namespace, svc.Name, isSharedHC) - start := time.Now() klog.V(2).Infof("Ensuring IPv4 Firewall for health check %s for service %s/%s, health check port %d, shared health check: %t, len(nodeNames): %d", hcFwName, svc.Namespace, svc.Name, hcPort, isSharedHC, len(nodeNames)) defer func() { klog.V(2).Infof("Finished ensuring IPv4 firewall for health check %s for service %s/%s, time taken %v", hcFwName, svc.Namespace, svc.Name, time.Since(start)) @@ -302,9 +303,10 @@ func (l4hc *l4HealthChecks) deleteHealthCheckWithDualStackFirewalls(svc *corev1. } func (l4hc *l4HealthChecks) deleteHealthCheck(svc *corev1.Service, namer namer.L4ResourcesNamer, sharedHC bool, scope meta.KeyType) (bool, error) { + start := time.Now() + hcName := namer.L4HealthCheck(svc.Namespace, svc.Name, sharedHC) - start := time.Now() klog.V(3).Infof("Deleting L4 healthcheck %s for service %s/%s, shared: %v, scope: %v", hcName, svc.Namespace, svc.Name, sharedHC, scope) defer func() { klog.V(3).Infof("Finished deleting L4 healthcheck %s for service %s/%s, time taken: %v", hcName, svc.Namespace, svc.Name, time.Since(start)) @@ -337,10 +339,11 @@ func (l4hc *l4HealthChecks) deleteIPv4HealthCheckFirewall(svc *corev1.Service, n } func (l4hc *l4HealthChecks) deleteIPv6HealthCheckFirewall(svc *corev1.Service, namer namer.L4ResourcesNamer, isSharedHC bool, l4type utils.L4LBType) (string, error) { + start := time.Now() + hcName := namer.L4HealthCheck(svc.Namespace, svc.Name, isSharedHC) ipv6hcFwName := namer.L4IPv6HealthCheckFirewall(svc.Namespace, svc.Name, isSharedHC) - start := time.Now() klog.V(3).Infof("Deleting IPv6 Firewall %s for health check %s", ipv6hcFwName, hcName) defer func() { klog.V(3).Infof("Finished deleting IPv6 Firewall %s for health check %s, time taken: %v", ipv6hcFwName, hcName, time.Since(start)) diff --git a/pkg/loadbalancers/forwarding_rules.go b/pkg/loadbalancers/forwarding_rules.go index 9d697214a4..eb1d869377 100644 --- a/pkg/loadbalancers/forwarding_rules.go +++ b/pkg/loadbalancers/forwarding_rules.go @@ -207,11 +207,12 @@ 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) { + start := time.Now() + // version used for creating the existing forwarding rule. version := meta.VersionGA frName := l4.GetFRName() - start := time.Now() klog.V(2).Infof("Ensuring internal forwarding rule %s for L4 ILB Service %s/%s, backend service link: %s", frName, l4.Service.Namespace, l4.Service.Name, bsLink) defer func() { klog.V(2).Infof("Finished ensuring internal forwarding rule %s for L4 ILB Service %s/%s, time taken: %v", frName, l4.Service.Namespace, l4.Service.Name, time.Since(start)) diff --git a/pkg/loadbalancers/forwarding_rules_ipv6.go b/pkg/loadbalancers/forwarding_rules_ipv6.go index 46591b59d3..7cc7fa6c38 100644 --- a/pkg/loadbalancers/forwarding_rules_ipv6.go +++ b/pkg/loadbalancers/forwarding_rules_ipv6.go @@ -32,12 +32,13 @@ import ( ) func (l4 *L4) ensureIPv6ForwardingRule(bsLink string, options gce.ILBOptions) (*composite.ForwardingRule, error) { + start := time.Now() + expectedIPv6FwdRule, err := l4.buildExpectedIPv6ForwardingRule(bsLink, options) if err != nil { return nil, fmt.Errorf("l4.buildExpectedIPv6ForwardingRule(%s, %v) returned error %w, want nil", bsLink, options, err) } - start := time.Now() klog.V(2).Infof("Ensuring internal ipv6 forwarding rule %s for L4 ILB Service %s/%s, backend service link: %s", expectedIPv6FwdRule.Name, l4.Service.Namespace, l4.Service.Name, bsLink) defer func() { klog.V(2).Infof("Finished ensuring internal ipv6 forwarding rule %s for L4 ILB Service %s/%s, time taken: %v", expectedIPv6FwdRule.Name, l4.Service.Namespace, l4.Service.Name, time.Since(start)) @@ -128,11 +129,18 @@ func (l4 *L4) deleteChangedIPv6ForwardingRule(existingFwdRule *composite.Forward } func (l4netlb *L4NetLB) ensureIPv6ForwardingRule(bsLink string) (*composite.ForwardingRule, error) { + start := time.Now() + expectedIPv6FwdRule, err := l4netlb.buildExpectedIPv6ForwardingRule(bsLink) if err != nil { return nil, fmt.Errorf("l4netlb.buildExpectedIPv6ForwardingRule(%s) returned error %w, want nil", bsLink, err) } + klog.V(2).Infof("Ensuring external ipv6 forwarding rule %s for L4 NetLB Service %s/%s, backend service link: %s", expectedIPv6FwdRule.Name, l4netlb.Service.Namespace, l4netlb.Service.Name, bsLink) + defer func() { + klog.V(2).Infof("Finished ensuring external ipv6 forwarding rule %s for L4 NetLB Service %s/%s, time taken: %v", expectedIPv6FwdRule.Name, l4netlb.Service.Namespace, l4netlb.Service.Name, time.Since(start)) + }() + existingIPv6FwdRule, err := l4netlb.forwardingRules.Get(expectedIPv6FwdRule.Name) if err != nil { return nil, fmt.Errorf("l4netlb.forwardingRules.GetForwardingRule(%s) returned error %w, want nil", expectedIPv6FwdRule.Name, err) diff --git a/pkg/loadbalancers/l4.go b/pkg/loadbalancers/l4.go index 09ed9dd932..7abf525c53 100644 --- a/pkg/loadbalancers/l4.go +++ b/pkg/loadbalancers/l4.go @@ -217,9 +217,10 @@ func (l4 *L4) deleteIPv4ResourcesAnnotationBased(result *L4ILBSyncResult, should } func (l4 *L4) deleteIPv4ForwardingRule() error { + start := time.Now() + frName := l4.GetFRName() - start := time.Now() klog.Infof("Deleting IPv4 forwarding rule %s for L4 ILB Service %s/%s", frName, l4.Service.Namespace, l4.Service.Name) defer func() { klog.Infof("Finished deleting IPv4 forwarding rule %s for L4 ILB Service %s/%s, time taken: %v", frName, l4.Service.Namespace, l4.Service.Name, time.Since(start)) @@ -241,9 +242,10 @@ func (l4 *L4) deleteIPv4Address() error { } func (l4 *L4) deleteIPv4NodesFirewall() error { + start := time.Now() + firewallName := l4.namer.L4Firewall(l4.Service.Namespace, l4.Service.Name) - start := time.Now() klog.Infof("Deleting IPv4 nodes firewall %s for L4 ILB Service %s/%s", firewallName, l4.Service.Namespace, l4.Service.Name) defer func() { klog.Infof("Finished deleting IPv4 nodes firewall %s for L4 ILB Service %s/%s, time taken: %v", firewallName, l4.Service.Namespace, l4.Service.Name, time.Since(start)) diff --git a/pkg/loadbalancers/l4ipv6.go b/pkg/loadbalancers/l4ipv6.go index 156b4cc455..f2072ee976 100644 --- a/pkg/loadbalancers/l4ipv6.go +++ b/pkg/loadbalancers/l4ipv6.go @@ -156,9 +156,10 @@ func (l4 *L4) ensureIPv6NodesFirewall(forwardingRule *composite.ForwardingRule, } func (l4 *L4) deleteIPv6ForwardingRule() error { + start := time.Now() + ipv6FrName := l4.getIPv6FRName() - start := time.Now() klog.V(2).Infof("Deleting IPv6 forwarding rule %s for L4 ILB Service %s/%s", ipv6FrName, l4.Service.Namespace, l4.Service.Name) defer func() { klog.V(2).Infof("Finished deleting IPv6 forwarding rule %s for L4 ILB Service %s/%s, time taken: %v", ipv6FrName, l4.Service.Namespace, l4.Service.Name, time.Since(start)) diff --git a/pkg/loadbalancers/l4netlb.go b/pkg/loadbalancers/l4netlb.go index ef1537cf4f..2393c28695 100644 --- a/pkg/loadbalancers/l4netlb.go +++ b/pkg/loadbalancers/l4netlb.go @@ -355,9 +355,10 @@ func (l4netlb *L4NetLB) deleteIPv4ResourcesAnnotationBased(result *L4NetLBSyncRe } func (l4netlb *L4NetLB) deleteIPv4ForwardingRule() error { + start := time.Now() + frName := l4netlb.frName() - start := time.Now() klog.V(2).Infof("Deleting IPv4 external forwarding rule %s for L4 NetLB Service %s/%s", frName, l4netlb.Service.Namespace, l4netlb.Service.Name) defer func() { klog.V(2).Infof("Finished deleting IPv4 external forwarding rule %s for L4 NetLB Service %s/%s, time taken: %v", frName, l4netlb.Service.Namespace, l4netlb.Service.Name, time.Since(start)) @@ -379,9 +380,10 @@ func (l4netlb *L4NetLB) deleteIPv4Address() error { } func (l4netlb *L4NetLB) deleteIPv4NodesFirewall() error { + start := time.Now() + firewallName := l4netlb.namer.L4Firewall(l4netlb.Service.Namespace, l4netlb.Service.Name) - start := time.Now() klog.V(2).Infof("Deleting IPv4 nodes firewall %s for L4 NetLB Service %s/%s", firewallName, l4netlb.Service.Namespace, l4netlb.Service.Name) defer func() { klog.V(2).Infof("Finished deleting IPv4 nodes firewall %s for L4 NetLB Service %s/%s, time taken: %v", firewallName, l4netlb.Service.Namespace, l4netlb.Service.Name, time.Since(start)) From 807e5c24dd681aba491800ffb13a8955da6e1781 Mon Sep 17 00:00:00 2001 From: Slavik Panasovets Date: Mon, 13 Feb 2023 17:30:06 +0000 Subject: [PATCH 3/4] Create ipv6 firewalls only with trimmed ipv6 address, without CIDR --- pkg/loadbalancers/l4ipv6.go | 13 +++++++------ pkg/loadbalancers/l4netlbipv6.go | 13 +++++++------ 2 files changed, 14 insertions(+), 12 deletions(-) diff --git a/pkg/loadbalancers/l4ipv6.go b/pkg/loadbalancers/l4ipv6.go index f2072ee976..783458b77c 100644 --- a/pkg/loadbalancers/l4ipv6.go +++ b/pkg/loadbalancers/l4ipv6.go @@ -22,7 +22,6 @@ import ( corev1 "k8s.io/api/core/v1" "k8s.io/ingress-gce/pkg/annotations" - "k8s.io/ingress-gce/pkg/composite" "k8s.io/ingress-gce/pkg/firewalls" "k8s.io/ingress-gce/pkg/utils" "k8s.io/klog/v2" @@ -48,13 +47,15 @@ func (l4 *L4) ensureIPv6Resources(syncResult *L4ILBSyncResult, nodeNames []strin syncResult.Annotations[annotations.UDPForwardingRuleIPv6Key] = ipv6fr.Name } - l4.ensureIPv6NodesFirewall(ipv6fr, nodeNames, syncResult) + // Google Cloud creates ipv6 forwarding rules with IPAddress in CIDR form. We will take only first address + trimmedIPv6Address := strings.Split(ipv6fr.IPAddress, "/")[0] + + l4.ensureIPv6NodesFirewall(trimmedIPv6Address, nodeNames, syncResult) if syncResult.Error != nil { klog.Errorf("ensureIPv6Resources: Failed to ensure ipv6 nodes firewall for L4 ILB - %v", err) return } - trimmedIPv6Address := strings.Split(ipv6fr.IPAddress, "/")[0] syncResult.Status = utils.AddIPToLBStatus(syncResult.Status, trimmedIPv6Address) } @@ -115,7 +116,7 @@ func (l4 *L4) getIPv6FRNameWithProtocol(protocol string) string { return l4.namer.L4IPv6ForwardingRule(l4.Service.Namespace, l4.Service.Name, strings.ToLower(protocol)) } -func (l4 *L4) ensureIPv6NodesFirewall(forwardingRule *composite.ForwardingRule, nodeNames []string, result *L4ILBSyncResult) { +func (l4 *L4) ensureIPv6NodesFirewall(ipAddress string, nodeNames []string, result *L4ILBSyncResult) { start := time.Now() firewallName := l4.namer.L4IPv6Firewall(l4.Service.Namespace, l4.Service.Name) @@ -124,7 +125,7 @@ func (l4 *L4) ensureIPv6NodesFirewall(forwardingRule *composite.ForwardingRule, portRanges := utils.GetServicePortRanges(svcPorts) protocol := utils.GetProtocol(svcPorts) - klog.V(2).Infof("Ensuring IPv6 nodes firewall %s for L4 ILB Service %s/%s, ipAddress: %s, protocol: %s, len(nodeNames): %v, portRanges: %v", firewallName, l4.Service.Namespace, l4.Service.Name, forwardingRule.IPAddress, protocol, len(nodeNames), portRanges) + klog.V(2).Infof("Ensuring IPv6 nodes firewall %s for L4 ILB Service %s/%s, ipAddress: %s, protocol: %s, len(nodeNames): %v, portRanges: %v", firewallName, l4.Service.Namespace, l4.Service.Name, ipAddress, protocol, len(nodeNames), portRanges) defer func() { klog.V(2).Infof("Finished ensuring IPv6 nodes firewall %s for L4 ILB Service %s/%s, time taken: %v", l4.Service.Namespace, l4.Service.Name, firewallName, time.Since(start)) }() @@ -139,7 +140,7 @@ func (l4 *L4) ensureIPv6NodesFirewall(forwardingRule *composite.ForwardingRule, ipv6nodesFWRParams := firewalls.FirewallParams{ PortRanges: portRanges, SourceRanges: ipv6SourceRanges, - DestinationRanges: []string{forwardingRule.IPAddress}, + DestinationRanges: []string{ipAddress}, Protocol: string(protocol), Name: firewallName, NodeNames: nodeNames, diff --git a/pkg/loadbalancers/l4netlbipv6.go b/pkg/loadbalancers/l4netlbipv6.go index d8f0ff1353..b44f4a771c 100644 --- a/pkg/loadbalancers/l4netlbipv6.go +++ b/pkg/loadbalancers/l4netlbipv6.go @@ -22,7 +22,6 @@ import ( corev1 "k8s.io/api/core/v1" "k8s.io/ingress-gce/pkg/annotations" - "k8s.io/ingress-gce/pkg/composite" "k8s.io/ingress-gce/pkg/firewalls" "k8s.io/ingress-gce/pkg/utils" "k8s.io/ingress-gce/pkg/utils/namer" @@ -52,12 +51,14 @@ func (l4netlb *L4NetLB) ensureIPv6Resources(syncResult *L4NetLBSyncResult, nodeN syncResult.Annotations[annotations.UDPForwardingRuleIPv6Key] = ipv6fr.Name } - l4netlb.ensureIPv6NodesFirewall(ipv6fr, nodeNames, syncResult) + // Google Cloud creates ipv6 forwarding rules with IPAddress in CIDR form. We will take only first address + trimmedIPv6Address := strings.Split(ipv6fr.IPAddress, "/")[0] + + l4netlb.ensureIPv6NodesFirewall(trimmedIPv6Address, nodeNames, syncResult) if syncResult.Error != nil { return } - trimmedIPv6Address := strings.Split(ipv6fr.IPAddress, "/")[0] syncResult.Status = utils.AddIPToLBStatus(syncResult.Status, trimmedIPv6Address) } @@ -101,7 +102,7 @@ func (l4netlb *L4NetLB) ipv6FRName() string { return namer.GetSuffixedName(l4netlb.frName(), ipv6Suffix) } -func (l4netlb *L4NetLB) ensureIPv6NodesFirewall(forwardingRule *composite.ForwardingRule, nodeNames []string, syncResult *L4NetLBSyncResult) { +func (l4netlb *L4NetLB) ensureIPv6NodesFirewall(ipAddress string, nodeNames []string, syncResult *L4NetLBSyncResult) { start := time.Now() firewallName := l4netlb.namer.L4IPv6Firewall(l4netlb.Service.Namespace, l4netlb.Service.Name) @@ -109,7 +110,7 @@ func (l4netlb *L4NetLB) ensureIPv6NodesFirewall(forwardingRule *composite.Forwar portRanges := utils.GetServicePortRanges(svcPorts) protocol := utils.GetProtocol(svcPorts) - klog.V(2).Infof("Ensuring IPv6 nodes firewall %s for L4 ILB Service %s/%s, ipAddress: %s, protocol: %s, len(nodeNames): %v, portRanges: %v", firewallName, l4netlb.Service.Namespace, l4netlb.Service.Name, forwardingRule.IPAddress, protocol, len(nodeNames), portRanges) + klog.V(2).Infof("Ensuring IPv6 nodes firewall %s for L4 ILB Service %s/%s, ipAddress: %s, protocol: %s, len(nodeNames): %v, portRanges: %v", firewallName, l4netlb.Service.Namespace, l4netlb.Service.Name, ipAddress, protocol, len(nodeNames), portRanges) defer func() { klog.V(2).Infof("Finished ensuring IPv6 nodes firewall %s for L4 ILB Service %s/%s, time taken: %v", l4netlb.Service.Namespace, l4netlb.Service.Name, firewallName, time.Since(start)) }() @@ -124,7 +125,7 @@ func (l4netlb *L4NetLB) ensureIPv6NodesFirewall(forwardingRule *composite.Forwar ipv6nodesFWRParams := firewalls.FirewallParams{ PortRanges: portRanges, SourceRanges: ipv6SourceRanges, - DestinationRanges: []string{forwardingRule.IPAddress}, + DestinationRanges: []string{ipAddress}, Protocol: string(protocol), Name: firewallName, NodeNames: nodeNames, From 8fa4acfdf47673f17aa459c4e381b2f3433fcb9a Mon Sep 17 00:00:00 2001 From: Slavik Panasovets Date: Mon, 13 Feb 2023 19:00:43 +0000 Subject: [PATCH 4/4] Refactor l4 dualstack transition tests --- pkg/loadbalancers/l4_test.go | 309 +++++++++++++++-------------- pkg/loadbalancers/l4netlb_test.go | 318 ++++++++++++++++-------------- pkg/test/utils.go | 4 +- 3 files changed, 342 insertions(+), 289 deletions(-) diff --git a/pkg/loadbalancers/l4_test.go b/pkg/loadbalancers/l4_test.go index 026989b110..23b3aba147 100644 --- a/pkg/loadbalancers/l4_test.go +++ b/pkg/loadbalancers/l4_test.go @@ -1703,13 +1703,9 @@ func TestEnsureInternalDualStackLoadBalancer(t *testing.T) { // - ServiceExternalTrafficPolicy // - Protocol // - IPFamilies -// for dual-stack service. In total 401 combinations -func TestDualStackLoadBalancerTransitions(t *testing.T) { +// for dual-stack service. In total 400 combinations +func TestDualStackILBTransitions(t *testing.T) { t.Parallel() - nodeNames := []string{"test-node-1"} - vals := gce.DefaultTestClusterValues() - - namer := namer_util.NewL4Namer(kubeSystemUID, nil) trafficPolicyStates := []v1.ServiceExternalTrafficPolicyType{v1.ServiceExternalTrafficPolicyTypeLocal, v1.ServiceExternalTrafficPolicyTypeCluster} protocols := []v1.Protocol{v1.ProtocolTCP, v1.ProtocolUDP} @@ -1721,78 +1717,32 @@ func TestDualStackLoadBalancerTransitions(t *testing.T) { {}, } + type testCase struct { + desc string + initialIPFamily []v1.IPFamily + finalIPFamily []v1.IPFamily + initialTrafficPolicy v1.ServiceExternalTrafficPolicyType + finalTrafficPolicy v1.ServiceExternalTrafficPolicyType + initialProtocol v1.Protocol + finalProtocol v1.Protocol + } + + var testCases []testCase + for _, initialIPFamily := range ipFamiliesStates { for _, finalIPFamily := range ipFamiliesStates { for _, initialTrafficPolicy := range trafficPolicyStates { for _, finalTrafficPolicy := range trafficPolicyStates { for _, initialProtocol := range protocols { for _, finalProtocol := range protocols { - initialIPFamily := initialIPFamily - finalIPFamily := finalIPFamily - initialTrafficPolicy := initialTrafficPolicy - finalTrafficPolicy := finalTrafficPolicy - initialProtocol := initialProtocol - finalProtocol := finalProtocol - - var stringInitialIPFamily []string - for _, f := range initialIPFamily { - stringInitialIPFamily = append(stringInitialIPFamily, string(f)) - } - - var stringFinalIPFamily []string - for _, f := range finalIPFamily { - stringFinalIPFamily = append(stringFinalIPFamily, string(f)) - } - desc := struct { - fromIPFamily string - toIPFamily string - fromTrafficPolicy string - toTrafficPolicy string - fromProtocol string - toProtocol string - }{ - strings.Join(stringInitialIPFamily, ","), - strings.Join(stringFinalIPFamily, ","), - string(initialTrafficPolicy), - string(finalTrafficPolicy), - string(initialProtocol), - string(finalProtocol), - } - - t.Run(fmt.Sprintf("+%v", desc), func(t *testing.T) { - t.Parallel() - - fakeGCE := getFakeGCECloud(vals) - - svc := test.NewL4ILBDualStackService(8080, initialProtocol, initialIPFamily, initialTrafficPolicy) - l4ilbParams := &L4ILBParams{ - Service: svc, - Cloud: fakeGCE, - Namer: namer, - Recorder: record.NewFakeRecorder(100), - DualStackEnabled: true, - } - l4 := NewL4Handler(l4ilbParams) - l4.healthChecks = healthchecksl4.Fake(fakeGCE, l4ilbParams.Recorder) - - if _, err := test.CreateAndInsertNodes(l4.cloud, nodeNames, vals.ZoneName); err != nil { - t.Errorf("Unexpected error when adding nodes %v", err) - } - - result := l4.EnsureInternalLoadBalancer(nodeNames, svc) - svc.Annotations = result.Annotations - assertDualStackILBResources(t, l4, nodeNames) - - finalSvc := test.NewL4ILBDualStackService(8080, finalProtocol, finalIPFamily, finalTrafficPolicy) - finalSvc.Annotations = svc.Annotations - l4.Service = finalSvc - - result = l4.EnsureInternalLoadBalancer(nodeNames, svc) - finalSvc.Annotations = result.Annotations - assertDualStackILBResources(t, l4, nodeNames) - - l4.EnsureInternalLoadBalancerDeleted(l4.Service) - assertDualStackILBResourcesDeleted(t, l4) + testCases = append(testCases, testCase{ + desc: dualStackILBTransitionTestDesc(initialIPFamily, finalIPFamily, initialTrafficPolicy, finalTrafficPolicy, initialProtocol, finalProtocol), + initialIPFamily: initialIPFamily, + finalIPFamily: finalIPFamily, + initialTrafficPolicy: initialTrafficPolicy, + finalTrafficPolicy: finalTrafficPolicy, + initialProtocol: initialProtocol, + finalProtocol: finalProtocol, }) } } @@ -1800,72 +1750,19 @@ func TestDualStackLoadBalancerTransitions(t *testing.T) { } } } -} - -func TestDualStackILBCleansOnlyAnnotationResources(t *testing.T) { - t.Parallel() - nodeNames := []string{"test-node-1"} - vals := gce.DefaultTestClusterValues() - - testCases := []struct { - desc string - ipFamiliesStates [2][]v1.IPFamily - annotationsToDelete []string - verifyResourcesNotDeleted func(l4 *L4) error - }{ - { - desc: "Should not delete IPv6 resources if they not exist in annotation", - ipFamiliesStates: [2][]v1.IPFamily{{v1.IPv4Protocol, v1.IPv6Protocol}, {v1.IPv4Protocol}}, - annotationsToDelete: []string{annotations.TCPForwardingRuleIPv6Key, annotations.FirewallRuleIPv6Key, annotations.FirewallRuleForHealthcheckIPv6Key}, - verifyResourcesNotDeleted: func(l4 *L4) error { - // Verify IPv6 Firewall was not deleted - ipv6FWName := l4.namer.L4IPv6Firewall(l4.Service.Namespace, l4.Service.Name) - err := verifyFirewallNotExists(l4.cloud, ipv6FWName) - if err == nil { - return fmt.Errorf("firewall rule %s was deleted, expected not", ipv6FWName) - } - - // Verify IPv6 Forwarding Rule was not deleted - ipv6FRName := l4.getIPv6FRName() - err = verifyForwardingRuleNotExists(l4.cloud, ipv6FRName) - if err == nil { - return fmt.Errorf("forwarding rule %s was deleted, expected not", ipv6FRName) - } - return nil - }, - }, - { - desc: "Should not delete IPv4 resources if they not exist in annotation", - ipFamiliesStates: [2][]v1.IPFamily{{v1.IPv6Protocol, v1.IPv4Protocol}, {v1.IPv6Protocol}}, - annotationsToDelete: []string{annotations.TCPForwardingRuleKey, annotations.FirewallRuleKey, annotations.FirewallRuleForHealthcheckKey}, - verifyResourcesNotDeleted: func(l4 *L4) error { - // Verify IPv6 Firewall was not deleted - backendServiceName := l4.namer.L4Backend(l4.Service.Namespace, l4.Service.Name) - err := verifyFirewallNotExists(l4.cloud, backendServiceName) - if err == nil { - return fmt.Errorf("firewall rule %s was deleted, expected not", backendServiceName) - } - - // Verify IPv6 Forwarding Rule was not deleted - ipv4FRName := l4.GetFRName() - err = verifyForwardingRuleNotExists(l4.cloud, ipv4FRName) - if err == nil { - return fmt.Errorf("forwarding rule %s was deleted, expected not", ipv4FRName) - } - return nil - }, - }, - } for _, tc := range testCases { tc := tc t.Run(tc.desc, func(t *testing.T) { t.Parallel() + nodeNames := []string{"test-node-1"} + vals := gce.DefaultTestClusterValues() + namer := namer_util.NewL4Namer(kubeSystemUID, nil) fakeGCE := getFakeGCECloud(vals) - svc := test.NewL4ILBService(false, 8080) + svc := test.NewL4ILBDualStackService(8080, tc.initialProtocol, tc.initialIPFamily, tc.initialTrafficPolicy) l4ilbParams := &L4ILBParams{ Service: svc, Cloud: fakeGCE, @@ -1880,33 +1777,159 @@ func TestDualStackILBCleansOnlyAnnotationResources(t *testing.T) { t.Errorf("Unexpected error when adding nodes %v", err) } - svc.Spec.IPFamilies = tc.ipFamiliesStates[0] result := l4.EnsureInternalLoadBalancer(nodeNames, svc) svc.Annotations = result.Annotations assertDualStackILBResources(t, l4, nodeNames) - // Delete resources annotation - for _, annotationToDelete := range tc.annotationsToDelete { - delete(svc.Annotations, annotationToDelete) - } - svc.Spec.IPFamilies = tc.ipFamiliesStates[1] + finalSvc := test.NewL4ILBDualStackService(8080, tc.finalProtocol, tc.finalIPFamily, tc.finalTrafficPolicy) + finalSvc.Annotations = svc.Annotations + l4.Service = finalSvc - // Run new sync. Controller should not delete resources, if they don't exist in annotation result = l4.EnsureInternalLoadBalancer(nodeNames, svc) - svc.Annotations = result.Annotations - - err := tc.verifyResourcesNotDeleted(l4) - if err != nil { - t.Errorf("tc.verifyResourcesNotDeleted(_) returned error %v, want nil", err) - } + finalSvc.Annotations = result.Annotations + assertDualStackILBResources(t, l4, nodeNames) l4.EnsureInternalLoadBalancerDeleted(l4.Service) - // After complete deletion, IPv6 and IPv4 resources should be cleaned up, even if the were leaked assertDualStackILBResourcesDeleted(t, l4) }) } } +func dualStackILBTransitionTestDesc(initialIPFamily []v1.IPFamily, finalIPFamily []v1.IPFamily, initialTrafficPolicy v1.ServiceExternalTrafficPolicyType, finalTrafficPolicy v1.ServiceExternalTrafficPolicyType, initialProtocol v1.Protocol, finalProtocol v1.Protocol) string { + var stringInitialIPFamily []string + for _, f := range initialIPFamily { + stringInitialIPFamily = append(stringInitialIPFamily, string(f)) + } + + var stringFinalIPFamily []string + for _, f := range finalIPFamily { + stringFinalIPFamily = append(stringFinalIPFamily, string(f)) + } + fromIPFamily := strings.Join(stringInitialIPFamily, ",") + toIPFamily := strings.Join(stringFinalIPFamily, ",") + fromTrafficPolicy := string(initialTrafficPolicy) + toTrafficPolicy := string(finalTrafficPolicy) + fromProtocol := string(initialProtocol) + toProtocol := string(finalProtocol) + + return fmt.Sprintf("IP family: %s->%s, Traffic Policy: %s->%s, Protocol: %s->%s,", fromIPFamily, toIPFamily, fromTrafficPolicy, toTrafficPolicy, fromProtocol, toProtocol) +} + +func TestDualStackILBSyncIgnoresNoAnnotationIPv6Resources(t *testing.T) { + t.Parallel() + nodeNames := []string{"test-node-1"} + vals := gce.DefaultTestClusterValues() + + namer := namer_util.NewL4Namer(kubeSystemUID, nil) + fakeGCE := getFakeGCECloud(vals) + + svc := test.NewL4ILBService(false, 8080) + l4ilbParams := &L4ILBParams{ + Service: svc, + Cloud: fakeGCE, + Namer: namer, + Recorder: record.NewFakeRecorder(100), + DualStackEnabled: true, + } + l4 := NewL4Handler(l4ilbParams) + l4.healthChecks = healthchecksl4.Fake(fakeGCE, l4ilbParams.Recorder) + + if _, err := test.CreateAndInsertNodes(l4.cloud, nodeNames, vals.ZoneName); err != nil { + t.Errorf("Unexpected error when adding nodes %v", err) + } + + svc.Spec.IPFamilies = []v1.IPFamily{v1.IPv4Protocol, v1.IPv6Protocol} + result := l4.EnsureInternalLoadBalancer(nodeNames, svc) + svc.Annotations = result.Annotations + assertDualStackILBResources(t, l4, nodeNames) + + // Delete resources annotation + annotationsToDelete := []string{annotations.TCPForwardingRuleIPv6Key, annotations.FirewallRuleIPv6Key, annotations.FirewallRuleForHealthcheckIPv6Key} + for _, annotationToDelete := range annotationsToDelete { + delete(svc.Annotations, annotationToDelete) + } + svc.Spec.IPFamilies = []v1.IPFamily{v1.IPv4Protocol} + + // Run new sync. Controller should not delete resources, if they don't exist in annotation + result = l4.EnsureInternalLoadBalancer(nodeNames, svc) + svc.Annotations = result.Annotations + + ipv6FWName := l4.namer.L4IPv6Firewall(l4.Service.Namespace, l4.Service.Name) + err := verifyFirewallNotExists(l4.cloud, ipv6FWName) + if err == nil { + t.Errorf("firewall rule %s was deleted, expected not", ipv6FWName) + } + + // Verify IPv6 Forwarding Rule was not deleted + ipv6FRName := l4.getIPv6FRName() + err = verifyForwardingRuleNotExists(l4.cloud, ipv6FRName) + if err == nil { + t.Errorf("forwarding rule %s was deleted, expected not", ipv6FRName) + } + + l4.EnsureInternalLoadBalancerDeleted(l4.Service) + // After complete deletion, IPv6 and IPv4 resources should be cleaned up, even if the were leaked + assertDualStackILBResourcesDeleted(t, l4) +} + +func TestDualStackILBSyncIgnoresNoAnnotationIPv4Resources(t *testing.T) { + t.Parallel() + nodeNames := []string{"test-node-1"} + vals := gce.DefaultTestClusterValues() + + namer := namer_util.NewL4Namer(kubeSystemUID, nil) + fakeGCE := getFakeGCECloud(vals) + + svc := test.NewL4ILBService(false, 8080) + l4ilbParams := &L4ILBParams{ + Service: svc, + Cloud: fakeGCE, + Namer: namer, + Recorder: record.NewFakeRecorder(100), + DualStackEnabled: true, + } + l4 := NewL4Handler(l4ilbParams) + l4.healthChecks = healthchecksl4.Fake(fakeGCE, l4ilbParams.Recorder) + + if _, err := test.CreateAndInsertNodes(l4.cloud, nodeNames, vals.ZoneName); err != nil { + t.Errorf("Unexpected error when adding nodes %v", err) + } + + svc.Spec.IPFamilies = []v1.IPFamily{v1.IPv6Protocol, v1.IPv4Protocol} + result := l4.EnsureInternalLoadBalancer(nodeNames, svc) + svc.Annotations = result.Annotations + assertDualStackILBResources(t, l4, nodeNames) + + // Delete resources annotation + annotationsToDelete := []string{annotations.TCPForwardingRuleKey, annotations.FirewallRuleKey, annotations.FirewallRuleForHealthcheckKey} + for _, annotationToDelete := range annotationsToDelete { + delete(svc.Annotations, annotationToDelete) + } + svc.Spec.IPFamilies = []v1.IPFamily{v1.IPv6Protocol} + + // Run new sync. Controller should not delete resources, if they don't exist in annotation + result = l4.EnsureInternalLoadBalancer(nodeNames, svc) + svc.Annotations = result.Annotations + + // Verify IPv6 Firewall was not deleted + backendServiceName := l4.namer.L4Backend(l4.Service.Namespace, l4.Service.Name) + err := verifyFirewallNotExists(l4.cloud, backendServiceName) + if err == nil { + t.Errorf("firewall rule %s was deleted, expected not", backendServiceName) + } + + // Verify IPv6 Forwarding Rule was not deleted + ipv4FRName := l4.GetFRName() + err = verifyForwardingRuleNotExists(l4.cloud, ipv4FRName) + if err == nil { + t.Errorf("forwarding rule %s was deleted, expected not", ipv4FRName) + } + + l4.EnsureInternalLoadBalancerDeleted(l4.Service) + // After complete deletion, IPv6 and IPv4 resources should be cleaned up, even if the were leaked + assertDualStackILBResourcesDeleted(t, l4) +} + func assertILBResources(t *testing.T, l4 *L4, nodeNames []string, resourceAnnotations map[string]string) { t.Helper() diff --git a/pkg/loadbalancers/l4netlb_test.go b/pkg/loadbalancers/l4netlb_test.go index 7d306cfe19..2b70fa5c40 100644 --- a/pkg/loadbalancers/l4netlb_test.go +++ b/pkg/loadbalancers/l4netlb_test.go @@ -363,7 +363,7 @@ func TestEnsureExternalDualStackLoadBalancer(t *testing.T) { fakeGCE := getFakeGCECloud(vals) - svc := test.NewL4NetLBRBSDualStackService(8080, v1.ProtocolTCP, tc.ipFamilies, tc.trafficPolicy) + svc := test.NewL4NetLBRBSDualStackService(v1.ProtocolTCP, tc.ipFamilies, tc.trafficPolicy) l4NetLBParams := &L4NetLBParams{ Service: svc, @@ -400,12 +400,8 @@ func TestEnsureExternalDualStackLoadBalancer(t *testing.T) { // - Protocol // - IPFamilies // for dual-stack service. In total 400 combinations -func TestDualStackNetLBLoadBalancerTransitions(t *testing.T) { +func TestDualStackNetLBTransitions(t *testing.T) { t.Parallel() - nodeNames := []string{"test-node-1"} - vals := gce.DefaultTestClusterValues() - - namer := namer_util.NewL4Namer(kubeSystemUID, nil) trafficPolicyStates := []v1.ServiceExternalTrafficPolicyType{v1.ServiceExternalTrafficPolicyTypeLocal, v1.ServiceExternalTrafficPolicyTypeCluster} protocols := []v1.Protocol{v1.ProtocolTCP, v1.ProtocolUDP} @@ -417,76 +413,32 @@ func TestDualStackNetLBLoadBalancerTransitions(t *testing.T) { {}, } + type testCase struct { + desc string + initialIPFamily []v1.IPFamily + finalIPFamily []v1.IPFamily + initialTrafficPolicy v1.ServiceExternalTrafficPolicyType + finalTrafficPolicy v1.ServiceExternalTrafficPolicyType + initialProtocol v1.Protocol + finalProtocol v1.Protocol + } + + var testCases []testCase + for _, initialIPFamily := range ipFamiliesStates { for _, finalIPFamily := range ipFamiliesStates { for _, initialTrafficPolicy := range trafficPolicyStates { for _, finalTrafficPolicy := range trafficPolicyStates { for _, initialProtocol := range protocols { for _, finalProtocol := range protocols { - initialIPFamily := initialIPFamily - finalIPFamily := finalIPFamily - initialTrafficPolicy := initialTrafficPolicy - finalTrafficPolicy := finalTrafficPolicy - initialProtocol := initialProtocol - finalProtocol := finalProtocol - - var stringInitialIPFamily []string - for _, f := range initialIPFamily { - stringInitialIPFamily = append(stringInitialIPFamily, string(f)) - } - - var stringFinalIPFamily []string - for _, f := range finalIPFamily { - stringFinalIPFamily = append(stringFinalIPFamily, string(f)) - } - desc := struct { - fromIPFamily string - toIPFamily string - fromTrafficPolicy string - toTrafficPolicy string - fromProtocol string - toProtocol string - }{ - strings.Join(stringInitialIPFamily, ","), - strings.Join(stringFinalIPFamily, ","), - string(initialTrafficPolicy), - string(finalTrafficPolicy), - string(initialProtocol), - string(finalProtocol), - } - - t.Run(fmt.Sprintf("+%v", desc), func(t *testing.T) { - t.Parallel() - - fakeGCE := getFakeGCECloud(vals) - - svc := test.NewL4NetLBRBSDualStackService(8080, initialProtocol, initialIPFamily, initialTrafficPolicy) - l4NetLBParams := &L4NetLBParams{ - Service: svc, - Cloud: fakeGCE, - Namer: namer, - Recorder: record.NewFakeRecorder(100), - DualStackEnabled: true, - } - l4NetLB := NewL4NetLB(l4NetLBParams) - l4NetLB.healthChecks = healthchecksl4.Fake(fakeGCE, l4NetLBParams.Recorder) - - if _, err := test.CreateAndInsertNodes(l4NetLB.cloud, nodeNames, vals.ZoneName); err != nil { - t.Errorf("Unexpected error when adding nodes %v", err) - } - - result := l4NetLB.EnsureFrontend(nodeNames, svc) - if result.Error != nil { - t.Errorf("Failed to ensure loadBalancer, err %v", result.Error) - } - if len(result.Status.Ingress) == 0 { - t.Errorf("Got empty loadBalancer status using handler %v", l4NetLB) - } - l4NetLB.Service.Annotations = result.Annotations - assertDualStackNetLBResources(t, l4NetLB, nodeNames) - - l4NetLB.EnsureLoadBalancerDeleted(l4NetLB.Service) - assertDualStackNetLBResourcesDeleted(t, l4NetLB) + testCases = append(testCases, testCase{ + desc: dualStackNetLBTransitionTestDesc(initialIPFamily, finalIPFamily, initialTrafficPolicy, finalTrafficPolicy, initialProtocol, finalProtocol), + initialIPFamily: initialIPFamily, + finalIPFamily: finalIPFamily, + initialTrafficPolicy: initialTrafficPolicy, + finalTrafficPolicy: finalTrafficPolicy, + initialProtocol: initialProtocol, + finalProtocol: finalProtocol, }) } } @@ -494,72 +446,19 @@ func TestDualStackNetLBLoadBalancerTransitions(t *testing.T) { } } } -} - -func TestDualStackNetLBCleansOnlyAnnotationResources(t *testing.T) { - t.Parallel() - nodeNames := []string{"test-node-1"} - vals := gce.DefaultTestClusterValues() - - testCases := []struct { - desc string - ipFamiliesStates [2][]v1.IPFamily - annotationsToDelete []string - verifyResourcesNotDeleted func(l4netlb *L4NetLB) error - }{ - { - desc: "Should not delete IPv6 resources if they not exist in annotation", - ipFamiliesStates: [2][]v1.IPFamily{{v1.IPv4Protocol, v1.IPv6Protocol}, {v1.IPv4Protocol}}, - annotationsToDelete: []string{annotations.TCPForwardingRuleIPv6Key, annotations.FirewallRuleIPv6Key, annotations.FirewallRuleForHealthcheckIPv6Key}, - verifyResourcesNotDeleted: func(l4netlb *L4NetLB) error { - // Verify IPv6 Firewall was not deleted - ipv6FWName := l4netlb.namer.L4IPv6Firewall(l4netlb.Service.Namespace, l4netlb.Service.Name) - err := verifyFirewallNotExists(l4netlb.cloud, ipv6FWName) - if err == nil { - return fmt.Errorf("firewall rule %s was deleted, expected not", ipv6FWName) - } - - // Verify IPv6 Forwarding Rule was not deleted - ipv6FRName := l4netlb.ipv6FRName() - err = verifyForwardingRuleNotExists(l4netlb.cloud, ipv6FRName) - if err == nil { - return fmt.Errorf("forwarding rule %s was deleted, expected not", ipv6FRName) - } - return nil - }, - }, - { - desc: "Should not delete IPv4 resources if they not exist in annotation", - ipFamiliesStates: [2][]v1.IPFamily{{v1.IPv6Protocol, v1.IPv4Protocol}, {v1.IPv6Protocol}}, - annotationsToDelete: []string{annotations.TCPForwardingRuleKey, annotations.FirewallRuleKey, annotations.FirewallRuleForHealthcheckKey}, - verifyResourcesNotDeleted: func(l4netlb *L4NetLB) error { - // Verify IPv6 Firewall was not deleted - backendServiceName := l4netlb.namer.L4Backend(l4netlb.Service.Namespace, l4netlb.Service.Name) - err := verifyFirewallNotExists(l4netlb.cloud, backendServiceName) - if err == nil { - return fmt.Errorf("firewall rule %s was deleted, expected not", backendServiceName) - } - - // Verify IPv6 Forwarding Rule was not deleted - ipv4FRName := l4netlb.frName() - err = verifyForwardingRuleNotExists(l4netlb.cloud, ipv4FRName) - if err == nil { - return fmt.Errorf("forwarding rule %s was deleted, expected not", ipv4FRName) - } - return nil - }, - }, - } for _, tc := range testCases { tc := tc t.Run(tc.desc, func(t *testing.T) { t.Parallel() + nodeNames := []string{"test-node-1"} + vals := gce.DefaultTestClusterValues() + namer := namer_util.NewL4Namer(kubeSystemUID, nil) fakeGCE := getFakeGCECloud(vals) - svc := test.NewL4NetLBRBSService(8080) + svc := test.NewL4NetLBRBSDualStackService(tc.initialProtocol, tc.initialIPFamily, tc.initialTrafficPolicy) l4NetLBParams := &L4NetLBParams{ Service: svc, Cloud: fakeGCE, @@ -574,33 +473,160 @@ func TestDualStackNetLBCleansOnlyAnnotationResources(t *testing.T) { t.Errorf("Unexpected error when adding nodes %v", err) } - svc.Spec.IPFamilies = tc.ipFamiliesStates[0] result := l4NetLB.EnsureFrontend(nodeNames, svc) svc.Annotations = result.Annotations assertDualStackNetLBResources(t, l4NetLB, nodeNames) - // Delete resources annotation - for _, annotationToDelete := range tc.annotationsToDelete { - delete(svc.Annotations, annotationToDelete) - } - svc.Spec.IPFamilies = tc.ipFamiliesStates[1] + finalSvc := test.NewL4NetLBRBSDualStackService(tc.finalProtocol, tc.finalIPFamily, tc.finalTrafficPolicy) + finalSvc.Annotations = svc.Annotations + l4NetLB.Service = finalSvc - // Run new sync. Controller should not delete resources, if they don't exist in annotation result = l4NetLB.EnsureFrontend(nodeNames, svc) - svc.Annotations = result.Annotations - - err := tc.verifyResourcesNotDeleted(l4NetLB) - if err != nil { - t.Errorf("tc.verifyResourcesNotDeleted(_) returned error %v, want nil", err) - } + finalSvc.Annotations = result.Annotations + assertDualStackNetLBResources(t, l4NetLB, nodeNames) l4NetLB.EnsureLoadBalancerDeleted(l4NetLB.Service) - // After complete deletion, IPv6 and IPv4 resources should be cleaned up, even if the were leaked assertDualStackNetLBResourcesDeleted(t, l4NetLB) }) } } +func dualStackNetLBTransitionTestDesc(initialIPFamily []v1.IPFamily, finalIPFamily []v1.IPFamily, initialTrafficPolicy v1.ServiceExternalTrafficPolicyType, finalTrafficPolicy v1.ServiceExternalTrafficPolicyType, initialProtocol v1.Protocol, finalProtocol v1.Protocol) string { + var stringInitialIPFamily []string + for _, f := range initialIPFamily { + stringInitialIPFamily = append(stringInitialIPFamily, string(f)) + } + + var stringFinalIPFamily []string + for _, f := range finalIPFamily { + stringFinalIPFamily = append(stringFinalIPFamily, string(f)) + } + fromIPFamily := strings.Join(stringInitialIPFamily, ",") + toIPFamily := strings.Join(stringFinalIPFamily, ",") + fromTrafficPolicy := string(initialTrafficPolicy) + toTrafficPolicy := string(finalTrafficPolicy) + fromProtocol := string(initialProtocol) + toProtocol := string(finalProtocol) + + return fmt.Sprintf("IP family: %s->%s, Traffic Policy: %s->%s, Protocol: %s->%s,", fromIPFamily, toIPFamily, fromTrafficPolicy, toTrafficPolicy, fromProtocol, toProtocol) +} + +func TestDualStackNetLBSyncIgnoresNoAnnotationIPv6Resources(t *testing.T) { + t.Parallel() + nodeNames := []string{"test-node-1"} + vals := gce.DefaultTestClusterValues() + + namer := namer_util.NewL4Namer(kubeSystemUID, nil) + fakeGCE := getFakeGCECloud(vals) + + svc := test.NewL4NetLBRBSService(8080) + l4NetLBParams := &L4NetLBParams{ + Service: svc, + Cloud: fakeGCE, + Namer: namer, + Recorder: record.NewFakeRecorder(100), + DualStackEnabled: true, + } + l4NetLB := NewL4NetLB(l4NetLBParams) + l4NetLB.healthChecks = healthchecksl4.Fake(fakeGCE, l4NetLBParams.Recorder) + + if _, err := test.CreateAndInsertNodes(l4NetLB.cloud, nodeNames, vals.ZoneName); err != nil { + t.Errorf("Unexpected error when adding nodes %v", err) + } + + svc.Spec.IPFamilies = []v1.IPFamily{v1.IPv4Protocol, v1.IPv6Protocol} + result := l4NetLB.EnsureFrontend(nodeNames, svc) + svc.Annotations = result.Annotations + assertDualStackNetLBResources(t, l4NetLB, nodeNames) + + // Delete IPv4 resources annotation + annotationsToDelete := []string{annotations.TCPForwardingRuleIPv6Key, annotations.FirewallRuleIPv6Key, annotations.FirewallRuleForHealthcheckIPv6Key} + for _, annotationToDelete := range annotationsToDelete { + delete(svc.Annotations, annotationToDelete) + } + svc.Spec.IPFamilies = []v1.IPFamily{v1.IPv6Protocol} + + // Run new sync. Controller should not delete resources, if they don't exist in annotation + result = l4NetLB.EnsureFrontend(nodeNames, svc) + svc.Annotations = result.Annotations + + // Verify IPv4 Firewall was not deleted + ipv6FWName := l4NetLB.namer.L4IPv6Firewall(l4NetLB.Service.Namespace, l4NetLB.Service.Name) + err := verifyFirewallNotExists(l4NetLB.cloud, ipv6FWName) + if err == nil { + t.Errorf("firewall rule %s was deleted, expected not", ipv6FWName) + } + + // Verify IPv6 Forwarding Rule was not deleted + ipv6FRName := l4NetLB.ipv6FRName() + err = verifyForwardingRuleNotExists(l4NetLB.cloud, ipv6FRName) + if err == nil { + t.Errorf("forwarding rule %s was deleted, expected not", ipv6FRName) + } + + l4NetLB.EnsureLoadBalancerDeleted(l4NetLB.Service) + // After complete deletion, IPv6 and IPv4 resources should be cleaned up, even if the were leaked + assertDualStackNetLBResourcesDeleted(t, l4NetLB) +} + +func TestDualStackNetLBSyncIgnoresNoAnnotationIPv4Resources(t *testing.T) { + t.Parallel() + nodeNames := []string{"test-node-1"} + vals := gce.DefaultTestClusterValues() + + namer := namer_util.NewL4Namer(kubeSystemUID, nil) + fakeGCE := getFakeGCECloud(vals) + + svc := test.NewL4NetLBRBSService(8080) + l4NetLBParams := &L4NetLBParams{ + Service: svc, + Cloud: fakeGCE, + Namer: namer, + Recorder: record.NewFakeRecorder(100), + DualStackEnabled: true, + } + l4NetLB := NewL4NetLB(l4NetLBParams) + l4NetLB.healthChecks = healthchecksl4.Fake(fakeGCE, l4NetLBParams.Recorder) + + if _, err := test.CreateAndInsertNodes(l4NetLB.cloud, nodeNames, vals.ZoneName); err != nil { + t.Errorf("Unexpected error when adding nodes %v", err) + } + + svc.Spec.IPFamilies = []v1.IPFamily{v1.IPv6Protocol, v1.IPv4Protocol} + result := l4NetLB.EnsureFrontend(nodeNames, svc) + svc.Annotations = result.Annotations + assertDualStackNetLBResources(t, l4NetLB, nodeNames) + + // Delete IPv4 resources annotation + annotationsToDelete := []string{annotations.TCPForwardingRuleKey, annotations.FirewallRuleKey, annotations.FirewallRuleForHealthcheckKey} + for _, annotationToDelete := range annotationsToDelete { + delete(svc.Annotations, annotationToDelete) + } + svc.Spec.IPFamilies = []v1.IPFamily{v1.IPv6Protocol} + + // Run new sync. Controller should not delete resources, if they don't exist in annotation + result = l4NetLB.EnsureFrontend(nodeNames, svc) + svc.Annotations = result.Annotations + + // Verify IPv4 Firewall was not deleted + fwName := l4NetLB.namer.L4Backend(l4NetLB.Service.Namespace, l4NetLB.Service.Name) + err := verifyFirewallNotExists(l4NetLB.cloud, fwName) + if err == nil { + t.Errorf("firewall rule %s was deleted, expected not", fwName) + } + + // Verify IPv4 Forwarding Rule was not deleted + ipv4FRName := l4NetLB.frName() + err = verifyForwardingRuleNotExists(l4NetLB.cloud, ipv4FRName) + if err == nil { + t.Errorf("forwarding rule %s was deleted, expected not", ipv4FRName) + } + + l4NetLB.EnsureLoadBalancerDeleted(l4NetLB.Service) + // After complete deletion, IPv6 and IPv4 resources should be cleaned up, even if the were leaked + assertDualStackNetLBResourcesDeleted(t, l4NetLB) +} + func ensureLoadBalancer(port int, vals gce.TestClusterValues, fakeGCE *gce.Cloud, t *testing.T) (*v1.Service, *L4NetLB) { svc := test.NewL4NetLBRBSService(port) namer := namer_util.NewL4Namer(kubeSystemUID, namer_util.NewNamer(vals.ClusterName, "cluster-fw")) @@ -849,11 +875,11 @@ func verifyNetLBIPv4NodesFirewall(l4netlb *L4NetLB, nodeNames []string) error { return fmt.Errorf("failed to create description for resources, err %w", err) } - sourceRanges, err := servicehelper.GetLoadBalancerSourceRanges(l4netlb.Service) + sourceRanges, err := utils.IPv4ServiceSourceRanges(l4netlb.Service) if err != nil { return fmt.Errorf("servicehelper.GetLoadBalancerSourceRanges(%+v) returned error %v, want nil", l4netlb.Service, err) } - return verifyFirewall(l4netlb.cloud, nodeNames, fwName, fwDesc, sourceRanges.StringSlice()) + return verifyFirewall(l4netlb.cloud, nodeNames, fwName, fwDesc, sourceRanges) } func verifyNetLBIPv6NodesFirewall(l4netlb *L4NetLB, nodeNames []string) error { @@ -864,7 +890,11 @@ func verifyNetLBIPv6NodesFirewall(l4netlb *L4NetLB, nodeNames []string) error { return fmt.Errorf("failed to create description for resources, err %w", err) } - return verifyFirewall(l4netlb.cloud, nodeNames, ipv6FirewallName, fwDesc, []string{"0::0/0"}) + sourceRanges, err := utils.IPv6ServiceSourceRanges(l4netlb.Service) + if err != nil { + return fmt.Errorf("servicehelper.GetLoadBalancerSourceRanges(%+v) returned error %v, want nil", l4netlb.Service, err) + } + return verifyFirewall(l4netlb.cloud, nodeNames, ipv6FirewallName, fwDesc, sourceRanges) } func verifyNetLBIPv4HealthCheckFirewall(l4netlb *L4NetLB, nodeNames []string) error { diff --git a/pkg/test/utils.go b/pkg/test/utils.go index a7f7638121..989c331878 100644 --- a/pkg/test/utils.go +++ b/pkg/test/utils.go @@ -154,11 +154,11 @@ func NewL4NetLBRBSService(port int) *api_v1.Service { } // NewL4NetLBRBSDualStackService creates a Service of type LoadBalancer with RBS Annotation and provided ipFamilies and ipFamilyPolicy -func NewL4NetLBRBSDualStackService(port int, protocol api_v1.Protocol, ipFamilies []api_v1.IPFamily, externalTrafficPolicy api_v1.ServiceExternalTrafficPolicyType) *api_v1.Service { +func NewL4NetLBRBSDualStackService(protocol api_v1.Protocol, ipFamilies []api_v1.IPFamily, externalTrafficPolicy api_v1.ServiceExternalTrafficPolicyType) *api_v1.Service { svc := NewL4LegacyNetLBServiceWithoutPorts() svc.ObjectMeta.Annotations[annotations.RBSAnnotationKey] = annotations.RBSEnabled svc.Spec.Ports = []api_v1.ServicePort{ - {Name: "testport", Port: int32(port), Protocol: protocol}, + {Name: "testport", Port: int32(8080), Protocol: protocol}, } svc.Spec.IPFamilies = ipFamilies svc.Spec.ExternalTrafficPolicy = externalTrafficPolicy