From 3742c6d36584565166bdf30dfa8d2a0126c8a285 Mon Sep 17 00:00:00 2001 From: Benjamin Jee Date: Wed, 7 Feb 2024 17:08:34 -0800 Subject: [PATCH 1/8] Add collection of deployment replica count --- deploy/helm-chart/templates/rbac.yaml | 8 ++++ deploy/manifests/nginx-gateway.yaml | 8 ++++ internal/mode/static/manager.go | 6 +++ internal/mode/static/telemetry/collector.go | 44 +++++++++++++++++++ .../mode/static/telemetry/collector_test.go | 7 +++ 5 files changed, 73 insertions(+) diff --git a/deploy/helm-chart/templates/rbac.yaml b/deploy/helm-chart/templates/rbac.yaml index 97963ed1b..fde368040 100644 --- a/deploy/helm-chart/templates/rbac.yaml +++ b/deploy/helm-chart/templates/rbac.yaml @@ -24,6 +24,7 @@ rules: # FIXME(bjee19): make nodes permission dependent on telemetry being enabled. # https://github.com/nginxinc/nginx-gateway-fabric/issues/1317. - nodes + - pods verbs: - list - watch @@ -34,6 +35,13 @@ rules: verbs: - create - patch +- apiGroups: + - "apps" + resources: + - replicasets + verbs: + - list + - watch - apiGroups: - discovery.k8s.io resources: diff --git a/deploy/manifests/nginx-gateway.yaml b/deploy/manifests/nginx-gateway.yaml index de2c2c1b1..de9c7ae89 100644 --- a/deploy/manifests/nginx-gateway.yaml +++ b/deploy/manifests/nginx-gateway.yaml @@ -35,6 +35,7 @@ rules: # FIXME(bjee19): make nodes permission dependent on telemetry being enabled. # https://github.com/nginxinc/nginx-gateway-fabric/issues/1317. - nodes + - pods verbs: - list - watch @@ -45,6 +46,13 @@ rules: verbs: - create - patch +- apiGroups: + - "apps" + resources: + - replicasets + verbs: + - list + - watch - apiGroups: - discovery.k8s.io resources: diff --git a/internal/mode/static/manager.go b/internal/mode/static/manager.go index 712bb6c46..ab6829014 100644 --- a/internal/mode/static/manager.go +++ b/internal/mode/static/manager.go @@ -8,6 +8,7 @@ import ( "github.com/go-logr/logr" ngxclient "github.com/nginxinc/nginx-plus-go-client/client" "github.com/prometheus/client_golang/prometheus" + appsv1 "k8s.io/api/apps/v1" apiv1 "k8s.io/api/core/v1" discoveryV1 "k8s.io/api/discovery/v1" apiext "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1" @@ -65,6 +66,7 @@ func init() { utilruntime.Must(discoveryV1.AddToScheme(scheme)) utilruntime.Must(ngfAPI.AddToScheme(scheme)) utilruntime.Must(apiext.AddToScheme(scheme)) + utilruntime.Must(appsv1.AddToScheme(scheme)) } // nolint:gocyclo @@ -218,6 +220,10 @@ func StartManager(cfg config.Config) error { GraphGetter: processor, ConfigurationGetter: eventHandler, Version: cfg.Version, + PodNSName: types.NamespacedName{ + Namespace: cfg.GatewayPodConfig.Namespace, + Name: cfg.LeaderElection.Identity, + }, }) if err = mgr.Add(createTelemetryJob(cfg, dataCollector, nginxChecker.getReadyCh())); err != nil { return fmt.Errorf("cannot register telemetry job: %w", err) diff --git a/internal/mode/static/telemetry/collector.go b/internal/mode/static/telemetry/collector.go index 416b004ee..906744b74 100644 --- a/internal/mode/static/telemetry/collector.go +++ b/internal/mode/static/telemetry/collector.go @@ -5,7 +5,9 @@ import ( "errors" "fmt" + appsv1 "k8s.io/api/apps/v1" v1 "k8s.io/api/core/v1" + "k8s.io/apimachinery/pkg/types" "sigs.k8s.io/controller-runtime/pkg/client" "github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/state/dataplane" @@ -49,6 +51,7 @@ type Data struct { ProjectMetadata ProjectMetadata NodeCount int NGFResourceCounts NGFResourceCounts + NGFReplicaCount int } // DataCollectorConfig holds configuration parameters for DataCollectorImpl. @@ -61,6 +64,8 @@ type DataCollectorConfig struct { ConfigurationGetter ConfigurationGetter // Version is the NGF version. Version string + // PodNSName is the NamespacedName of the NGF Pod. + PodNSName types.NamespacedName } // DataCollectorImpl is am implementation of DataCollector. @@ -89,6 +94,11 @@ func (c DataCollectorImpl) Collect(ctx context.Context) (Data, error) { return Data{}, fmt.Errorf("failed to collect NGF resource counts: %w", err) } + ngfReplicaCount, err := collectNGFReplicaCount(ctx, c.cfg.K8sClientReader, c.cfg.PodNSName) + if err != nil { + return Data{}, fmt.Errorf("failed to collect NGF replica count: %w", err) + } + data := Data{ NodeCount: nodeCount, NGFResourceCounts: graphResourceCount, @@ -96,6 +106,7 @@ func (c DataCollectorImpl) Collect(ctx context.Context) (Data, error) { Name: "NGF", Version: c.cfg.Version, }, + NGFReplicaCount: ngfReplicaCount, } return data, nil @@ -147,3 +158,36 @@ func collectGraphResourceCount( return ngfResourceCounts, nil } + +func collectNGFReplicaCount(ctx context.Context, k8sClient client.Reader, podNSName types.NamespacedName) (int, error) { + var pod v1.Pod + if err := k8sClient.Get(ctx, + types.NamespacedName{Namespace: podNSName.Namespace, Name: podNSName.Name}, + &pod, + ); err != nil { + return 0, err + } + + podOwnerRefs := pod.GetOwnerReferences() + if podOwnerRefs == nil { + return 0, fmt.Errorf("could not get owner reference of NGF Pod") + } + if len(podOwnerRefs) != 1 { + return 0, fmt.Errorf("multiple owner references of NGF Pod") + } + + switch kind := podOwnerRefs[0].Kind; kind { + case "ReplicaSet": + var replicaSet appsv1.ReplicaSet + if err := k8sClient.Get(ctx, + types.NamespacedName{Namespace: podNSName.Namespace, Name: podOwnerRefs[0].Name}, + &replicaSet, + ); err != nil { + return 0, err + } + + return int(*replicaSet.Spec.Replicas), nil + default: + return 0, fmt.Errorf("pod owner reference was not ReplicaSet, instead was %s", kind) + } +} diff --git a/internal/mode/static/telemetry/collector_test.go b/internal/mode/static/telemetry/collector_test.go index e30eb1376..b614040ad 100644 --- a/internal/mode/static/telemetry/collector_test.go +++ b/internal/mode/static/telemetry/collector_test.go @@ -48,6 +48,7 @@ var _ = Describe("Collector", Ordered, func() { version string expData telemetry.Data ctx context.Context + podNSName types.NamespacedName ) BeforeAll(func() { @@ -60,11 +61,16 @@ var _ = Describe("Collector", Ordered, func() { ProjectMetadata: telemetry.ProjectMetadata{Name: "NGF", Version: version}, NodeCount: 0, NGFResourceCounts: telemetry.NGFResourceCounts{}, + NGFReplicaCount: 0, } k8sClientReader = &eventsfakes.FakeReader{} fakeGraphGetter = &telemetryfakes.FakeGraphGetter{} fakeConfigurationGetter = &telemetryfakes.FakeConfigurationGetter{} + podNSName = types.NamespacedName{ + Namespace: "nginx-gateway", + Name: "ngf-pod", + } fakeGraphGetter.GetLatestGraphReturns(&graph.Graph{}) fakeConfigurationGetter.GetLatestConfigurationReturns(&dataplane.Configuration{}) @@ -74,6 +80,7 @@ var _ = Describe("Collector", Ordered, func() { GraphGetter: fakeGraphGetter, ConfigurationGetter: fakeConfigurationGetter, Version: version, + PodNSName: podNSName, }) }) From dc54614c15374c5ed165e34b29df192aedd8bea2 Mon Sep 17 00:00:00 2001 From: Benjamin Jee Date: Thu, 8 Feb 2024 12:13:18 -0800 Subject: [PATCH 2/8] Add deployment replica count tests --- .../mode/static/telemetry/collector_test.go | 138 +++++++++++++++++- 1 file changed, 137 insertions(+), 1 deletion(-) diff --git a/internal/mode/static/telemetry/collector_test.go b/internal/mode/static/telemetry/collector_test.go index b614040ad..69848b03d 100644 --- a/internal/mode/static/telemetry/collector_test.go +++ b/internal/mode/static/telemetry/collector_test.go @@ -7,6 +7,7 @@ import ( . "github.com/onsi/ginkgo/v2" . "github.com/onsi/gomega" + appsv1 "k8s.io/api/apps/v1" v1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/types" @@ -39,6 +40,38 @@ func createListCallsFunc(nodes []v1.Node) func( } } +func createGetCallsFunc() func( + ctx context.Context, + key client.ObjectKey, + object client.Object, + option ...client.GetOption, +) error { + return func(ctx context.Context, key client.ObjectKey, object client.Object, option ...client.GetOption) error { + Expect(option).To(BeEmpty()) + + switch typedObj := object.(type) { + case *v1.Pod: + typedObj.ObjectMeta = metav1.ObjectMeta{ + Name: "pod1", + OwnerReferences: []metav1.OwnerReference{ + { + Kind: "ReplicaSet", + Name: "replicaset1", + }, + }, + } + case *appsv1.ReplicaSet: + replicas := int32(1) + typedObj.Spec = appsv1.ReplicaSetSpec{ + Replicas: &replicas, + } + default: + Fail(fmt.Sprintf("unknown type: %T", typedObj)) + } + return nil + } +} + var _ = Describe("Collector", Ordered, func() { var ( k8sClientReader *eventsfakes.FakeReader @@ -61,7 +94,7 @@ var _ = Describe("Collector", Ordered, func() { ProjectMetadata: telemetry.ProjectMetadata{Name: "NGF", Version: version}, NodeCount: 0, NGFResourceCounts: telemetry.NGFResourceCounts{}, - NGFReplicaCount: 0, + NGFReplicaCount: 1, } k8sClientReader = &eventsfakes.FakeReader{} @@ -82,6 +115,7 @@ var _ = Describe("Collector", Ordered, func() { Version: version, PodNSName: podNSName, }) + k8sClientReader.GetCalls(createGetCallsFunc()) }) Describe("Normal case", func() { @@ -370,4 +404,106 @@ var _ = Describe("Collector", Ordered, func() { }) }) }) + + Describe("NGF replica count collector", func() { + When("collecting NGF replica count", func() { + When("it encounters an error while collecting data", func() { + BeforeEach(func() { + k8sClientReader.GetReturns(nil) + k8sClientReader.GetCalls(createGetCallsFunc()) + }) + + It("should error if the kubernetes client errored when getting the Pod", func() { + k8sClientReader.GetReturnsOnCall(0, errors.New("there was an error")) + + _, err := dataCollector.Collect(ctx) + Expect(err).To(HaveOccurred()) + }) + + It("should error if the Pod's owner reference is nil", func() { + k8sClientReader.GetCalls( + func(ctx context.Context, key client.ObjectKey, object client.Object, option ...client.GetOption) error { + Expect(option).To(BeEmpty()) + + switch typedObj := object.(type) { + case *v1.Pod: + typedObj.ObjectMeta = metav1.ObjectMeta{ + Name: "pod1", + OwnerReferences: nil, + } + default: + Fail(fmt.Sprintf("unknown type: %T", typedObj)) + } + return nil + }) + + _, err := dataCollector.Collect(ctx) + Expect(err).To(HaveOccurred()) + }) + + It("should error if the Pod has multiple owner references", func() { + k8sClientReader.GetCalls( + func(ctx context.Context, key client.ObjectKey, object client.Object, option ...client.GetOption) error { + Expect(option).To(BeEmpty()) + + switch typedObj := object.(type) { + case *v1.Pod: + typedObj.ObjectMeta = metav1.ObjectMeta{ + Name: "pod1", + OwnerReferences: []metav1.OwnerReference{ + { + Kind: "ReplicaSet", + Name: "replicaset1", + }, + { + Kind: "ReplicaSet", + Name: "replicaset2", + }, + }, + } + default: + Fail(fmt.Sprintf("unknown type: %T", typedObj)) + } + return nil + }) + + _, err := dataCollector.Collect(ctx) + Expect(err).To(HaveOccurred()) + }) + + It("should error if the Pod's owner reference is not a ReplicaSet", func() { + k8sClientReader.GetCalls( + func(ctx context.Context, key client.ObjectKey, object client.Object, option ...client.GetOption) error { + Expect(option).To(BeEmpty()) + + switch typedObj := object.(type) { + case *v1.Pod: + typedObj.ObjectMeta = metav1.ObjectMeta{ + Name: "pod1", + OwnerReferences: []metav1.OwnerReference{ + { + Kind: "Deployment", + Name: "deployment1", + }, + }, + } + default: + Fail(fmt.Sprintf("unknown type: %T", typedObj)) + } + return nil + }) + + _, err := dataCollector.Collect(ctx) + Expect(err).To(HaveOccurred()) + }) + + It("should error if the kubernetes client errored when getting the ReplicaSet", func() { + k8sClientReader.GetReturnsOnCall(1, errors.New("there was an error")) + + _, err := dataCollector.Collect(ctx) + Expect(err).To(HaveOccurred()) + }) + }) + }) + }) }) From 63218f348abb6e7bf79527e9c02cee58dbc7fc85 Mon Sep 17 00:00:00 2001 From: Benjamin Jee Date: Thu, 8 Feb 2024 13:31:51 -0800 Subject: [PATCH 3/8] Add check for replica being nil --- internal/mode/static/telemetry/collector.go | 8 +++-- .../mode/static/telemetry/collector_test.go | 30 +++++++++++++++++++ 2 files changed, 36 insertions(+), 2 deletions(-) diff --git a/internal/mode/static/telemetry/collector.go b/internal/mode/static/telemetry/collector.go index 906744b74..bbc2a53e3 100644 --- a/internal/mode/static/telemetry/collector.go +++ b/internal/mode/static/telemetry/collector.go @@ -170,10 +170,10 @@ func collectNGFReplicaCount(ctx context.Context, k8sClient client.Reader, podNSN podOwnerRefs := pod.GetOwnerReferences() if podOwnerRefs == nil { - return 0, fmt.Errorf("could not get owner reference of NGF Pod") + return 0, errors.New("could not get owner reference of NGF Pod") } if len(podOwnerRefs) != 1 { - return 0, fmt.Errorf("multiple owner references of NGF Pod") + return 0, errors.New("multiple owner references of NGF Pod") } switch kind := podOwnerRefs[0].Kind; kind { @@ -186,6 +186,10 @@ func collectNGFReplicaCount(ctx context.Context, k8sClient client.Reader, podNSN return 0, err } + if replicaSet.Spec.Replicas == nil { + return 0, errors.New("replica set replicas was nil") + } + return int(*replicaSet.Spec.Replicas), nil default: return 0, fmt.Errorf("pod owner reference was not ReplicaSet, instead was %s", kind) diff --git a/internal/mode/static/telemetry/collector_test.go b/internal/mode/static/telemetry/collector_test.go index 69848b03d..7ea3d0f7f 100644 --- a/internal/mode/static/telemetry/collector_test.go +++ b/internal/mode/static/telemetry/collector_test.go @@ -497,6 +497,36 @@ var _ = Describe("Collector", Ordered, func() { Expect(err).To(HaveOccurred()) }) + It("should error if the replica set's replicas is nil", func() { + k8sClientReader.GetCalls( + func(ctx context.Context, key client.ObjectKey, object client.Object, option ...client.GetOption) error { + Expect(option).To(BeEmpty()) + + switch typedObj := object.(type) { + case *v1.Pod: + typedObj.ObjectMeta = metav1.ObjectMeta{ + Name: "pod1", + OwnerReferences: []metav1.OwnerReference{ + { + Kind: "ReplicaSet", + Name: "replicaset1", + }, + }, + } + case *appsv1.ReplicaSet: + typedObj.Spec = appsv1.ReplicaSetSpec{ + Replicas: nil, + } + default: + Fail(fmt.Sprintf("unknown type: %T", typedObj)) + } + return nil + }) + + _, err := dataCollector.Collect(ctx) + Expect(err).To(HaveOccurred()) + }) + It("should error if the kubernetes client errored when getting the ReplicaSet", func() { k8sClientReader.GetReturnsOnCall(1, errors.New("there was an error")) From 8bd005239ba206b69a758ab45b567ce861f988bb Mon Sep 17 00:00:00 2001 From: Benjamin Jee Date: Fri, 9 Feb 2024 10:23:39 -0800 Subject: [PATCH 4/8] Add some feedback --- cmd/gateway/commands.go | 1 + deploy/helm-chart/templates/rbac.yaml | 2 +- internal/mode/static/config/config.go | 2 ++ internal/mode/static/manager.go | 2 +- internal/mode/static/telemetry/collector.go | 38 ++++++++++----------- 5 files changed, 23 insertions(+), 22 deletions(-) diff --git a/cmd/gateway/commands.go b/cmd/gateway/commands.go index 4d8fb8ce9..ac97185fd 100644 --- a/cmd/gateway/commands.go +++ b/cmd/gateway/commands.go @@ -153,6 +153,7 @@ func createStaticModeCommand() *cobra.Command { PodIP: podIP, ServiceName: serviceName.value, Namespace: namespace, + Name: podName, }, HealthConfig: config.HealthConfig{ Enabled: !disableHealth, diff --git a/deploy/helm-chart/templates/rbac.yaml b/deploy/helm-chart/templates/rbac.yaml index fde368040..93411a05b 100644 --- a/deploy/helm-chart/templates/rbac.yaml +++ b/deploy/helm-chart/templates/rbac.yaml @@ -36,7 +36,7 @@ rules: - create - patch - apiGroups: - - "apps" + - apps resources: - replicasets verbs: diff --git a/internal/mode/static/config/config.go b/internal/mode/static/config/config.go index abbec1faa..cb1ee1efb 100644 --- a/internal/mode/static/config/config.go +++ b/internal/mode/static/config/config.go @@ -48,6 +48,8 @@ type GatewayPodConfig struct { ServiceName string // Namespace is the namespace of this Pod. Namespace string + // Name is the name of the Pod. + Name string } // MetricsConfig specifies the metrics config. diff --git a/internal/mode/static/manager.go b/internal/mode/static/manager.go index ab6829014..f1f47f89f 100644 --- a/internal/mode/static/manager.go +++ b/internal/mode/static/manager.go @@ -222,7 +222,7 @@ func StartManager(cfg config.Config) error { Version: cfg.Version, PodNSName: types.NamespacedName{ Namespace: cfg.GatewayPodConfig.Namespace, - Name: cfg.LeaderElection.Identity, + Name: cfg.GatewayPodConfig.Name, }, }) if err = mgr.Add(createTelemetryJob(cfg, dataCollector, nginxChecker.getReadyCh())); err != nil { diff --git a/internal/mode/static/telemetry/collector.go b/internal/mode/static/telemetry/collector.go index bbc2a53e3..8f8cb196b 100644 --- a/internal/mode/static/telemetry/collector.go +++ b/internal/mode/static/telemetry/collector.go @@ -161,7 +161,8 @@ func collectGraphResourceCount( func collectNGFReplicaCount(ctx context.Context, k8sClient client.Reader, podNSName types.NamespacedName) (int, error) { var pod v1.Pod - if err := k8sClient.Get(ctx, + if err := k8sClient.Get( + ctx, types.NamespacedName{Namespace: podNSName.Namespace, Name: podNSName.Name}, &pod, ); err != nil { @@ -169,29 +170,26 @@ func collectNGFReplicaCount(ctx context.Context, k8sClient client.Reader, podNSN } podOwnerRefs := pod.GetOwnerReferences() - if podOwnerRefs == nil { - return 0, errors.New("could not get owner reference of NGF Pod") - } if len(podOwnerRefs) != 1 { - return 0, errors.New("multiple owner references of NGF Pod") + return 0, fmt.Errorf("expected one owner reference of the NGF Pod, got %d", len(podOwnerRefs)) } - switch kind := podOwnerRefs[0].Kind; kind { - case "ReplicaSet": - var replicaSet appsv1.ReplicaSet - if err := k8sClient.Get(ctx, - types.NamespacedName{Namespace: podNSName.Namespace, Name: podOwnerRefs[0].Name}, - &replicaSet, - ); err != nil { - return 0, err - } + if podOwnerRefs[0].Kind != "ReplicaSet" { + return 0, fmt.Errorf("expected pod owner reference to be ReplicaSet, got %s", podOwnerRefs[0].Kind) + } - if replicaSet.Spec.Replicas == nil { - return 0, errors.New("replica set replicas was nil") - } + var replicaSet appsv1.ReplicaSet + if err := k8sClient.Get( + ctx, + types.NamespacedName{Namespace: podNSName.Namespace, Name: podOwnerRefs[0].Name}, + &replicaSet, + ); err != nil { + return 0, err + } - return int(*replicaSet.Spec.Replicas), nil - default: - return 0, fmt.Errorf("pod owner reference was not ReplicaSet, instead was %s", kind) + if replicaSet.Spec.Replicas == nil { + return 0, errors.New("replica set replicas was nil") } + + return int(*replicaSet.Spec.Replicas), nil } From b859a1a3eb78bfd19048c4c2d9b54304d0564ddf Mon Sep 17 00:00:00 2001 From: Benjamin Jee Date: Fri, 9 Feb 2024 15:33:06 -0800 Subject: [PATCH 5/8] Add rest of feedback --- deploy/manifests/nginx-gateway.yaml | 2 +- internal/mode/static/telemetry/collector.go | 6 +- .../mode/static/telemetry/collector_test.go | 70 ++++++++++++++----- 3 files changed, 57 insertions(+), 21 deletions(-) diff --git a/deploy/manifests/nginx-gateway.yaml b/deploy/manifests/nginx-gateway.yaml index de9c7ae89..699969e98 100644 --- a/deploy/manifests/nginx-gateway.yaml +++ b/deploy/manifests/nginx-gateway.yaml @@ -47,7 +47,7 @@ rules: - create - patch - apiGroups: - - "apps" + - apps resources: - replicasets verbs: diff --git a/internal/mode/static/telemetry/collector.go b/internal/mode/static/telemetry/collector.go index 8f8cb196b..6842b5b62 100644 --- a/internal/mode/static/telemetry/collector.go +++ b/internal/mode/static/telemetry/collector.go @@ -115,7 +115,7 @@ func (c DataCollectorImpl) Collect(ctx context.Context) (Data, error) { func collectNodeCount(ctx context.Context, k8sClient client.Reader) (int, error) { var nodes v1.NodeList if err := k8sClient.List(ctx, &nodes); err != nil { - return 0, err + return 0, fmt.Errorf("failed to get NodeList: %w", err) } return len(nodes.Items), nil @@ -166,7 +166,7 @@ func collectNGFReplicaCount(ctx context.Context, k8sClient client.Reader, podNSN types.NamespacedName{Namespace: podNSName.Namespace, Name: podNSName.Name}, &pod, ); err != nil { - return 0, err + return 0, fmt.Errorf("failed to get NGF Pod: %w", err) } podOwnerRefs := pod.GetOwnerReferences() @@ -184,7 +184,7 @@ func collectNGFReplicaCount(ctx context.Context, k8sClient client.Reader, podNSN types.NamespacedName{Namespace: podNSName.Namespace, Name: podOwnerRefs[0].Name}, &replicaSet, ); err != nil { - return 0, err + return 0, fmt.Errorf("failed to get NGF Pod's ReplicaSet: %w", err) } if replicaSet.Spec.Replicas == nil { diff --git a/internal/mode/static/telemetry/collector_test.go b/internal/mode/static/telemetry/collector_test.go index 7ea3d0f7f..d9c0bf3a3 100644 --- a/internal/mode/static/telemetry/collector_test.go +++ b/internal/mode/static/telemetry/collector_test.go @@ -257,10 +257,11 @@ var _ = Describe("Collector", Ordered, func() { }) When("it encounters an error while collecting data", func() { It("should error on kubernetes client api errors", func() { - k8sClientReader.ListReturns(errors.New("there was an error")) + expectedError := errors.New("there was an error getting NodeList") + k8sClientReader.ListReturns(expectedError) _, err := dataCollector.Collect(ctx) - Expect(err).To(HaveOccurred()) + Expect(err).To(MatchError(expectedError)) }) }) }) @@ -389,17 +390,19 @@ var _ = Describe("Collector", Ordered, func() { fakeConfigurationGetter.GetLatestConfigurationReturns(&dataplane.Configuration{}) }) It("should error on nil latest graph", func() { + expectedError := errors.New("latest graph cannot be nil") fakeGraphGetter.GetLatestGraphReturns(nil) _, err := dataCollector.Collect(ctx) - Expect(err).To(HaveOccurred()) + Expect(err).To(MatchError(expectedError)) }) It("should error on nil latest configuration", func() { + expectedError := errors.New("latest configuration cannot be nil") fakeConfigurationGetter.GetLatestConfigurationReturns(nil) _, err := dataCollector.Collect(ctx) - Expect(err).To(HaveOccurred()) + Expect(err).To(MatchError(expectedError)) }) }) }) @@ -408,19 +411,27 @@ var _ = Describe("Collector", Ordered, func() { Describe("NGF replica count collector", func() { When("collecting NGF replica count", func() { When("it encounters an error while collecting data", func() { - BeforeEach(func() { - k8sClientReader.GetReturns(nil) - k8sClientReader.GetCalls(createGetCallsFunc()) - }) - It("should error if the kubernetes client errored when getting the Pod", func() { - k8sClientReader.GetReturnsOnCall(0, errors.New("there was an error")) + expectedErr := errors.New("there was an error getting the Pod") + k8sClientReader.GetCalls( + func(ctx context.Context, key client.ObjectKey, object client.Object, option ...client.GetOption) error { + Expect(option).To(BeEmpty()) + + switch typedObj := object.(type) { + case *v1.Pod: + return expectedErr + default: + Fail(fmt.Sprintf("unknown type: %T", typedObj)) + } + return nil + }) _, err := dataCollector.Collect(ctx) - Expect(err).To(HaveOccurred()) + Expect(err).To(MatchError(expectedErr)) }) It("should error if the Pod's owner reference is nil", func() { + expectedErr := errors.New("expected one owner reference of the NGF Pod, got 0") k8sClientReader.GetCalls( func(ctx context.Context, key client.ObjectKey, object client.Object, option ...client.GetOption) error { Expect(option).To(BeEmpty()) @@ -438,10 +449,11 @@ var _ = Describe("Collector", Ordered, func() { }) _, err := dataCollector.Collect(ctx) - Expect(err).To(HaveOccurred()) + Expect(err).To(MatchError(expectedErr)) }) It("should error if the Pod has multiple owner references", func() { + expectedErr := errors.New("expected one owner reference of the NGF Pod, got 2") k8sClientReader.GetCalls( func(ctx context.Context, key client.ObjectKey, object client.Object, option ...client.GetOption) error { Expect(option).To(BeEmpty()) @@ -468,10 +480,11 @@ var _ = Describe("Collector", Ordered, func() { }) _, err := dataCollector.Collect(ctx) - Expect(err).To(HaveOccurred()) + Expect(err).To(MatchError(expectedErr)) }) It("should error if the Pod's owner reference is not a ReplicaSet", func() { + expectedErr := errors.New("expected pod owner reference to be ReplicaSet, got Deployment") k8sClientReader.GetCalls( func(ctx context.Context, key client.ObjectKey, object client.Object, option ...client.GetOption) error { Expect(option).To(BeEmpty()) @@ -494,10 +507,11 @@ var _ = Describe("Collector", Ordered, func() { }) _, err := dataCollector.Collect(ctx) - Expect(err).To(HaveOccurred()) + Expect(err).To(MatchError(expectedErr)) }) It("should error if the replica set's replicas is nil", func() { + expectedErr := errors.New("replica set replicas was nil") k8sClientReader.GetCalls( func(ctx context.Context, key client.ObjectKey, object client.Object, option ...client.GetOption) error { Expect(option).To(BeEmpty()) @@ -524,14 +538,36 @@ var _ = Describe("Collector", Ordered, func() { }) _, err := dataCollector.Collect(ctx) - Expect(err).To(HaveOccurred()) + Expect(err).To(MatchError(expectedErr)) }) It("should error if the kubernetes client errored when getting the ReplicaSet", func() { - k8sClientReader.GetReturnsOnCall(1, errors.New("there was an error")) + expectedErr := errors.New("there was an error getting the ReplicaSet") + k8sClientReader.GetCalls( + func(ctx context.Context, key client.ObjectKey, object client.Object, option ...client.GetOption) error { + Expect(option).To(BeEmpty()) + + switch typedObj := object.(type) { + case *v1.Pod: + typedObj.ObjectMeta = metav1.ObjectMeta{ + Name: "pod1", + OwnerReferences: []metav1.OwnerReference{ + { + Kind: "ReplicaSet", + Name: "replicaset1", + }, + }, + } + case *appsv1.ReplicaSet: + return expectedErr + default: + Fail(fmt.Sprintf("unknown type: %T", typedObj)) + } + return nil + }) _, err := dataCollector.Collect(ctx) - Expect(err).To(HaveOccurred()) + Expect(err).To(MatchError(expectedErr)) }) }) }) From 2b073655cece49ca82528cecf6a5608d08b7c3a0 Mon Sep 17 00:00:00 2001 From: Benjamin Jee Date: Fri, 9 Feb 2024 15:48:08 -0800 Subject: [PATCH 6/8] Fix lint issues --- internal/mode/static/telemetry/collector_test.go | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/internal/mode/static/telemetry/collector_test.go b/internal/mode/static/telemetry/collector_test.go index d9c0bf3a3..164e97e22 100644 --- a/internal/mode/static/telemetry/collector_test.go +++ b/internal/mode/static/telemetry/collector_test.go @@ -46,7 +46,7 @@ func createGetCallsFunc() func( object client.Object, option ...client.GetOption, ) error { - return func(ctx context.Context, key client.ObjectKey, object client.Object, option ...client.GetOption) error { + return func(_ context.Context, _ client.ObjectKey, object client.Object, option ...client.GetOption) error { Expect(option).To(BeEmpty()) switch typedObj := object.(type) { @@ -414,7 +414,7 @@ var _ = Describe("Collector", Ordered, func() { It("should error if the kubernetes client errored when getting the Pod", func() { expectedErr := errors.New("there was an error getting the Pod") k8sClientReader.GetCalls( - func(ctx context.Context, key client.ObjectKey, object client.Object, option ...client.GetOption) error { + func(_ context.Context, _ client.ObjectKey, object client.Object, option ...client.GetOption) error { Expect(option).To(BeEmpty()) switch typedObj := object.(type) { @@ -433,7 +433,7 @@ var _ = Describe("Collector", Ordered, func() { It("should error if the Pod's owner reference is nil", func() { expectedErr := errors.New("expected one owner reference of the NGF Pod, got 0") k8sClientReader.GetCalls( - func(ctx context.Context, key client.ObjectKey, object client.Object, option ...client.GetOption) error { + func(_ context.Context, _ client.ObjectKey, object client.Object, option ...client.GetOption) error { Expect(option).To(BeEmpty()) switch typedObj := object.(type) { @@ -455,7 +455,7 @@ var _ = Describe("Collector", Ordered, func() { It("should error if the Pod has multiple owner references", func() { expectedErr := errors.New("expected one owner reference of the NGF Pod, got 2") k8sClientReader.GetCalls( - func(ctx context.Context, key client.ObjectKey, object client.Object, option ...client.GetOption) error { + func(_ context.Context, _ client.ObjectKey, object client.Object, option ...client.GetOption) error { Expect(option).To(BeEmpty()) switch typedObj := object.(type) { @@ -486,7 +486,7 @@ var _ = Describe("Collector", Ordered, func() { It("should error if the Pod's owner reference is not a ReplicaSet", func() { expectedErr := errors.New("expected pod owner reference to be ReplicaSet, got Deployment") k8sClientReader.GetCalls( - func(ctx context.Context, key client.ObjectKey, object client.Object, option ...client.GetOption) error { + func(_ context.Context, _ client.ObjectKey, object client.Object, option ...client.GetOption) error { Expect(option).To(BeEmpty()) switch typedObj := object.(type) { @@ -513,7 +513,7 @@ var _ = Describe("Collector", Ordered, func() { It("should error if the replica set's replicas is nil", func() { expectedErr := errors.New("replica set replicas was nil") k8sClientReader.GetCalls( - func(ctx context.Context, key client.ObjectKey, object client.Object, option ...client.GetOption) error { + func(_ context.Context, _ client.ObjectKey, object client.Object, option ...client.GetOption) error { Expect(option).To(BeEmpty()) switch typedObj := object.(type) { @@ -544,7 +544,7 @@ var _ = Describe("Collector", Ordered, func() { It("should error if the kubernetes client errored when getting the ReplicaSet", func() { expectedErr := errors.New("there was an error getting the ReplicaSet") k8sClientReader.GetCalls( - func(ctx context.Context, key client.ObjectKey, object client.Object, option ...client.GetOption) error { + func(_ context.Context, _ client.ObjectKey, object client.Object, option ...client.GetOption) error { Expect(option).To(BeEmpty()) switch typedObj := object.(type) { From 73322c54579bae294c3d84872abc7297f6317c0e Mon Sep 17 00:00:00 2001 From: Benjamin Jee Date: Mon, 12 Feb 2024 14:14:45 -0800 Subject: [PATCH 7/8] Use GetAPIReader and fix RBAC --- deploy/helm-chart/templates/rbac.yaml | 25 +++++++++++++++++-------- deploy/manifests/nginx-gateway.yaml | 25 +++++++++++++++++-------- internal/mode/static/manager.go | 2 +- 3 files changed, 35 insertions(+), 17 deletions(-) diff --git a/deploy/helm-chart/templates/rbac.yaml b/deploy/helm-chart/templates/rbac.yaml index 93411a05b..f539c2805 100644 --- a/deploy/helm-chart/templates/rbac.yaml +++ b/deploy/helm-chart/templates/rbac.yaml @@ -21,13 +21,23 @@ rules: - namespaces - services - secrets - # FIXME(bjee19): make nodes permission dependent on telemetry being enabled. - # https://github.com/nginxinc/nginx-gateway-fabric/issues/1317. - - nodes - - pods verbs: - list - watch +# FIXME(bjee19): make nodes, pods, replicasets permission dependent on telemetry being enabled. +# https://github.com/nginxinc/nginx-gateway-fabric/issues/1317. +- apiGroups: + - "" + resources: + - pods + verbs: + - get +- apiGroups: + - "" + resources: + - nodes + verbs: + - list - apiGroups: - "" resources: @@ -36,12 +46,11 @@ rules: - create - patch - apiGroups: - - apps + - apps resources: - - replicasets + - replicasets verbs: - - list - - watch + - get - apiGroups: - discovery.k8s.io resources: diff --git a/deploy/manifests/nginx-gateway.yaml b/deploy/manifests/nginx-gateway.yaml index 699969e98..578e4950b 100644 --- a/deploy/manifests/nginx-gateway.yaml +++ b/deploy/manifests/nginx-gateway.yaml @@ -32,13 +32,23 @@ rules: - namespaces - services - secrets - # FIXME(bjee19): make nodes permission dependent on telemetry being enabled. - # https://github.com/nginxinc/nginx-gateway-fabric/issues/1317. - - nodes - - pods verbs: - list - watch +# FIXME(bjee19): make nodes, pods, replicasets permission dependent on telemetry being enabled. +# https://github.com/nginxinc/nginx-gateway-fabric/issues/1317. +- apiGroups: + - "" + resources: + - pods + verbs: + - get +- apiGroups: + - "" + resources: + - nodes + verbs: + - list - apiGroups: - "" resources: @@ -47,12 +57,11 @@ rules: - create - patch - apiGroups: - - apps + - apps resources: - - replicasets + - replicasets verbs: - - list - - watch + - get - apiGroups: - discovery.k8s.io resources: diff --git a/internal/mode/static/manager.go b/internal/mode/static/manager.go index f1f47f89f..5967190cb 100644 --- a/internal/mode/static/manager.go +++ b/internal/mode/static/manager.go @@ -216,7 +216,7 @@ func StartManager(cfg config.Config) error { } dataCollector := telemetry.NewDataCollectorImpl(telemetry.DataCollectorConfig{ - K8sClientReader: mgr.GetClient(), + K8sClientReader: mgr.GetAPIReader(), GraphGetter: processor, ConfigurationGetter: eventHandler, Version: cfg.Version, From 8406a1861ea015c748f2df735d4de8a84a909356 Mon Sep 17 00:00:00 2001 From: Benjamin Jee Date: Tue, 13 Feb 2024 09:42:51 -0800 Subject: [PATCH 8/8] Refactor code to use new createGetCallsFunc --- .../mode/static/telemetry/collector_test.go | 201 ++++++++---------- 1 file changed, 93 insertions(+), 108 deletions(-) diff --git a/internal/mode/static/telemetry/collector_test.go b/internal/mode/static/telemetry/collector_test.go index 164e97e22..a2afb16ff 100644 --- a/internal/mode/static/telemetry/collector_test.go +++ b/internal/mode/static/telemetry/collector_test.go @@ -4,6 +4,7 @@ import ( "context" "errors" "fmt" + "reflect" . "github.com/onsi/ginkgo/v2" . "github.com/onsi/gomega" @@ -40,34 +41,23 @@ func createListCallsFunc(nodes []v1.Node) func( } } -func createGetCallsFunc() func( - ctx context.Context, - key client.ObjectKey, - object client.Object, - option ...client.GetOption, +func createGetCallsFunc(objects ...client.Object) func( + context.Context, + types.NamespacedName, + client.Object, + ...client.GetOption, ) error { - return func(_ context.Context, _ client.ObjectKey, object client.Object, option ...client.GetOption) error { + return func(_ context.Context, _ types.NamespacedName, object client.Object, option ...client.GetOption) error { Expect(option).To(BeEmpty()) - switch typedObj := object.(type) { - case *v1.Pod: - typedObj.ObjectMeta = metav1.ObjectMeta{ - Name: "pod1", - OwnerReferences: []metav1.OwnerReference{ - { - Kind: "ReplicaSet", - Name: "replicaset1", - }, - }, + for _, obj := range objects { + if reflect.TypeOf(obj) == reflect.TypeOf(object) { + reflect.ValueOf(object).Elem().Set(reflect.ValueOf(obj).Elem()) + return nil } - case *appsv1.ReplicaSet: - replicas := int32(1) - typedObj.Spec = appsv1.ReplicaSetSpec{ - Replicas: &replicas, - } - default: - Fail(fmt.Sprintf("unknown type: %T", typedObj)) } + + Fail(fmt.Sprintf("unknown type: %T", object)) return nil } } @@ -82,11 +72,37 @@ var _ = Describe("Collector", Ordered, func() { expData telemetry.Data ctx context.Context podNSName types.NamespacedName + ngfPod *v1.Pod + ngfReplicaSet *appsv1.ReplicaSet ) BeforeAll(func() { ctx = context.Background() version = "1.1" + + ngfPod = &v1.Pod{ + ObjectMeta: metav1.ObjectMeta{ + Name: "pod1", + OwnerReferences: []metav1.OwnerReference{ + { + Kind: "ReplicaSet", + Name: "replicaset1", + }, + }, + }, + } + + replicas := int32(1) + ngfReplicaSet = &appsv1.ReplicaSet{ + Spec: appsv1.ReplicaSetSpec{ + Replicas: &replicas, + }, + } + + podNSName = types.NamespacedName{ + Namespace: "nginx-gateway", + Name: "ngf-pod", + } }) BeforeEach(func() { @@ -100,10 +116,6 @@ var _ = Describe("Collector", Ordered, func() { k8sClientReader = &eventsfakes.FakeReader{} fakeGraphGetter = &telemetryfakes.FakeGraphGetter{} fakeConfigurationGetter = &telemetryfakes.FakeConfigurationGetter{} - podNSName = types.NamespacedName{ - Namespace: "nginx-gateway", - Name: "ngf-pod", - } fakeGraphGetter.GetLatestGraphReturns(&graph.Graph{}) fakeConfigurationGetter.GetLatestConfigurationReturns(&dataplane.Configuration{}) @@ -115,7 +127,7 @@ var _ = Describe("Collector", Ordered, func() { Version: version, PodNSName: podNSName, }) - k8sClientReader.GetCalls(createGetCallsFunc()) + k8sClientReader.GetCalls(createGetCallsFunc(ngfPod, ngfReplicaSet)) }) Describe("Normal case", func() { @@ -432,21 +444,14 @@ var _ = Describe("Collector", Ordered, func() { It("should error if the Pod's owner reference is nil", func() { expectedErr := errors.New("expected one owner reference of the NGF Pod, got 0") - k8sClientReader.GetCalls( - func(_ context.Context, _ client.ObjectKey, object client.Object, option ...client.GetOption) error { - Expect(option).To(BeEmpty()) - - switch typedObj := object.(type) { - case *v1.Pod: - typedObj.ObjectMeta = metav1.ObjectMeta{ - Name: "pod1", - OwnerReferences: nil, - } - default: - Fail(fmt.Sprintf("unknown type: %T", typedObj)) - } - return nil - }) + k8sClientReader.GetCalls(createGetCallsFunc( + &v1.Pod{ + ObjectMeta: metav1.ObjectMeta{ + Name: "pod1", + OwnerReferences: nil, + }, + }, + )) _, err := dataCollector.Collect(ctx) Expect(err).To(MatchError(expectedErr)) @@ -454,30 +459,23 @@ var _ = Describe("Collector", Ordered, func() { It("should error if the Pod has multiple owner references", func() { expectedErr := errors.New("expected one owner reference of the NGF Pod, got 2") - k8sClientReader.GetCalls( - func(_ context.Context, _ client.ObjectKey, object client.Object, option ...client.GetOption) error { - Expect(option).To(BeEmpty()) - - switch typedObj := object.(type) { - case *v1.Pod: - typedObj.ObjectMeta = metav1.ObjectMeta{ - Name: "pod1", - OwnerReferences: []metav1.OwnerReference{ - { - Kind: "ReplicaSet", - Name: "replicaset1", - }, - { - Kind: "ReplicaSet", - Name: "replicaset2", - }, + k8sClientReader.GetCalls(createGetCallsFunc( + &v1.Pod{ + ObjectMeta: metav1.ObjectMeta{ + Name: "pod1", + OwnerReferences: []metav1.OwnerReference{ + { + Kind: "ReplicaSet", + Name: "replicaset1", }, - } - default: - Fail(fmt.Sprintf("unknown type: %T", typedObj)) - } - return nil - }) + { + Kind: "ReplicaSet", + Name: "replicaset2", + }, + }, + }, + }, + )) _, err := dataCollector.Collect(ctx) Expect(err).To(MatchError(expectedErr)) @@ -485,26 +483,19 @@ var _ = Describe("Collector", Ordered, func() { It("should error if the Pod's owner reference is not a ReplicaSet", func() { expectedErr := errors.New("expected pod owner reference to be ReplicaSet, got Deployment") - k8sClientReader.GetCalls( - func(_ context.Context, _ client.ObjectKey, object client.Object, option ...client.GetOption) error { - Expect(option).To(BeEmpty()) - - switch typedObj := object.(type) { - case *v1.Pod: - typedObj.ObjectMeta = metav1.ObjectMeta{ - Name: "pod1", - OwnerReferences: []metav1.OwnerReference{ - { - Kind: "Deployment", - Name: "deployment1", - }, + k8sClientReader.GetCalls(createGetCallsFunc( + &v1.Pod{ + ObjectMeta: metav1.ObjectMeta{ + Name: "pod1", + OwnerReferences: []metav1.OwnerReference{ + { + Kind: "Deployment", + Name: "deployment1", }, - } - default: - Fail(fmt.Sprintf("unknown type: %T", typedObj)) - } - return nil - }) + }, + }, + }, + )) _, err := dataCollector.Collect(ctx) Expect(err).To(MatchError(expectedErr)) @@ -512,30 +503,24 @@ var _ = Describe("Collector", Ordered, func() { It("should error if the replica set's replicas is nil", func() { expectedErr := errors.New("replica set replicas was nil") - k8sClientReader.GetCalls( - func(_ context.Context, _ client.ObjectKey, object client.Object, option ...client.GetOption) error { - Expect(option).To(BeEmpty()) - - switch typedObj := object.(type) { - case *v1.Pod: - typedObj.ObjectMeta = metav1.ObjectMeta{ - Name: "pod1", - OwnerReferences: []metav1.OwnerReference{ - { - Kind: "ReplicaSet", - Name: "replicaset1", - }, + k8sClientReader.GetCalls(createGetCallsFunc( + &v1.Pod{ + ObjectMeta: metav1.ObjectMeta{ + Name: "pod1", + OwnerReferences: []metav1.OwnerReference{ + { + Kind: "ReplicaSet", + Name: "replicaset1", }, - } - case *appsv1.ReplicaSet: - typedObj.Spec = appsv1.ReplicaSetSpec{ - Replicas: nil, - } - default: - Fail(fmt.Sprintf("unknown type: %T", typedObj)) - } - return nil - }) + }, + }, + }, + &appsv1.ReplicaSet{ + Spec: appsv1.ReplicaSetSpec{ + Replicas: nil, + }, + }, + )) _, err := dataCollector.Collect(ctx) Expect(err).To(MatchError(expectedErr))