review fixes

address review comments, improve tests, added mutex guard preventing re-initialization of L4 healhtchecks, renamed a few places, added comments

It is important that all controllers in a single test case use the same fakeHC, I have fixed this in
TestHealthCheckFirewallDeletionWithILB
TestHealthCheckFirewallDeletionWithNetLB

cmd/glbc/main.go

@@ -19,14 +19,14 @@ package main
 import (
 	"context"
 	"fmt"
-	"k8s.io/ingress-gce/pkg/healthchecks"
 	"math/rand"
 	"os"
 	"time"
 
 	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)

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

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 {

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,15 +26,17 @@ 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"
 	"k8s.io/legacy-cloud-providers/gce"
 )
 
 const (
-
 	// L4 Load Balancer parameters
 	gceHcCheckIntervalSeconds = int64(8)
 	gceHcTimeoutSeconds       = int64(1)
@@ -47,44 +46,60 @@ 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
 	cloud           *gce.Cloud
 	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)

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)
 }
 

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 {

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)