From 5a21bbfa613479afa6dd58e56ea5395a2aead5e5 Mon Sep 17 00:00:00 2001 From: Benjamin Jee Date: Mon, 26 Feb 2024 13:17:14 -0800 Subject: [PATCH 01/24] WIP --- internal/mode/static/telemetry/collector.go | 23 +++++++++++++++++++-- 1 file changed, 21 insertions(+), 2 deletions(-) diff --git a/internal/mode/static/telemetry/collector.go b/internal/mode/static/telemetry/collector.go index b809df6ea2..cf8c3fc352 100644 --- a/internal/mode/static/telemetry/collector.go +++ b/internal/mode/static/telemetry/collector.go @@ -5,6 +5,7 @@ import ( "errors" "fmt" "runtime" + "strings" tel "github.com/nginxinc/telemetry-exporter/pkg/telemetry" appsv1 "k8s.io/api/apps/v1" @@ -110,6 +111,15 @@ func (c DataCollectorImpl) Collect(ctx context.Context) (Data, error) { return Data{}, fmt.Errorf("failed to collect node count: %w", err) } + nodes, err := collectNodeList(ctx, c.cfg.K8sClientReader) + if err != nil { + return Data{}, err + } + + node := nodes.Items[0] + k8sVersion := node.Status.NodeInfo.KubeletVersion + k8sPlatform := strings.Split(node.Spec.ProviderID, "://")[0] + graphResourceCount, err := collectGraphResourceCount(c.cfg.GraphGetter, c.cfg.ConfigurationGetter) if err != nil { return Data{}, fmt.Errorf("failed to collect NGF resource counts: %w", err) @@ -141,8 +151,8 @@ func (c DataCollectorImpl) Collect(ctx context.Context) (Data, error) { ProjectVersion: c.cfg.Version, ProjectArchitecture: runtime.GOARCH, ClusterID: clusterID, - ClusterVersion: notImplemented, - ClusterPlatform: notImplemented, + ClusterVersion: k8sVersion, + ClusterPlatform: k8sPlatform, InstallationID: deploymentID, ClusterNodeCount: int64(nodeCount), }, @@ -275,3 +285,12 @@ func CollectClusterID(ctx context.Context, k8sClient client.Reader) (string, err } return string(kubeNamespace.GetUID()), nil } + +func collectNodeList(ctx context.Context, k8sClient client.Reader) (v1.NodeList, error) { + var nodes v1.NodeList + if err := k8sClient.List(ctx, &nodes); err != nil { + return nodes, fmt.Errorf("failed to get NodeList: %w", err) + } + + return nodes, nil +} From d3fb3d728439f9ed93ccfda9a5b3c421fab61850 Mon Sep 17 00:00:00 2001 From: Benjamin Jee Date: Wed, 6 Mar 2024 16:27:24 -0800 Subject: [PATCH 02/24] Add platform collection --- .../mode/static/telemetry/collector_test.go | 367 +++++++++++++++--- internal/mode/static/usage/job_worker.go | 6 +- pkg/telemetry/platform.go | 100 +++++ 3 files changed, 416 insertions(+), 57 deletions(-) create mode 100644 pkg/telemetry/platform.go diff --git a/internal/mode/static/telemetry/collector_test.go b/internal/mode/static/telemetry/collector_test.go index 65525d0e61..0e6ed7db83 100644 --- a/internal/mode/static/telemetry/collector_test.go +++ b/internal/mode/static/telemetry/collector_test.go @@ -3,7 +3,6 @@ package telemetry_test import ( "context" "errors" - "fmt" "reflect" "runtime" @@ -26,20 +25,23 @@ import ( "github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/telemetry/telemetryfakes" ) -func createListCallsFunc(nodes []v1.Node) func( - ctx context.Context, - list client.ObjectList, - option ...client.ListOption, -) error { - return func(_ context.Context, list client.ObjectList, option ...client.ListOption) error { +type listCallsFunc = func( + context.Context, + client.ObjectList, +...client.ListOption, +) error + +func createListCallsFunc(objects ...client.ObjectList) listCallsFunc { + return func(_ context.Context, object client.ObjectList, option ...client.ListOption) error { Expect(option).To(BeEmpty()) - switch typedList := list.(type) { - case *v1.NodeList: - typedList.Items = append(typedList.Items, nodes...) - default: - Fail(fmt.Sprintf("unknown type: %T", typedList)) + for _, obj := range objects { + if reflect.TypeOf(obj) == reflect.TypeOf(object) { + reflect.ValueOf(object).Elem().Set(reflect.ValueOf(obj).Elem()) + return nil + } } + return nil } } @@ -48,7 +50,7 @@ type getCallsFunc = func( context.Context, types.NamespacedName, client.Object, - ...client.GetOption, +...client.GetOption, ) error func createGetCallsFunc(objects ...client.Object) getCallsFunc { @@ -80,7 +82,9 @@ var _ = Describe("Collector", Ordered, func() { ngfReplicaSet *appsv1.ReplicaSet kubeNamespace *v1.Namespace baseGetCalls getCallsFunc + baseListCalls listCallsFunc flags config.Flags + nodeList *v1.NodeList ) BeforeAll(func() { @@ -132,6 +136,24 @@ var _ = Describe("Collector", Ordered, func() { Names: []string{"boolFlag", "intFlag", "stringFlag"}, Values: []string{"false", "default", "user-defined"}, } + + nodeList = &v1.NodeList{ + Items: []v1.Node{ + { + ObjectMeta: metav1.ObjectMeta{ + Name: "node1", + }, + Spec: v1.NodeSpec{ + ProviderID: "k3s://ip-172-16-0-210", + }, + Status: v1.NodeStatus{ + NodeInfo: v1.NodeSystemInfo{ + KubeletVersion: "v1.28.6+k3s2", + }, + }, + }, + }, + } }) BeforeEach(func() { @@ -141,8 +163,8 @@ var _ = Describe("Collector", Ordered, func() { ProjectVersion: version, ProjectArchitecture: runtime.GOARCH, ClusterID: string(kubeNamespace.GetUID()), - ClusterVersion: "not-implemented", - ClusterPlatform: "not-implemented", + ClusterVersion: "v1.28.6+k3s2", + ClusterPlatform: "k3s", InstallationID: string(ngfReplicaSet.ObjectMeta.OwnerReferences[0].UID), ClusterNodeCount: 0, }, @@ -172,6 +194,9 @@ var _ = Describe("Collector", Ordered, func() { baseGetCalls = createGetCallsFunc(ngfPod, ngfReplicaSet, kubeNamespace) k8sClientReader.GetCalls(baseGetCalls) + + baseListCalls = createListCallsFunc(nodeList) + k8sClientReader.ListCalls(baseListCalls) }) mergeGetCallsWithBase := func(f getCallsFunc) getCallsFunc { @@ -191,20 +216,30 @@ var _ = Describe("Collector", Ordered, func() { Describe("Normal case", func() { When("collecting telemetry data", func() { It("collects all fields", func() { - nodes := []v1.Node{ - { - ObjectMeta: metav1.ObjectMeta{ - Name: "node1", + nodes := &v1.NodeList{ + Items: []v1.Node{ + { + ObjectMeta: metav1.ObjectMeta{ + Name: "node1", + }, + Spec: v1.NodeSpec{ + ProviderID: "kind://docker/kind/kind-control-plane", + }, + Status: v1.NodeStatus{ + NodeInfo: v1.NodeSystemInfo{ + KubeletVersion: "v1.29.2", + }, + }, }, - }, - { - ObjectMeta: metav1.ObjectMeta{ - Name: "node2", + { + ObjectMeta: metav1.ObjectMeta{ + Name: "node2", + }, }, - }, - { - ObjectMeta: metav1.ObjectMeta{ - Name: "node3", + { + ObjectMeta: metav1.ObjectMeta{ + Name: "node3", + }, }, }, } @@ -294,6 +329,8 @@ var _ = Describe("Collector", Ordered, func() { ServiceCount: 3, EndpointCount: 4, } + expData.ClusterVersion = "v1.29.2" + expData.ClusterPlatform = "kind" data, err := dataCollector.Collect(ctx) @@ -303,31 +340,61 @@ var _ = Describe("Collector", Ordered, func() { }) }) - Describe("clusterID collector", func() { - When("collecting clusterID", func() { + Describe("cluster information collector", func() { + When("collecting node count data", func() { + It("collects correct data for one node", func() { + k8sClientReader.ListCalls(createListCallsFunc(nodeList)) + + expData.Data.ClusterNodeCount = 1 + + data, err := dataCollector.Collect(ctx) + + Expect(err).To(BeNil()) + Expect(expData).To(Equal(data)) + }) + When("it encounters an error while collecting data", func() { - It("should error if the kubernetes client errored when getting the namespace", func() { - expectedError := errors.New("there was an error getting clusterID") - k8sClientReader.GetCalls(mergeGetCallsWithBase( - func(_ context.Context, _ types.NamespacedName, object client.Object, _ ...client.GetOption) error { - switch object.(type) { - case *v1.Namespace: - return expectedError - } - return nil - })) + It("should error when there are no nodes", func() { + expectedError := errors.New("failed to collect cluster information: NodeList length is zero") + k8sClientReader.ListCalls(createListCallsFunc(nil)) + + _, err := dataCollector.Collect(ctx) + + Expect(err).To(MatchError(expectedError)) + }) + It("should error on kubernetes client api errors", func() { + expectedError := errors.New("there was an error getting NodeList") + k8sClientReader.ListReturns(expectedError) _, err := dataCollector.Collect(ctx) Expect(err).To(MatchError(expectedError)) }) }) }) - }) - Describe("node count collector", func() { - When("collecting node count data", func() { - It("collects correct data for no nodes", func() { - k8sClientReader.ListCalls(createListCallsFunc(nil)) + When("collecting cluster platform data", func() { + It("collects Kind platform", func() { + nodes := &v1.NodeList{ + Items: []v1.Node{ + { + ObjectMeta: metav1.ObjectMeta{ + Name: "node1", + }, + Spec: v1.NodeSpec{ + ProviderID: "kind://docker/kind/kind-control-plane", + }, + Status: v1.NodeStatus{ + NodeInfo: v1.NodeSystemInfo{ + KubeletVersion: "v1.29.2", + }, + }, + }, + }, + } + + k8sClientReader.ListCalls(createListCallsFunc(nodes)) + expData.ClusterVersion = "v1.29.2" + expData.ClusterPlatform = "kind" data, err := dataCollector.Collect(ctx) @@ -335,30 +402,220 @@ var _ = Describe("Collector", Ordered, func() { Expect(expData).To(Equal(data)) }) - It("collects correct data for one node", func() { - nodes := []v1.Node{ - { - ObjectMeta: metav1.ObjectMeta{Name: "node1"}, + It("collects GKE platform", func() { + nodes := &v1.NodeList{ + Items: []v1.Node{ + { + ObjectMeta: metav1.ObjectMeta{ + Name: "node1", + }, + Spec: v1.NodeSpec{ + ProviderID: "gce://test-data/us-central1-c/test-data", + }, + Status: v1.NodeStatus{ + NodeInfo: v1.NodeSystemInfo{ + KubeletVersion: "v1.29.2", + }, + }, + }, }, } k8sClientReader.ListCalls(createListCallsFunc(nodes)) + expData.ClusterVersion = "v1.29.2" + expData.ClusterPlatform = "gke" - expData.ClusterNodeCount = 1 + data, err := dataCollector.Collect(ctx) + + Expect(err).To(BeNil()) + Expect(expData).To(Equal(data)) + }) + + It("collects AKS platform", func() { + nodes := &v1.NodeList{ + Items: []v1.Node{ + { + ObjectMeta: metav1.ObjectMeta{ + Name: "node1", + }, + Spec: v1.NodeSpec{ + ProviderID: "azure://test-data/us-central1-c/test-data", + }, + Status: v1.NodeStatus{ + NodeInfo: v1.NodeSystemInfo{ + KubeletVersion: "v1.29.2", + }, + }, + }, + }, + } + + k8sClientReader.ListCalls(createListCallsFunc(nodes)) + expData.ClusterVersion = "v1.29.2" + expData.ClusterPlatform = "aks" + + data, err := dataCollector.Collect(ctx) + + Expect(err).To(BeNil()) + Expect(expData).To(Equal(data)) + }) + + It("collects EKS platform", func() { + nodes := &v1.NodeList{ + Items: []v1.Node{ + { + ObjectMeta: metav1.ObjectMeta{ + Name: "node1", + }, + Spec: v1.NodeSpec{ + ProviderID: "aws://test-data/us-central1-c/test-data", + }, + Status: v1.NodeStatus{ + NodeInfo: v1.NodeSystemInfo{ + KubeletVersion: "v1.29.2", + }, + }, + }, + }, + } + + k8sClientReader.ListCalls(createListCallsFunc(nodes)) + expData.ClusterVersion = "v1.29.2" + expData.ClusterPlatform = "eks" + + data, err := dataCollector.Collect(ctx) + + Expect(err).To(BeNil()) + Expect(expData).To(Equal(data)) + }) + + It("collects Rancher platform", func() { + namespaceList := &v1.NamespaceList{ + Items: []v1.Namespace{ + { + ObjectMeta: metav1.ObjectMeta{ + Name: "cattle-system", + }, + }, + }, + } + + k8sClientReader.ListCalls(createListCallsFunc(nodeList, namespaceList)) + + expData.ClusterVersion = "v1.28.6+k3s2" + expData.ClusterPlatform = "rancher" + + data, err := dataCollector.Collect(ctx) + + Expect(err).To(BeNil()) + Expect(expData).To(Equal(data)) + }) + + It("collects Openshift platform", func() { + nodes := &v1.NodeList{ + Items: []v1.Node{ + { + ObjectMeta: metav1.ObjectMeta{ + Name: "node1", + Labels: map[string]string{"node.openshift.io/os_id": "test"}, + }, + Spec: v1.NodeSpec{ + ProviderID: "k3s://test-data/us-central1-c/test-data", + }, + Status: v1.NodeStatus{ + NodeInfo: v1.NodeSystemInfo{ + KubeletVersion: "v1.29.2", + }, + }, + }, + }, + } + + k8sClientReader.ListCalls(createListCallsFunc(nodes)) + expData.ClusterVersion = "v1.29.2" + expData.ClusterPlatform = "openshift" data, err := dataCollector.Collect(ctx) Expect(err).To(BeNil()) Expect(expData).To(Equal(data)) }) + + When("platform is none of the above", func() { + It("marks the platform as 'other'", func() { + nodes := &v1.NodeList{ + Items: []v1.Node{ + { + ObjectMeta: metav1.ObjectMeta{ + Name: "node1", + }, + Spec: v1.NodeSpec{ + ProviderID: "other-cloud-provider", + }, + Status: v1.NodeStatus{ + NodeInfo: v1.NodeSystemInfo{ + KubeletVersion: "v1.29.2", + }, + }, + }, + }, + } + + k8sClientReader.ListCalls(createListCallsFunc(nodes)) + expData.ClusterVersion = "v1.29.2" + expData.ClusterPlatform = "other" + + data, err := dataCollector.Collect(ctx) + + Expect(err).To(BeNil()) + Expect(expData).To(Equal(data)) + }) + }) + }) + When("collecting cluster version data", func() { + When("the kublet version is missing", func() { + It("should be report 'unknown'", func() { + nodes := &v1.NodeList{ + Items: []v1.Node{ + { + ObjectMeta: metav1.ObjectMeta{ + Name: "node1", + }, + Spec: v1.NodeSpec{ + ProviderID: "k3s://ip-172-16-0-210", + }, + }, + }, + } + + k8sClientReader.ListCalls(createListCallsFunc(nodes)) + expData.ClusterVersion = "unknown" + expData.ClusterPlatform = "k3s" + + data, err := dataCollector.Collect(ctx) + + Expect(err).To(BeNil()) + Expect(expData).To(Equal(data)) + }) + }) }) - When("it encounters an error while collecting data", func() { - It("should error on kubernetes client api errors", func() { - expectedError := errors.New("there was an error getting NodeList") - k8sClientReader.ListReturns(expectedError) - _, err := dataCollector.Collect(ctx) - Expect(err).To(MatchError(expectedError)) + When("collecting clusterID data", func() { + When("it encounters an error while collecting data", func() { + It("should error if the kubernetes client errored when getting the namespace", func() { + expectedError := errors.New("there was an error getting clusterID") + k8sClientReader.GetCalls(mergeGetCallsWithBase( + func(_ context.Context, _ types.NamespacedName, object client.Object, _ ...client.GetOption) error { + switch object.(type) { + case *v1.Namespace: + return expectedError + } + return nil + })) + + _, err := dataCollector.Collect(ctx) + Expect(err).To(MatchError(expectedError)) + }) }) }) }) diff --git a/internal/mode/static/usage/job_worker.go b/internal/mode/static/usage/job_worker.go index 3f039181e3..4fcb5b61cb 100644 --- a/internal/mode/static/usage/job_worker.go +++ b/internal/mode/static/usage/job_worker.go @@ -19,11 +19,13 @@ func CreateUsageJobWorker( cfg config.Config, ) func(ctx context.Context) { return func(ctx context.Context) { - nodeCount, err := telemetry.CollectNodeCount(ctx, k8sClient) + nodelist, err := telemetry.CollectNodeList(ctx, k8sClient) if err != nil { - logger.Error(err, "Failed to collect node count") + logger.Error(err, "Failed to collect nodes") } + nodeCount := telemetry.CollectNodeCount(nodelist) + podCount, err := GetTotalNGFPodCount(ctx, k8sClient) if err != nil { logger.Error(err, "Failed to collect replica count") diff --git a/pkg/telemetry/platform.go b/pkg/telemetry/platform.go new file mode 100644 index 0000000000..a6f5836628 --- /dev/null +++ b/pkg/telemetry/platform.go @@ -0,0 +1,100 @@ +package telemetry + +import ( + "strings" + + v1 "k8s.io/api/core/v1" +) + +const ( + openshiftIdentifier = "node.openshift.io/os_id" + k3sIdentifier = "k3s" + awsIdentifier = "aws" + gkeIdentifier = "gce" + azureIdentifier = "azure" + kindIdentifier = "kind" + rancherIdentifier = "cattle-system" +) + +func CollectK8sPlatform(node v1.Node, namespaces v1.NamespaceList) string { + if result := isMultiplePlatforms(node, namespaces); result != "" { + return result + } + + if isAWSPlatform(node) { + return "eks" + } + if isGKEPlatform(node) { + return "gke" + } + if isAzurePlatform(node) { + return "aks" + } + if isKindPlatform(node) { + return "kind" + } + if isK3SPlatform(node) { + return "k3s" + } + + return "other" +} + +// isMultiplePlatforms checks for platforms that run on other platforms. e.g. Rancher on K3s. +func isMultiplePlatforms(node v1.Node, namespaces v1.NamespaceList) string { + if isRancherPlatform(namespaces) { + return "rancher" + } + + if isOpenshiftPlatform(node) { + return "openshift" + } + + return "" +} + +// For each of these, if we want to we can do both check the providerID AND check labels/annotations, +// I'm not too sure why we would want to do BOTH. +// +// I think doing both would add a greater certainty of a specific platform, however will potentially add to upkeep +// where if either the label/annotation or providerID changes it will mess this up and may group more clusters in +// the "Other" platform if they messed with any of the node labels/annotations. + +// I think it will be fine just to do the providerID check as + +func isOpenshiftPlatform(node v1.Node) bool { + // openshift platform won't show up in node's ProviderID + value, ok := node.Labels[openshiftIdentifier] + + return ok && value != "" +} + +func isK3SPlatform(node v1.Node) bool { + return strings.HasPrefix(node.Spec.ProviderID, k3sIdentifier) +} + +func isAWSPlatform(node v1.Node) bool { + return strings.HasPrefix(node.Spec.ProviderID, awsIdentifier) +} + +func isGKEPlatform(node v1.Node) bool { + return strings.HasPrefix(node.Spec.ProviderID, gkeIdentifier) +} + +func isAzurePlatform(node v1.Node) bool { + return strings.HasPrefix(node.Spec.ProviderID, azureIdentifier) +} + +func isKindPlatform(node v1.Node) bool { + return strings.HasPrefix(node.Spec.ProviderID, kindIdentifier) +} + +func isRancherPlatform(namespaces v1.NamespaceList) bool { + // rancher platform won't show up in the node's ProviderID + for _, ns := range namespaces.Items { + if ns.Name == rancherIdentifier { + return true + } + } + return false +} From 37989fa8f4359597e6382d8a3e0e500e019c2105 Mon Sep 17 00:00:00 2001 From: Benjamin Jee Date: Wed, 6 Mar 2024 16:33:14 -0800 Subject: [PATCH 03/24] Add small fixes --- internal/mode/static/telemetry/collector.go | 5 +++-- pkg/telemetry/platform.go | 3 +-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/internal/mode/static/telemetry/collector.go b/internal/mode/static/telemetry/collector.go index cf8c3fc352..76632c4995 100644 --- a/internal/mode/static/telemetry/collector.go +++ b/internal/mode/static/telemetry/collector.go @@ -111,7 +111,7 @@ func (c DataCollectorImpl) Collect(ctx context.Context) (Data, error) { return Data{}, fmt.Errorf("failed to collect node count: %w", err) } - nodes, err := collectNodeList(ctx, c.cfg.K8sClientReader) + nodes, err := CollectNodeList(ctx, c.cfg.K8sClientReader) if err != nil { return Data{}, err } @@ -286,7 +286,8 @@ func CollectClusterID(ctx context.Context, k8sClient client.Reader) (string, err return string(kubeNamespace.GetUID()), nil } -func collectNodeList(ctx context.Context, k8sClient client.Reader) (v1.NodeList, error) { +// CollectNodeList returns a NodeList of all the Nodes in the cluster. +func CollectNodeList(ctx context.Context, k8sClient client.Reader) (v1.NodeList, error) { var nodes v1.NodeList if err := k8sClient.List(ctx, &nodes); err != nil { return nodes, fmt.Errorf("failed to get NodeList: %w", err) diff --git a/pkg/telemetry/platform.go b/pkg/telemetry/platform.go index a6f5836628..4dcff8f20b 100644 --- a/pkg/telemetry/platform.go +++ b/pkg/telemetry/platform.go @@ -60,8 +60,6 @@ func isMultiplePlatforms(node v1.Node, namespaces v1.NamespaceList) string { // where if either the label/annotation or providerID changes it will mess this up and may group more clusters in // the "Other" platform if they messed with any of the node labels/annotations. -// I think it will be fine just to do the providerID check as - func isOpenshiftPlatform(node v1.Node) bool { // openshift platform won't show up in node's ProviderID value, ok := node.Labels[openshiftIdentifier] @@ -96,5 +94,6 @@ func isRancherPlatform(namespaces v1.NamespaceList) bool { return true } } + return false } From 1527d4a2ae91e9a930e8e5bc3eec442fdf15980a Mon Sep 17 00:00:00 2001 From: Benjamin Jee Date: Thu, 7 Mar 2024 09:58:28 -0800 Subject: [PATCH 04/24] Add contant values for cluster distribution results --- pkg/telemetry/platform.go | 25 +++++++++++++++++-------- 1 file changed, 17 insertions(+), 8 deletions(-) diff --git a/pkg/telemetry/platform.go b/pkg/telemetry/platform.go index 4dcff8f20b..100e7d386f 100644 --- a/pkg/telemetry/platform.go +++ b/pkg/telemetry/platform.go @@ -14,6 +14,15 @@ const ( azureIdentifier = "azure" kindIdentifier = "kind" rancherIdentifier = "cattle-system" + + clusterPlatformGKE = "gke" + clusterPlatformAWS = "eks" + clusterPlatformAzure = "aks" + clusterPlatformKind = "kind" + clusterPlatformK3S = "k3s" + clusterPlatformOpenShift = "openshift" + clusterPlatformRancher = "rancher" + clusterPlatformOther = "other" ) func CollectK8sPlatform(node v1.Node, namespaces v1.NamespaceList) string { @@ -22,32 +31,32 @@ func CollectK8sPlatform(node v1.Node, namespaces v1.NamespaceList) string { } if isAWSPlatform(node) { - return "eks" + return clusterPlatformAWS } if isGKEPlatform(node) { - return "gke" + return clusterPlatformGKE } if isAzurePlatform(node) { - return "aks" + return clusterPlatformAzure } if isKindPlatform(node) { - return "kind" + return clusterPlatformKind } if isK3SPlatform(node) { - return "k3s" + return clusterPlatformK3S } - return "other" + return clusterPlatformOther } // isMultiplePlatforms checks for platforms that run on other platforms. e.g. Rancher on K3s. func isMultiplePlatforms(node v1.Node, namespaces v1.NamespaceList) string { if isRancherPlatform(namespaces) { - return "rancher" + return clusterPlatformRancher } if isOpenshiftPlatform(node) { - return "openshift" + return clusterPlatformOpenShift } return "" From 6970fcf0ee01fd22f7c1b38cbe0644df0d9989cc Mon Sep 17 00:00:00 2001 From: Benjamin Jee Date: Thu, 7 Mar 2024 14:09:30 -0800 Subject: [PATCH 05/24] Add parsing of kubelet version --- internal/mode/static/telemetry/collector.go | 51 ++++++++ .../mode/static/telemetry/collector_test.go | 120 ++++++++++++++++-- 2 files changed, 159 insertions(+), 12 deletions(-) diff --git a/internal/mode/static/telemetry/collector.go b/internal/mode/static/telemetry/collector.go index 76632c4995..2b191e21b0 100644 --- a/internal/mode/static/telemetry/collector.go +++ b/internal/mode/static/telemetry/collector.go @@ -5,7 +5,9 @@ import ( "errors" "fmt" "runtime" + "strconv" "strings" + "unicode" tel "github.com/nginxinc/telemetry-exporter/pkg/telemetry" appsv1 "k8s.io/api/apps/v1" @@ -295,3 +297,52 @@ func CollectNodeList(ctx context.Context, k8sClient client.Reader) (v1.NodeList, return nodes, nil } + +// ParseKubeletVersion takes a string and turns it into a semver format. +func ParseKubeletVersion(s string) (string, error) { + s = strings.TrimSpace(s) + s = strings.TrimPrefix(s, "v") + + if s == "" { + return "", errors.New("string cannot be empty") + } + + parts := strings.SplitN(s, ".", 3) + + if _, err := strconv.Atoi(parts[0]); err != nil { + return "", errors.New("string must have a number as the major version") + } + + if len(parts) == 1 || parts[1] == "" { + return "", errors.New("string must have at least a major and minor version specified") + } + + // in the edge case where the kubeletVersion is missing the patch version and has trailing characters which include + // '.' e.g. "v1.27-gke.1067004" + if _, err := strconv.Atoi(parts[1]); err != nil && len(parts) == 3 { + parts[1] = parts[1] + parts[2] + parts = parts[:len(parts)-1] + } + + lastString := parts[len(parts)-1] + + for index := range lastString { + // cut off trailing characters after the patch version. + // e.g. if kubeletVersion = "1.27.4+500050039", will return "4" + if !unicode.IsDigit(rune(lastString[index])) { + parts[len(parts)-1] = lastString[:index] + break + } + } + + if len(parts) == 2 { + parts = append(parts, "0") + } + + // in the case where lastString was "" + if parts[2] == "" { + parts[2] = "0" + } + + return strings.Join(parts, "."), nil +} diff --git a/internal/mode/static/telemetry/collector_test.go b/internal/mode/static/telemetry/collector_test.go index 0e6ed7db83..a51318569b 100644 --- a/internal/mode/static/telemetry/collector_test.go +++ b/internal/mode/static/telemetry/collector_test.go @@ -5,6 +5,7 @@ import ( "errors" "reflect" "runtime" + "testing" tel "github.com/nginxinc/telemetry-exporter/pkg/telemetry" . "github.com/onsi/ginkgo/v2" @@ -28,7 +29,7 @@ import ( type listCallsFunc = func( context.Context, client.ObjectList, -...client.ListOption, + ...client.ListOption, ) error func createListCallsFunc(objects ...client.ObjectList) listCallsFunc { @@ -50,7 +51,7 @@ type getCallsFunc = func( context.Context, types.NamespacedName, client.Object, -...client.GetOption, + ...client.GetOption, ) error func createGetCallsFunc(objects ...client.Object) getCallsFunc { @@ -163,7 +164,7 @@ var _ = Describe("Collector", Ordered, func() { ProjectVersion: version, ProjectArchitecture: runtime.GOARCH, ClusterID: string(kubeNamespace.GetUID()), - ClusterVersion: "v1.28.6+k3s2", + ClusterVersion: "1.28.6", ClusterPlatform: "k3s", InstallationID: string(ngfReplicaSet.ObjectMeta.OwnerReferences[0].UID), ClusterNodeCount: 0, @@ -329,7 +330,7 @@ var _ = Describe("Collector", Ordered, func() { ServiceCount: 3, EndpointCount: 4, } - expData.ClusterVersion = "v1.29.2" + expData.ClusterVersion = "1.29.2" expData.ClusterPlatform = "kind" data, err := dataCollector.Collect(ctx) @@ -393,7 +394,7 @@ var _ = Describe("Collector", Ordered, func() { } k8sClientReader.ListCalls(createListCallsFunc(nodes)) - expData.ClusterVersion = "v1.29.2" + expData.ClusterVersion = "1.29.2" expData.ClusterPlatform = "kind" data, err := dataCollector.Collect(ctx) @@ -422,7 +423,7 @@ var _ = Describe("Collector", Ordered, func() { } k8sClientReader.ListCalls(createListCallsFunc(nodes)) - expData.ClusterVersion = "v1.29.2" + expData.ClusterVersion = "1.29.2" expData.ClusterPlatform = "gke" data, err := dataCollector.Collect(ctx) @@ -451,7 +452,7 @@ var _ = Describe("Collector", Ordered, func() { } k8sClientReader.ListCalls(createListCallsFunc(nodes)) - expData.ClusterVersion = "v1.29.2" + expData.ClusterVersion = "1.29.2" expData.ClusterPlatform = "aks" data, err := dataCollector.Collect(ctx) @@ -480,7 +481,7 @@ var _ = Describe("Collector", Ordered, func() { } k8sClientReader.ListCalls(createListCallsFunc(nodes)) - expData.ClusterVersion = "v1.29.2" + expData.ClusterVersion = "1.29.2" expData.ClusterPlatform = "eks" data, err := dataCollector.Collect(ctx) @@ -501,8 +502,7 @@ var _ = Describe("Collector", Ordered, func() { } k8sClientReader.ListCalls(createListCallsFunc(nodeList, namespaceList)) - - expData.ClusterVersion = "v1.28.6+k3s2" + expData.ClusterVersion = "1.28.6" expData.ClusterPlatform = "rancher" data, err := dataCollector.Collect(ctx) @@ -532,7 +532,7 @@ var _ = Describe("Collector", Ordered, func() { } k8sClientReader.ListCalls(createListCallsFunc(nodes)) - expData.ClusterVersion = "v1.29.2" + expData.ClusterVersion = "1.29.2" expData.ClusterPlatform = "openshift" data, err := dataCollector.Collect(ctx) @@ -562,7 +562,7 @@ var _ = Describe("Collector", Ordered, func() { } k8sClientReader.ListCalls(createListCallsFunc(nodes)) - expData.ClusterVersion = "v1.29.2" + expData.ClusterVersion = "1.29.2" expData.ClusterPlatform = "other" data, err := dataCollector.Collect(ctx) @@ -939,3 +939,99 @@ var _ = Describe("Collector", Ordered, func() { }) }) }) + +func TestParseKubeletVersion(t *testing.T) { + tests := []struct { + expError error + input string + expected string + name string + }{ + { + input: "v1.27.9", + expected: "1.27.9", + name: "normal case", + expError: nil, + }, + { + input: " v1.27.9 ", + expected: "1.27.9", + name: "removes added whitespace", + expError: nil, + }, + { + input: "v1.27", + expected: "1.27.0", + name: "adds appended 0's if missing semver patch number", + expError: nil, + }, + { + input: "v1.27.8-gke.1067004", + expected: "1.27.8", + name: "removes trailing characters from semver version", + expError: nil, + }, + { + input: "v1.27.9+", + expected: "1.27.9", + name: "removes trailing characters from semver version no following characters", + expError: nil, + }, + { + input: "v1.27-gke.1067004", + expected: "1.27.0", + name: "removes trailing characters from semver version no patch version", + expError: nil, + }, + { + input: "v1.27.", + expected: "1.27.0", + name: "edge case where patch version is missing but separating period is", + expError: nil, + }, + { + input: "v1.27.gke+323", + expected: "1.27.0", + name: "edge case where patch version is missing but additional characters are present", + expError: nil, + }, + { + input: "", + expected: "", + name: "error on empty string", + expError: errors.New("string cannot be empty"), + }, + { + input: "1", + expected: "", + name: "errors when major and minor version are not present", + expError: errors.New("string must have at least a major and minor version specified"), + }, + { + input: "1.", + expected: "", + name: "errors when major and minor version are not present", + expError: errors.New("string must have at least a major and minor version specified"), + }, + { + input: "123gke", + expected: "", + name: "errors when string does not contain a number as the major version", + expError: errors.New("string must have a number as the major version"), + }, + } + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + g := NewWithT(t) + + result, err := telemetry.ParseKubeletVersion(test.input) + g.Expect(result).To(Equal(test.expected)) + + if test.expError != nil { + g.Expect(err).To(MatchError(test.expError)) + } else { + g.Expect(err).To(BeNil()) + } + }) + } +} From 651e9f6170392c361b2cfbca018ab6303a555466 Mon Sep 17 00:00:00 2001 From: Benjamin Jee Date: Fri, 8 Mar 2024 09:31:48 -0800 Subject: [PATCH 06/24] Move platform code to internal directory --- {pkg => internal/mode/static}/telemetry/platform.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) rename {pkg => internal/mode/static}/telemetry/platform.go (97%) diff --git a/pkg/telemetry/platform.go b/internal/mode/static/telemetry/platform.go similarity index 97% rename from pkg/telemetry/platform.go rename to internal/mode/static/telemetry/platform.go index 100e7d386f..aea388e198 100644 --- a/pkg/telemetry/platform.go +++ b/internal/mode/static/telemetry/platform.go @@ -25,7 +25,7 @@ const ( clusterPlatformOther = "other" ) -func CollectK8sPlatform(node v1.Node, namespaces v1.NamespaceList) string { +func collectK8sPlatform(node v1.Node, namespaces v1.NamespaceList) string { if result := isMultiplePlatforms(node, namespaces); result != "" { return result } From 086bf40ca7849d9282e680d30f694e59c2dc8f7a Mon Sep 17 00:00:00 2001 From: Benjamin Jee Date: Fri, 8 Mar 2024 09:46:51 -0800 Subject: [PATCH 07/24] Move semver parsing code --- internal/mode/static/telemetry/collector.go | 66 +++++------ .../mode/static/telemetry/collector_test.go | 97 ---------------- .../mode/static/telemetry/parse_semver.go | 57 ++++++++++ .../static/telemetry/parse_semver_test.go | 104 ++++++++++++++++++ internal/mode/static/telemetry/platform.go | 32 +++--- 5 files changed, 207 insertions(+), 149 deletions(-) create mode 100644 internal/mode/static/telemetry/parse_semver.go create mode 100644 internal/mode/static/telemetry/parse_semver_test.go diff --git a/internal/mode/static/telemetry/collector.go b/internal/mode/static/telemetry/collector.go index 2b191e21b0..c31d77add1 100644 --- a/internal/mode/static/telemetry/collector.go +++ b/internal/mode/static/telemetry/collector.go @@ -5,9 +5,7 @@ import ( "errors" "fmt" "runtime" - "strconv" "strings" - "unicode" tel "github.com/nginxinc/telemetry-exporter/pkg/telemetry" appsv1 "k8s.io/api/apps/v1" @@ -298,51 +296,47 @@ func CollectNodeList(ctx context.Context, k8sClient client.Reader) (v1.NodeList, return nodes, nil } -// ParseKubeletVersion takes a string and turns it into a semver format. -func ParseKubeletVersion(s string) (string, error) { - s = strings.TrimSpace(s) - s = strings.TrimPrefix(s, "v") - - if s == "" { - return "", errors.New("string cannot be empty") - } +type clusterInformation struct { + Platform string + Version string + ClusterID string + Nodes v1.NodeList +} - parts := strings.SplitN(s, ".", 3) +func collectClusterInformation(ctx context.Context, k8sClient client.Reader) (clusterInformation, error) { + var clusterInfo clusterInformation - if _, err := strconv.Atoi(parts[0]); err != nil { - return "", errors.New("string must have a number as the major version") + nodes, err := CollectNodeList(ctx, k8sClient) + if err != nil { + return clusterInformation{}, fmt.Errorf("failed to collect cluster information: %w", err) } - - if len(parts) == 1 || parts[1] == "" { - return "", errors.New("string must have at least a major and minor version specified") + if len(nodes.Items) == 0 { + return clusterInformation{}, errors.New("failed to collect cluster information: NodeList length is zero") } - // in the edge case where the kubeletVersion is missing the patch version and has trailing characters which include - // '.' e.g. "v1.27-gke.1067004" - if _, err := strconv.Atoi(parts[1]); err != nil && len(parts) == 3 { - parts[1] = parts[1] + parts[2] - parts = parts[:len(parts)-1] + clusterInfo.Nodes = nodes + + var clusterID string + if clusterID, err = CollectClusterID(ctx, k8sClient); err != nil { + return clusterInformation{}, fmt.Errorf("failed to collect cluster information: %w", err) } + clusterInfo.ClusterID = clusterID - lastString := parts[len(parts)-1] + node := nodes.Items[0] - for index := range lastString { - // cut off trailing characters after the patch version. - // e.g. if kubeletVersion = "1.27.4+500050039", will return "4" - if !unicode.IsDigit(rune(lastString[index])) { - parts[len(parts)-1] = lastString[:index] - break - } - } + clusterInfo.Version = "unknown" + kubeletVersion := node.Status.NodeInfo.KubeletVersion - if len(parts) == 2 { - parts = append(parts, "0") + if version, err := parseSemver(kubeletVersion); err == nil { + clusterInfo.Version = version } - // in the case where lastString was "" - if parts[2] == "" { - parts[2] = "0" + var namespaces v1.NamespaceList + if err = k8sClient.List(ctx, &namespaces); err != nil { + return clusterInformation{}, fmt.Errorf("failed to collect cluster information: %w", err) } - return strings.Join(parts, "."), nil + clusterInfo.Platform = collectK8sPlatform(node, namespaces) + + return clusterInfo, nil } diff --git a/internal/mode/static/telemetry/collector_test.go b/internal/mode/static/telemetry/collector_test.go index a51318569b..b488c8ec20 100644 --- a/internal/mode/static/telemetry/collector_test.go +++ b/internal/mode/static/telemetry/collector_test.go @@ -5,7 +5,6 @@ import ( "errors" "reflect" "runtime" - "testing" tel "github.com/nginxinc/telemetry-exporter/pkg/telemetry" . "github.com/onsi/ginkgo/v2" @@ -939,99 +938,3 @@ var _ = Describe("Collector", Ordered, func() { }) }) }) - -func TestParseKubeletVersion(t *testing.T) { - tests := []struct { - expError error - input string - expected string - name string - }{ - { - input: "v1.27.9", - expected: "1.27.9", - name: "normal case", - expError: nil, - }, - { - input: " v1.27.9 ", - expected: "1.27.9", - name: "removes added whitespace", - expError: nil, - }, - { - input: "v1.27", - expected: "1.27.0", - name: "adds appended 0's if missing semver patch number", - expError: nil, - }, - { - input: "v1.27.8-gke.1067004", - expected: "1.27.8", - name: "removes trailing characters from semver version", - expError: nil, - }, - { - input: "v1.27.9+", - expected: "1.27.9", - name: "removes trailing characters from semver version no following characters", - expError: nil, - }, - { - input: "v1.27-gke.1067004", - expected: "1.27.0", - name: "removes trailing characters from semver version no patch version", - expError: nil, - }, - { - input: "v1.27.", - expected: "1.27.0", - name: "edge case where patch version is missing but separating period is", - expError: nil, - }, - { - input: "v1.27.gke+323", - expected: "1.27.0", - name: "edge case where patch version is missing but additional characters are present", - expError: nil, - }, - { - input: "", - expected: "", - name: "error on empty string", - expError: errors.New("string cannot be empty"), - }, - { - input: "1", - expected: "", - name: "errors when major and minor version are not present", - expError: errors.New("string must have at least a major and minor version specified"), - }, - { - input: "1.", - expected: "", - name: "errors when major and minor version are not present", - expError: errors.New("string must have at least a major and minor version specified"), - }, - { - input: "123gke", - expected: "", - name: "errors when string does not contain a number as the major version", - expError: errors.New("string must have a number as the major version"), - }, - } - for _, test := range tests { - t.Run(test.name, func(t *testing.T) { - g := NewWithT(t) - - result, err := telemetry.ParseKubeletVersion(test.input) - g.Expect(result).To(Equal(test.expected)) - - if test.expError != nil { - g.Expect(err).To(MatchError(test.expError)) - } else { - g.Expect(err).To(BeNil()) - } - }) - } -} diff --git a/internal/mode/static/telemetry/parse_semver.go b/internal/mode/static/telemetry/parse_semver.go new file mode 100644 index 0000000000..41521415c8 --- /dev/null +++ b/internal/mode/static/telemetry/parse_semver.go @@ -0,0 +1,57 @@ +package telemetry + +import ( + "errors" + "strconv" + "strings" + "unicode" +) + +// parseSemver takes a string and turns it into a semver format. +func parseSemver(s string) (string, error) { + s = strings.TrimSpace(s) + s = strings.TrimPrefix(s, "v") + + if s == "" { + return "", errors.New("string cannot be empty") + } + + parts := strings.SplitN(s, ".", 3) + + if _, err := strconv.Atoi(parts[0]); err != nil { + return "", errors.New("string must have a number as the major version") + } + + if len(parts) == 1 || parts[1] == "" { + return "", errors.New("string must have at least a major and minor version specified") + } + + // in the edge case where the kubeletVersion is missing the patch version and has trailing characters which include + // '.' e.g. "v1.27-gke.1067004" + if _, err := strconv.Atoi(parts[1]); err != nil && len(parts) == 3 { + parts[1] = parts[1] + parts[2] + parts = parts[:len(parts)-1] + } + + lastString := parts[len(parts)-1] + + for index := range lastString { + // cut off trailing characters after the patch version. + // e.g. if kubeletVersion = "1.27.4+500050039", will return "4" + if !unicode.IsDigit(rune(lastString[index])) { + parts[len(parts)-1] = lastString[:index] + break + } + } + + if len(parts) == 2 { + parts = append(parts, "0") + } + + // in the case where lastString was "" + if parts[2] == "" { + parts[2] = "0" + } + + return strings.Join(parts, "."), nil +} diff --git a/internal/mode/static/telemetry/parse_semver_test.go b/internal/mode/static/telemetry/parse_semver_test.go new file mode 100644 index 0000000000..345e5646db --- /dev/null +++ b/internal/mode/static/telemetry/parse_semver_test.go @@ -0,0 +1,104 @@ +package telemetry + +import ( + "errors" + "testing" + + . "github.com/onsi/gomega" +) + +func TestParseKubeletVersion(t *testing.T) { + tests := []struct { + expError error + input string + expected string + name string + }{ + { + input: "v1.27.9", + expected: "1.27.9", + name: "normal case", + expError: nil, + }, + { + input: " v1.27.9 ", + expected: "1.27.9", + name: "removes added whitespace", + expError: nil, + }, + { + input: "v1.27", + expected: "1.27.0", + name: "adds appended 0's if missing semver patch number", + expError: nil, + }, + { + input: "v1.27.8-gke.1067004", + expected: "1.27.8", + name: "removes trailing characters from semver version", + expError: nil, + }, + { + input: "v1.27.9+", + expected: "1.27.9", + name: "removes trailing characters from semver version no following characters", + expError: nil, + }, + { + input: "v1.27-gke.1067004", + expected: "1.27.0", + name: "removes trailing characters from semver version no patch version", + expError: nil, + }, + { + input: "v1.27.", + expected: "1.27.0", + name: "edge case where patch version is missing but separating period is", + expError: nil, + }, + { + input: "v1.27.gke+323", + expected: "1.27.0", + name: "edge case where patch version is missing but additional characters are present", + expError: nil, + }, + { + input: "", + expected: "", + name: "error on empty string", + expError: errors.New("string cannot be empty"), + }, + { + input: "1", + expected: "", + name: "errors when major and minor version are not present", + expError: errors.New("string must have at least a major and minor version specified"), + }, + { + input: "1.", + expected: "", + name: "errors when major and minor version are not present", + expError: errors.New("string must have at least a major and minor version specified"), + }, + { + input: "123gke", + expected: "", + name: "errors when string does not contain a number as the major version", + expError: errors.New("string must have a number as the major version"), + }, + } + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + g := NewWithT(t) + + result, err := parseSemver(test.input) + g.Expect(result).To(Equal(test.expected)) + + if test.expError != nil { + g.Expect(err).To(MatchError(test.expError)) + } else { + g.Expect(err).To(BeNil()) + } + }) + } +} diff --git a/internal/mode/static/telemetry/platform.go b/internal/mode/static/telemetry/platform.go index aea388e198..8d9afe92d8 100644 --- a/internal/mode/static/telemetry/platform.go +++ b/internal/mode/static/telemetry/platform.go @@ -15,14 +15,14 @@ const ( kindIdentifier = "kind" rancherIdentifier = "cattle-system" - clusterPlatformGKE = "gke" - clusterPlatformAWS = "eks" - clusterPlatformAzure = "aks" - clusterPlatformKind = "kind" - clusterPlatformK3S = "k3s" - clusterPlatformOpenShift = "openshift" - clusterPlatformRancher = "rancher" - clusterPlatformOther = "other" + platformGKE = "gke" + platformAWS = "eks" + platformAzure = "aks" + platformKind = "kind" + platformK3S = "k3s" + platformOpenShift = "openshift" + platformRancher = "rancher" + platformOther = "other" ) func collectK8sPlatform(node v1.Node, namespaces v1.NamespaceList) string { @@ -31,32 +31,32 @@ func collectK8sPlatform(node v1.Node, namespaces v1.NamespaceList) string { } if isAWSPlatform(node) { - return clusterPlatformAWS + return platformAWS } if isGKEPlatform(node) { - return clusterPlatformGKE + return platformGKE } if isAzurePlatform(node) { - return clusterPlatformAzure + return platformAzure } if isKindPlatform(node) { - return clusterPlatformKind + return platformKind } if isK3SPlatform(node) { - return clusterPlatformK3S + return platformK3S } - return clusterPlatformOther + return platformOther } // isMultiplePlatforms checks for platforms that run on other platforms. e.g. Rancher on K3s. func isMultiplePlatforms(node v1.Node, namespaces v1.NamespaceList) string { if isRancherPlatform(namespaces) { - return clusterPlatformRancher + return platformRancher } if isOpenshiftPlatform(node) { - return clusterPlatformOpenShift + return platformOpenShift } return "" From fcede7e06fbd70cae9cf8fd1449f9421e6f976d4 Mon Sep 17 00:00:00 2001 From: Benjamin Jee Date: Fri, 8 Mar 2024 11:16:49 -0800 Subject: [PATCH 08/24] Add additional information when reporting platform as other --- internal/mode/static/telemetry/collector.go | 2 +- .../mode/static/telemetry/collector_test.go | 57 +++++++- internal/mode/static/telemetry/platform.go | 125 +++++++++--------- 3 files changed, 116 insertions(+), 68 deletions(-) diff --git a/internal/mode/static/telemetry/collector.go b/internal/mode/static/telemetry/collector.go index c31d77add1..0f64ca7ab8 100644 --- a/internal/mode/static/telemetry/collector.go +++ b/internal/mode/static/telemetry/collector.go @@ -336,7 +336,7 @@ func collectClusterInformation(ctx context.Context, k8sClient client.Reader) (cl return clusterInformation{}, fmt.Errorf("failed to collect cluster information: %w", err) } - clusterInfo.Platform = collectK8sPlatform(node, namespaces) + clusterInfo.Platform = getPlatform(node, namespaces) return clusterInfo, nil } diff --git a/internal/mode/static/telemetry/collector_test.go b/internal/mode/static/telemetry/collector_test.go index b488c8ec20..0c5dd352cd 100644 --- a/internal/mode/static/telemetry/collector_test.go +++ b/internal/mode/static/telemetry/collector_test.go @@ -541,7 +541,60 @@ var _ = Describe("Collector", Ordered, func() { }) When("platform is none of the above", func() { - It("marks the platform as 'other'", func() { + It("marks the platform as 'other: ' with whatever was in the providerID's providerName", func() { + nodes := &v1.NodeList{ + Items: []v1.Node{ + { + ObjectMeta: metav1.ObjectMeta{ + Name: "node1", + }, + Spec: v1.NodeSpec{ + ProviderID: "other-cloud-provider://test-here", + }, + Status: v1.NodeStatus{ + NodeInfo: v1.NodeSystemInfo{ + KubeletVersion: "v1.29.2", + }, + }, + }, + }, + } + + k8sClientReader.ListCalls(createListCallsFunc(nodes)) + expData.ClusterVersion = "1.29.2" + expData.ClusterPlatform = "other: other-cloud-provider" + + data, err := dataCollector.Collect(ctx) + + Expect(err).To(BeNil()) + Expect(expData).To(Equal(data)) + }) + It("marks the platform as 'other: ' when providerID is empty", func() { + nodes := &v1.NodeList{ + Items: []v1.Node{ + { + ObjectMeta: metav1.ObjectMeta{ + Name: "node1", + }, + Status: v1.NodeStatus{ + NodeInfo: v1.NodeSystemInfo{ + KubeletVersion: "v1.29.2", + }, + }, + }, + }, + } + + k8sClientReader.ListCalls(createListCallsFunc(nodes)) + expData.ClusterVersion = "1.29.2" + expData.ClusterPlatform = "other: " + + data, err := dataCollector.Collect(ctx) + + Expect(err).To(BeNil()) + Expect(expData).To(Equal(data)) + }) + It("marks the platform as 'other: ' when providerID is missing '://' separator", func() { nodes := &v1.NodeList{ Items: []v1.Node{ { @@ -562,7 +615,7 @@ var _ = Describe("Collector", Ordered, func() { k8sClientReader.ListCalls(createListCallsFunc(nodes)) expData.ClusterVersion = "1.29.2" - expData.ClusterPlatform = "other" + expData.ClusterPlatform = "other: " data, err := dataCollector.Collect(ctx) diff --git a/internal/mode/static/telemetry/platform.go b/internal/mode/static/telemetry/platform.go index 8d9afe92d8..d9d8183704 100644 --- a/internal/mode/static/telemetry/platform.go +++ b/internal/mode/static/telemetry/platform.go @@ -6,13 +6,29 @@ import ( v1 "k8s.io/api/core/v1" ) +type k8sState struct { + node v1.Node + namespaces v1.NamespaceList +} + +type platformExtractor func(k8sState) (string, bool) + +func buildProviderIDExtractor(id string, platform string) platformExtractor { + return func(state k8sState) (string, bool) { + if strings.HasPrefix(state.node.Spec.ProviderID, id) { + return platform, true + } + return "", false + } +} + const ( - openshiftIdentifier = "node.openshift.io/os_id" - k3sIdentifier = "k3s" - awsIdentifier = "aws" gkeIdentifier = "gce" + awsIdentifier = "aws" azureIdentifier = "azure" kindIdentifier = "kind" + k3sIdentifier = "k3s" + openshiftIdentifier = "node.openshift.io/os_id" rancherIdentifier = "cattle-system" platformGKE = "gke" @@ -22,87 +38,66 @@ const ( platformK3S = "k3s" platformOpenShift = "openshift" platformRancher = "rancher" - platformOther = "other" ) -func collectK8sPlatform(node v1.Node, namespaces v1.NamespaceList) string { - if result := isMultiplePlatforms(node, namespaces); result != "" { - return result - } - - if isAWSPlatform(node) { - return platformAWS - } - if isGKEPlatform(node) { - return platformGKE - } - if isAzurePlatform(node) { - return platformAzure - } - if isKindPlatform(node) { - return platformKind - } - if isK3SPlatform(node) { - return platformK3S - } +var multiDistributionPlatformExtractors = []platformExtractor{ + rancherExtractor, + openShiftExtractor, +} - return platformOther +var platformExtractors = []platformExtractor{ + buildProviderIDExtractor(gkeIdentifier, platformGKE), + buildProviderIDExtractor(awsIdentifier, platformAWS), + buildProviderIDExtractor(azureIdentifier, platformAzure), + buildProviderIDExtractor(kindIdentifier, platformKind), + buildProviderIDExtractor(k3sIdentifier, platformK3S), } -// isMultiplePlatforms checks for platforms that run on other platforms. e.g. Rancher on K3s. -func isMultiplePlatforms(node v1.Node, namespaces v1.NamespaceList) string { - if isRancherPlatform(namespaces) { - return platformRancher +func getPlatform(node v1.Node, namespaces v1.NamespaceList) string { + state := k8sState{ + node: node, + namespaces: namespaces, } - if isOpenshiftPlatform(node) { - return platformOpenShift + // must be run before providerIDPlatformExtractors as these platforms + // may have multiple platforms e.g. Rancher on K3S, and we want to record the + // higher level platform. + for _, extractor := range multiDistributionPlatformExtractors { + if platform, ok := extractor(state); ok { + return platform + } } - return "" -} - -// For each of these, if we want to we can do both check the providerID AND check labels/annotations, -// I'm not too sure why we would want to do BOTH. -// -// I think doing both would add a greater certainty of a specific platform, however will potentially add to upkeep -// where if either the label/annotation or providerID changes it will mess this up and may group more clusters in -// the "Other" platform if they messed with any of the node labels/annotations. - -func isOpenshiftPlatform(node v1.Node) bool { - // openshift platform won't show up in node's ProviderID - value, ok := node.Labels[openshiftIdentifier] - - return ok && value != "" -} - -func isK3SPlatform(node v1.Node) bool { - return strings.HasPrefix(node.Spec.ProviderID, k3sIdentifier) -} + for _, extractor := range platformExtractors { + if platform, ok := extractor(state); ok { + return platform + } + } -func isAWSPlatform(node v1.Node) bool { - return strings.HasPrefix(node.Spec.ProviderID, awsIdentifier) -} + var providerName string + if prefix, _, found := strings.Cut(node.Spec.ProviderID, "://"); found { + providerName = prefix + } -func isGKEPlatform(node v1.Node) bool { - return strings.HasPrefix(node.Spec.ProviderID, gkeIdentifier) + return "other: " + providerName } -func isAzurePlatform(node v1.Node) bool { - return strings.HasPrefix(node.Spec.ProviderID, azureIdentifier) -} +func openShiftExtractor(state k8sState) (string, bool) { + // openshift platform won't show up in node's ProviderID + if value, ok := state.node.Labels[openshiftIdentifier]; ok && value != "" { + return platformOpenShift, true + } -func isKindPlatform(node v1.Node) bool { - return strings.HasPrefix(node.Spec.ProviderID, kindIdentifier) + return "", false } -func isRancherPlatform(namespaces v1.NamespaceList) bool { +func rancherExtractor(state k8sState) (string, bool) { // rancher platform won't show up in the node's ProviderID - for _, ns := range namespaces.Items { + for _, ns := range state.namespaces.Items { if ns.Name == rancherIdentifier { - return true + return platformRancher, true } } - return false + return "", false } From 35633714286efb8c164ab55dbc190b6a1d29d74d Mon Sep 17 00:00:00 2001 From: Benjamin Jee Date: Mon, 11 Mar 2024 11:56:07 -0700 Subject: [PATCH 09/24] Fix rebase issues --- internal/mode/static/telemetry/collector.go | 28 ++++--------------- .../mode/static/telemetry/collector_test.go | 2 +- internal/mode/static/usage/job_worker.go | 4 +-- 3 files changed, 9 insertions(+), 25 deletions(-) diff --git a/internal/mode/static/telemetry/collector.go b/internal/mode/static/telemetry/collector.go index 0f64ca7ab8..b64612b4f8 100644 --- a/internal/mode/static/telemetry/collector.go +++ b/internal/mode/static/telemetry/collector.go @@ -5,7 +5,6 @@ import ( "errors" "fmt" "runtime" - "strings" tel "github.com/nginxinc/telemetry-exporter/pkg/telemetry" appsv1 "k8s.io/api/apps/v1" @@ -101,24 +100,14 @@ func NewDataCollectorImpl( } } -// notImplemented is a value for string field, for which collection is not implemented yet. -const notImplemented = "not-implemented" - // Collect collects and returns telemetry Data. func (c DataCollectorImpl) Collect(ctx context.Context) (Data, error) { - nodeCount, err := CollectNodeCount(ctx, c.cfg.K8sClientReader) - if err != nil { - return Data{}, fmt.Errorf("failed to collect node count: %w", err) - } - - nodes, err := CollectNodeList(ctx, c.cfg.K8sClientReader) + clusterInfo, err := collectClusterInformation(ctx, c.cfg.K8sClientReader) if err != nil { - return Data{}, err + return Data{}, fmt.Errorf("failed to collect cluster information: %w", err) } - node := nodes.Items[0] - k8sVersion := node.Status.NodeInfo.KubeletVersion - k8sPlatform := strings.Split(node.Spec.ProviderID, "://")[0] + nodeCount := len(clusterInfo.Nodes.Items) graphResourceCount, err := collectGraphResourceCount(c.cfg.GraphGetter, c.cfg.ConfigurationGetter) if err != nil { @@ -140,19 +129,14 @@ func (c DataCollectorImpl) Collect(ctx context.Context) (Data, error) { return Data{}, fmt.Errorf("failed to get NGF deploymentID: %w", err) } - var clusterID string - if clusterID, err = CollectClusterID(ctx, c.cfg.K8sClientReader); err != nil { - return Data{}, fmt.Errorf("failed to collect clusterID: %w", err) - } - data := Data{ Data: tel.Data{ ProjectName: "NGF", ProjectVersion: c.cfg.Version, ProjectArchitecture: runtime.GOARCH, - ClusterID: clusterID, - ClusterVersion: k8sVersion, - ClusterPlatform: k8sPlatform, + ClusterID: clusterInfo.ClusterID, + ClusterVersion: clusterInfo.Version, + ClusterPlatform: clusterInfo.Platform, InstallationID: deploymentID, ClusterNodeCount: int64(nodeCount), }, diff --git a/internal/mode/static/telemetry/collector_test.go b/internal/mode/static/telemetry/collector_test.go index 0c5dd352cd..1544fd64fc 100644 --- a/internal/mode/static/telemetry/collector_test.go +++ b/internal/mode/static/telemetry/collector_test.go @@ -166,7 +166,7 @@ var _ = Describe("Collector", Ordered, func() { ClusterVersion: "1.28.6", ClusterPlatform: "k3s", InstallationID: string(ngfReplicaSet.ObjectMeta.OwnerReferences[0].UID), - ClusterNodeCount: 0, + ClusterNodeCount: 1, }, NGFResourceCounts: telemetry.NGFResourceCounts{}, NGFReplicaCount: 1, diff --git a/internal/mode/static/usage/job_worker.go b/internal/mode/static/usage/job_worker.go index 4fcb5b61cb..2b365ea6b1 100644 --- a/internal/mode/static/usage/job_worker.go +++ b/internal/mode/static/usage/job_worker.go @@ -19,12 +19,12 @@ func CreateUsageJobWorker( cfg config.Config, ) func(ctx context.Context) { return func(ctx context.Context) { - nodelist, err := telemetry.CollectNodeList(ctx, k8sClient) + nodeList, err := telemetry.CollectNodeList(ctx, k8sClient) if err != nil { logger.Error(err, "Failed to collect nodes") } - nodeCount := telemetry.CollectNodeCount(nodelist) + nodeCount := len(nodeList.Items) podCount, err := GetTotalNGFPodCount(ctx, k8sClient) if err != nil { From 2dcf5b353187d9c8c0d81e4425bbc45cb3973d95 Mon Sep 17 00:00:00 2001 From: Benjamin Jee Date: Mon, 11 Mar 2024 12:13:15 -0700 Subject: [PATCH 10/24] Remove Collect Node Count --- internal/mode/static/telemetry/collector.go | 10 ---------- 1 file changed, 10 deletions(-) diff --git a/internal/mode/static/telemetry/collector.go b/internal/mode/static/telemetry/collector.go index b64612b4f8..fa49682694 100644 --- a/internal/mode/static/telemetry/collector.go +++ b/internal/mode/static/telemetry/collector.go @@ -150,16 +150,6 @@ func (c DataCollectorImpl) Collect(ctx context.Context) (Data, error) { return data, nil } -// CollectNodeCount returns the number of nodes in the cluster. -func CollectNodeCount(ctx context.Context, k8sClient client.Reader) (int, error) { - var nodes v1.NodeList - if err := k8sClient.List(ctx, &nodes); err != nil { - return 0, fmt.Errorf("failed to get NodeList: %w", err) - } - - return len(nodes.Items), nil -} - func collectGraphResourceCount( graphGetter GraphGetter, configurationGetter ConfigurationGetter, From 39d0f148b484cbf60882e9fc72473587e61a33e7 Mon Sep 17 00:00:00 2001 From: Benjamin Jee Date: Mon, 11 Mar 2024 13:06:31 -0700 Subject: [PATCH 11/24] Revert CollectNodeCount and remove CollectNodeList --- internal/mode/static/telemetry/collector.go | 15 ++++++++------- internal/mode/static/usage/job_worker.go | 6 ++---- 2 files changed, 10 insertions(+), 11 deletions(-) diff --git a/internal/mode/static/telemetry/collector.go b/internal/mode/static/telemetry/collector.go index fa49682694..ca8f31bcd9 100644 --- a/internal/mode/static/telemetry/collector.go +++ b/internal/mode/static/telemetry/collector.go @@ -260,14 +260,14 @@ func CollectClusterID(ctx context.Context, k8sClient client.Reader) (string, err return string(kubeNamespace.GetUID()), nil } -// CollectNodeList returns a NodeList of all the Nodes in the cluster. -func CollectNodeList(ctx context.Context, k8sClient client.Reader) (v1.NodeList, error) { +// CollectNodeCount returns the number of nodes in the cluster. +func CollectNodeCount(ctx context.Context, k8sClient client.Reader) (int, error) { var nodes v1.NodeList if err := k8sClient.List(ctx, &nodes); err != nil { - return nodes, fmt.Errorf("failed to get NodeList: %w", err) + return 0, fmt.Errorf("failed to get NodeList: %w", err) } - return nodes, nil + return len(nodes.Items), nil } type clusterInformation struct { @@ -279,10 +279,11 @@ type clusterInformation struct { func collectClusterInformation(ctx context.Context, k8sClient client.Reader) (clusterInformation, error) { var clusterInfo clusterInformation + var err error - nodes, err := CollectNodeList(ctx, k8sClient) - if err != nil { - return clusterInformation{}, fmt.Errorf("failed to collect cluster information: %w", err) + var nodes v1.NodeList + if err = k8sClient.List(ctx, &nodes); err != nil { + return clusterInformation{}, fmt.Errorf("failed to get NodeList: %w", err) } if len(nodes.Items) == 0 { return clusterInformation{}, errors.New("failed to collect cluster information: NodeList length is zero") diff --git a/internal/mode/static/usage/job_worker.go b/internal/mode/static/usage/job_worker.go index 2b365ea6b1..3f039181e3 100644 --- a/internal/mode/static/usage/job_worker.go +++ b/internal/mode/static/usage/job_worker.go @@ -19,13 +19,11 @@ func CreateUsageJobWorker( cfg config.Config, ) func(ctx context.Context) { return func(ctx context.Context) { - nodeList, err := telemetry.CollectNodeList(ctx, k8sClient) + nodeCount, err := telemetry.CollectNodeCount(ctx, k8sClient) if err != nil { - logger.Error(err, "Failed to collect nodes") + logger.Error(err, "Failed to collect node count") } - nodeCount := len(nodeList.Items) - podCount, err := GetTotalNGFPodCount(ctx, k8sClient) if err != nil { logger.Error(err, "Failed to collect replica count") From 704751361264396d4273c8ab18d3b546a7887adf Mon Sep 17 00:00:00 2001 From: Benjamin Jee Date: Mon, 11 Mar 2024 15:09:52 -0700 Subject: [PATCH 12/24] Adjust other provider --- internal/mode/static/telemetry/collector.go | 8 ++++---- internal/mode/static/telemetry/collector_test.go | 12 ++++++------ internal/mode/static/telemetry/platform.go | 9 +++++++-- 3 files changed, 17 insertions(+), 12 deletions(-) diff --git a/internal/mode/static/telemetry/collector.go b/internal/mode/static/telemetry/collector.go index ca8f31bcd9..bf34a4bfdd 100644 --- a/internal/mode/static/telemetry/collector.go +++ b/internal/mode/static/telemetry/collector.go @@ -107,7 +107,10 @@ func (c DataCollectorImpl) Collect(ctx context.Context) (Data, error) { return Data{}, fmt.Errorf("failed to collect cluster information: %w", err) } - nodeCount := len(clusterInfo.Nodes.Items) + nodeCount, err := CollectNodeCount(ctx, c.cfg.K8sClientReader) + if err != nil { + return Data{}, fmt.Errorf("failed to collect node count: %w", err) + } graphResourceCount, err := collectGraphResourceCount(c.cfg.GraphGetter, c.cfg.ConfigurationGetter) if err != nil { @@ -274,7 +277,6 @@ type clusterInformation struct { Platform string Version string ClusterID string - Nodes v1.NodeList } func collectClusterInformation(ctx context.Context, k8sClient client.Reader) (clusterInformation, error) { @@ -289,8 +291,6 @@ func collectClusterInformation(ctx context.Context, k8sClient client.Reader) (cl return clusterInformation{}, errors.New("failed to collect cluster information: NodeList length is zero") } - clusterInfo.Nodes = nodes - var clusterID string if clusterID, err = CollectClusterID(ctx, k8sClient); err != nil { return clusterInformation{}, fmt.Errorf("failed to collect cluster information: %w", err) diff --git a/internal/mode/static/telemetry/collector_test.go b/internal/mode/static/telemetry/collector_test.go index 1544fd64fc..bb9cfe0802 100644 --- a/internal/mode/static/telemetry/collector_test.go +++ b/internal/mode/static/telemetry/collector_test.go @@ -541,7 +541,7 @@ var _ = Describe("Collector", Ordered, func() { }) When("platform is none of the above", func() { - It("marks the platform as 'other: ' with whatever was in the providerID's providerName", func() { + It("marks the platform as 'other_' with whatever was in the providerID's providerName", func() { nodes := &v1.NodeList{ Items: []v1.Node{ { @@ -562,14 +562,14 @@ var _ = Describe("Collector", Ordered, func() { k8sClientReader.ListCalls(createListCallsFunc(nodes)) expData.ClusterVersion = "1.29.2" - expData.ClusterPlatform = "other: other-cloud-provider" + expData.ClusterPlatform = "other_other-cloud-provider" data, err := dataCollector.Collect(ctx) Expect(err).To(BeNil()) Expect(expData).To(Equal(data)) }) - It("marks the platform as 'other: ' when providerID is empty", func() { + It("marks the platform as 'other' when providerID is empty", func() { nodes := &v1.NodeList{ Items: []v1.Node{ { @@ -587,14 +587,14 @@ var _ = Describe("Collector", Ordered, func() { k8sClientReader.ListCalls(createListCallsFunc(nodes)) expData.ClusterVersion = "1.29.2" - expData.ClusterPlatform = "other: " + expData.ClusterPlatform = "other" data, err := dataCollector.Collect(ctx) Expect(err).To(BeNil()) Expect(expData).To(Equal(data)) }) - It("marks the platform as 'other: ' when providerID is missing '://' separator", func() { + It("marks the platform as 'other' when providerID is missing '://' separator", func() { nodes := &v1.NodeList{ Items: []v1.Node{ { @@ -615,7 +615,7 @@ var _ = Describe("Collector", Ordered, func() { k8sClientReader.ListCalls(createListCallsFunc(nodes)) expData.ClusterVersion = "1.29.2" - expData.ClusterPlatform = "other: " + expData.ClusterPlatform = "other" data, err := dataCollector.Collect(ctx) diff --git a/internal/mode/static/telemetry/platform.go b/internal/mode/static/telemetry/platform.go index d9d8183704..04c909bfff 100644 --- a/internal/mode/static/telemetry/platform.go +++ b/internal/mode/static/telemetry/platform.go @@ -38,6 +38,7 @@ const ( platformK3S = "k3s" platformOpenShift = "openshift" platformRancher = "rancher" + platformOther = "other" ) var multiDistributionPlatformExtractors = []platformExtractor{ @@ -76,10 +77,14 @@ func getPlatform(node v1.Node, namespaces v1.NamespaceList) string { var providerName string if prefix, _, found := strings.Cut(node.Spec.ProviderID, "://"); found { - providerName = prefix + providerName = strings.TrimSpace(prefix) } - return "other: " + providerName + if providerName == "" { + return platformOther + } + + return platformOther + "_" + providerName } func openShiftExtractor(state k8sState) (string, bool) { From 692b1d076b2ea3c371f8f09f99af3b5a54808af5 Mon Sep 17 00:00:00 2001 From: Benjamin Jee Date: Mon, 11 Mar 2024 16:45:16 -0700 Subject: [PATCH 13/24] Refactor platform extractors --- internal/mode/static/telemetry/platform.go | 41 +++++++++------------- 1 file changed, 17 insertions(+), 24 deletions(-) diff --git a/internal/mode/static/telemetry/platform.go b/internal/mode/static/telemetry/platform.go index 04c909bfff..b1edd12f75 100644 --- a/internal/mode/static/telemetry/platform.go +++ b/internal/mode/static/telemetry/platform.go @@ -41,12 +41,10 @@ const ( platformOther = "other" ) -var multiDistributionPlatformExtractors = []platformExtractor{ - rancherExtractor, - openShiftExtractor, -} - var platformExtractors = []platformExtractor{ + openShiftExtractor, + rancherExtractor, + // ID provider extractors must run after the rest buildProviderIDExtractor(gkeIdentifier, platformGKE), buildProviderIDExtractor(awsIdentifier, platformAWS), buildProviderIDExtractor(azureIdentifier, platformAzure), @@ -60,31 +58,13 @@ func getPlatform(node v1.Node, namespaces v1.NamespaceList) string { namespaces: namespaces, } - // must be run before providerIDPlatformExtractors as these platforms - // may have multiple platforms e.g. Rancher on K3S, and we want to record the - // higher level platform. - for _, extractor := range multiDistributionPlatformExtractors { - if platform, ok := extractor(state); ok { - return platform - } - } - for _, extractor := range platformExtractors { if platform, ok := extractor(state); ok { return platform } } - var providerName string - if prefix, _, found := strings.Cut(node.Spec.ProviderID, "://"); found { - providerName = strings.TrimSpace(prefix) - } - - if providerName == "" { - return platformOther - } - - return platformOther + "_" + providerName + return unknownProviderIDExtractor(state) } func openShiftExtractor(state k8sState) (string, bool) { @@ -106,3 +86,16 @@ func rancherExtractor(state k8sState) (string, bool) { return "", false } + +func unknownProviderIDExtractor(state k8sState) string { + var providerName string + if prefix, _, found := strings.Cut(state.node.Spec.ProviderID, "://"); found { + providerName = strings.TrimSpace(prefix) + } + + if providerName == "" { + return platformOther + } + + return platformOther + "_" + providerName +} From 3abe5ed3f334adae9bbe7a4824eba2b821d0f079 Mon Sep 17 00:00:00 2001 From: Benjamin Jee Date: Mon, 11 Mar 2024 16:52:44 -0700 Subject: [PATCH 14/24] Use ParseGeneric and remove parseSemver --- internal/mode/static/telemetry/collector.go | 5 +- .../mode/static/telemetry/parse_semver.go | 57 ---------- .../static/telemetry/parse_semver_test.go | 104 ------------------ 3 files changed, 3 insertions(+), 163 deletions(-) delete mode 100644 internal/mode/static/telemetry/parse_semver.go delete mode 100644 internal/mode/static/telemetry/parse_semver_test.go diff --git a/internal/mode/static/telemetry/collector.go b/internal/mode/static/telemetry/collector.go index bf34a4bfdd..beaa3a73f0 100644 --- a/internal/mode/static/telemetry/collector.go +++ b/internal/mode/static/telemetry/collector.go @@ -11,6 +11,7 @@ import ( v1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/types" + version2 "k8s.io/apimachinery/pkg/util/version" "sigs.k8s.io/controller-runtime/pkg/client" "github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/config" @@ -302,8 +303,8 @@ func collectClusterInformation(ctx context.Context, k8sClient client.Reader) (cl clusterInfo.Version = "unknown" kubeletVersion := node.Status.NodeInfo.KubeletVersion - if version, err := parseSemver(kubeletVersion); err == nil { - clusterInfo.Version = version + if version, err := version2.ParseGeneric(kubeletVersion); err == nil { + clusterInfo.Version = version.String() } var namespaces v1.NamespaceList diff --git a/internal/mode/static/telemetry/parse_semver.go b/internal/mode/static/telemetry/parse_semver.go deleted file mode 100644 index 41521415c8..0000000000 --- a/internal/mode/static/telemetry/parse_semver.go +++ /dev/null @@ -1,57 +0,0 @@ -package telemetry - -import ( - "errors" - "strconv" - "strings" - "unicode" -) - -// parseSemver takes a string and turns it into a semver format. -func parseSemver(s string) (string, error) { - s = strings.TrimSpace(s) - s = strings.TrimPrefix(s, "v") - - if s == "" { - return "", errors.New("string cannot be empty") - } - - parts := strings.SplitN(s, ".", 3) - - if _, err := strconv.Atoi(parts[0]); err != nil { - return "", errors.New("string must have a number as the major version") - } - - if len(parts) == 1 || parts[1] == "" { - return "", errors.New("string must have at least a major and minor version specified") - } - - // in the edge case where the kubeletVersion is missing the patch version and has trailing characters which include - // '.' e.g. "v1.27-gke.1067004" - if _, err := strconv.Atoi(parts[1]); err != nil && len(parts) == 3 { - parts[1] = parts[1] + parts[2] - parts = parts[:len(parts)-1] - } - - lastString := parts[len(parts)-1] - - for index := range lastString { - // cut off trailing characters after the patch version. - // e.g. if kubeletVersion = "1.27.4+500050039", will return "4" - if !unicode.IsDigit(rune(lastString[index])) { - parts[len(parts)-1] = lastString[:index] - break - } - } - - if len(parts) == 2 { - parts = append(parts, "0") - } - - // in the case where lastString was "" - if parts[2] == "" { - parts[2] = "0" - } - - return strings.Join(parts, "."), nil -} diff --git a/internal/mode/static/telemetry/parse_semver_test.go b/internal/mode/static/telemetry/parse_semver_test.go deleted file mode 100644 index 345e5646db..0000000000 --- a/internal/mode/static/telemetry/parse_semver_test.go +++ /dev/null @@ -1,104 +0,0 @@ -package telemetry - -import ( - "errors" - "testing" - - . "github.com/onsi/gomega" -) - -func TestParseKubeletVersion(t *testing.T) { - tests := []struct { - expError error - input string - expected string - name string - }{ - { - input: "v1.27.9", - expected: "1.27.9", - name: "normal case", - expError: nil, - }, - { - input: " v1.27.9 ", - expected: "1.27.9", - name: "removes added whitespace", - expError: nil, - }, - { - input: "v1.27", - expected: "1.27.0", - name: "adds appended 0's if missing semver patch number", - expError: nil, - }, - { - input: "v1.27.8-gke.1067004", - expected: "1.27.8", - name: "removes trailing characters from semver version", - expError: nil, - }, - { - input: "v1.27.9+", - expected: "1.27.9", - name: "removes trailing characters from semver version no following characters", - expError: nil, - }, - { - input: "v1.27-gke.1067004", - expected: "1.27.0", - name: "removes trailing characters from semver version no patch version", - expError: nil, - }, - { - input: "v1.27.", - expected: "1.27.0", - name: "edge case where patch version is missing but separating period is", - expError: nil, - }, - { - input: "v1.27.gke+323", - expected: "1.27.0", - name: "edge case where patch version is missing but additional characters are present", - expError: nil, - }, - { - input: "", - expected: "", - name: "error on empty string", - expError: errors.New("string cannot be empty"), - }, - { - input: "1", - expected: "", - name: "errors when major and minor version are not present", - expError: errors.New("string must have at least a major and minor version specified"), - }, - { - input: "1.", - expected: "", - name: "errors when major and minor version are not present", - expError: errors.New("string must have at least a major and minor version specified"), - }, - { - input: "123gke", - expected: "", - name: "errors when string does not contain a number as the major version", - expError: errors.New("string must have a number as the major version"), - }, - } - for _, test := range tests { - t.Run(test.name, func(t *testing.T) { - g := NewWithT(t) - - result, err := parseSemver(test.input) - g.Expect(result).To(Equal(test.expected)) - - if test.expError != nil { - g.Expect(err).To(MatchError(test.expError)) - } else { - g.Expect(err).To(BeNil()) - } - }) - } -} From fe07dcf737a8d0500fe64c520e60055e140ee54f Mon Sep 17 00:00:00 2001 From: Benjamin Jee Date: Mon, 11 Mar 2024 17:38:31 -0700 Subject: [PATCH 15/24] Move platform tests to unit test and adjust collector tests --- .../mode/static/telemetry/collector_test.go | 260 +----------------- .../mode/static/telemetry/platform_test.go | 108 ++++++++ 2 files changed, 114 insertions(+), 254 deletions(-) create mode 100644 internal/mode/static/telemetry/platform_test.go diff --git a/internal/mode/static/telemetry/collector_test.go b/internal/mode/static/telemetry/collector_test.go index bb9cfe0802..99d39782b2 100644 --- a/internal/mode/static/telemetry/collector_test.go +++ b/internal/mode/static/telemetry/collector_test.go @@ -340,7 +340,7 @@ var _ = Describe("Collector", Ordered, func() { }) }) - Describe("cluster information collector", func() { + Describe("node count collector", func() { When("collecting node count data", func() { It("collects correct data for one node", func() { k8sClientReader.ListCalls(createListCallsFunc(nodeList)) @@ -371,261 +371,11 @@ var _ = Describe("Collector", Ordered, func() { }) }) }) + }) - When("collecting cluster platform data", func() { - It("collects Kind platform", func() { - nodes := &v1.NodeList{ - Items: []v1.Node{ - { - ObjectMeta: metav1.ObjectMeta{ - Name: "node1", - }, - Spec: v1.NodeSpec{ - ProviderID: "kind://docker/kind/kind-control-plane", - }, - Status: v1.NodeStatus{ - NodeInfo: v1.NodeSystemInfo{ - KubeletVersion: "v1.29.2", - }, - }, - }, - }, - } - - k8sClientReader.ListCalls(createListCallsFunc(nodes)) - expData.ClusterVersion = "1.29.2" - expData.ClusterPlatform = "kind" - - data, err := dataCollector.Collect(ctx) - - Expect(err).To(BeNil()) - Expect(expData).To(Equal(data)) - }) - - It("collects GKE platform", func() { - nodes := &v1.NodeList{ - Items: []v1.Node{ - { - ObjectMeta: metav1.ObjectMeta{ - Name: "node1", - }, - Spec: v1.NodeSpec{ - ProviderID: "gce://test-data/us-central1-c/test-data", - }, - Status: v1.NodeStatus{ - NodeInfo: v1.NodeSystemInfo{ - KubeletVersion: "v1.29.2", - }, - }, - }, - }, - } - - k8sClientReader.ListCalls(createListCallsFunc(nodes)) - expData.ClusterVersion = "1.29.2" - expData.ClusterPlatform = "gke" - - data, err := dataCollector.Collect(ctx) - - Expect(err).To(BeNil()) - Expect(expData).To(Equal(data)) - }) - - It("collects AKS platform", func() { - nodes := &v1.NodeList{ - Items: []v1.Node{ - { - ObjectMeta: metav1.ObjectMeta{ - Name: "node1", - }, - Spec: v1.NodeSpec{ - ProviderID: "azure://test-data/us-central1-c/test-data", - }, - Status: v1.NodeStatus{ - NodeInfo: v1.NodeSystemInfo{ - KubeletVersion: "v1.29.2", - }, - }, - }, - }, - } - - k8sClientReader.ListCalls(createListCallsFunc(nodes)) - expData.ClusterVersion = "1.29.2" - expData.ClusterPlatform = "aks" - - data, err := dataCollector.Collect(ctx) - - Expect(err).To(BeNil()) - Expect(expData).To(Equal(data)) - }) - - It("collects EKS platform", func() { - nodes := &v1.NodeList{ - Items: []v1.Node{ - { - ObjectMeta: metav1.ObjectMeta{ - Name: "node1", - }, - Spec: v1.NodeSpec{ - ProviderID: "aws://test-data/us-central1-c/test-data", - }, - Status: v1.NodeStatus{ - NodeInfo: v1.NodeSystemInfo{ - KubeletVersion: "v1.29.2", - }, - }, - }, - }, - } - - k8sClientReader.ListCalls(createListCallsFunc(nodes)) - expData.ClusterVersion = "1.29.2" - expData.ClusterPlatform = "eks" - - data, err := dataCollector.Collect(ctx) - - Expect(err).To(BeNil()) - Expect(expData).To(Equal(data)) - }) - - It("collects Rancher platform", func() { - namespaceList := &v1.NamespaceList{ - Items: []v1.Namespace{ - { - ObjectMeta: metav1.ObjectMeta{ - Name: "cattle-system", - }, - }, - }, - } - - k8sClientReader.ListCalls(createListCallsFunc(nodeList, namespaceList)) - expData.ClusterVersion = "1.28.6" - expData.ClusterPlatform = "rancher" - - data, err := dataCollector.Collect(ctx) - - Expect(err).To(BeNil()) - Expect(expData).To(Equal(data)) - }) - - It("collects Openshift platform", func() { - nodes := &v1.NodeList{ - Items: []v1.Node{ - { - ObjectMeta: metav1.ObjectMeta{ - Name: "node1", - Labels: map[string]string{"node.openshift.io/os_id": "test"}, - }, - Spec: v1.NodeSpec{ - ProviderID: "k3s://test-data/us-central1-c/test-data", - }, - Status: v1.NodeStatus{ - NodeInfo: v1.NodeSystemInfo{ - KubeletVersion: "v1.29.2", - }, - }, - }, - }, - } - - k8sClientReader.ListCalls(createListCallsFunc(nodes)) - expData.ClusterVersion = "1.29.2" - expData.ClusterPlatform = "openshift" - - data, err := dataCollector.Collect(ctx) - - Expect(err).To(BeNil()) - Expect(expData).To(Equal(data)) - }) - - When("platform is none of the above", func() { - It("marks the platform as 'other_' with whatever was in the providerID's providerName", func() { - nodes := &v1.NodeList{ - Items: []v1.Node{ - { - ObjectMeta: metav1.ObjectMeta{ - Name: "node1", - }, - Spec: v1.NodeSpec{ - ProviderID: "other-cloud-provider://test-here", - }, - Status: v1.NodeStatus{ - NodeInfo: v1.NodeSystemInfo{ - KubeletVersion: "v1.29.2", - }, - }, - }, - }, - } - - k8sClientReader.ListCalls(createListCallsFunc(nodes)) - expData.ClusterVersion = "1.29.2" - expData.ClusterPlatform = "other_other-cloud-provider" - - data, err := dataCollector.Collect(ctx) - - Expect(err).To(BeNil()) - Expect(expData).To(Equal(data)) - }) - It("marks the platform as 'other' when providerID is empty", func() { - nodes := &v1.NodeList{ - Items: []v1.Node{ - { - ObjectMeta: metav1.ObjectMeta{ - Name: "node1", - }, - Status: v1.NodeStatus{ - NodeInfo: v1.NodeSystemInfo{ - KubeletVersion: "v1.29.2", - }, - }, - }, - }, - } - - k8sClientReader.ListCalls(createListCallsFunc(nodes)) - expData.ClusterVersion = "1.29.2" - expData.ClusterPlatform = "other" - - data, err := dataCollector.Collect(ctx) - - Expect(err).To(BeNil()) - Expect(expData).To(Equal(data)) - }) - It("marks the platform as 'other' when providerID is missing '://' separator", func() { - nodes := &v1.NodeList{ - Items: []v1.Node{ - { - ObjectMeta: metav1.ObjectMeta{ - Name: "node1", - }, - Spec: v1.NodeSpec{ - ProviderID: "other-cloud-provider", - }, - Status: v1.NodeStatus{ - NodeInfo: v1.NodeSystemInfo{ - KubeletVersion: "v1.29.2", - }, - }, - }, - }, - } - - k8sClientReader.ListCalls(createListCallsFunc(nodes)) - expData.ClusterVersion = "1.29.2" - expData.ClusterPlatform = "other" - - data, err := dataCollector.Collect(ctx) - - Expect(err).To(BeNil()) - Expect(expData).To(Equal(data)) - }) - }) - }) + Describe("cluster version collector", func() { When("collecting cluster version data", func() { - When("the kublet version is missing", func() { + When("the kubelet version is missing", func() { It("should be report 'unknown'", func() { nodes := &v1.NodeList{ Items: []v1.Node{ @@ -651,7 +401,9 @@ var _ = Describe("Collector", Ordered, func() { }) }) }) + }) + Describe("clusterID collector", func() { When("collecting clusterID data", func() { When("it encounters an error while collecting data", func() { It("should error if the kubernetes client errored when getting the namespace", func() { diff --git a/internal/mode/static/telemetry/platform_test.go b/internal/mode/static/telemetry/platform_test.go new file mode 100644 index 0000000000..794b35ff48 --- /dev/null +++ b/internal/mode/static/telemetry/platform_test.go @@ -0,0 +1,108 @@ +package telemetry + +import ( + "testing" + + . "github.com/onsi/gomega" + v1 "k8s.io/api/core/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" +) + +func TestGetPlatform(t *testing.T) { + tests := []struct { + expectedPlatform string + name string + node v1.Node + namespaces v1.NamespaceList + }{ + { + node: v1.Node{ + Spec: v1.NodeSpec{ + ProviderID: "kind://docker/kind/kind-control-plane", + }, + }, + namespaces: v1.NamespaceList{}, + expectedPlatform: "kind", + name: "kind platform", + }, + { + node: v1.Node{ + Spec: v1.NodeSpec{ + ProviderID: "k3s://ip-172-16-0-210", + }, + }, + namespaces: v1.NamespaceList{}, + expectedPlatform: "k3s", + name: "k3s platform", + }, + { + node: v1.Node{ + Spec: v1.NodeSpec{ + ProviderID: "gce://test-data/us-central1-c/test-data", + }, + }, + namespaces: v1.NamespaceList{}, + expectedPlatform: "gke", + name: "gke platform", + }, + { + node: v1.Node{ + Spec: v1.NodeSpec{ + ProviderID: "azure://test-data/us-central1-c/test-data", + }, + }, + namespaces: v1.NamespaceList{}, + expectedPlatform: "aks", + name: "aks platform", + }, + { + node: v1.Node{ + Spec: v1.NodeSpec{ + ProviderID: "aws://test-data/us-central1-c/test-data", + }, + }, + namespaces: v1.NamespaceList{}, + expectedPlatform: "eks", + name: "eks platform", + }, + { + node: v1.Node{ + Spec: v1.NodeSpec{ + ProviderID: "k3s://ip-172-16-0-210", + }, + }, + namespaces: v1.NamespaceList{ + Items: []v1.Namespace{ + { + ObjectMeta: metav1.ObjectMeta{ + Name: "cattle-system", + }, + }, + }, + }, + expectedPlatform: "rancher", + name: "rancher platform", + }, + { + node: v1.Node{ + ObjectMeta: metav1.ObjectMeta{ + Labels: map[string]string{"node.openshift.io/os_id": "test"}, + }, + Spec: v1.NodeSpec{ + ProviderID: "k3s://ip-172-16-0-210", + }, + }, + namespaces: v1.NamespaceList{}, + expectedPlatform: "openshift", + name: "openshift platform", + }, + } + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + g := NewWithT(t) + + platform := getPlatform(test.node, test.namespaces) + g.Expect(platform).To(Equal(test.expectedPlatform)) + }) + } +} From e9d47b7bf04f2457d2be2b2651005582406b8b06 Mon Sep 17 00:00:00 2001 From: Benjamin Jee Date: Mon, 11 Mar 2024 17:43:31 -0700 Subject: [PATCH 16/24] Move around collectr tests and update test get platform to use pointers --- .../mode/static/telemetry/collector_test.go | 42 +++++++++---------- .../mode/static/telemetry/platform_test.go | 34 +++++++-------- 2 files changed, 38 insertions(+), 38 deletions(-) diff --git a/internal/mode/static/telemetry/collector_test.go b/internal/mode/static/telemetry/collector_test.go index 99d39782b2..df53661eef 100644 --- a/internal/mode/static/telemetry/collector_test.go +++ b/internal/mode/static/telemetry/collector_test.go @@ -340,6 +340,27 @@ var _ = Describe("Collector", Ordered, func() { }) }) + Describe("clusterID collector", func() { + When("collecting clusterID data", func() { + When("it encounters an error while collecting data", func() { + It("should error if the kubernetes client errored when getting the namespace", func() { + expectedError := errors.New("there was an error getting clusterID") + k8sClientReader.GetCalls(mergeGetCallsWithBase( + func(_ context.Context, _ types.NamespacedName, object client.Object, _ ...client.GetOption) error { + switch object.(type) { + case *v1.Namespace: + return expectedError + } + return nil + })) + + _, err := dataCollector.Collect(ctx) + Expect(err).To(MatchError(expectedError)) + }) + }) + }) + }) + Describe("node count collector", func() { When("collecting node count data", func() { It("collects correct data for one node", func() { @@ -403,27 +424,6 @@ var _ = Describe("Collector", Ordered, func() { }) }) - Describe("clusterID collector", func() { - When("collecting clusterID data", func() { - When("it encounters an error while collecting data", func() { - It("should error if the kubernetes client errored when getting the namespace", func() { - expectedError := errors.New("there was an error getting clusterID") - k8sClientReader.GetCalls(mergeGetCallsWithBase( - func(_ context.Context, _ types.NamespacedName, object client.Object, _ ...client.GetOption) error { - switch object.(type) { - case *v1.Namespace: - return expectedError - } - return nil - })) - - _, err := dataCollector.Collect(ctx) - Expect(err).To(MatchError(expectedError)) - }) - }) - }) - }) - Describe("NGF resource count collector", func() { var ( graph1 *graph.Graph diff --git a/internal/mode/static/telemetry/platform_test.go b/internal/mode/static/telemetry/platform_test.go index 794b35ff48..9ca82eee14 100644 --- a/internal/mode/static/telemetry/platform_test.go +++ b/internal/mode/static/telemetry/platform_test.go @@ -10,68 +10,68 @@ import ( func TestGetPlatform(t *testing.T) { tests := []struct { + node *v1.Node + namespaces *v1.NamespaceList expectedPlatform string name string - node v1.Node - namespaces v1.NamespaceList }{ { - node: v1.Node{ + node: &v1.Node{ Spec: v1.NodeSpec{ ProviderID: "kind://docker/kind/kind-control-plane", }, }, - namespaces: v1.NamespaceList{}, + namespaces: &v1.NamespaceList{}, expectedPlatform: "kind", name: "kind platform", }, { - node: v1.Node{ + node: &v1.Node{ Spec: v1.NodeSpec{ ProviderID: "k3s://ip-172-16-0-210", }, }, - namespaces: v1.NamespaceList{}, + namespaces: &v1.NamespaceList{}, expectedPlatform: "k3s", name: "k3s platform", }, { - node: v1.Node{ + node: &v1.Node{ Spec: v1.NodeSpec{ ProviderID: "gce://test-data/us-central1-c/test-data", }, }, - namespaces: v1.NamespaceList{}, + namespaces: &v1.NamespaceList{}, expectedPlatform: "gke", name: "gke platform", }, { - node: v1.Node{ + node: &v1.Node{ Spec: v1.NodeSpec{ ProviderID: "azure://test-data/us-central1-c/test-data", }, }, - namespaces: v1.NamespaceList{}, + namespaces: &v1.NamespaceList{}, expectedPlatform: "aks", name: "aks platform", }, { - node: v1.Node{ + node: &v1.Node{ Spec: v1.NodeSpec{ ProviderID: "aws://test-data/us-central1-c/test-data", }, }, - namespaces: v1.NamespaceList{}, + namespaces: &v1.NamespaceList{}, expectedPlatform: "eks", name: "eks platform", }, { - node: v1.Node{ + node: &v1.Node{ Spec: v1.NodeSpec{ ProviderID: "k3s://ip-172-16-0-210", }, }, - namespaces: v1.NamespaceList{ + namespaces: &v1.NamespaceList{ Items: []v1.Namespace{ { ObjectMeta: metav1.ObjectMeta{ @@ -84,7 +84,7 @@ func TestGetPlatform(t *testing.T) { name: "rancher platform", }, { - node: v1.Node{ + node: &v1.Node{ ObjectMeta: metav1.ObjectMeta{ Labels: map[string]string{"node.openshift.io/os_id": "test"}, }, @@ -92,7 +92,7 @@ func TestGetPlatform(t *testing.T) { ProviderID: "k3s://ip-172-16-0-210", }, }, - namespaces: v1.NamespaceList{}, + namespaces: &v1.NamespaceList{}, expectedPlatform: "openshift", name: "openshift platform", }, @@ -101,7 +101,7 @@ func TestGetPlatform(t *testing.T) { t.Run(test.name, func(t *testing.T) { g := NewWithT(t) - platform := getPlatform(test.node, test.namespaces) + platform := getPlatform(*test.node, *test.namespaces) g.Expect(platform).To(Equal(test.expectedPlatform)) }) } From 6380a63557c878e4846c917809cc59d164ff49db Mon Sep 17 00:00:00 2001 From: Benjamin Jee Date: Mon, 11 Mar 2024 17:46:23 -0700 Subject: [PATCH 17/24] Add more cases to platform test --- internal/mode/static/telemetry/platform_test.go | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/internal/mode/static/telemetry/platform_test.go b/internal/mode/static/telemetry/platform_test.go index 9ca82eee14..98127111bf 100644 --- a/internal/mode/static/telemetry/platform_test.go +++ b/internal/mode/static/telemetry/platform_test.go @@ -96,6 +96,22 @@ func TestGetPlatform(t *testing.T) { expectedPlatform: "openshift", name: "openshift platform", }, + { + node: &v1.Node{ + Spec: v1.NodeSpec{ + ProviderID: "different-platform://ip-172-16-0-210", + }, + }, + namespaces: &v1.NamespaceList{}, + expectedPlatform: "other_different-platform", + name: "other platform", + }, + { + node: &v1.Node{}, + namespaces: &v1.NamespaceList{}, + expectedPlatform: "other", + name: "missing providerID", + }, } for _, test := range tests { t.Run(test.name, func(t *testing.T) { From 9e9bcb793675e74a2cbaebc45689570a32bf9661 Mon Sep 17 00:00:00 2001 From: Benjamin Jee Date: Tue, 12 Mar 2024 15:56:40 -0700 Subject: [PATCH 18/24] Adjust collector and platform tests --- .../mode/static/telemetry/collector_test.go | 104 ++++++++++++------ .../mode/static/telemetry/platform_test.go | 17 ++- 2 files changed, 86 insertions(+), 35 deletions(-) diff --git a/internal/mode/static/telemetry/collector_test.go b/internal/mode/static/telemetry/collector_test.go index df53661eef..cd5ae9cdaf 100644 --- a/internal/mode/static/telemetry/collector_test.go +++ b/internal/mode/static/telemetry/collector_test.go @@ -213,6 +213,19 @@ var _ = Describe("Collector", Ordered, func() { } } + mergeListCallsWithBase := func(f listCallsFunc) listCallsFunc { + return func( + ctx context.Context, + object client.ObjectList, + option ...client.ListOption, + ) error { + err := baseListCalls(ctx, object, option...) + Expect(err).ToNot(HaveOccurred()) + + return f(ctx, object, option...) + } + } + Describe("Normal case", func() { When("collecting telemetry data", func() { It("collects all fields", func() { @@ -340,7 +353,26 @@ var _ = Describe("Collector", Ordered, func() { }) }) - Describe("clusterID collector", func() { + Describe("cluster information collector", func() { + When("collecting cluster platform", func() { + When("it encounters an error while collecting data", func() { + It("should error if the kubernetes client errored when getting the NamespaceList", func() { + expectedError := errors.New("failed to get NamespaceList") + k8sClientReader.ListCalls(mergeListCallsWithBase( + func(_ context.Context, object client.ObjectList, _ ...client.ListOption) error { + switch object.(type) { + case *v1.NamespaceList: + return expectedError + } + return nil + })) + + _, err := dataCollector.Collect(ctx) + Expect(err).To(MatchError(expectedError)) + }) + }) + }) + When("collecting clusterID data", func() { When("it encounters an error while collecting data", func() { It("should error if the kubernetes client errored when getting the namespace", func() { @@ -359,6 +391,34 @@ var _ = Describe("Collector", Ordered, func() { }) }) }) + + When("collecting cluster version data", func() { + When("the kubelet version is missing", func() { + It("should be report 'unknown'", func() { + nodes := &v1.NodeList{ + Items: []v1.Node{ + { + ObjectMeta: metav1.ObjectMeta{ + Name: "node1", + }, + Spec: v1.NodeSpec{ + ProviderID: "k3s://ip-172-16-0-210", + }, + }, + }, + } + + k8sClientReader.ListCalls(createListCallsFunc(nodes)) + expData.ClusterVersion = "unknown" + expData.ClusterPlatform = "k3s" + + data, err := dataCollector.Collect(ctx) + + Expect(err).To(BeNil()) + Expect(expData).To(Equal(data)) + }) + }) + }) }) Describe("node count collector", func() { @@ -383,9 +443,17 @@ var _ = Describe("Collector", Ordered, func() { Expect(err).To(MatchError(expectedError)) }) + It("should error on kubernetes client api errors", func() { - expectedError := errors.New("there was an error getting NodeList") - k8sClientReader.ListReturns(expectedError) + expectedError := errors.New("failed to get NodeList") + k8sClientReader.ListCalls( + func(_ context.Context, object client.ObjectList, _ ...client.ListOption) error { + switch object.(type) { + case *v1.NodeList: + return expectedError + } + return nil + }) _, err := dataCollector.Collect(ctx) Expect(err).To(MatchError(expectedError)) @@ -394,36 +462,6 @@ var _ = Describe("Collector", Ordered, func() { }) }) - Describe("cluster version collector", func() { - When("collecting cluster version data", func() { - When("the kubelet version is missing", func() { - It("should be report 'unknown'", func() { - nodes := &v1.NodeList{ - Items: []v1.Node{ - { - ObjectMeta: metav1.ObjectMeta{ - Name: "node1", - }, - Spec: v1.NodeSpec{ - ProviderID: "k3s://ip-172-16-0-210", - }, - }, - }, - } - - k8sClientReader.ListCalls(createListCallsFunc(nodes)) - expData.ClusterVersion = "unknown" - expData.ClusterPlatform = "k3s" - - data, err := dataCollector.Collect(ctx) - - Expect(err).To(BeNil()) - Expect(expData).To(Equal(data)) - }) - }) - }) - }) - Describe("NGF resource count collector", func() { var ( graph1 *graph.Graph diff --git a/internal/mode/static/telemetry/platform_test.go b/internal/mode/static/telemetry/platform_test.go index 98127111bf..0c2939d820 100644 --- a/internal/mode/static/telemetry/platform_test.go +++ b/internal/mode/static/telemetry/platform_test.go @@ -81,7 +81,7 @@ func TestGetPlatform(t *testing.T) { }, }, expectedPlatform: "rancher", - name: "rancher platform", + name: "rancher platform on k3s", }, { node: &v1.Node{ @@ -94,7 +94,20 @@ func TestGetPlatform(t *testing.T) { }, namespaces: &v1.NamespaceList{}, expectedPlatform: "openshift", - name: "openshift platform", + name: "openshift platform on k3s", + }, + { + node: &v1.Node{ + ObjectMeta: metav1.ObjectMeta{ + Labels: map[string]string{"node.openshift.io/os_id": "test"}, + }, + Spec: v1.NodeSpec{ + ProviderID: "aws://test-data/us-central1-c/test-data", + }, + }, + namespaces: &v1.NamespaceList{}, + expectedPlatform: "openshift", + name: "openshift platform on aws", }, { node: &v1.Node{ From 3dbe990d441803e770891350d64e4d51129610d8 Mon Sep 17 00:00:00 2001 From: Benjamin Jee Date: Wed, 13 Mar 2024 09:32:58 -0700 Subject: [PATCH 19/24] Move CollectNodeCount to original location --- internal/mode/static/telemetry/collector.go | 20 ++++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-) diff --git a/internal/mode/static/telemetry/collector.go b/internal/mode/static/telemetry/collector.go index beaa3a73f0..18c86bad58 100644 --- a/internal/mode/static/telemetry/collector.go +++ b/internal/mode/static/telemetry/collector.go @@ -154,6 +154,16 @@ func (c DataCollectorImpl) Collect(ctx context.Context) (Data, error) { return data, nil } +// CollectNodeCount returns the number of nodes in the cluster. +func CollectNodeCount(ctx context.Context, k8sClient client.Reader) (int, error) { + var nodes v1.NodeList + if err := k8sClient.List(ctx, &nodes); err != nil { + return 0, fmt.Errorf("failed to get NodeList: %w", err) + } + + return len(nodes.Items), nil +} + func collectGraphResourceCount( graphGetter GraphGetter, configurationGetter ConfigurationGetter, @@ -264,16 +274,6 @@ func CollectClusterID(ctx context.Context, k8sClient client.Reader) (string, err return string(kubeNamespace.GetUID()), nil } -// CollectNodeCount returns the number of nodes in the cluster. -func CollectNodeCount(ctx context.Context, k8sClient client.Reader) (int, error) { - var nodes v1.NodeList - if err := k8sClient.List(ctx, &nodes); err != nil { - return 0, fmt.Errorf("failed to get NodeList: %w", err) - } - - return len(nodes.Items), nil -} - type clusterInformation struct { Platform string Version string From 2fe1f72470106a1f9cf9bdd2d1eac53e44c00aee Mon Sep 17 00:00:00 2001 From: Benjamin Jee Date: Wed, 13 Mar 2024 09:35:31 -0700 Subject: [PATCH 20/24] Small line fix --- internal/mode/static/telemetry/collector.go | 1 - 1 file changed, 1 deletion(-) diff --git a/internal/mode/static/telemetry/collector.go b/internal/mode/static/telemetry/collector.go index 18c86bad58..f3360987d2 100644 --- a/internal/mode/static/telemetry/collector.go +++ b/internal/mode/static/telemetry/collector.go @@ -302,7 +302,6 @@ func collectClusterInformation(ctx context.Context, k8sClient client.Reader) (cl clusterInfo.Version = "unknown" kubeletVersion := node.Status.NodeInfo.KubeletVersion - if version, err := version2.ParseGeneric(kubeletVersion); err == nil { clusterInfo.Version = version.String() } From 925d736b7e5a6626df9f1db8f717075d3a54832a Mon Sep 17 00:00:00 2001 From: Benjamin Jee Date: Wed, 13 Mar 2024 10:00:31 -0700 Subject: [PATCH 21/24] Add review feedback --- internal/mode/static/telemetry/collector.go | 23 +++++++++++---------- 1 file changed, 12 insertions(+), 11 deletions(-) diff --git a/internal/mode/static/telemetry/collector.go b/internal/mode/static/telemetry/collector.go index f3360987d2..bfa7f45539 100644 --- a/internal/mode/static/telemetry/collector.go +++ b/internal/mode/static/telemetry/collector.go @@ -11,7 +11,7 @@ import ( v1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/types" - version2 "k8s.io/apimachinery/pkg/util/version" + k8sversion "k8s.io/apimachinery/pkg/util/version" "sigs.k8s.io/controller-runtime/pkg/client" "github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/config" @@ -282,27 +282,21 @@ type clusterInformation struct { func collectClusterInformation(ctx context.Context, k8sClient client.Reader) (clusterInformation, error) { var clusterInfo clusterInformation - var err error var nodes v1.NodeList - if err = k8sClient.List(ctx, &nodes); err != nil { + if err := k8sClient.List(ctx, &nodes); err != nil { return clusterInformation{}, fmt.Errorf("failed to get NodeList: %w", err) } if len(nodes.Items) == 0 { return clusterInformation{}, errors.New("failed to collect cluster information: NodeList length is zero") } - - var clusterID string - if clusterID, err = CollectClusterID(ctx, k8sClient); err != nil { - return clusterInformation{}, fmt.Errorf("failed to collect cluster information: %w", err) - } - clusterInfo.ClusterID = clusterID - node := nodes.Items[0] + var version *k8sversion.Version clusterInfo.Version = "unknown" kubeletVersion := node.Status.NodeInfo.KubeletVersion - if version, err := version2.ParseGeneric(kubeletVersion); err == nil { + version, err := k8sversion.ParseGeneric(kubeletVersion) + if err == nil { clusterInfo.Version = version.String() } @@ -313,5 +307,12 @@ func collectClusterInformation(ctx context.Context, k8sClient client.Reader) (cl clusterInfo.Platform = getPlatform(node, namespaces) + var clusterID string + clusterID, err = CollectClusterID(ctx, k8sClient) + if err != nil { + return clusterInformation{}, fmt.Errorf("failed to collect cluster information: %w", err) + } + clusterInfo.ClusterID = clusterID + return clusterInfo, nil } From 1609e8045278c5384350dae90347d36b488a3267 Mon Sep 17 00:00:00 2001 From: Benjamin Jee Date: Wed, 13 Mar 2024 11:07:35 -0700 Subject: [PATCH 22/24] Add more review feedback --- internal/mode/static/telemetry/collector.go | 1 - internal/mode/static/telemetry/platform.go | 24 ++++++++++----------- 2 files changed, 12 insertions(+), 13 deletions(-) diff --git a/internal/mode/static/telemetry/collector.go b/internal/mode/static/telemetry/collector.go index bfa7f45539..17fc8f1fed 100644 --- a/internal/mode/static/telemetry/collector.go +++ b/internal/mode/static/telemetry/collector.go @@ -292,7 +292,6 @@ func collectClusterInformation(ctx context.Context, k8sClient client.Reader) (cl } node := nodes.Items[0] - var version *k8sversion.Version clusterInfo.Version = "unknown" kubeletVersion := node.Status.NodeInfo.KubeletVersion version, err := k8sversion.ParseGeneric(kubeletVersion) diff --git a/internal/mode/static/telemetry/platform.go b/internal/mode/static/telemetry/platform.go index b1edd12f75..858c951a21 100644 --- a/internal/mode/static/telemetry/platform.go +++ b/internal/mode/static/telemetry/platform.go @@ -11,14 +11,14 @@ type k8sState struct { namespaces v1.NamespaceList } -type platformExtractor func(k8sState) (string, bool) +type platformExtractor func(k8sState) string -func buildProviderIDExtractor(id string, platform string) platformExtractor { - return func(state k8sState) (string, bool) { +func buildProviderIDExtractor(id, platform string) platformExtractor { + return func(state k8sState) string { if strings.HasPrefix(state.node.Spec.ProviderID, id) { - return platform, true + return platform } - return "", false + return "" } } @@ -59,7 +59,7 @@ func getPlatform(node v1.Node, namespaces v1.NamespaceList) string { } for _, extractor := range platformExtractors { - if platform, ok := extractor(state); ok { + if platform := extractor(state); platform != "" { return platform } } @@ -67,24 +67,24 @@ func getPlatform(node v1.Node, namespaces v1.NamespaceList) string { return unknownProviderIDExtractor(state) } -func openShiftExtractor(state k8sState) (string, bool) { +func openShiftExtractor(state k8sState) string { // openshift platform won't show up in node's ProviderID if value, ok := state.node.Labels[openshiftIdentifier]; ok && value != "" { - return platformOpenShift, true + return platformOpenShift } - return "", false + return "" } -func rancherExtractor(state k8sState) (string, bool) { +func rancherExtractor(state k8sState) string { // rancher platform won't show up in the node's ProviderID for _, ns := range state.namespaces.Items { if ns.Name == rancherIdentifier { - return platformRancher, true + return platformRancher } } - return "", false + return "" } func unknownProviderIDExtractor(state k8sState) string { From 006d4ddb30324f6d14a1d8b518ab3eef08063357 Mon Sep 17 00:00:00 2001 From: Benjamin Jee Date: Wed, 13 Mar 2024 12:11:40 -0700 Subject: [PATCH 23/24] Add more feedback from reviews --- internal/mode/static/telemetry/collector.go | 5 +++-- internal/mode/static/telemetry/platform.go | 2 +- 2 files changed, 4 insertions(+), 3 deletions(-) diff --git a/internal/mode/static/telemetry/collector.go b/internal/mode/static/telemetry/collector.go index 17fc8f1fed..757e2528be 100644 --- a/internal/mode/static/telemetry/collector.go +++ b/internal/mode/static/telemetry/collector.go @@ -292,10 +292,11 @@ func collectClusterInformation(ctx context.Context, k8sClient client.Reader) (cl } node := nodes.Items[0] - clusterInfo.Version = "unknown" kubeletVersion := node.Status.NodeInfo.KubeletVersion version, err := k8sversion.ParseGeneric(kubeletVersion) - if err == nil { + if err != nil { + clusterInfo.Version = "unknown" + } else { clusterInfo.Version = version.String() } diff --git a/internal/mode/static/telemetry/platform.go b/internal/mode/static/telemetry/platform.go index 858c951a21..ff63117c70 100644 --- a/internal/mode/static/telemetry/platform.go +++ b/internal/mode/static/telemetry/platform.go @@ -69,7 +69,7 @@ func getPlatform(node v1.Node, namespaces v1.NamespaceList) string { func openShiftExtractor(state k8sState) string { // openshift platform won't show up in node's ProviderID - if value, ok := state.node.Labels[openshiftIdentifier]; ok && value != "" { + if state.node.Labels[openshiftIdentifier] != "" { return platformOpenShift } From 3550249fca93a4791296c8eacc933bf32bc519a5 Mon Sep 17 00:00:00 2001 From: Benjamin Jee Date: Wed, 13 Mar 2024 14:21:39 -0700 Subject: [PATCH 24/24] Add nodecount to cluster information --- internal/mode/static/telemetry/collector.go | 24 +++++---------- internal/mode/static/usage/job_worker.go | 13 ++++++++- internal/mode/static/usage/job_worker_test.go | 29 +++++++++++++++++++ 3 files changed, 48 insertions(+), 18 deletions(-) diff --git a/internal/mode/static/telemetry/collector.go b/internal/mode/static/telemetry/collector.go index 757e2528be..e400999070 100644 --- a/internal/mode/static/telemetry/collector.go +++ b/internal/mode/static/telemetry/collector.go @@ -108,11 +108,6 @@ func (c DataCollectorImpl) Collect(ctx context.Context) (Data, error) { return Data{}, fmt.Errorf("failed to collect cluster information: %w", err) } - nodeCount, err := CollectNodeCount(ctx, c.cfg.K8sClientReader) - if err != nil { - return Data{}, fmt.Errorf("failed to collect node count: %w", err) - } - graphResourceCount, err := collectGraphResourceCount(c.cfg.GraphGetter, c.cfg.ConfigurationGetter) if err != nil { return Data{}, fmt.Errorf("failed to collect NGF resource counts: %w", err) @@ -142,7 +137,7 @@ func (c DataCollectorImpl) Collect(ctx context.Context) (Data, error) { ClusterVersion: clusterInfo.Version, ClusterPlatform: clusterInfo.Platform, InstallationID: deploymentID, - ClusterNodeCount: int64(nodeCount), + ClusterNodeCount: int64(clusterInfo.NodeCount), }, NGFResourceCounts: graphResourceCount, ImageSource: c.cfg.ImageSource, @@ -154,16 +149,6 @@ func (c DataCollectorImpl) Collect(ctx context.Context) (Data, error) { return data, nil } -// CollectNodeCount returns the number of nodes in the cluster. -func CollectNodeCount(ctx context.Context, k8sClient client.Reader) (int, error) { - var nodes v1.NodeList - if err := k8sClient.List(ctx, &nodes); err != nil { - return 0, fmt.Errorf("failed to get NodeList: %w", err) - } - - return len(nodes.Items), nil -} - func collectGraphResourceCount( graphGetter GraphGetter, configurationGetter ConfigurationGetter, @@ -278,6 +263,7 @@ type clusterInformation struct { Platform string Version string ClusterID string + NodeCount int } func collectClusterInformation(ctx context.Context, k8sClient client.Reader) (clusterInformation, error) { @@ -287,9 +273,13 @@ func collectClusterInformation(ctx context.Context, k8sClient client.Reader) (cl if err := k8sClient.List(ctx, &nodes); err != nil { return clusterInformation{}, fmt.Errorf("failed to get NodeList: %w", err) } - if len(nodes.Items) == 0 { + + nodeCount := len(nodes.Items) + if nodeCount == 0 { return clusterInformation{}, errors.New("failed to collect cluster information: NodeList length is zero") } + clusterInfo.NodeCount = nodeCount + node := nodes.Items[0] kubeletVersion := node.Status.NodeInfo.KubeletVersion diff --git a/internal/mode/static/usage/job_worker.go b/internal/mode/static/usage/job_worker.go index 3f039181e3..f71fdf64fb 100644 --- a/internal/mode/static/usage/job_worker.go +++ b/internal/mode/static/usage/job_worker.go @@ -6,6 +6,7 @@ import ( "github.com/go-logr/logr" appsv1 "k8s.io/api/apps/v1" + v1 "k8s.io/api/core/v1" "sigs.k8s.io/controller-runtime/pkg/client" "github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/config" @@ -19,7 +20,7 @@ func CreateUsageJobWorker( cfg config.Config, ) func(ctx context.Context) { return func(ctx context.Context) { - nodeCount, err := telemetry.CollectNodeCount(ctx, k8sClient) + nodeCount, err := CollectNodeCount(ctx, k8sClient) if err != nil { logger.Error(err, "Failed to collect node count") } @@ -79,3 +80,13 @@ func GetTotalNGFPodCount(ctx context.Context, k8sClient client.Reader) (int, err return count, nil } + +// CollectNodeCount returns the number of nodes in the cluster. +func CollectNodeCount(ctx context.Context, k8sClient client.Reader) (int, error) { + var nodes v1.NodeList + if err := k8sClient.List(ctx, &nodes); err != nil { + return 0, fmt.Errorf("failed to get NodeList: %w", err) + } + + return len(nodes.Items), nil +} diff --git a/internal/mode/static/usage/job_worker_test.go b/internal/mode/static/usage/job_worker_test.go index d7272ae28a..d0c9270a6f 100644 --- a/internal/mode/static/usage/job_worker_test.go +++ b/internal/mode/static/usage/job_worker_test.go @@ -156,3 +156,32 @@ func TestGetTotalNGFPodCount(t *testing.T) { g.Expect(err).ToNot(HaveOccurred()) g.Expect(count).To(Equal(expCount)) } + +func TestCollectNodeCount(t *testing.T) { + g := NewWithT(t) + + node1 := &v1.Node{ + ObjectMeta: metav1.ObjectMeta{ + Name: "node1", + }, + Spec: v1.NodeSpec{ + ProviderID: "k3s://ip-172-16-0-210", + }, + } + + node2 := &v1.Node{ + ObjectMeta: metav1.ObjectMeta{ + Name: "node2", + }, + Spec: v1.NodeSpec{ + ProviderID: "k3s://ip-172-16-0-210", + }, + } + + k8sClient := fake.NewFakeClient(node1, node2) + + expCount := 2 + count, err := usage.CollectNodeCount(context.Background(), k8sClient) + g.Expect(err).ToNot(HaveOccurred()) + g.Expect(count).To(Equal(expCount)) +}