From 12559941d9dbbd6f21d7a413943d1bb9fac911ae Mon Sep 17 00:00:00 2001 From: mmamczur Date: Tue, 14 Mar 2023 17:35:28 +0100 Subject: [PATCH] Multi Network support for the ILB L4 controller. --- pkg/backends/backends.go | 9 +- pkg/backends/backends_test.go | 61 ++++++ pkg/backends/regional_ig_linker_test.go | 6 +- pkg/firewalls/firewalls_l4.go | 4 +- pkg/firewalls/firewalls_l4_test.go | 131 +++++++++++++ pkg/healthchecksl4/healthchecksl4.go | 7 +- pkg/l4lb/l4controller.go | 27 ++- pkg/l4lb/l4netlbcontroller.go | 3 + pkg/loadbalancers/forwarding_rules.go | 9 +- pkg/loadbalancers/forwarding_rules_test.go | 216 +++++++++++++++++---- pkg/loadbalancers/l4.go | 10 +- pkg/loadbalancers/l4_test.go | 9 +- pkg/loadbalancers/l4netlb.go | 6 +- pkg/network/network.go | 6 +- pkg/network/network_test.go | 6 +- 15 files changed, 444 insertions(+), 66 deletions(-) create mode 100644 pkg/backends/backends_test.go create mode 100644 pkg/firewalls/firewalls_l4_test.go diff --git a/pkg/backends/backends.go b/pkg/backends/backends.go index 4a00e62760..b80b74ff60 100644 --- a/pkg/backends/backends.go +++ b/pkg/backends/backends.go @@ -25,6 +25,7 @@ import ( "k8s.io/cloud-provider-gcp/providers/gce" "k8s.io/ingress-gce/pkg/backends/features" "k8s.io/ingress-gce/pkg/composite" + "k8s.io/ingress-gce/pkg/network" "k8s.io/ingress-gce/pkg/utils" "k8s.io/ingress-gce/pkg/utils/namer" "k8s.io/klog/v2" @@ -284,7 +285,7 @@ func (b *Backends) DeleteSignedUrlKey(be *composite.BackendService, keyName stri } // EnsureL4BackendService creates or updates the backend service with the given name. -func (b *Backends) EnsureL4BackendService(name, hcLink, protocol, sessionAffinity, scheme string, nm types.NamespacedName) (*composite.BackendService, error) { +func (b *Backends) EnsureL4BackendService(name, hcLink, protocol, sessionAffinity, scheme string, nm types.NamespacedName, network network.NetworkInfo) (*composite.BackendService, error) { start := time.Now() klog.V(2).Infof("EnsureL4BackendService(%v, %v, %v): started", name, scheme, protocol) defer func() { @@ -313,6 +314,9 @@ func (b *Backends) EnsureL4BackendService(name, hcLink, protocol, sessionAffinit SessionAffinity: utils.TranslateAffinityType(sessionAffinity), LoadBalancingScheme: scheme, } + if !network.IsDefault { + expectedBS.Network = network.NetworkURL + } if protocol == string(api_v1.ProtocolTCP) { expectedBS.ConnectionDraining = &composite.ConnectionDraining{DrainingTimeoutSec: DefaultConnectionDrainingTimeoutSeconds} } else { @@ -362,5 +366,6 @@ func backendSvcEqual(a, b *composite.BackendService) bool { a.Description == b.Description && a.SessionAffinity == b.SessionAffinity && a.LoadBalancingScheme == b.LoadBalancingScheme && - utils.EqualStringSets(a.HealthChecks, b.HealthChecks) + utils.EqualStringSets(a.HealthChecks, b.HealthChecks) && + a.Network == b.Network } diff --git a/pkg/backends/backends_test.go b/pkg/backends/backends_test.go new file mode 100644 index 0000000000..5bfcab8f3b --- /dev/null +++ b/pkg/backends/backends_test.go @@ -0,0 +1,61 @@ +package backends + +import ( + "strings" + "testing" + + "github.com/GoogleCloudPlatform/k8s-cloud-provider/pkg/cloud" + "github.com/GoogleCloudPlatform/k8s-cloud-provider/pkg/cloud/meta" + v1 "k8s.io/api/core/v1" + "k8s.io/apimachinery/pkg/types" + "k8s.io/cloud-provider-gcp/providers/gce" + "k8s.io/ingress-gce/pkg/network" + "k8s.io/ingress-gce/pkg/utils" + namer "k8s.io/ingress-gce/pkg/utils/namer" +) + +const ( + kubeSystemUID = "ksuid123" +) + +func TestEnsureL4BackendService(t *testing.T) { + serviceName := types.NamespacedName{Name: "test-service", Namespace: "test-ns"} + fakeGCE := gce.NewFakeGCECloud(gce.DefaultTestClusterValues()) + l4namer := namer.NewL4Namer(kubeSystemUID, nil) + backendPool := NewPool(fakeGCE, l4namer) + + hcLink := l4namer.L4HealthCheck(serviceName.Namespace, serviceName.Name, false) + bsName := l4namer.L4Backend(serviceName.Namespace, serviceName.Name) + network := network.NetworkInfo{IsDefault: false, NetworkURL: "https://www.googleapis.com/compute/v1/projects/test-poject/global/networks/test-vpc"} + bs, err := backendPool.EnsureL4BackendService(bsName, hcLink, "TCP", string(v1.ServiceAffinityNone), string(cloud.SchemeInternal), serviceName, network) + if err != nil { + t.Errorf("EnsureL4BackendService failed") + } + + if bs.SessionAffinity != strings.ToUpper(string(v1.ServiceAffinityNone)) { + t.Errorf("BackendService.SessionAffinity was not populated correctly want=%q, got=%q", strings.ToUpper(string(v1.ServiceAffinityNone)), bs.SessionAffinity) + } + if bs.Network != network.NetworkURL { + t.Errorf("BackendService.Network was not populated correctly, want=%q, got=%q", network.NetworkURL, bs.Network) + } + if len(bs.HealthChecks) != 1 || bs.HealthChecks[0] != hcLink { + t.Errorf("BackendService.HealthChecks was not populated correctly, want=%q, got=%q", hcLink, bs.HealthChecks) + } + description, err := utils.MakeL4LBServiceDescription(serviceName.String(), "", meta.VersionGA, false, utils.ILB) + if err != nil { + t.Errorf("utils.MakeL4LBServiceDescription() failed %v", err) + } + if bs.Description != description { + t.Errorf("BackendService.Description was not populated correctly, want=%q, got=%q", description, bs.Description) + } + if bs.Protocol != "TCP" { + t.Errorf("BackendService.Protocol was not populated correctly, want=%q, got=%q", "TCP", bs.Protocol) + } + if bs.LoadBalancingScheme != string(cloud.SchemeInternal) { + t.Errorf("BackendService.LoadBalancingScheme was not populated correctly, want=%q, got=%q", string(cloud.SchemeInternal), bs.LoadBalancingScheme) + } + if bs.ConnectionDraining == nil || bs.ConnectionDraining.DrainingTimeoutSec != DefaultConnectionDrainingTimeoutSeconds { + t.Errorf("BackendService.ConnectionDraining was not populated correctly, want=connection draining with %q, got=%q", DefaultConnectionDrainingTimeoutSeconds, bs.ConnectionDraining) + } + +} diff --git a/pkg/backends/regional_ig_linker_test.go b/pkg/backends/regional_ig_linker_test.go index 6d809b87de..fff0ac1597 100644 --- a/pkg/backends/regional_ig_linker_test.go +++ b/pkg/backends/regional_ig_linker_test.go @@ -24,6 +24,7 @@ import ( "k8s.io/apimachinery/pkg/types" "k8s.io/cloud-provider-gcp/providers/gce" "k8s.io/ingress-gce/pkg/instancegroups" + "k8s.io/ingress-gce/pkg/network" "k8s.io/ingress-gce/pkg/test" "k8s.io/ingress-gce/pkg/utils" "k8s.io/ingress-gce/pkg/utils/namer" @@ -229,7 +230,10 @@ func createBackendService(t *testing.T, sp utils.ServicePort, backendPool *Backe t.Helper() namespacedName := types.NamespacedName{Name: "service.Name", Namespace: "service.Namespace"} protocol := string(apiv1.ProtocolTCP) - if _, err := backendPool.EnsureL4BackendService(sp.BackendName(), hcLink, protocol, string(apiv1.ServiceAffinityNone), string(cloud.SchemeExternal), namespacedName); err != nil { + serviceAffinityNone := string(apiv1.ServiceAffinityNone) + schemeExternal := string(cloud.SchemeExternal) + defaultNetworkInfo := network.NetworkInfo{IsDefault: true} + if _, err := backendPool.EnsureL4BackendService(sp.BackendName(), hcLink, protocol, serviceAffinityNone, schemeExternal, namespacedName, defaultNetworkInfo); err != nil { t.Fatalf("Error creating backend service %v", err) } } diff --git a/pkg/firewalls/firewalls_l4.go b/pkg/firewalls/firewalls_l4.go index 7687616504..7151858916 100644 --- a/pkg/firewalls/firewalls_l4.go +++ b/pkg/firewalls/firewalls_l4.go @@ -25,6 +25,7 @@ import ( "k8s.io/client-go/tools/record" "k8s.io/cloud-provider-gcp/providers/gce" "k8s.io/ingress-gce/pkg/flags" + "k8s.io/ingress-gce/pkg/network" "k8s.io/ingress-gce/pkg/utils" "k8s.io/klog/v2" ) @@ -39,6 +40,7 @@ type FirewallParams struct { NodeNames []string Protocol string L4Type utils.L4LBType + Network network.NetworkInfo } func EnsureL4FirewallRule(cloud *gce.Cloud, nsName string, params *FirewallParams, sharedRule bool) error { @@ -59,7 +61,7 @@ func EnsureL4FirewallRule(cloud *gce.Cloud, nsName string, params *FirewallParam expectedFw := &compute.Firewall{ Name: params.Name, Description: fwDesc, - Network: cloud.NetworkURL(), + Network: params.Network.NetworkURL, SourceRanges: params.SourceRanges, TargetTags: nodeTags, Allowed: []*compute.FirewallAllowed{ diff --git a/pkg/firewalls/firewalls_l4_test.go b/pkg/firewalls/firewalls_l4_test.go new file mode 100644 index 0000000000..118b962668 --- /dev/null +++ b/pkg/firewalls/firewalls_l4_test.go @@ -0,0 +1,131 @@ +package firewalls + +import ( + "context" + "testing" + + "github.com/GoogleCloudPlatform/k8s-cloud-provider/pkg/cloud/meta" + "github.com/google/go-cmp/cmp" + "github.com/google/go-cmp/cmp/cmpopts" + compute "google.golang.org/api/compute/v1" + "k8s.io/cloud-provider-gcp/providers/gce" + "k8s.io/ingress-gce/pkg/network" + "k8s.io/ingress-gce/pkg/utils" +) + +func TestEnsureL4FirewallRule(t *testing.T) { + firewallDescription, err := utils.MakeL4LBFirewallDescription(utils.ServiceKeyFunc("test-ns", "test-name"), "10.0.0.1", meta.VersionGA, false) + if err != nil { + t.Errorf("Failed making the description, err=%v", err) + } + tests := []struct { + desc string + nsName string + params *FirewallParams + shared bool + want *compute.Firewall + }{ + { + desc: "default setup", + nsName: utils.ServiceKeyFunc("test-ns", "test-name"), + params: &FirewallParams{ + Name: "test-firewall", + IP: "10.0.0.1", + SourceRanges: []string{ + "10.1.2.8/29", + }, + DestinationRanges: []string{ + "10.1.2.16/29", + }, + PortRanges: []string{"8080"}, + NodeNames: []string{"k8s-test-node"}, + Protocol: "TCP", + L4Type: utils.ILB, + Network: network.NetworkInfo{IsDefault: true}, + }, + shared: false, + want: &compute.Firewall{ + Name: "test-firewall", + Network: "", + SourceRanges: []string{ + "10.1.2.8/29", + }, + TargetTags: []string{"k8s-test"}, + Description: firewallDescription, + Allowed: []*compute.FirewallAllowed{ + { + IPProtocol: "tcp", + Ports: []string{"8080"}, + }, + }, + }, + }, + { + desc: "non default network", + nsName: utils.ServiceKeyFunc("test-ns", "test-name"), + params: &FirewallParams{ + Name: "test-firewall", + IP: "10.0.0.1", + SourceRanges: []string{ + "10.1.2.8/29", + }, + PortRanges: []string{"8080"}, + NodeNames: []string{"k8s-test-node"}, + Protocol: "TCP", + L4Type: utils.ILB, + Network: network.NetworkInfo{ + IsDefault: false, + NetworkURL: "https://www.googleapis.com/compute/v1/projects/test-poject/global/networks/test-vpc", + }, + }, + shared: false, + want: &compute.Firewall{ + Name: "test-firewall", + Network: "https://www.googleapis.com/compute/v1/projects/test-poject/global/networks/test-vpc", + SourceRanges: []string{ + "10.1.2.8/29", + }, + TargetTags: []string{"k8s-test"}, + Description: firewallDescription, + Allowed: []*compute.FirewallAllowed{ + { + IPProtocol: "tcp", + Ports: []string{"8080"}, + }, + }, + }, + }, + } + for _, tc := range tests { + t.Run(tc.desc, func(t *testing.T) { + fakeGCE := gce.NewFakeGCECloud(gce.DefaultTestClusterValues()) + // Add some instance to act as the node so that target tags in the firewall can be resolved. + createVMInstanceWithTag(t, fakeGCE, "k8s-test") + if err := EnsureL4FirewallRule(fakeGCE, tc.nsName, tc.params, tc.shared); err != nil { + t.Errorf("EnsureL4FirewallRule() failed, err=%v", err) + } + firewall, err := fakeGCE.GetFirewall(tc.params.Name) + if err != nil { + t.Errorf("failed to get firewall err=%v", err) + } + if diff := cmp.Diff(tc.want, firewall, cmpopts.IgnoreFields(compute.Firewall{}, "SelfLink")); diff != "" { + t.Errorf("EnsureL4FirewallRule() diff -want +got\n%v\n", diff) + } + }) + } +} + +func createVMInstanceWithTag(t *testing.T, fakeGCE *gce.Cloud, tag string) { + err := fakeGCE.Compute().Instances().Insert(context.Background(), + meta.ZonalKey("k8s-test-node", "us-central1-b"), + &compute.Instance{ + Name: "test-node", + Zone: "us-central1-b", + Tags: &compute.Tags{ + Items: []string{tag}, + }, + }) + if err != nil { + t.Errorf("failed to create instance err=%v", err) + } +} diff --git a/pkg/healthchecksl4/healthchecksl4.go b/pkg/healthchecksl4/healthchecksl4.go index cd129de4bc..fa9a0f08ef 100644 --- a/pkg/healthchecksl4/healthchecksl4.go +++ b/pkg/healthchecksl4/healthchecksl4.go @@ -32,6 +32,7 @@ import ( "k8s.io/ingress-gce/pkg/composite" "k8s.io/ingress-gce/pkg/firewalls" "k8s.io/ingress-gce/pkg/healthchecksprovider" + "k8s.io/ingress-gce/pkg/network" "k8s.io/ingress-gce/pkg/utils" "k8s.io/ingress-gce/pkg/utils/namer" "k8s.io/klog/v2" @@ -62,14 +63,16 @@ type l4HealthChecks struct { hcProvider healthChecksProvider cloud *gce.Cloud recorder record.EventRecorder + network network.NetworkInfo } -func NewL4HealthChecks(cloud *gce.Cloud, recorder record.EventRecorder) *l4HealthChecks { +func NewL4HealthChecks(cloud *gce.Cloud, recorder record.EventRecorder, network network.NetworkInfo) *l4HealthChecks { return &l4HealthChecks{ sharedResourcesLock: sharedLock, cloud: cloud, recorder: recorder, hcProvider: healthchecksprovider.NewHealthChecks(cloud, meta.VersionGA), + network: network, } } @@ -223,6 +226,7 @@ func (l4hc *l4HealthChecks) ensureIPv4Firewall(svc *corev1.Service, namer namer. Protocol: string(corev1.ProtocolTCP), Name: hcFwName, NodeNames: nodeNames, + Network: l4hc.network, } err := firewalls.EnsureL4LBFirewallForHc(svc, isSharedHC, &hcFWRParams, l4hc.cloud, l4hc.recorder) if err != nil { @@ -249,6 +253,7 @@ func (l4hc *l4HealthChecks) ensureIPv6Firewall(svc *corev1.Service, namer namer. Protocol: string(corev1.ProtocolTCP), Name: ipv6HCFWName, NodeNames: nodeNames, + Network: l4hc.network, } err := firewalls.EnsureL4LBFirewallForHc(svc, isSharedHC, &hcFWRParams, l4hc.cloud, l4hc.recorder) if err != nil { diff --git a/pkg/l4lb/l4controller.go b/pkg/l4lb/l4controller.go index 6fcd5987df..16666c0d2b 100644 --- a/pkg/l4lb/l4controller.go +++ b/pkg/l4lb/l4controller.go @@ -39,6 +39,7 @@ import ( "k8s.io/ingress-gce/pkg/loadbalancers" "k8s.io/ingress-gce/pkg/metrics" negtypes "k8s.io/ingress-gce/pkg/neg/types" + "k8s.io/ingress-gce/pkg/network" "k8s.io/ingress-gce/pkg/utils" "k8s.io/ingress-gce/pkg/utils/common" "k8s.io/ingress-gce/pkg/utils/namer" @@ -56,12 +57,14 @@ const ( type L4Controller struct { ctx *context.ControllerContext // kubeClient, needed for attaching finalizer - client kubernetes.Interface - svcQueue utils.TaskQueue - numWorkers int - serviceLister cache.Indexer - nodeLister listers.NodeLister - stopCh chan struct{} + client kubernetes.Interface + svcQueue utils.TaskQueue + numWorkers int + serviceLister cache.Indexer + nodeLister listers.NodeLister + networkLister cache.Indexer + gkeNetworkParamSetLister cache.Indexer + stopCh chan struct{} // needed for listing the zones in the cluster. translator *translator.Translator // needed for linking the NEG with the backend service for each ILB service. @@ -100,6 +103,13 @@ func NewILBController(ctx *context.ControllerContext, stopCh chan struct{}) *L4C l4c.svcQueue = utils.NewPeriodicTaskQueueWithMultipleWorkers("l4", "services", l4c.numWorkers, l4c.sync) + if ctx.NetworkInformer != nil { + l4c.networkLister = ctx.NetworkInformer.GetIndexer() + } + if ctx.GKENetworkParamsInformer != nil { + l4c.gkeNetworkParamSetLister = ctx.GKENetworkParamsInformer.GetIndexer() + } + ctx.ServiceInformer.AddEventHandler(cache.ResourceEventHandlerFuncs{ AddFunc: func(obj interface{}) { addSvc := obj.(*v1.Service) @@ -227,6 +237,10 @@ func (l4c *L4Controller) processServiceCreateOrUpdate(service *v1.Service) *load if err != nil { return &loadbalancers.L4ILBSyncResult{Error: err} } + network, err := network.ServiceNetwork(service, l4c.networkLister, l4c.gkeNetworkParamSetLister, l4c.ctx.Cloud, klog.TODO()) + if err != nil { + return &loadbalancers.L4ILBSyncResult{Error: err} + } // Use the same function for both create and updates. If controller crashes and restarts, // all existing services will show up as Service Adds. l4ilbParams := &loadbalancers.L4ILBParams{ @@ -235,6 +249,7 @@ func (l4c *L4Controller) processServiceCreateOrUpdate(service *v1.Service) *load Namer: l4c.namer, Recorder: l4c.ctx.Recorder(service.Namespace), DualStackEnabled: l4c.enableDualStack, + Network: *network, } l4 := loadbalancers.NewL4Handler(l4ilbParams) syncResult := l4.EnsureInternalLoadBalancer(nodeNames, service) diff --git a/pkg/l4lb/l4netlbcontroller.go b/pkg/l4lb/l4netlbcontroller.go index 79025a17fb..4c3cd9687e 100644 --- a/pkg/l4lb/l4netlbcontroller.go +++ b/pkg/l4lb/l4netlbcontroller.go @@ -36,6 +36,7 @@ import ( "k8s.io/ingress-gce/pkg/instancegroups" "k8s.io/ingress-gce/pkg/l4lb/metrics" "k8s.io/ingress-gce/pkg/loadbalancers" + "k8s.io/ingress-gce/pkg/network" "k8s.io/ingress-gce/pkg/utils" "k8s.io/ingress-gce/pkg/utils/common" "k8s.io/ingress-gce/pkg/utils/namer" @@ -442,6 +443,7 @@ func (lc *L4NetLBController) syncInternal(service *v1.Service) *loadbalancers.L4 Namer: lc.namer, Recorder: lc.ctx.Recorder(service.Namespace), DualStackEnabled: lc.enableDualStack, + NetworkInfo: *network.DefaultNetwork(lc.ctx.Cloud), } l4netlb := loadbalancers.NewL4NetLB(l4NetLBParams) // check again that rbs is enabled. @@ -584,6 +586,7 @@ func (lc *L4NetLBController) garbageCollectRBSNetLB(key string, svc *v1.Service) Namer: lc.namer, Recorder: lc.ctx.Recorder(svc.Namespace), DualStackEnabled: lc.enableDualStack, + NetworkInfo: *network.DefaultNetwork(lc.ctx.Cloud), } l4netLB := loadbalancers.NewL4NetLB(l4NetLBParams) lc.ctx.Recorder(svc.Namespace).Eventf(svc, v1.EventTypeNormal, "DeletingLoadBalancer", diff --git a/pkg/loadbalancers/forwarding_rules.go b/pkg/loadbalancers/forwarding_rules.go index 5605d438bd..bad04ac266 100644 --- a/pkg/loadbalancers/forwarding_rules.go +++ b/pkg/loadbalancers/forwarding_rules.go @@ -236,7 +236,7 @@ func (l4 *L4) ensureIPv4ForwardingRule(bsLink string, options gce.ILBOptions, ex IPProtocol: string(protocol), LoadBalancingScheme: string(cloud.SchemeInternal), Subnetwork: subnetworkURL, - Network: l4.cloud.NetworkURL(), + Network: l4.network.NetworkURL, NetworkTier: cloud.NetworkTierDefault.ToGCEValue(), Version: version, BackendService: bsLink, @@ -427,10 +427,15 @@ func Equal(fr1, fr2 *composite.ForwardingRule) (bool, error) { id1.Equal(id2) && fr1.AllowGlobalAccess == fr2.AllowGlobalAccess && fr1.AllPorts == fr2.AllPorts && - fr1.Subnetwork == fr2.Subnetwork && + equalResourcePaths(fr1.Subnetwork, fr2.Subnetwork) && + equalResourcePaths(fr1.Network, fr2.Network) && fr1.NetworkTier == fr2.NetworkTier, nil } +func equalResourcePaths(rp1, rp2 string) bool { + return rp1 == rp2 || utils.EqualResourceIDs(rp1, rp2) +} + // l4lbIPToUse determines which IP address needs to be used in the ForwardingRule. If an IP has been // specified by the user, that is used. If there is an existing ForwardingRule, the ip address from // that is reused. In case a subnetwork change is requested, the existing ForwardingRule IP is ignored. diff --git a/pkg/loadbalancers/forwarding_rules_test.go b/pkg/loadbalancers/forwarding_rules_test.go index c823712763..c56b366df9 100644 --- a/pkg/loadbalancers/forwarding_rules_test.go +++ b/pkg/loadbalancers/forwarding_rules_test.go @@ -6,6 +6,8 @@ import ( "github.com/GoogleCloudPlatform/k8s-cloud-provider/pkg/cloud" "github.com/GoogleCloudPlatform/k8s-cloud-provider/pkg/cloud/meta" + "github.com/google/go-cmp/cmp" + "github.com/google/go-cmp/cmp/cmpopts" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" compute "google.golang.org/api/compute/v1" @@ -16,6 +18,7 @@ import ( "k8s.io/cloud-provider-gcp/providers/gce" "k8s.io/ingress-gce/pkg/composite" "k8s.io/ingress-gce/pkg/forwardingrules" + "k8s.io/ingress-gce/pkg/network" "k8s.io/ingress-gce/pkg/test" "k8s.io/ingress-gce/pkg/utils" "k8s.io/ingress-gce/pkg/utils/namer" @@ -197,58 +200,128 @@ func TestForwardingRulesEqual(t *testing.T) { } for _, tc := range []struct { - desc string - oldFwdRule *composite.ForwardingRule - newFwdRule *composite.ForwardingRule - expect bool + desc string + oldFwdRule *composite.ForwardingRule + newFwdRule *composite.ForwardingRule + expectEqual bool }{ { - desc: "empty ip address does not match valid ip", - oldFwdRule: fwdRules[0], - newFwdRule: fwdRules[1], - expect: false, + desc: "empty ip address does not match valid ip", + oldFwdRule: fwdRules[0], + newFwdRule: fwdRules[1], + expectEqual: false, + }, + { + desc: "global access enabled", + oldFwdRule: fwdRules[1], + newFwdRule: fwdRules[3], + expectEqual: false, + }, + { + desc: "IP protocol changed", + oldFwdRule: fwdRules[1], + newFwdRule: fwdRules[2], + expectEqual: false, }, { - desc: "global access enabled", - oldFwdRule: fwdRules[1], - newFwdRule: fwdRules[3], - expect: false, + desc: "same forwarding rule", + oldFwdRule: fwdRules[3], + newFwdRule: fwdRules[3], + expectEqual: true, }, { - desc: "IP protocol changed", - oldFwdRule: fwdRules[1], - newFwdRule: fwdRules[2], - expect: false, + desc: "same forwarding rule, different basepath", + oldFwdRule: fwdRules[4], + newFwdRule: fwdRules[5], + expectEqual: true, }, { - desc: "same forwarding rule", - oldFwdRule: fwdRules[3], - newFwdRule: fwdRules[3], - expect: true, + desc: "same forwarding rule, one uses ALL keyword for ports", + oldFwdRule: fwdRules[2], + newFwdRule: fwdRules[6], + expectEqual: false, }, { - desc: "same forwarding rule, different basepath", - oldFwdRule: fwdRules[4], - newFwdRule: fwdRules[5], - expect: true, + desc: "network tier mismatch", + oldFwdRule: fwdRules[6], + newFwdRule: fwdRules[7], + expectEqual: false, }, { - desc: "same forwarding rule, one uses ALL keyword for ports", - oldFwdRule: fwdRules[2], - newFwdRule: fwdRules[6], - expect: false, + desc: "same forwarding rule, different port ranges", + oldFwdRule: frPortRange1, + newFwdRule: frPortRange2, + expectEqual: false, + }, + { + desc: "network mismatch", + oldFwdRule: &composite.ForwardingRule{ + Name: "tcp-fwd-rule", + IPAddress: "10.0.0.0", + Ports: []string{"123"}, + IPProtocol: "TCP", + LoadBalancingScheme: string(cloud.SchemeInternal), + BackendService: "http://www.googleapis.com/projects/test/regions/us-central1/backendServices/bs1", + Network: "https://www.googleapis.com/compute/v1/projects/test-poject/global/networks/test-vpc", + }, + newFwdRule: &composite.ForwardingRule{ + Name: "tcp-fwd-rule", + IPAddress: "10.0.0.0", + Ports: []string{"123"}, + IPProtocol: "TCP", + LoadBalancingScheme: string(cloud.SchemeInternal), + BackendService: "http://www.googleapis.com/projects/test/regions/us-central1/backendServices/bs1", + Network: "https://www.googleapis.com/compute/v1/projects/test-poject/global/networks/test-other-vpc", + }, + expectEqual: false, }, { - desc: "network tier mismatch", - oldFwdRule: fwdRules[6], - newFwdRule: fwdRules[7], - expect: false, + desc: "subnetwork mismatch", + oldFwdRule: &composite.ForwardingRule{ + Name: "tcp-fwd-rule", + IPAddress: "10.0.0.0", + Ports: []string{"123"}, + IPProtocol: "TCP", + LoadBalancingScheme: string(cloud.SchemeInternal), + BackendService: "http://www.googleapis.com/projects/test/regions/us-central1/backendServices/bs1", + Network: "https://www.googleapis.com/compute/v1/projects/test-poject/global/networks/test-vpc", + Subnetwork: "https://www.googleapis.com/compute/v1/projects/test-poject/regions/us-central1/subnetworks/default-subnet", + }, + newFwdRule: &composite.ForwardingRule{ + Name: "tcp-fwd-rule", + IPAddress: "10.0.0.0", + Ports: []string{"123"}, + IPProtocol: "TCP", + LoadBalancingScheme: string(cloud.SchemeInternal), + BackendService: "http://www.googleapis.com/projects/test/regions/us-central1/backendServices/bs1", + Network: "https://www.googleapis.com/compute/v1/projects/test-poject/global/networks/test-vpc", + Subnetwork: "https://www.googleapis.com/compute/v1/projects/test-poject/regions/us-central1/subnetworks/other-subnet", + }, + expectEqual: false, }, { - desc: "same forwarding rule, different port ranges", - oldFwdRule: frPortRange1, - newFwdRule: frPortRange2, - expect: false, + desc: "equal network data", + oldFwdRule: &composite.ForwardingRule{ + Name: "tcp-fwd-rule", + IPAddress: "10.0.0.0", + Ports: []string{"123"}, + IPProtocol: "TCP", + LoadBalancingScheme: string(cloud.SchemeInternal), + BackendService: "http://www.googleapis.com/projects/test/regions/us-central1/backendServices/bs1", + Network: "https://www.googleapis.com/compute/v1/projects/test-poject/global/networks/test-vpc", + Subnetwork: "https://www.googleapis.com/compute/v1/projects/test-poject/regions/us-central1/subnetworks/default-subnet", + }, + newFwdRule: &composite.ForwardingRule{ + Name: "tcp-fwd-rule", + IPAddress: "10.0.0.0", + Ports: []string{"123"}, + IPProtocol: "TCP", + LoadBalancingScheme: string(cloud.SchemeInternal), + BackendService: "http://www.googleapis.com/projects/test/regions/us-central1/backendServices/bs1", + Network: "projects/test-poject/global/networks/test-vpc", + Subnetwork: "projects/test-poject/regions/us-central1/subnetworks/default-subnet", + }, + expectEqual: true, }, } { t.Run(tc.desc, func(t *testing.T) { @@ -256,8 +329,8 @@ func TestForwardingRulesEqual(t *testing.T) { if err != nil { t.Errorf("forwardingRulesEqual(_, _) = %v, want nil error", err) } - if got != tc.expect { - t.Errorf("forwardingRulesEqual(_, _) = %t, want %t", got, tc.expect) + if got != tc.expectEqual { + t.Errorf("forwardingRulesEqual(_, _) = %t, want %t", got, tc.expectEqual) } }) } @@ -323,3 +396,70 @@ func TestL4EnsureIPv4ForwardingRuleAddressAlreadyInUse(t *testing.T) { require.Error(t, err) assert.True(t, utils.IsIPConfigurationError(err)) } + +func TestL4EnsureIPv4ForwardingRule(t *testing.T) { + subnetworkURL := "https://www.googleapis.com/compute/v1/projects/test-poject/regions/us-central1/subnetworks/default-subnet" + networkURL := "https://www.googleapis.com/compute/v1/projects/test-poject/global/networks/test-vpc" + l4namer := namer.NewL4Namer("test", namer.NewNamer("testCluster", "testFirewall")) + serviceName := "testService" + serviceNamespace := "default" + defaultService := &corev1.Service{ + ObjectMeta: metav1.ObjectMeta{Name: serviceName, Namespace: serviceNamespace, UID: types.UID("1")}, + Spec: corev1.ServiceSpec{ + Ports: []corev1.ServicePort{ + { + Port: 8080, + Protocol: corev1.ProtocolTCP, + }, + }, + Type: "LoadBalancer", + }, + } + + fakeGCE := gce.NewFakeGCECloud(gce.DefaultTestClusterValues()) + forwardingRules := forwardingrules.New(fakeGCE, meta.VersionGA, meta.Regional) + + l4 := &L4{ + cloud: fakeGCE, + forwardingRules: forwardingRules, + namer: l4namer, + Service: defaultService, + network: network.NetworkInfo{ + IsDefault: false, + NetworkURL: networkURL, + }, + } + ipToUse := "1.1.1.1" + bsLink := "http://www.googleapis.com/projects/test/regions/us-central1/backendServices/bs1" + + forwardingRule, err := l4.ensureIPv4ForwardingRule(bsLink, gce.ILBOptions{}, nil, subnetworkURL, ipToUse) + require.NoError(t, err) + + wantForwardingRule := &composite.ForwardingRule{ + Name: l4namer.L4ForwardingRule(serviceNamespace, serviceName, "tcp"), + IPAddress: "1.1.1.1", + Ports: []string{"8080"}, + IPProtocol: "TCP", + LoadBalancingScheme: string(cloud.SchemeInternal), + Subnetwork: subnetworkURL, + Network: networkURL, + NetworkTier: cloud.NetworkTierDefault.ToGCEValue(), + Version: meta.VersionGA, + BackendService: "http://www.googleapis.com/projects/test/regions/us-central1/backendServices/bs1", + AllowGlobalAccess: false, + Description: ilbServiceDescription(t, serviceName, serviceNamespace, "1.1.1.1"), + } + if diff := cmp.Diff(wantForwardingRule, forwardingRule, cmpopts.IgnoreFields(composite.ForwardingRule{}, "SelfLink", "Region", "Scope")); diff != "" { + t.Errorf("ensureIPv4ForwardingRule() diff -want +got\n%v\n", diff) + } + +} + +func ilbServiceDescription(t *testing.T, svcName, svcNamespace, ipToUse string) string { + description, err := utils.MakeL4LBServiceDescription(utils.ServiceKeyFunc(svcNamespace, svcName), ipToUse, + meta.VersionGA, false, utils.ILB) + if err != nil { + t.Errorf("utils.MakeL4LBServiceDescription() failed, err=%v", err) + } + return description +} diff --git a/pkg/loadbalancers/l4.go b/pkg/loadbalancers/l4.go index 8ca6a63c4a..14bd046913 100644 --- a/pkg/loadbalancers/l4.go +++ b/pkg/loadbalancers/l4.go @@ -35,6 +35,7 @@ import ( "k8s.io/ingress-gce/pkg/forwardingrules" "k8s.io/ingress-gce/pkg/healthchecksl4" "k8s.io/ingress-gce/pkg/metrics" + "k8s.io/ingress-gce/pkg/network" "k8s.io/ingress-gce/pkg/utils" "k8s.io/ingress-gce/pkg/utils/namer" "k8s.io/klog/v2" @@ -55,6 +56,7 @@ type L4 struct { forwardingRules ForwardingRulesProvider healthChecks healthchecksl4.L4HealthChecks enableDualStack bool + network network.NetworkInfo } // L4ILBSyncResult contains information about the outcome of an L4 ILB sync. It stores the list of resource name annotations, @@ -76,6 +78,7 @@ type L4ILBParams struct { Namer namer.L4ResourcesNamer Recorder record.EventRecorder DualStackEnabled bool + Network network.NetworkInfo } // NewL4Handler creates a new L4Handler for the given L4 service. @@ -87,9 +90,10 @@ func NewL4Handler(params *L4ILBParams) *L4 { namer: params.Namer, recorder: params.Recorder, Service: params.Service, - healthChecks: healthchecksl4.NewL4HealthChecks(params.Cloud, params.Recorder), + healthChecks: healthchecksl4.NewL4HealthChecks(params.Cloud, params.Recorder, params.Network), forwardingRules: forwardingrules.New(params.Cloud, meta.VersionGA, scope), enableDualStack: params.DualStackEnabled, + network: params.Network, } l4.NamespacedName = types.NamespacedName{Name: params.Service.Name, Namespace: params.Service.Namespace} l4.backendPool = backends.NewPool(l4.cloud, l4.namer) @@ -360,7 +364,7 @@ func (l4 *L4) EnsureInternalLoadBalancer(nodeNames []string, svc *corev1.Service } // ensure backend service - bs, err := l4.backendPool.EnsureL4BackendService(bsName, hcLink, string(protocol), string(l4.Service.Spec.SessionAffinity), string(cloud.SchemeInternal), l4.NamespacedName) + bs, err := l4.backendPool.EnsureL4BackendService(bsName, hcLink, string(protocol), string(l4.Service.Spec.SessionAffinity), string(cloud.SchemeInternal), l4.NamespacedName, l4.network) if err != nil { result.GCEResourceInError = annotations.BackendServiceResource result.Error = err @@ -518,7 +522,7 @@ func (l4 *L4) getServiceSubnetworkURL(options gce.ILBOptions) (string, error) { if options.SubnetName != "" { return l4.getSubnetworkURLByName(options.SubnetName) } - return l4.cloud.SubnetworkURL(), nil + return l4.network.SubnetworkURL, nil } func (l4 *L4) getSubnetworkURLByName(subnetName string) (string, error) { diff --git a/pkg/loadbalancers/l4_test.go b/pkg/loadbalancers/l4_test.go index 1cdbe21774..48826f71a3 100644 --- a/pkg/loadbalancers/l4_test.go +++ b/pkg/loadbalancers/l4_test.go @@ -29,6 +29,7 @@ import ( "k8s.io/ingress-gce/pkg/backends" "k8s.io/ingress-gce/pkg/firewalls" "k8s.io/ingress-gce/pkg/healthchecksl4" + "k8s.io/ingress-gce/pkg/network" "k8s.io/ingress-gce/pkg/utils" "github.com/GoogleCloudPlatform/k8s-cloud-provider/pkg/cloud" @@ -78,13 +79,13 @@ func TestEnsureInternalBackendServiceUpdates(t *testing.T) { l4.healthChecks = healthchecksl4.Fake(fakeGCE, l4ilbParams.Recorder) bsName := l4.namer.L4Backend(l4.Service.Namespace, l4.Service.Name) - _, err := l4.backendPool.EnsureL4BackendService(bsName, "", "TCP", string(svc.Spec.SessionAffinity), string(cloud.SchemeInternal), l4.NamespacedName) + _, err := l4.backendPool.EnsureL4BackendService(bsName, "", "TCP", string(svc.Spec.SessionAffinity), string(cloud.SchemeInternal), l4.NamespacedName, *network.DefaultNetwork(fakeGCE)) if err != nil { t.Errorf("Failed to ensure backend service %s - err %v", bsName, err) } // Update the Internal Backend Service with a new ServiceAffinity - _, err = l4.backendPool.EnsureL4BackendService(bsName, "", "TCP", string(v1.ServiceAffinityNone), string(cloud.SchemeInternal), l4.NamespacedName) + _, err = l4.backendPool.EnsureL4BackendService(bsName, "", "TCP", string(v1.ServiceAffinityNone), string(cloud.SchemeInternal), l4.NamespacedName, *network.DefaultNetwork(fakeGCE)) if err != nil { t.Errorf("Failed to ensure backend service %s - err %v", bsName, err) } @@ -105,7 +106,7 @@ func TestEnsureInternalBackendServiceUpdates(t *testing.T) { if err != nil { t.Errorf("Failed to update backend service with new connection draining timeout - err %v", err) } - bs, err = l4.backendPool.EnsureL4BackendService(bsName, "", "TCP", string(v1.ServiceAffinityNone), string(cloud.SchemeInternal), l4.NamespacedName) + bs, err = l4.backendPool.EnsureL4BackendService(bsName, "", "TCP", string(v1.ServiceAffinityNone), string(cloud.SchemeInternal), l4.NamespacedName, *network.DefaultNetwork(fakeGCE)) if err != nil { t.Errorf("Failed to ensure backend service %s - err %v", bsName, err) } @@ -252,7 +253,7 @@ func TestEnsureInternalLoadBalancerWithExistingResources(t *testing.T) { if hcResult.Err != nil { t.Errorf("Failed to create healthcheck, err %v", hcResult.Err) } - _, err := l4.backendPool.EnsureL4BackendService(lbName, hcResult.HCLink, "TCP", string(l4.Service.Spec.SessionAffinity), string(cloud.SchemeInternal), l4.NamespacedName) + _, err := l4.backendPool.EnsureL4BackendService(lbName, hcResult.HCLink, "TCP", string(l4.Service.Spec.SessionAffinity), string(cloud.SchemeInternal), l4.NamespacedName, *network.DefaultNetwork(fakeGCE)) if err != nil { t.Errorf("Failed to create backendservice, err %v", err) } diff --git a/pkg/loadbalancers/l4netlb.go b/pkg/loadbalancers/l4netlb.go index 130fe02db6..8812fb2523 100644 --- a/pkg/loadbalancers/l4netlb.go +++ b/pkg/loadbalancers/l4netlb.go @@ -34,6 +34,7 @@ import ( "k8s.io/ingress-gce/pkg/forwardingrules" "k8s.io/ingress-gce/pkg/healthchecksl4" "k8s.io/ingress-gce/pkg/metrics" + "k8s.io/ingress-gce/pkg/network" "k8s.io/ingress-gce/pkg/utils" "k8s.io/ingress-gce/pkg/utils/namer" "k8s.io/klog/v2" @@ -92,6 +93,7 @@ type L4NetLBParams struct { Namer namer.L4ResourcesNamer Recorder record.EventRecorder DualStackEnabled bool + NetworkInfo network.NetworkInfo } // NewL4NetLB creates a new Handler for the given L4NetLB service. @@ -104,7 +106,7 @@ func NewL4NetLB(params *L4NetLBParams) *L4NetLB { Service: params.Service, NamespacedName: types.NamespacedName{Name: params.Service.Name, Namespace: params.Service.Namespace}, backendPool: backends.NewPool(params.Cloud, params.Namer), - healthChecks: healthchecksl4.NewL4HealthChecks(params.Cloud, params.Recorder), + healthChecks: healthchecksl4.NewL4HealthChecks(params.Cloud, params.Recorder, params.NetworkInfo), forwardingRules: forwardingrules.New(params.Cloud, meta.VersionGA, meta.Regional), enableDualStack: params.DualStackEnabled, } @@ -192,7 +194,7 @@ func (l4netlb *L4NetLB) provideBackendService(syncResult *L4NetLBSyncResult, hcL 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, 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, *network.DefaultNetwork(l4netlb.cloud)) if err != nil { syncResult.GCEResourceInError = annotations.BackendServiceResource syncResult.Error = fmt.Errorf("Failed to ensure backend service %s - %w", bsName, err) diff --git a/pkg/network/network.go b/pkg/network/network.go index 7f30c12e8e..4a43d247db 100644 --- a/pkg/network/network.go +++ b/pkg/network/network.go @@ -40,12 +40,12 @@ const ( // secondary networks information for multi-networked services in the future. func ServiceNetwork(service *apiv1.Service, networkLister, gkeNetworkParamSetLister cache.Indexer, cloudProvider cloudNetworkProvider, logger klog.Logger) (*NetworkInfo, error) { if networkLister == nil || gkeNetworkParamSetLister == nil { - return defaultNetwork(cloudProvider), nil + return DefaultNetwork(cloudProvider), nil } logger.Info("Network lookup for service", "service", service.Name, "namespace", service.Namespace) networkName, ok := service.Spec.Selector[networkSelector] if !ok || networkName == "" || networkName == networkv1.DefaultPodNetworkName { - return defaultNetwork(cloudProvider), nil + return DefaultNetwork(cloudProvider), nil } obj, exists, err := networkLister.GetByKey(networkName) if err != nil { @@ -89,7 +89,7 @@ func ServiceNetwork(service *apiv1.Service, networkLister, gkeNetworkParamSetLis }, nil } -func defaultNetwork(cloudProvider cloudNetworkProvider) *NetworkInfo { +func DefaultNetwork(cloudProvider cloudNetworkProvider) *NetworkInfo { return &NetworkInfo{ IsDefault: true, K8sNetwork: networkv1.DefaultPodNetworkName, diff --git a/pkg/network/network_test.go b/pkg/network/network_test.go index ac488abb1f..d61960a6bd 100644 --- a/pkg/network/network_test.go +++ b/pkg/network/network_test.go @@ -78,7 +78,7 @@ func TestServiceNetwork(t *testing.T) { }, }, }, - want: defaultNetwork(cloud), + want: DefaultNetwork(cloud), }, { desc: "service with empty network selector", @@ -90,7 +90,7 @@ func TestServiceNetwork(t *testing.T) { }, }, }, - want: defaultNetwork(cloud), + want: DefaultNetwork(cloud), }, { desc: "service with network selector for the default network", @@ -102,7 +102,7 @@ func TestServiceNetwork(t *testing.T) { }, }, }, - want: defaultNetwork(cloud), + want: DefaultNetwork(cloud), }, { desc: "network not defined",