diff --git a/cmd/glbc/main.go b/cmd/glbc/main.go index c2f63842c7..17af41ed1d 100644 --- a/cmd/glbc/main.go +++ b/cmd/glbc/main.go @@ -19,7 +19,6 @@ package main import ( "context" "fmt" - "k8s.io/ingress-gce/pkg/healthchecks" "math/rand" "os" "time" @@ -27,6 +26,7 @@ import ( flag "github.com/spf13/pflag" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/ingress-gce/pkg/frontendconfig" + "k8s.io/ingress-gce/pkg/healthchecks" "k8s.io/ingress-gce/pkg/ingparams" "k8s.io/ingress-gce/pkg/l4lb" "k8s.io/ingress-gce/pkg/psc" @@ -275,7 +275,7 @@ func runControllers(ctx *ingctx.ControllerContext) { fwc := firewalls.NewFirewallController(ctx, flags.F.NodePortRanges.Values()) - healthchecks.Initialize(ctx.Cloud, ctx) + healthchecks.InitializeL4(ctx.Cloud, ctx) if flags.F.RunL4Controller { l4Controller := l4lb.NewILBController(ctx, stopCh) diff --git a/hack/run-local-glbc.sh b/hack/run-local-glbc.sh index fcc2e83234..f5451c3c35 100755 --- a/hack/run-local-glbc.sh +++ b/hack/run-local-glbc.sh @@ -43,7 +43,5 @@ ${GLBC} \ --running-in-cluster=false \ --logtostderr --v=${V} \ --config-file-path=${GCECONF} \ - --run-l4-controller \ - --run-l4-netlb-controller \ "${@}" \ 2>&1 | tee -a /tmp/glbc.log diff --git a/pkg/firewalls/firewalls_l4.go b/pkg/firewalls/firewalls_l4.go index f004d0649e..238095a250 100644 --- a/pkg/firewalls/firewalls_l4.go +++ b/pkg/firewalls/firewalls_l4.go @@ -76,7 +76,9 @@ func EnsureL4FirewallRule(cloud *gce.Cloud, nsName string, params *FirewallParam } return err } - if firewallRuleEqual(expectedFw, existingFw) { + + // Don't compare the "description" field for shared firewall rules + if firewallRuleEqual(expectedFw, existingFw, sharedRule) { return nil } klog.V(2).Infof("EnsureL4FirewallRule(%v): updating L4 %s firewall", params.Name, params.L4Type.ToString()) @@ -101,13 +103,19 @@ func EnsureL4FirewallRuleDeleted(cloud *gce.Cloud, fwName string) error { return nil } -func firewallRuleEqual(a, b *compute.Firewall) bool { - // let's skip description not to trigger flood of updates - return len(a.Allowed) == 1 && len(a.Allowed) == len(b.Allowed) && +func firewallRuleEqual(a, b *compute.Firewall, skipDescription bool) bool { + fwrEqual := len(a.Allowed) == 1 && + len(a.Allowed) == len(b.Allowed) && a.Allowed[0].IPProtocol == b.Allowed[0].IPProtocol && utils.EqualStringSets(a.Allowed[0].Ports, b.Allowed[0].Ports) && utils.EqualStringSets(a.SourceRanges, b.SourceRanges) && utils.EqualStringSets(a.TargetTags, b.TargetTags) + + // Don't compare the "description" field for shared firewall rules + if skipDescription { + return fwrEqual + } + return fwrEqual && a.Description == b.Description } func ensureFirewall(svc *v1.Service, shared bool, params *FirewallParams, cloud *gce.Cloud, recorder record.EventRecorder) error { diff --git a/pkg/healthchecks/healthchecks_l4.go b/pkg/healthchecks/healthchecks_l4.go index f016263390..fa5d6c2b82 100644 --- a/pkg/healthchecks/healthchecks_l4.go +++ b/pkg/healthchecks/healthchecks_l4.go @@ -18,9 +18,6 @@ package healthchecks import ( "fmt" - "k8s.io/ingress-gce/pkg/annotations" - "k8s.io/ingress-gce/pkg/events" - "k8s.io/ingress-gce/pkg/firewalls" "strconv" "sync" @@ -29,7 +26,10 @@ import ( corev1 "k8s.io/api/core/v1" "k8s.io/apimachinery/pkg/types" "k8s.io/cloud-provider/service/helpers" + "k8s.io/ingress-gce/pkg/annotations" "k8s.io/ingress-gce/pkg/composite" + "k8s.io/ingress-gce/pkg/events" + "k8s.io/ingress-gce/pkg/firewalls" "k8s.io/ingress-gce/pkg/utils" "k8s.io/ingress-gce/pkg/utils/namer" "k8s.io/klog" @@ -37,7 +37,6 @@ import ( ) const ( - // L4 Load Balancer parameters gceHcCheckIntervalSeconds = int64(8) gceHcTimeoutSeconds = int64(1) @@ -47,7 +46,12 @@ const ( gceHcUnhealthyThreshold = int64(3) ) -var instance *l4HealthChecks +var ( + // instance is a sinngleton instance, created by InitializeL4 + instance *l4HealthChecks + // mutex for preventing multiple initialization + initLock = &sync.Mutex{} +) type l4HealthChecks struct { mutex sync.Mutex @@ -55,36 +59,47 @@ type l4HealthChecks struct { recorderFactory events.RecorderProducer } -func Initialize(cloud *gce.Cloud, recorderFactory events.RecorderProducer) { - instance = &l4HealthChecks{ - cloud: cloud, - recorderFactory: recorderFactory, +// InitializeL4 creates singleton instance, must be run before GetL4() func +func InitializeL4(cloud *gce.Cloud, recorderFactory events.RecorderProducer) { + if instance == nil { + initLock.Lock() + defer initLock.Unlock() + + if instance == nil { + instance = &l4HealthChecks{ + cloud: cloud, + recorderFactory: recorderFactory, + } + } } } -func Fake(cloud *gce.Cloud, recorderFactory events.RecorderProducer) *l4HealthChecks { - return &l4HealthChecks{ +// FakeL4 creates instance of l4HealthChecks> USe for test only. +func FakeL4(cloud *gce.Cloud, recorderFactory events.RecorderProducer) *l4HealthChecks { + instance = &l4HealthChecks{ cloud: cloud, recorderFactory: recorderFactory, } + return instance } +// GetL4 returns singleton instance, must be run after InitializeL4 func GetL4() *l4HealthChecks { return instance } -// EnsureL4HealthCheck creates a new HTTP health check for an L4 LoadBalancer service, based on the parameters provided. +// EnsureL4HealthCheck creates a new HTTP health check for an L4 LoadBalancer service, and the firewall rule required +// for the healthcheck. If healthcheck is shared (external traffic policy 'cluster') then firewall rules will be shared +// regardless of scope param. // If the healthcheck already exists, it is updated as needed. func (l4hc *l4HealthChecks) EnsureL4HealthCheck(svc *corev1.Service, namer namer.L4ResourcesNamer, sharedHC bool, scope meta.KeyType, l4Type utils.L4LBType, nodeNames []string) (string, string, string, string, error) { hcName, hcFwName := namer.L4HealthCheck(svc.Namespace, svc.Name, sharedHC) - hcPath, hcPort := gce.GetNodesHealthCheckPath(), gce.GetNodesHealthCheckPort() - + hcPath, hcPort := helpers.GetServiceHealthCheckPathPort(svc) if sharedHC { + hcPath, hcPort = gce.GetNodesHealthCheckPath(), gce.GetNodesHealthCheckPort() // lock out entire EnsureL4HealthCheck process l4hc.mutex.Lock() defer l4hc.mutex.Unlock() - } else { - hcPath, hcPort = helpers.GetServiceHealthCheckPathPort(svc) } namespacedName := types.NamespacedName{Name: svc.Name, Namespace: svc.Namespace} @@ -100,6 +115,7 @@ func (l4hc *l4HealthChecks) EnsureL4HealthCheck(svc *corev1.Service, namer namer return hcLink, hcFwName, hcName, "", err } +// DeleteHealthCheck deletes health check (and firewall rule) for l4 service. Checks if shared resources are safe to delete. func (l4hc *l4HealthChecks) DeleteHealthCheck(svc *corev1.Service, namer namer.L4ResourcesNamer, sharedHC bool, scope meta.KeyType, l4Type utils.L4LBType) (string, error) { if sharedHC { // lock out entire DeleteHealthCheck process @@ -128,7 +144,7 @@ func (l4hc *l4HealthChecks) ensureL4HealthCheckInternal(name string, svcName typ selfLink := "" key, err := composite.CreateKey(l4hc.cloud, name, scope) if err != nil { - return nil, selfLink, fmt.Errorf("Failed to create key for for healthcheck with name %s for service %s", name, svcName.String()) + return nil, selfLink, fmt.Errorf("Failed to create key for healthcheck with name %s for service %s", name, svcName.String()) } hc, err := composite.GetHealthCheck(l4hc.cloud, key, meta.VersionGA) if err != nil { @@ -165,6 +181,9 @@ func (l4hc *l4HealthChecks) ensureL4HealthCheckInternal(name string, svcName typ return expectedHC, selfLink, err } +// ensureFirewall rule for L4 service. +// The firewall rules are the same for ILB and NetLB that use external traffic policy 'local' (sharedHC == true) +// despite healthchecks have different scopes (global vs regional) func (l4hc *l4HealthChecks) ensureFirewall(svc *corev1.Service, hcFwName string, hcPort int32, sharedHC bool, nodeNames []string) error { // Add firewall rule for healthchecks to nodes hcFWRParams := firewalls.FirewallParams{ @@ -174,15 +193,10 @@ func (l4hc *l4HealthChecks) ensureFirewall(svc *corev1.Service, hcFwName string, Name: hcFwName, NodeNames: nodeNames, } - err := firewalls.EnsureL4LBFirewallForHc(svc, sharedHC, &hcFWRParams, l4hc.cloud, l4hc.recorderFactory.Recorder(svc.Namespace)) - if err != nil { - return err - } - return nil + return firewalls.EnsureL4LBFirewallForHc(svc, sharedHC, &hcFWRParams, l4hc.cloud, l4hc.recorderFactory.Recorder(svc.Namespace)) } func (l4hc *l4HealthChecks) deleteHealthCheck(name string, scope meta.KeyType) error { - // always check mutex key, err := composite.CreateKey(l4hc.cloud, name, scope) if err != nil { return fmt.Errorf("Failed to create composite key for healthcheck %s - %w", name, err) @@ -202,7 +216,7 @@ func (l4hc *l4HealthChecks) deleteHealthCheckFirewall(svc *corev1.Service, hcNam klog.V(2).Infof("Failed to delete health check firewall rule %s: health check in use.", hcName) return "", nil } - // Delete healthcheck firewall rule if no healthcheck uses the firewall rule + // Delete healthcheck firewall rule if no healthcheck uses the firewall rule. err = l4hc.deleteFirewall(hcFwName, svc) if err != nil { klog.Errorf("Failed to delete firewall rule %s for internal loadbalancer service %s, err %v", hcFwName, namespacedName.String(), err) diff --git a/pkg/l4lb/l4controller_test.go b/pkg/l4lb/l4controller_test.go index 51a852285a..d49c989cb2 100644 --- a/pkg/l4lb/l4controller_test.go +++ b/pkg/l4lb/l4controller_test.go @@ -71,7 +71,7 @@ func newServiceController(t *testing.T, fakeGCE *gce.Cloud) *L4Controller { for _, n := range nodes { ctx.NodeInformer.GetIndexer().Add(n) } - healthchecks.Initialize(ctx.Cloud, ctx) + healthchecks.FakeL4(ctx.Cloud, ctx) return NewILBController(ctx, stopCh) } diff --git a/pkg/l4lb/l4netlbcontroller_test.go b/pkg/l4lb/l4netlbcontroller_test.go index 955077035f..668a8bb786 100644 --- a/pkg/l4lb/l4netlbcontroller_test.go +++ b/pkg/l4lb/l4netlbcontroller_test.go @@ -239,7 +239,7 @@ func newL4NetLBServiceController() *L4NetLBController { for _, n := range nodes { ctx.NodeInformer.GetIndexer().Add(n) } - healthchecks.Initialize(ctx.Cloud, ctx) + healthchecks.FakeL4(ctx.Cloud, ctx) return NewL4NetLBController(ctx, stopCh) } @@ -835,7 +835,7 @@ func TestHealthCheckWhenExternalTrafficPolicyWasUpdated(t *testing.T) { // delete shared health check if is created, update service to Cluster and // check that non-shared health check was created hcNameShared, _ := lc.namer.L4HealthCheck(svc.Namespace, svc.Name, true) - healthchecks.Fake(lc.ctx.Cloud, lc.ctx).DeleteHealthCheck(svc, lc.namer, true, meta.Regional, utils.XLB) + healthchecks.FakeL4(lc.ctx.Cloud, lc.ctx).DeleteHealthCheck(svc, lc.namer, true, meta.Regional, utils.XLB) // Update ExternalTrafficPolicy to Cluster check if shared HC was created err = updateAndAssertExternalTrafficPolicy(newSvc, lc, v1.ServiceExternalTrafficPolicyTypeCluster, hcNameShared) if err != nil { diff --git a/pkg/loadbalancers/interfaces.go b/pkg/loadbalancers/interfaces.go index 57086b2543..c6106cba3e 100644 --- a/pkg/loadbalancers/interfaces.go +++ b/pkg/loadbalancers/interfaces.go @@ -41,7 +41,7 @@ type LoadBalancerPool interface { HasUrlMap(ing *v1.Ingress) (bool, error) } -// L4HealthChecks defines methods for creating adn deleting health checks (and their firewall rules) for l4 services +// L4HealthChecks defines methods for creating and deleting health checks (and their firewall rules) for l4 services type L4HealthChecks interface { // EnsureL4HealthCheck creates health check (and firewall rule) for l4 service EnsureL4HealthCheck(svc *corev1.Service, namer namer.L4ResourcesNamer, sharedHC bool, scope meta.KeyType, l4Type utils.L4LBType, nodeNames []string) (string, string, string, string, error) diff --git a/pkg/loadbalancers/l4_test.go b/pkg/loadbalancers/l4_test.go index 67a2596344..c5bdbea5ae 100644 --- a/pkg/loadbalancers/l4_test.go +++ b/pkg/loadbalancers/l4_test.go @@ -68,7 +68,7 @@ func TestEnsureInternalBackendServiceUpdates(t *testing.T) { svc := test.NewL4ILBService(false, 8080) namer := namer_util.NewL4Namer(kubeSystemUID, nil) l := NewL4Handler(svc, fakeGCE, meta.Regional, namer, record.NewFakeRecorder(100)) - l.l4HealthChecks = healthchecks.Fake(fakeGCE, &test.FakeRecorderSource{}) + l.l4HealthChecks = healthchecks.FakeL4(fakeGCE, &test.FakeRecorderSource{}) bsName, _ := l.namer.L4Backend(l.Service.Namespace, l.Service.Name) _, err := l.backendPool.EnsureL4BackendService(bsName, "", "TCP", string(svc.Spec.SessionAffinity), string(cloud.SchemeInternal), l.NamespacedName, meta.VersionGA) @@ -119,7 +119,7 @@ func TestEnsureInternalLoadBalancer(t *testing.T) { svc := test.NewL4ILBService(false, 8080) namer := namer_util.NewL4Namer(kubeSystemUID, nil) l := NewL4Handler(svc, fakeGCE, meta.Regional, namer, record.NewFakeRecorder(100)) - l.l4HealthChecks = healthchecks.Fake(fakeGCE, &test.FakeRecorderSource{}) + l.l4HealthChecks = healthchecks.FakeL4(fakeGCE, &test.FakeRecorderSource{}) if _, err := test.CreateAndInsertNodes(l.cloud, nodeNames, vals.ZoneName); err != nil { t.Errorf("Unexpected error when adding nodes %v", err) @@ -176,7 +176,7 @@ func TestEnsureInternalLoadBalancerTypeChange(t *testing.T) { svc := test.NewL4ILBService(false, 8080) namer := namer_util.NewL4Namer(kubeSystemUID, nil) l := NewL4Handler(svc, fakeGCE, meta.Regional, namer, record.NewFakeRecorder(100)) - l.l4HealthChecks = healthchecks.Fake(fakeGCE, &test.FakeRecorderSource{}) + l.l4HealthChecks = healthchecks.FakeL4(fakeGCE, &test.FakeRecorderSource{}) if _, err := test.CreateAndInsertNodes(l.cloud, nodeNames, vals.ZoneName); err != nil { t.Errorf("Unexpected error when adding nodes %v", err) @@ -210,7 +210,7 @@ func TestEnsureInternalLoadBalancerWithExistingResources(t *testing.T) { namer := namer_util.NewL4Namer(kubeSystemUID, nil) l := NewL4Handler(svc, fakeGCE, meta.Regional, namer, record.NewFakeRecorder(100)) - l.l4HealthChecks = healthchecks.Fake(fakeGCE, &test.FakeRecorderSource{}) + l.l4HealthChecks = healthchecks.FakeL4(fakeGCE, &test.FakeRecorderSource{}) if _, err := test.CreateAndInsertNodes(l.cloud, nodeNames, vals.ZoneName); err != nil { t.Errorf("Unexpected error when adding nodes %v", err) @@ -253,7 +253,7 @@ func TestEnsureInternalLoadBalancerClearPreviousResources(t *testing.T) { svc := test.NewL4ILBService(true, 8080) namer := namer_util.NewL4Namer(kubeSystemUID, nil) l := NewL4Handler(svc, fakeGCE, meta.Regional, namer, record.NewFakeRecorder(100)) - l.l4HealthChecks = healthchecks.Fake(fakeGCE, &test.FakeRecorderSource{}) + l.l4HealthChecks = healthchecks.FakeL4(fakeGCE, &test.FakeRecorderSource{}) _, err := test.CreateAndInsertNodes(l.cloud, nodeNames, vals.ZoneName) if err != nil { @@ -373,7 +373,7 @@ func TestUpdateResourceLinks(t *testing.T) { svc := test.NewL4ILBService(true, 8080) namer := namer_util.NewL4Namer(kubeSystemUID, nil) l := NewL4Handler(svc, fakeGCE, meta.Regional, namer, record.NewFakeRecorder(100)) - l.l4HealthChecks = healthchecks.Fake(fakeGCE, &test.FakeRecorderSource{}) + l.l4HealthChecks = healthchecks.FakeL4(fakeGCE, &test.FakeRecorderSource{}) _, err := test.CreateAndInsertNodes(l.cloud, nodeNames, vals.ZoneName) if err != nil { @@ -451,7 +451,7 @@ func TestEnsureInternalLoadBalancerHealthCheckConfigurable(t *testing.T) { svc := test.NewL4ILBService(true, 8080) namer := namer_util.NewL4Namer(kubeSystemUID, nil) l := NewL4Handler(svc, fakeGCE, meta.Regional, namer, record.NewFakeRecorder(100)) - l.l4HealthChecks = healthchecks.Fake(fakeGCE, &test.FakeRecorderSource{}) + l.l4HealthChecks = healthchecks.FakeL4(fakeGCE, &test.FakeRecorderSource{}) _, err := test.CreateAndInsertNodes(l.cloud, nodeNames, vals.ZoneName) if err != nil { @@ -494,7 +494,7 @@ func TestEnsureInternalLoadBalancerDeleted(t *testing.T) { svc := test.NewL4ILBService(false, 8080) namer := namer_util.NewL4Namer(kubeSystemUID, nil) l := NewL4Handler(svc, fakeGCE, meta.Regional, namer, record.NewFakeRecorder(100)) - l.l4HealthChecks = healthchecks.Fake(fakeGCE, &test.FakeRecorderSource{}) + l.l4HealthChecks = healthchecks.FakeL4(fakeGCE, &test.FakeRecorderSource{}) if _, err := test.CreateAndInsertNodes(l.cloud, nodeNames, vals.ZoneName); err != nil { t.Errorf("Unexpected error when adding nodes %v", err) @@ -526,7 +526,7 @@ func TestEnsureInternalLoadBalancerDeletedTwiceDoesNotError(t *testing.T) { svc := test.NewL4ILBService(false, 8080) namer := namer_util.NewL4Namer(kubeSystemUID, nil) l := NewL4Handler(svc, fakeGCE, meta.Regional, namer, record.NewFakeRecorder(100)) - l.l4HealthChecks = healthchecks.Fake(fakeGCE, &test.FakeRecorderSource{}) + l.l4HealthChecks = healthchecks.FakeL4(fakeGCE, &test.FakeRecorderSource{}) if _, err := test.CreateAndInsertNodes(l.cloud, nodeNames, vals.ZoneName); err != nil { t.Errorf("Unexpected error when adding nodes %v", err) @@ -592,7 +592,6 @@ func TestHealthCheckFirewallDeletionWithNetLB(t *testing.T) { vals := gce.DefaultTestClusterValues() fakeGCE := getFakeGCECloud(vals) - (fakeGCE.Compute().(*cloud.MockGCE)).MockHealthChecks.DeleteHook = test.DeleteHealthCheckResourceInUseErrorHook nodeNames := []string{"test-node-1"} namer := namer_util.NewL4Namer(kubeSystemUID, nil) @@ -605,11 +604,10 @@ func TestHealthCheckFirewallDeletionWithNetLB(t *testing.T) { // Create NetLB Service netlbSvc := test.NewL4NetLBRBSService(8080) l4NetLB := NewL4NetLB(netlbSvc, fakeGCE, meta.Regional, namer, record.NewFakeRecorder(100)) - l4NetLB.l4HealthChecks = healthchecks.Fake(fakeGCE, &test.FakeRecorderSource{}) + // make sure both ilb and netlb use the same l4 healtcheck instance + l4NetLB.l4HealthChecks = l.l4HealthChecks - if _, err := test.CreateAndInsertNodes(l4NetLB.cloud, nodeNames, vals.ZoneName); err != nil { - t.Errorf("Unexpected error when adding nodes %v", err) - } + // create netlb resources xlbResult := l4NetLB.EnsureFrontend(nodeNames, netlbSvc) if xlbResult.Error != nil { t.Errorf("Failed to ensure loadBalancer, err %v", xlbResult.Error) @@ -624,18 +622,25 @@ func TestHealthCheckFirewallDeletionWithNetLB(t *testing.T) { if result.Error != nil { t.Errorf("Unexpected error %v", result.Error) } + // When NetLB health check uses the same firewall rules we expect that hc firewall rule will not be deleted. - _, hcFwName := l.namer.L4HealthCheck(l.Service.Namespace, l.Service.Name, true) + haName, hcFwName := l.namer.L4HealthCheck(l.Service.Namespace, l.Service.Name, true) firewall, err := l.cloud.GetFirewall(hcFwName) if err != nil || firewall == nil { t.Errorf("Expected firewall exists err: %v, fwR: %v", err, firewall) } + + // The healthcheck itself should be deleted. + healthcheck, err := l.cloud.GetHealthCheck(haName) + if err == nil || healthcheck != nil { + t.Errorf("Expected error when looking up shared healthcheck after deletion") + } } func ensureService(fakeGCE *gce.Cloud, namer *namer_util.L4Namer, nodeNames []string, zoneName string, port int, t *testing.T) (*v1.Service, *L4, *L4ILBSyncResult) { svc := test.NewL4ILBService(false, 8080) l := NewL4Handler(svc, fakeGCE, meta.Regional, namer, record.NewFakeRecorder(100)) - l.l4HealthChecks = healthchecks.Fake(fakeGCE, &test.FakeRecorderSource{}) + l.l4HealthChecks = healthchecks.FakeL4(fakeGCE, &test.FakeRecorderSource{}) if _, err := test.CreateAndInsertNodes(l.cloud, nodeNames, zoneName); err != nil { return nil, nil, &L4ILBSyncResult{Error: fmt.Errorf("Unexpected error when adding nodes %v", err)} @@ -660,7 +665,7 @@ func TestEnsureInternalLoadBalancerWithSpecialHealthCheck(t *testing.T) { svc := test.NewL4ILBService(false, 8080) namer := namer_util.NewL4Namer(kubeSystemUID, nil) l := NewL4Handler(svc, fakeGCE, meta.Regional, namer, record.NewFakeRecorder(100)) - l.l4HealthChecks = healthchecks.Fake(fakeGCE, &test.FakeRecorderSource{}) + l.l4HealthChecks = healthchecks.FakeL4(fakeGCE, &test.FakeRecorderSource{}) if _, err := test.CreateAndInsertNodes(l.cloud, nodeNames, vals.ZoneName); err != nil { t.Errorf("Unexpected error when adding nodes %v", err) @@ -767,7 +772,7 @@ func TestEnsureInternalLoadBalancerErrors(t *testing.T) { fakeGCE := getFakeGCECloud(gce.DefaultTestClusterValues()) l := NewL4Handler(params.service, fakeGCE, meta.Regional, namer, record.NewFakeRecorder(100)) - l.l4HealthChecks = healthchecks.Fake(fakeGCE, &test.FakeRecorderSource{}) + l.l4HealthChecks = healthchecks.FakeL4(fakeGCE, &test.FakeRecorderSource{}) //lbName := l.namer.L4Backend(params.service.Namespace, params.service.Name) frName := l.GetFRName() @@ -850,7 +855,7 @@ func TestEnsureInternalLoadBalancerEnableGlobalAccess(t *testing.T) { svc := test.NewL4ILBService(false, 8080) namer := namer_util.NewL4Namer(kubeSystemUID, nil) l := NewL4Handler(svc, fakeGCE, meta.Regional, namer, record.NewFakeRecorder(100)) - l.l4HealthChecks = healthchecks.Fake(fakeGCE, &test.FakeRecorderSource{}) + l.l4HealthChecks = healthchecks.FakeL4(fakeGCE, &test.FakeRecorderSource{}) if _, err := test.CreateAndInsertNodes(l.cloud, nodeNames, vals.ZoneName); err != nil { t.Errorf("Unexpected error when adding nodes %v", err) @@ -932,7 +937,7 @@ func TestEnsureInternalLoadBalancerCustomSubnet(t *testing.T) { svc := test.NewL4ILBService(false, 8080) namer := namer_util.NewL4Namer(kubeSystemUID, nil) l := NewL4Handler(svc, fakeGCE, meta.Regional, namer, record.NewFakeRecorder(100)) - l.l4HealthChecks = healthchecks.Fake(fakeGCE, &test.FakeRecorderSource{}) + l.l4HealthChecks = healthchecks.FakeL4(fakeGCE, &test.FakeRecorderSource{}) if _, err := test.CreateAndInsertNodes(l.cloud, nodeNames, vals.ZoneName); err != nil { t.Errorf("Unexpected error when adding nodes %v", err) @@ -1030,7 +1035,7 @@ func TestEnsureInternalFirewallPortRanges(t *testing.T) { svc := test.NewL4ILBService(false, 8080) namer := namer_util.NewL4Namer(kubeSystemUID, nil) l := NewL4Handler(svc, fakeGCE, meta.Regional, namer, record.NewFakeRecorder(100)) - l.l4HealthChecks = healthchecks.Fake(fakeGCE, &test.FakeRecorderSource{}) + l.l4HealthChecks = healthchecks.FakeL4(fakeGCE, &test.FakeRecorderSource{}) fwName, _ := l.namer.L4Backend(l.Service.Namespace, l.Service.Name) tc := struct { @@ -1084,7 +1089,7 @@ func TestEnsureInternalLoadBalancerModifyProtocol(t *testing.T) { svc := test.NewL4ILBService(false, 8080) namer := namer_util.NewL4Namer(kubeSystemUID, nil) l := NewL4Handler(svc, fakeGCE, meta.Regional, namer, record.NewFakeRecorder(100)) - l.l4HealthChecks = healthchecks.Fake(fakeGCE, &test.FakeRecorderSource{}) + l.l4HealthChecks = healthchecks.FakeL4(fakeGCE, &test.FakeRecorderSource{}) _, err := test.CreateAndInsertNodes(l.cloud, nodeNames, vals.ZoneName) if err != nil { @@ -1176,7 +1181,7 @@ func TestEnsureInternalLoadBalancerAllPorts(t *testing.T) { svc := test.NewL4ILBService(false, 8080) namer := namer_util.NewL4Namer(kubeSystemUID, nil) l := NewL4Handler(svc, fakeGCE, meta.Regional, namer, record.NewFakeRecorder(100)) - l.l4HealthChecks = healthchecks.Fake(fakeGCE, &test.FakeRecorderSource{}) + l.l4HealthChecks = healthchecks.FakeL4(fakeGCE, &test.FakeRecorderSource{}) if _, err := test.CreateAndInsertNodes(l.cloud, nodeNames, vals.ZoneName); err != nil { t.Errorf("Unexpected error when adding nodes %v", err) @@ -1309,9 +1314,9 @@ func assertInternalLbResources(t *testing.T, apiService *v1.Service, l *L4, node if len(firewall.SourceRanges) == 0 { t.Fatalf("Unexpected empty source range for firewall rule %v", firewall) } - //if firewall.Description != info.fwDesc { - // t.Errorf("Unexpected description in firewall %q - Expected %s, Got %s", info.fwName, firewall.Description, info.fwDesc) - //} + if !sharedHC && firewall.Description != info.fwDesc { + t.Errorf("Unexpected description in firewall %q - Expected %s, Got %s", info.fwName, firewall.Description, info.fwDesc) + } } // Check that HealthCheck is created diff --git a/pkg/loadbalancers/l4netlb.go b/pkg/loadbalancers/l4netlb.go index 32aa115f56..75ce995b87 100644 --- a/pkg/loadbalancers/l4netlb.go +++ b/pkg/loadbalancers/l4netlb.go @@ -82,7 +82,6 @@ func NewL4NetLB(service *corev1.Service, cloud *gce.Cloud, scope meta.KeyType, n NodePort: int64(service.Spec.Ports[0].NodePort), L4RBSEnabled: true, } - return l4netlb } diff --git a/pkg/loadbalancers/l4netlb_test.go b/pkg/loadbalancers/l4netlb_test.go index 66f77f7d75..eae3654281 100644 --- a/pkg/loadbalancers/l4netlb_test.go +++ b/pkg/loadbalancers/l4netlb_test.go @@ -56,7 +56,7 @@ func TestEnsureL4NetLoadBalancer(t *testing.T) { namer := namer_util.NewL4Namer(kubeSystemUID, namer_util.NewNamer(vals.ClusterName, "cluster-fw")) l4netlb := NewL4NetLB(svc, fakeGCE, meta.Regional, namer, record.NewFakeRecorder(100)) - l4netlb.l4HealthChecks = healthchecks.Fake(fakeGCE, &test.FakeRecorderSource{}) + l4netlb.l4HealthChecks = healthchecks.FakeL4(fakeGCE, &test.FakeRecorderSource{}) if _, err := test.CreateAndInsertNodes(l4netlb.cloud, nodeNames, vals.ZoneName); err != nil { t.Errorf("Unexpected error when adding nodes %v", err) @@ -107,7 +107,7 @@ func TestDeleteL4NetLoadBalancer(t *testing.T) { namer := namer_util.NewL4Namer(kubeSystemUID, namer_util.NewNamer(vals.ClusterName, "cluster-fw")) l4NetLB := NewL4NetLB(svc, fakeGCE, meta.Regional, namer, record.NewFakeRecorder(100)) - l4NetLB.l4HealthChecks = healthchecks.Fake(fakeGCE, &test.FakeRecorderSource{}) + l4NetLB.l4HealthChecks = healthchecks.FakeL4(fakeGCE, &test.FakeRecorderSource{}) if _, err := test.CreateAndInsertNodes(l4NetLB.cloud, nodeNames, vals.ZoneName); err != nil { t.Errorf("Unexpected error when adding nodes %v", err) @@ -144,26 +144,33 @@ func TestDeleteL4NetLoadBalancerWithSharedHC(t *testing.T) { _, hcFwName := l4NetLB.namer.L4HealthCheck(svc.Namespace, svc.Name, true) firewall, err := l4NetLB.cloud.GetFirewall(hcFwName) if err != nil || firewall == nil { - t.Fatalf("Firewall rule should not be deleted err: %v", err) + t.Errorf("Expected firewall exists err: %v, fwR: %v", err, firewall) } } -func TestHealthCheckFirewallDeletionWhithILB(t *testing.T) { +func TestHealthCheckFirewallDeletionWithILB(t *testing.T) { t.Parallel() nodeNames := []string{"test-node-1"} vals := gce.DefaultTestClusterValues() + fakeGCE := getFakeGCECloud(vals) + namer := namer_util.NewL4Namer(kubeSystemUID, namer_util.NewNamer(vals.ClusterName, "cluster-fw")) + + // Create ILB service + _, l4ilb, ilbResult := ensureService(fakeGCE, namer, nodeNames, vals.ZoneName, 8081, t) + if ilbResult != nil && ilbResult.Error != nil { + t.Fatalf("Error ensuring service err: %v", ilbResult.Error) + } // Create NetLB Service netlbSvc := test.NewL4NetLBRBSService(8080) - namer := namer_util.NewL4Namer(kubeSystemUID, namer_util.NewNamer(vals.ClusterName, "cluster-fw")) l4NetLB := NewL4NetLB(netlbSvc, fakeGCE, meta.Regional, namer, record.NewFakeRecorder(100)) - l4NetLB.l4HealthChecks = healthchecks.Fake(fakeGCE, &test.FakeRecorderSource{}) - if _, err := test.CreateAndInsertNodes(l4NetLB.cloud, nodeNames, vals.ZoneName); err != nil { - t.Errorf("Unexpected error when adding nodes %v", err) - } + // make sure both ilb and netlb use the same l4 healtcheck instance + l4NetLB.l4HealthChecks = l4ilb.l4HealthChecks + + // create netlb resources result := l4NetLB.EnsureFrontend(nodeNames, netlbSvc) if result.Error != nil { t.Errorf("Failed to ensure loadBalancer, err %v", result.Error) @@ -173,22 +180,24 @@ func TestHealthCheckFirewallDeletionWhithILB(t *testing.T) { } assertNetLbResources(t, netlbSvc, l4NetLB, nodeNames) - // Create ILB service - _, _, ilbResult := ensureService(fakeGCE, namer, nodeNames, vals.ZoneName, 8081, t) - if ilbResult != nil && result.Error != nil { - t.Fatalf("Error ensuring service err: %v", result.Error) - } - // Delete the NetLB loadbalancer. if err := l4NetLB.EnsureLoadBalancerDeleted(netlbSvc); err.Error != nil { t.Errorf("UnexpectedError %v", err.Error) } + // When ILB health check uses the same firewall rules we expect that hc firewall rule will not be deleted. - _, hcFwName := l4NetLB.namer.L4HealthCheck(l4NetLB.Service.Namespace, l4NetLB.Service.Name, true) + hcName, hcFwName := l4NetLB.namer.L4HealthCheck(l4NetLB.Service.Namespace, l4NetLB.Service.Name, true) firewall, err := l4NetLB.cloud.GetFirewall(hcFwName) if err != nil || firewall == nil { t.Errorf("Expected firewall exists err: %v, fwR: %v", err, firewall) } + + // The healthcheck itself should be deleted. + _, err = composite.GetHealthCheck(l4NetLB.cloud, meta.RegionalKey(hcName, l4NetLB.cloud.Region()), meta.VersionGA) + if err == nil || !strings.Contains(err.Error(), "not found") { + t.Errorf("Healthcheck %s should be deleted", hcName) + } + } func ensureLoadBalancer(port int, vals gce.TestClusterValues, fakeGCE *gce.Cloud, t *testing.T) (*v1.Service, *L4NetLB) { @@ -197,7 +206,7 @@ func ensureLoadBalancer(port int, vals gce.TestClusterValues, fakeGCE *gce.Cloud emptyNodes := []string{} l4NetLB := NewL4NetLB(svc, fakeGCE, meta.Regional, namer, record.NewFakeRecorder(100)) - l4NetLB.l4HealthChecks = healthchecks.Fake(fakeGCE, &test.FakeRecorderSource{}) + l4NetLB.l4HealthChecks = healthchecks.FakeL4(fakeGCE, &test.FakeRecorderSource{}) result := l4NetLB.EnsureFrontend(emptyNodes, svc) if result.Error != nil { @@ -220,7 +229,7 @@ func ensureNetLBResourceDeleted(t *testing.T, apiService *v1.Service, l4NetLb *L for _, fwName := range []string{resourceName, hcFwName} { _, err := l4NetLb.cloud.GetFirewall(fwName) if err == nil || !utils.IsNotFoundError(err) { - t.Fatalf("Firewall rule %q should be deleted", fwName) + t.Errorf("Firewall rule %q should be deleted", fwName) } } @@ -339,7 +348,7 @@ func TestMetricsForUserStaticService(t *testing.T) { namer := namer_util.NewL4Namer(kubeSystemUID, namer_util.NewNamer(vals.ClusterName, "cluster-fw")) l4netlb := NewL4NetLB(svc, fakeGCE, meta.Regional, namer, record.NewFakeRecorder(100)) - l4netlb.l4HealthChecks = healthchecks.Fake(fakeGCE, &test.FakeRecorderSource{}) + l4netlb.l4HealthChecks = healthchecks.FakeL4(fakeGCE, &test.FakeRecorderSource{}) if _, err := test.CreateAndInsertNodes(l4netlb.cloud, nodeNames, vals.ZoneName); err != nil { t.Errorf("Unexpected error when adding nodes %v", err)