From 4411df241f8653b6fe964a245807a583bc04b11d Mon Sep 17 00:00:00 2001 From: Benjamin Jee Date: Fri, 8 Mar 2024 11:16:49 -0800 Subject: [PATCH] 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 }