Skip to content

Commit cfc5b4f

Browse files
author
Kate Osborn
committed
Even more tests
1 parent 4c468a4 commit cfc5b4f

File tree

9 files changed

+79
-59
lines changed

9 files changed

+79
-59
lines changed

‎cmd/gateway/commands.go

Lines changed: 6 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -550,11 +550,6 @@ func createInitializeCommand() *cobra.Command {
550550
return fmt.Errorf("unable to initialize k8s client: %w", err)
551551
}
552552

553-
dcc := licensing.NewDeploymentContextCollector(licensing.DeploymentContextCollectorConfig{
554-
K8sClientReader: k8sReader,
555-
PodNSName: podNsName,
556-
})
557-
558553
logger := ctlrZap.New()
559554
klog.SetLogger(logger)
560555
logger.Info(
@@ -566,6 +561,12 @@ func createInitializeCommand() *cobra.Command {
566561
)
567562
log.SetLogger(logger)
568563

564+
dcc := licensing.NewDeploymentContextCollector(licensing.DeploymentContextCollectorConfig{
565+
K8sClientReader: k8sReader,
566+
PodNSName: podNsName,
567+
Logger: logger.WithName("deployCtxCollector"),
568+
})
569+
569570
return initialize(initializeConfig{
570571
fileManager: file.NewStdLibOSFileManager(),
571572
logger: logger,

‎cmd/gateway/initialize.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -42,7 +42,7 @@ func initialize(cfg initializeConfig) error {
4242
ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second)
4343
defer cancel()
4444

45-
depCtx, err := cfg.collector.Collect(ctx, cfg.logger.WithName("deployCtxCollector"))
45+
depCtx, err := cfg.collector.Collect(ctx)
4646
if err != nil {
4747
return fmt.Errorf("failed to collect deployment context: %w", err)
4848
}

‎cmd/gateway/initialize_test.go

Lines changed: 2 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -8,7 +8,6 @@ import (
88
"path/filepath"
99
"testing"
1010

11-
"github.com/go-logr/logr"
1211
. "github.com/onsi/gomega"
1312
"sigs.k8s.io/controller-runtime/pkg/log/zap"
1413

@@ -35,7 +34,7 @@ func TestInitialize_OSS(t *testing.T) {
3534
}
3635

3736
err := initialize(ic)
38-
g.Expect(err).To(BeNil())
37+
g.Expect(err).ToNot(HaveOccurred())
3938
g.Expect(fakeFileMgr.CreateCallCount()).To(Equal(2))
4039
g.Expect(fakeFileMgr.OpenCallCount()).To(Equal(2))
4140
g.Expect(fakeFileMgr.CopyCallCount()).To(Equal(2))
@@ -106,7 +105,7 @@ func TestInitialize_Plus_Error(t *testing.T) {
106105
collectErr := errors.New("collect error")
107106
fakeFileMgr := &filefakes.FakeOSFileManager{}
108107
fakeCollector := &licensingfakes.FakeCollector{
109-
CollectStub: func(_ context.Context, _ logr.Logger) (dataplane.DeploymentContext, error) {
108+
CollectStub: func(_ context.Context) (dataplane.DeploymentContext, error) {
110109
return dataplane.DeploymentContext{}, collectErr
111110
},
112111
}

‎internal/mode/static/handler.go

Lines changed: 6 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -59,6 +59,8 @@ type eventHandlerConfig struct {
5959
logLevelSetter logLevelSetter
6060
// eventRecorder records events for Kubernetes resources.
6161
eventRecorder record.EventRecorder
62+
// deployCtxCollector collects the deployment context for N+ licensing
63+
deployCtxCollector licensing.Collector
6264
// nginxConfiguredOnStartChecker sets the health of the Pod to Ready once we've written out our initial config.
6365
nginxConfiguredOnStartChecker *nginxConfiguredOnStartChecker
6466
// gatewayPodConfig contains information about this Pod.
@@ -91,11 +93,6 @@ type objectFilter struct {
9193
captureChangeInGraph bool
9294
}
9395

94-
// deployCtxCollector collects the deployment context for N+ licensing.
95-
type deployCtxCollector interface {
96-
Collect(ctx context.Context, logger logr.Logger) (dataplane.DeploymentContext, error)
97-
}
98-
9996
// eventHandlerImpl implements EventHandler.
10097
// eventHandlerImpl is responsible for:
10198
// (1) Reconciling the Gateway API and Kubernetes built-in resources with the NGINX configuration.
@@ -111,8 +108,6 @@ type eventHandlerImpl struct {
111108

112109
latestReloadResult status.NginxReloadResult
113110

114-
deployCtxCollector deployCtxCollector
115-
116111
cfg eventHandlerConfig
117112
lock sync.Mutex
118113

@@ -124,13 +119,6 @@ type eventHandlerImpl struct {
124119
func newEventHandlerImpl(cfg eventHandlerConfig) *eventHandlerImpl {
125120
handler := &eventHandlerImpl{
126121
cfg: cfg,
127-
deployCtxCollector: licensing.NewDeploymentContextCollector(licensing.DeploymentContextCollectorConfig{
128-
K8sClientReader: cfg.k8sReader,
129-
PodNSName: types.NamespacedName{
130-
Namespace: cfg.gatewayPodConfig.Namespace,
131-
Name: cfg.gatewayPodConfig.Name,
132-
},
133-
}),
134122
}
135123

136124
handler.objectFilters = map[filterKey]objectFilter{
@@ -186,7 +174,7 @@ func (h *eventHandlerImpl) HandleEventBatch(ctx context.Context, logger logr.Log
186174
case state.EndpointsOnlyChange:
187175
h.version++
188176
cfg := dataplane.BuildConfiguration(ctx, gr, h.cfg.serviceResolver, h.version)
189-
depCtx, getErr := h.getDeploymentContext(ctx, logger)
177+
depCtx, getErr := h.getDeploymentContext(ctx)
190178
if getErr != nil {
191179
logger.Error(getErr, "error getting deployment context for usage reporting")
192180
}
@@ -202,7 +190,7 @@ func (h *eventHandlerImpl) HandleEventBatch(ctx context.Context, logger logr.Log
202190
case state.ClusterStateChange:
203191
h.version++
204192
cfg := dataplane.BuildConfiguration(ctx, gr, h.cfg.serviceResolver, h.version)
205-
depCtx, getErr := h.getDeploymentContext(ctx, logger)
193+
depCtx, getErr := h.getDeploymentContext(ctx)
206194
if getErr != nil {
207195
logger.Error(getErr, "error getting deployment context for usage reporting")
208196
}
@@ -514,15 +502,12 @@ func getGatewayAddresses(
514502
}
515503

516504
// getDeploymentContext gets the deployment context metadata for N+ reporting.
517-
func (h *eventHandlerImpl) getDeploymentContext(
518-
ctx context.Context,
519-
logger logr.Logger,
520-
) (dataplane.DeploymentContext, error) {
505+
func (h *eventHandlerImpl) getDeploymentContext(ctx context.Context) (dataplane.DeploymentContext, error) {
521506
if !h.cfg.plus {
522507
return dataplane.DeploymentContext{}, nil
523508
}
524509

525-
return h.deployCtxCollector.Collect(ctx, logger.WithName("deployCtxCollector"))
510+
return h.cfg.deployCtxCollector.Collect(ctx)
526511
}
527512

528513
// GetLatestConfiguration gets the latest configuration.

‎internal/mode/static/handler_test.go

Lines changed: 36 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -23,6 +23,7 @@ import (
2323
"github.com/nginxinc/nginx-gateway-fabric/internal/framework/helpers"
2424
"github.com/nginxinc/nginx-gateway-fabric/internal/framework/status/statusfakes"
2525
"github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/config"
26+
"github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/licensing/licensingfakes"
2627
"github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/metrics/collectors"
2728
"github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/nginx/config/configfakes"
2829
"github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/nginx/file"
@@ -105,6 +106,7 @@ var _ = Describe("eventHandler", func() {
105106
nginxRuntimeMgr: fakeNginxRuntimeMgr,
106107
statusUpdater: fakeStatusUpdater,
107108
eventRecorder: fakeEventRecorder,
109+
deployCtxCollector: &licensingfakes.FakeCollector{},
108110
nginxConfiguredOnStartChecker: newNginxConfiguredOnStartChecker(),
109111
controlConfigNSName: types.NamespacedName{Namespace: namespace, Name: configName},
110112
gatewayPodConfig: config.GatewayPodConfig{
@@ -702,21 +704,49 @@ var _ = Describe("getDeploymentContext", func() {
702704
It("doesn't set the deployment context", func() {
703705
handler := eventHandlerImpl{}
704706

705-
depCtx, err := handler.getDeploymentContext(context.Background(), ctlrZap.New())
707+
depCtx, err := handler.getDeploymentContext(context.Background())
706708
Expect(err).ToNot(HaveOccurred())
707709
Expect(depCtx).To(Equal(dataplane.DeploymentContext{}))
708710
})
709711
})
710712

711713
When("nginx plus is true", func() {
712-
It("returns an error on failure ", func() {
714+
It("returns deployment context", func() {
715+
expDepCtx := dataplane.DeploymentContext{
716+
Integration: "ngf",
717+
ClusterID: "cluster-id",
718+
InstallationID: "installation-id",
719+
ClusterNodeCount: 1,
720+
}
721+
722+
handler := newEventHandlerImpl(eventHandlerConfig{
723+
plus: true,
724+
deployCtxCollector: &licensingfakes.FakeCollector{
725+
CollectStub: func(_ context.Context) (dataplane.DeploymentContext, error) {
726+
return expDepCtx, nil
727+
},
728+
},
729+
})
730+
731+
dc, err := handler.getDeploymentContext(context.Background())
732+
Expect(err).ToNot(HaveOccurred())
733+
Expect(dc).To(Equal(expDepCtx))
734+
})
735+
It("returns error if it occurs", func() {
736+
expErr := errors.New("collect error")
737+
713738
handler := newEventHandlerImpl(eventHandlerConfig{
714-
plus: true,
715-
k8sReader: fake.NewFakeClient(), // client with no runtime objects will cause the collector to error
739+
plus: true,
740+
deployCtxCollector: &licensingfakes.FakeCollector{
741+
CollectStub: func(_ context.Context) (dataplane.DeploymentContext, error) {
742+
return dataplane.DeploymentContext{}, expErr
743+
},
744+
},
716745
})
717746

718-
_, err := handler.getDeploymentContext(context.Background(), ctlrZap.New())
719-
Expect(err).To(HaveOccurred())
747+
dc, err := handler.getDeploymentContext(context.Background())
748+
Expect(err).To(MatchError(expErr))
749+
Expect(dc).To(Equal(dataplane.DeploymentContext{}))
720750
})
721751
})
722752
})

‎internal/mode/static/licensing/collector.go

Lines changed: 6 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -18,7 +18,7 @@ import (
1818

1919
// Collector collects licensing information for N+.
2020
type Collector interface {
21-
Collect(ctx context.Context, log logr.Logger) (dataplane.DeploymentContext, error)
21+
Collect(ctx context.Context) (dataplane.DeploymentContext, error)
2222
}
2323

2424
const integrationID = "ngf"
@@ -29,6 +29,8 @@ type DeploymentContextCollectorConfig struct {
2929
K8sClientReader client.Reader
3030
// PodNSName is the NamespacedName of the NGF Pod.
3131
PodNSName types.NamespacedName
32+
// Logger is the logger.
33+
Logger logr.Logger
3234
}
3335

3436
// DeploymentContextCollector collects the deployment context information needed for N+ licensing.
@@ -46,10 +48,7 @@ func NewDeploymentContextCollector(
4648
}
4749

4850
// Collect collects all the information needed to create the deployment context for N+ licensing.
49-
func (c *DeploymentContextCollector) Collect(
50-
ctx context.Context,
51-
logger logr.Logger,
52-
) (dataplane.DeploymentContext, error) {
51+
func (c *DeploymentContextCollector) Collect(ctx context.Context) (dataplane.DeploymentContext, error) {
5352
clusterInfo, err := telemetry.CollectClusterInformation(ctx, c.cfg.K8sClientReader)
5453
if err != nil {
5554
return dataplane.DeploymentContext{}, fmt.Errorf("error getting cluster information: %w", err)
@@ -60,11 +59,11 @@ func (c *DeploymentContextCollector) Collect(
6059
// InstallationID is not required by the usage API, so if we can't get it, don't return an error
6160
replicaSet, err := telemetry.GetPodReplicaSet(ctx, c.cfg.K8sClientReader, c.cfg.PodNSName)
6261
if err != nil {
63-
logger.Error(err, "failed to get NGF installationID")
62+
c.cfg.Logger.Error(err, "failed to get NGF installationID")
6463
} else {
6564
installationID, err = telemetry.GetDeploymentID(replicaSet)
6665
if err != nil {
67-
logger.Error(err, "failed to get NGF installationID")
66+
c.cfg.Logger.Error(err, "failed to get NGF installationID")
6867
}
6968
}
7069

‎internal/mode/static/licensing/collector_test.go

Lines changed: 4 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -10,7 +10,6 @@ import (
1010
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
1111
"k8s.io/apimachinery/pkg/types"
1212
"sigs.k8s.io/controller-runtime/pkg/client/fake"
13-
ctlrZap "sigs.k8s.io/controller-runtime/pkg/log/zap"
1413

1514
"github.com/nginxinc/nginx-gateway-fabric/internal/framework/helpers"
1615
"github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/licensing"
@@ -73,7 +72,7 @@ var _ = Describe("DeploymentContextCollector", func() {
7372
ClusterNodeCount: 1,
7473
}
7574

76-
depCtx, err := collector.Collect(context.Background(), ctlrZap.New())
75+
depCtx, err := collector.Collect(context.Background())
7776
Expect(err).ToNot(HaveOccurred())
7877
Expect(depCtx).To(Equal(expCtx))
7978
})
@@ -83,7 +82,7 @@ var _ = Describe("DeploymentContextCollector", func() {
8382
K8sClientReader: fake.NewFakeClient(),
8483
})
8584

86-
_, err := collector.Collect(context.Background(), ctlrZap.New())
85+
_, err := collector.Collect(context.Background())
8786
Expect(err).To(HaveOccurred())
8887
Expect(err.Error()).To(ContainSubstring("error getting cluster information"))
8988
})
@@ -100,7 +99,7 @@ var _ = Describe("DeploymentContextCollector", func() {
10099
ClusterNodeCount: 1,
101100
}
102101

103-
depCtx, err := collector.Collect(context.Background(), ctlrZap.New())
102+
depCtx, err := collector.Collect(context.Background())
104103
Expect(err).ToNot(HaveOccurred())
105104
Expect(depCtx).To(Equal(expCtx))
106105
})
@@ -119,7 +118,7 @@ var _ = Describe("DeploymentContextCollector", func() {
119118
ClusterNodeCount: 1,
120119
}
121120

122-
depCtx, err := collector.Collect(context.Background(), ctlrZap.New())
121+
depCtx, err := collector.Collect(context.Background())
123122
Expect(err).ToNot(HaveOccurred())
124123
Expect(depCtx).To(Equal(expCtx))
125124
})

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

Lines changed: 8 additions & 11 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎internal/mode/static/manager.go

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -47,6 +47,7 @@ import (
4747
"github.com/nginxinc/nginx-gateway-fabric/internal/framework/status"
4848
ngftypes "github.com/nginxinc/nginx-gateway-fabric/internal/framework/types"
4949
"github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/config"
50+
"github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/licensing"
5051
"github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/metrics/collectors"
5152
ngxcfg "github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/nginx/config"
5253
"github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/nginx/config/policies"
@@ -213,6 +214,14 @@ func StartManager(cfg config.Config) error {
213214
)
214215

215216
groupStatusUpdater := status.NewLeaderAwareGroupUpdater(statusUpdater)
217+
deployCtxCollector := licensing.NewDeploymentContextCollector(licensing.DeploymentContextCollectorConfig{
218+
K8sClientReader: mgr.GetAPIReader(),
219+
PodNSName: types.NamespacedName{
220+
Namespace: cfg.GatewayPodConfig.Namespace,
221+
Name: cfg.GatewayPodConfig.Name,
222+
},
223+
Logger: cfg.Logger.WithName("deployCtxCollector"),
224+
})
216225

217226
eventHandler := newEventHandlerImpl(eventHandlerConfig{
218227
nginxFileMgr: file.NewManagerImpl(
@@ -239,6 +248,7 @@ func StartManager(cfg config.Config) error {
239248
k8sReader: mgr.GetAPIReader(),
240249
logLevelSetter: logLevelSetter,
241250
eventRecorder: recorder,
251+
deployCtxCollector: deployCtxCollector,
242252
nginxConfiguredOnStartChecker: nginxChecker,
243253
gatewayPodConfig: cfg.GatewayPodConfig,
244254
controlConfigNSName: controlConfigNSName,

0 commit comments

Comments
 (0)