Fix webhook conversion erasing nil includeList

netobserv · Nov 17, 2023 · 7d73a56 · 7d73a56
1 parent 27335aa
commit 7d73a56
Show file tree

Hide file tree

Showing 5 changed files with 43 additions and 17 deletions.
diff --git a/api/v1alpha1/flowcollector_webhook.go b/api/v1alpha1/flowcollector_webhook.go
@@ -184,7 +184,6 @@ func Convert_v1beta2_ServerTLS_To_v1alpha1_ServerTLS(in *v1beta2.ServerTLS, out
 // we have new defined fields in v1beta2 not in v1beta1
 // nolint:golint,stylecheck,revive
 func Convert_v1alpha1_FLPMetrics_To_v1beta2_FLPMetrics(in *FLPMetrics, out *v1beta2.FLPMetrics, s apiconversion.Scope) error {
-	includeList := metrics.GetEnabledNames(in.IgnoreTags, nil)
-	out.IncludeList = &includeList
+	out.IncludeList = metrics.GetAsIncludeList(in.IgnoreTags, nil)
 	return autoConvert_v1alpha1_FLPMetrics_To_v1beta2_FLPMetrics(in, out, s)
 }
diff --git a/api/v1beta1/flowcollector_webhook.go b/api/v1beta1/flowcollector_webhook.go
@@ -139,7 +139,6 @@ func Convert_v1beta1_FLPMetrics_To_v1beta2_FLPMetrics(in *FLPMetrics, out *v1bet
 	if err != nil {
 		return err
 	}
-	includeList := metrics.GetEnabledNames(in.IgnoreTags, in.IncludeList)
-	out.IncludeList = &includeList
+	out.IncludeList = metrics.GetAsIncludeList(in.IgnoreTags, in.IncludeList)
 	return nil
 }
diff --git a/api/v1beta1/flowcollector_webhook_test.go b/api/v1beta1/flowcollector_webhook_test.go
@@ -159,3 +159,30 @@ func TestBeta2ConversionRoundtrip_Metrics(t *testing.T) {
 	assert.NoError(err)
 	assert.Equal(initial.Spec.Processor.Metrics, back.Spec.Processor.Metrics)
 }
+
+func TestBeta2ConversionRoundtrip_Metrics_Default(t *testing.T) {
+	// Testing beta2 -> beta1 -> beta2
+	assert := assert.New(t)
+
+	initial := v1beta2.FlowCollector{
+		Spec: v1beta2.FlowCollectorSpec{
+			Processor: v1beta2.FlowCollectorFLP{
+				Metrics: v1beta2.FLPMetrics{
+					DisableAlerts: []v1beta2.FLPAlert{v1beta2.AlertLokiError},
+				},
+			},
+		},
+	}
+
+	var converted FlowCollector
+	err := converted.ConvertFrom(&initial)
+	assert.NoError(err)
+
+	assert.Empty(converted.Spec.Processor.Metrics.IgnoreTags)
+	assert.Nil(converted.Spec.Processor.Metrics.IncludeList)
+
+	var back v1beta2.FlowCollector
+	err = converted.ConvertTo(&back)
+	assert.NoError(err)
+	assert.Nil(back.Spec.Processor.Metrics.IncludeList)
+}
diff --git a/pkg/metrics/predefined_metrics.go b/pkg/metrics/predefined_metrics.go
@@ -165,14 +165,15 @@ func convertIgnoreTagsToIncludeList(ignoreTags []string) []string {
 	return ret
 }
 
-func GetEnabledNames(ignoreTags []string, includeList *[]string) []string {
-	if includeList == nil {
+func GetAsIncludeList(ignoreTags []string, includeList *[]string) *[]string {
+	if includeList == nil && len(ignoreTags) > 0 {
 		if reflect.DeepEqual(ignoreTags, defaultIgnoreTags1_4) {
-			return DefaultIncludeList
+			return nil
 		}
-		return convertIgnoreTagsToIncludeList(ignoreTags)
+		converted := convertIgnoreTagsToIncludeList(ignoreTags)
+		return &converted
 	}
-	return *includeList
+	return includeList
 }
 
 func GetAllNames() []string {

diff --git a/pkg/metrics/predefined_metrics_test.go b/pkg/metrics/predefined_metrics_test.go
@@ -10,7 +10,7 @@ func TestIncludeExclude(t *testing.T) {
 	assert := assert.New(t)
 
 	// IgnoreTags set, Include list unset => resolving ignore tags
-	res := GetEnabledNames([]string{"egress", "packets", "flows"}, nil)
+	res := GetAsIncludeList([]string{"egress", "packets", "flows"}, nil)
 	assert.Equal([]string{
 		"node_ingress_bytes_total",
 		"node_rtt_seconds",
@@ -24,19 +24,19 @@ func TestIncludeExclude(t *testing.T) {
 		"workload_rtt_seconds",
 		"workload_drop_bytes_total",
 		"workload_dns_latency_seconds",
-	}, res)
+	}, *res)
 
 	// IgnoreTags set, Include list set => keep include list
-	res = GetEnabledNames([]string{"egress", "packets"}, &[]string{"namespace_flows_total"})
-	assert.Equal([]string{"namespace_flows_total"}, res)
+	res = GetAsIncludeList([]string{"egress", "packets"}, &[]string{"namespace_flows_total"})
+	assert.Equal([]string{"namespace_flows_total"}, *res)
 
 	// IgnoreTags set as defaults, Include list unset => use default include list
-	res = GetEnabledNames([]string{"egress", "packets", "nodes-flows", "namespaces-flows", "workloads-flows", "namespaces"}, nil)
-	assert.Equal(DefaultIncludeList, res)
+	res = GetAsIncludeList([]string{"egress", "packets", "nodes-flows", "namespaces-flows", "workloads-flows", "namespaces"}, nil)
+	assert.Nil(res)
 
 	// IgnoreTags set as defaults, Include list set => use include list
-	res = GetEnabledNames([]string{"egress", "packets", "nodes-flows", "namespaces-flows", "workloads-flows", "namespaces"}, &[]string{"namespace_flows_total"})
-	assert.Equal([]string{"namespace_flows_total"}, res)
+	res = GetAsIncludeList([]string{"egress", "packets", "nodes-flows", "namespaces-flows", "workloads-flows", "namespaces"}, &[]string{"namespace_flows_total"})
+	assert.Equal([]string{"namespace_flows_total"}, *res)
 }
 
 func TestGetDefinitions(t *testing.T) {