Merge pull request #893 from kubecost/niko/labelfix

Fix label alias aggregation on sanitized labels

pkg/kubecost/allocation.go

@@ -1463,7 +1463,7 @@ func (a *Allocation) generateKey(aggregateBy []string, labelConfig *LabelConfig)
 			if labels == nil {
 				names = append(names, UnallocatedSuffix)
 			} else {
-				labelName := strings.TrimPrefix(agg, "label:")
+				labelName := labelConfig.Sanitize(strings.TrimPrefix(agg, "label:"))
 				if labelValue, ok := labels[labelName]; ok {
 					names = append(names, fmt.Sprintf("%s=%s", labelName, labelValue))
 				} else {
@@ -1475,7 +1475,7 @@ func (a *Allocation) generateKey(aggregateBy []string, labelConfig *LabelConfig)
 			if annotations == nil {
 				names = append(names, UnallocatedSuffix)
 			} else {
-				annotationName := strings.TrimPrefix(agg, "annotation:")
+				annotationName := labelConfig.Sanitize(strings.TrimPrefix(agg, "annotation:"))
 				if annotationValue, ok := annotations[annotationName]; ok {
 					names = append(names, fmt.Sprintf("%s=%s", annotationName, annotationValue))
 				} else {
@@ -1487,7 +1487,7 @@ func (a *Allocation) generateKey(aggregateBy []string, labelConfig *LabelConfig)
 			if labels == nil {
 				names = append(names, UnallocatedSuffix)
 			} else {
-				labelName := labelConfig.DepartmentLabel
+				labelName := labelConfig.Sanitize(labelConfig.DepartmentLabel)
 				if labelValue, ok := labels[labelName]; ok {
 					names = append(names, labelValue)
 				} else {
@@ -1499,7 +1499,7 @@ func (a *Allocation) generateKey(aggregateBy []string, labelConfig *LabelConfig)
 			if labels == nil {
 				names = append(names, UnallocatedSuffix)
 			} else {
-				labelName := labelConfig.EnvironmentLabel
+				labelName := labelConfig.Sanitize(labelConfig.EnvironmentLabel)
 				if labelValue, ok := labels[labelName]; ok {
 					names = append(names, labelValue)
 				} else {
@@ -1511,7 +1511,7 @@ func (a *Allocation) generateKey(aggregateBy []string, labelConfig *LabelConfig)
 			if labels == nil {
 				names = append(names, UnallocatedSuffix)
 			} else {
-				labelName := labelConfig.OwnerLabel
+				labelName := labelConfig.Sanitize(labelConfig.OwnerLabel)
 				if labelValue, ok := labels[labelName]; ok {
 					names = append(names, labelValue)
 				} else {
@@ -1523,7 +1523,7 @@ func (a *Allocation) generateKey(aggregateBy []string, labelConfig *LabelConfig)
 			if labels == nil {
 				names = append(names, UnallocatedSuffix)
 			} else {
-				labelName := labelConfig.ProductLabel
+				labelName := labelConfig.Sanitize(labelConfig.ProductLabel)
 				if labelValue, ok := labels[labelName]; ok {
 					names = append(names, labelValue)
 				} else {
@@ -1535,7 +1535,7 @@ func (a *Allocation) generateKey(aggregateBy []string, labelConfig *LabelConfig)
 			if labels == nil {
 				names = append(names, UnallocatedSuffix)
 			} else {
-				labelName := labelConfig.TeamLabel
+				labelName := labelConfig.Sanitize(labelConfig.TeamLabel)
 				if labelValue, ok := labels[labelName]; ok {
 					names = append(names, labelValue)
 				} else {
@@ -1553,18 +1553,6 @@ func (a *Allocation) generateKey(aggregateBy []string, labelConfig *LabelConfig)
 	return strings.Join(names, "/")
 }
 
-// TODO:CLEANUP get rid of this
-// Helper function to check for slice membership. Not sure if repeated elsewhere in our codebase.
-func indexOf(v string, arr []string) int {
-	for i, s := range arr {
-		// This is caseless equivalence
-		if strings.EqualFold(v, s) {
-			return i
-		}
-	}
-	return -1
-}
-
 // Clone returns a new AllocationSet with a deep copy of the given
 // AllocationSet's allocations.
 func (as *AllocationSet) Clone() *AllocationSet {

pkg/kubecost/allocation_test.go

@@ -503,6 +503,37 @@ func TestAllocationSet_generateKey(t *testing.T) {
 	if key != "dept1/envt1/ownr1/prod1/team1" {
 		t.Fatalf("generateKey: expected \"dept1/envt1/ownr1/prod1/team1\"; actual \"%s\"", key)
 	}
+
+	// Ensure that labels with illegal Prometheus characters in LabelConfig
+	// still match their sanitized values.
+
+	labelConfig.DepartmentLabel = "prom/illegal-department"
+	labelConfig.EnvironmentLabel = " env "
+	labelConfig.OwnerLabel = "$owner%"
+	labelConfig.ProductLabel = "app.kubernetes.io/app"
+
+	alloc.Properties = &AllocationProperties{
+		Cluster:   "cluster1",
+		Namespace: "namespace1",
+		Labels: map[string]string{
+			"prom_illegal_department": "dept1",
+			"env":                     "envt1",
+			"_owner_":                 "ownr1",
+			"app_kubernetes_io_app":   "prod1",
+		},
+	}
+
+	props = []string{
+		AllocationDepartmentProp,
+		AllocationEnvironmentProp,
+		AllocationOwnerProp,
+		AllocationProductProp,
+	}
+
+	key = alloc.generateKey(props, labelConfig)
+	if key != "dept1/envt1/ownr1/prod1" {
+		t.Fatalf("generateKey: expected \"dept1/envt1/ownr1/prod\"; actual \"%s\"", key)
+	}
 }
 
 func TestNewAllocationSet(t *testing.T) {

pkg/kubecost/config.go

@@ -169,6 +169,13 @@ func (lc *LabelConfig) Map() map[string]string {
 	return m
 }
 
+// Sanitize returns a sanitized version of the given string, which converts
+// all illegal characters to underscores. Illegal characters are those that
+// Prometheus does not support; i.e. [^a-zA-Z0-9_]
+func (lc *LabelConfig) Sanitize(label string) string {
+	return prom.SanitizeLabelName(strings.TrimSpace(label))
+}
+
 // GetExternalAllocationName derives an external allocation name from a set of
 // labels, given an aggregation property. If the aggregation property is,
 // itself, a label (e.g. label:app) then this function looks for a

pkg/kubecost/config_test.go

@@ -104,7 +104,7 @@ func TestLabelConfig_GetExternalAllocationName(t *testing.T) {
 
 	// Change the external label for namespace and confirm it still works
 	lc.NamespaceExternalLabel = "kubens"
-	lc.ServiceExternalLabel = "prom-sanitization-test"
+	lc.ServiceExternalLabel = "prom/sanitization-test"
 	lc.PodExternalLabel = "Non__GlueFormattedLabel"
 	lc.OwnerExternalLabel = "kubeowner"
 	lc.DepartmentExternalLabel = "doesntexist,env"
@@ -130,3 +130,23 @@ func TestLabelConfig_GetExternalAllocationName(t *testing.T) {
 		}
 	}
 }
+
+func TestLabelConfig_Sanitize(t *testing.T) {
+	testCases := []struct {
+		label    string
+		expected string
+	}{
+		{"", ""},
+		{"simple", "simple"},
+		{"prom/sanitization-test", "prom_sanitization_test"},
+		{" prom/sanitization-test$  ", "prom_sanitization_test_"},
+	}
+
+	lc := NewLabelConfig()
+	for _, tc := range testCases {
+		actual := lc.Sanitize(tc.label)
+		if actual != tc.expected {
+			t.Fatalf("Sanitize failed; expected '%s'; got '%s'", tc.expected, actual)
+		}
+	}
+}