Even more tests

Author: Kate Osborn

cmd/gateway/commands.go

@@ -550,11 +550,6 @@ func createInitializeCommand() *cobra.Command {
 				return fmt.Errorf("unable to initialize k8s client: %w", err)
 			}
 
-			dcc := licensing.NewDeploymentContextCollector(licensing.DeploymentContextCollectorConfig{
-				K8sClientReader: k8sReader,
-				PodNSName:       podNsName,
-			})
-
 			logger := ctlrZap.New()
 			klog.SetLogger(logger)
 			logger.Info(
@@ -566,6 +561,12 @@ func createInitializeCommand() *cobra.Command {
 			)
 			log.SetLogger(logger)
 
+			dcc := licensing.NewDeploymentContextCollector(licensing.DeploymentContextCollectorConfig{
+				K8sClientReader: k8sReader,
+				PodNSName:       podNsName,
+				Logger:          logger.WithName("deployCtxCollector"),
+			})
+
 			return initialize(initializeConfig{
 				fileManager: file.NewStdLibOSFileManager(),
 				logger:      logger,

cmd/gateway/initialize.go

@@ -42,7 +42,7 @@ func initialize(cfg initializeConfig) error {
 	ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second)
 	defer cancel()
 
-	depCtx, err := cfg.collector.Collect(ctx, cfg.logger.WithName("deployCtxCollector"))
+	depCtx, err := cfg.collector.Collect(ctx)
 	if err != nil {
 		return fmt.Errorf("failed to collect deployment context: %w", err)
 	}

cmd/gateway/initialize_test.go

@@ -8,7 +8,6 @@ import (
 	"path/filepath"
 	"testing"
 
-	"github.com/go-logr/logr"
 	. "github.com/onsi/gomega"
 	"sigs.k8s.io/controller-runtime/pkg/log/zap"
 
@@ -35,7 +34,7 @@ func TestInitialize_OSS(t *testing.T) {
 	}
 
 	err := initialize(ic)
-	g.Expect(err).To(BeNil())
+	g.Expect(err).ToNot(HaveOccurred())
 	g.Expect(fakeFileMgr.CreateCallCount()).To(Equal(2))
 	g.Expect(fakeFileMgr.OpenCallCount()).To(Equal(2))
 	g.Expect(fakeFileMgr.CopyCallCount()).To(Equal(2))
@@ -106,7 +105,7 @@ func TestInitialize_Plus_Error(t *testing.T) {
 	collectErr := errors.New("collect error")
 	fakeFileMgr := &filefakes.FakeOSFileManager{}
 	fakeCollector := &licensingfakes.FakeCollector{
-		CollectStub: func(_ context.Context, _ logr.Logger) (dataplane.DeploymentContext, error) {
+		CollectStub: func(_ context.Context) (dataplane.DeploymentContext, error) {
 			return dataplane.DeploymentContext{}, collectErr
 		},
 	}

internal/mode/static/handler.go

@@ -59,6 +59,8 @@ type eventHandlerConfig struct {
 	logLevelSetter logLevelSetter
 	// eventRecorder records events for Kubernetes resources.
 	eventRecorder record.EventRecorder
+	// deployCtxCollector collects the deployment context for N+ licensing
+	deployCtxCollector licensing.Collector
 	// nginxConfiguredOnStartChecker sets the health of the Pod to Ready once we've written out our initial config.
 	nginxConfiguredOnStartChecker *nginxConfiguredOnStartChecker
 	// gatewayPodConfig contains information about this Pod.
@@ -91,11 +93,6 @@ type objectFilter struct {
 	captureChangeInGraph bool
 }
 
-// deployCtxCollector collects the deployment context for N+ licensing.
-type deployCtxCollector interface {
-	Collect(ctx context.Context, logger logr.Logger) (dataplane.DeploymentContext, error)
-}
-
 // eventHandlerImpl implements EventHandler.
 // eventHandlerImpl is responsible for:
 // (1) Reconciling the Gateway API and Kubernetes built-in resources with the NGINX configuration.
@@ -111,8 +108,6 @@ type eventHandlerImpl struct {
 
 	latestReloadResult status.NginxReloadResult
 
-	deployCtxCollector deployCtxCollector
-
 	cfg  eventHandlerConfig
 	lock sync.Mutex
 
@@ -124,13 +119,6 @@ type eventHandlerImpl struct {
 func newEventHandlerImpl(cfg eventHandlerConfig) *eventHandlerImpl {
 	handler := &eventHandlerImpl{
 		cfg: cfg,
-		deployCtxCollector: licensing.NewDeploymentContextCollector(licensing.DeploymentContextCollectorConfig{
-			K8sClientReader: cfg.k8sReader,
-			PodNSName: types.NamespacedName{
-				Namespace: cfg.gatewayPodConfig.Namespace,
-				Name:      cfg.gatewayPodConfig.Name,
-			},
-		}),
 	}
 
 	handler.objectFilters = map[filterKey]objectFilter{
@@ -186,7 +174,7 @@ func (h *eventHandlerImpl) HandleEventBatch(ctx context.Context, logger logr.Log
 	case state.EndpointsOnlyChange:
 		h.version++
 		cfg := dataplane.BuildConfiguration(ctx, gr, h.cfg.serviceResolver, h.version)
-		depCtx, getErr := h.getDeploymentContext(ctx, logger)
+		depCtx, getErr := h.getDeploymentContext(ctx)
 		if getErr != nil {
 			logger.Error(getErr, "error getting deployment context for usage reporting")
 		}
@@ -202,7 +190,7 @@ func (h *eventHandlerImpl) HandleEventBatch(ctx context.Context, logger logr.Log
 	case state.ClusterStateChange:
 		h.version++
 		cfg := dataplane.BuildConfiguration(ctx, gr, h.cfg.serviceResolver, h.version)
-		depCtx, getErr := h.getDeploymentContext(ctx, logger)
+		depCtx, getErr := h.getDeploymentContext(ctx)
 		if getErr != nil {
 			logger.Error(getErr, "error getting deployment context for usage reporting")
 		}
@@ -514,15 +502,12 @@ func getGatewayAddresses(
 }
 
 // getDeploymentContext gets the deployment context metadata for N+ reporting.
-func (h *eventHandlerImpl) getDeploymentContext(
-	ctx context.Context,
-	logger logr.Logger,
-) (dataplane.DeploymentContext, error) {
+func (h *eventHandlerImpl) getDeploymentContext(ctx context.Context) (dataplane.DeploymentContext, error) {
 	if !h.cfg.plus {
 		return dataplane.DeploymentContext{}, nil
 	}
 
-	return h.deployCtxCollector.Collect(ctx, logger.WithName("deployCtxCollector"))
+	return h.cfg.deployCtxCollector.Collect(ctx)
 }
 
 // GetLatestConfiguration gets the latest configuration.

internal/mode/static/handler_test.go

@@ -23,6 +23,7 @@ import (
 	"github.com/nginxinc/nginx-gateway-fabric/internal/framework/helpers"
 	"github.com/nginxinc/nginx-gateway-fabric/internal/framework/status/statusfakes"
 	"github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/config"
+	"github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/licensing/licensingfakes"
 	"github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/metrics/collectors"
 	"github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/nginx/config/configfakes"
 	"github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/nginx/file"
@@ -105,6 +106,7 @@ var _ = Describe("eventHandler", func() {
 			nginxRuntimeMgr:               fakeNginxRuntimeMgr,
 			statusUpdater:                 fakeStatusUpdater,
 			eventRecorder:                 fakeEventRecorder,
+			deployCtxCollector:            &licensingfakes.FakeCollector{},
 			nginxConfiguredOnStartChecker: newNginxConfiguredOnStartChecker(),
 			controlConfigNSName:           types.NamespacedName{Namespace: namespace, Name: configName},
 			gatewayPodConfig: config.GatewayPodConfig{
@@ -702,21 +704,49 @@ var _ = Describe("getDeploymentContext", func() {
 		It("doesn't set the deployment context", func() {
 			handler := eventHandlerImpl{}
 
-			depCtx, err := handler.getDeploymentContext(context.Background(), ctlrZap.New())
+			depCtx, err := handler.getDeploymentContext(context.Background())
 			Expect(err).ToNot(HaveOccurred())
 			Expect(depCtx).To(Equal(dataplane.DeploymentContext{}))
 		})
 	})
 
 	When("nginx plus is true", func() {
-		It("returns an error on failure ", func() {
+		It("returns deployment context", func() {
+			expDepCtx := dataplane.DeploymentContext{
+				Integration:      "ngf",
+				ClusterID:        "cluster-id",
+				InstallationID:   "installation-id",
+				ClusterNodeCount: 1,
+			}
+
+			handler := newEventHandlerImpl(eventHandlerConfig{
+				plus: true,
+				deployCtxCollector: &licensingfakes.FakeCollector{
+					CollectStub: func(_ context.Context) (dataplane.DeploymentContext, error) {
+						return expDepCtx, nil
+					},
+				},
+			})
+
+			dc, err := handler.getDeploymentContext(context.Background())
+			Expect(err).ToNot(HaveOccurred())
+			Expect(dc).To(Equal(expDepCtx))
+		})
+		It("returns error if it occurs", func() {
+			expErr := errors.New("collect error")
+
 			handler := newEventHandlerImpl(eventHandlerConfig{
-				plus:      true,
-				k8sReader: fake.NewFakeClient(), // client with no runtime objects will cause the collector to error
+				plus: true,
+				deployCtxCollector: &licensingfakes.FakeCollector{
+					CollectStub: func(_ context.Context) (dataplane.DeploymentContext, error) {
+						return dataplane.DeploymentContext{}, expErr
+					},
+				},
 			})
 
-			_, err := handler.getDeploymentContext(context.Background(), ctlrZap.New())
-			Expect(err).To(HaveOccurred())
+			dc, err := handler.getDeploymentContext(context.Background())
+			Expect(err).To(MatchError(expErr))
+			Expect(dc).To(Equal(dataplane.DeploymentContext{}))
 		})
 	})
 })

internal/mode/static/licensing/collector.go

@@ -18,7 +18,7 @@ import (
 
 // Collector collects licensing information for N+.
 type Collector interface {
-	Collect(ctx context.Context, log logr.Logger) (dataplane.DeploymentContext, error)
+	Collect(ctx context.Context) (dataplane.DeploymentContext, error)
 }
 
 const integrationID = "ngf"
@@ -29,6 +29,8 @@ type DeploymentContextCollectorConfig struct {
 	K8sClientReader client.Reader
 	// PodNSName is the NamespacedName of the NGF Pod.
 	PodNSName types.NamespacedName
+	// Logger is the logger.
+	Logger logr.Logger
 }
 
 // DeploymentContextCollector collects the deployment context information needed for N+ licensing.
@@ -46,10 +48,7 @@ func NewDeploymentContextCollector(
 }
 
 // Collect collects all the information needed to create the deployment context for N+ licensing.
-func (c *DeploymentContextCollector) Collect(
-	ctx context.Context,
-	logger logr.Logger,
-) (dataplane.DeploymentContext, error) {
+func (c *DeploymentContextCollector) Collect(ctx context.Context) (dataplane.DeploymentContext, error) {
 	clusterInfo, err := telemetry.CollectClusterInformation(ctx, c.cfg.K8sClientReader)
 	if err != nil {
 		return dataplane.DeploymentContext{}, fmt.Errorf("error getting cluster information: %w", err)
@@ -60,11 +59,11 @@ func (c *DeploymentContextCollector) Collect(
 	// InstallationID is not required by the usage API, so if we can't get it, don't return an error
 	replicaSet, err := telemetry.GetPodReplicaSet(ctx, c.cfg.K8sClientReader, c.cfg.PodNSName)
 	if err != nil {
-		logger.Error(err, "failed to get NGF installationID")
+		c.cfg.Logger.Error(err, "failed to get NGF installationID")
 	} else {
 		installationID, err = telemetry.GetDeploymentID(replicaSet)
 		if err != nil {
-			logger.Error(err, "failed to get NGF installationID")
+			c.cfg.Logger.Error(err, "failed to get NGF installationID")
 		}
 	}
 

internal/mode/static/licensing/collector_test.go

@@ -10,7 +10,6 @@ import (
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/types"
 	"sigs.k8s.io/controller-runtime/pkg/client/fake"
-	ctlrZap "sigs.k8s.io/controller-runtime/pkg/log/zap"
 
 	"github.com/nginxinc/nginx-gateway-fabric/internal/framework/helpers"
 	"github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/licensing"
@@ -73,7 +72,7 @@ var _ = Describe("DeploymentContextCollector", func() {
 			ClusterNodeCount: 1,
 		}
 
-		depCtx, err := collector.Collect(context.Background(), ctlrZap.New())
+		depCtx, err := collector.Collect(context.Background())
 		Expect(err).ToNot(HaveOccurred())
 		Expect(depCtx).To(Equal(expCtx))
 	})
@@ -83,7 +82,7 @@ var _ = Describe("DeploymentContextCollector", func() {
 			K8sClientReader: fake.NewFakeClient(),
 		})
 
-		_, err := collector.Collect(context.Background(), ctlrZap.New())
+		_, err := collector.Collect(context.Background())
 		Expect(err).To(HaveOccurred())
 		Expect(err.Error()).To(ContainSubstring("error getting cluster information"))
 	})
@@ -100,7 +99,7 @@ var _ = Describe("DeploymentContextCollector", func() {
 			ClusterNodeCount: 1,
 		}
 
-		depCtx, err := collector.Collect(context.Background(), ctlrZap.New())
+		depCtx, err := collector.Collect(context.Background())
 		Expect(err).ToNot(HaveOccurred())
 		Expect(depCtx).To(Equal(expCtx))
 	})
@@ -119,7 +118,7 @@ var _ = Describe("DeploymentContextCollector", func() {
 			ClusterNodeCount: 1,
 		}
 
-		depCtx, err := collector.Collect(context.Background(), ctlrZap.New())
+		depCtx, err := collector.Collect(context.Background())
 		Expect(err).ToNot(HaveOccurred())
 		Expect(depCtx).To(Equal(expCtx))
 	})

internal/mode/static/licensing/licensingfakes/fake_collector.go

@@ -47,6 +47,7 @@ import (
 	"github.com/nginxinc/nginx-gateway-fabric/internal/framework/status"
 	ngftypes "github.com/nginxinc/nginx-gateway-fabric/internal/framework/types"
 	"github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/config"
+	"github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/licensing"
 	"github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/metrics/collectors"
 	ngxcfg "github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/nginx/config"
 	"github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/nginx/config/policies"
@@ -213,6 +214,14 @@ func StartManager(cfg config.Config) error {
 	)
 
 	groupStatusUpdater := status.NewLeaderAwareGroupUpdater(statusUpdater)
+	deployCtxCollector := licensing.NewDeploymentContextCollector(licensing.DeploymentContextCollectorConfig{
+		K8sClientReader: mgr.GetAPIReader(),
+		PodNSName: types.NamespacedName{
+			Namespace: cfg.GatewayPodConfig.Namespace,
+			Name:      cfg.GatewayPodConfig.Name,
+		},
+		Logger: cfg.Logger.WithName("deployCtxCollector"),
+	})
 
 	eventHandler := newEventHandlerImpl(eventHandlerConfig{
 		nginxFileMgr: file.NewManagerImpl(
@@ -239,6 +248,7 @@ func StartManager(cfg config.Config) error {
 		k8sReader:                     mgr.GetAPIReader(),
 		logLevelSetter:                logLevelSetter,
 		eventRecorder:                 recorder,
+		deployCtxCollector:            deployCtxCollector,
 		nginxConfiguredOnStartChecker: nginxChecker,
 		gatewayPodConfig:              cfg.GatewayPodConfig,
 		controlConfigNSName:           controlConfigNSName,