Merge branch 'develop' of https://github.com/kubecost/cost-model into AjayTripathy-gpu-allocation

README.md

@@ -72,9 +72,9 @@ As an example, let's imagine a node with 1 CPU and 1 Gb of RAM that costs $20/mo
 
 Resources are allocated based on the time-weighted maximum of resource Requests and Usage over the measured period. For example, a pod with no usage and 1 CPU requested for 12 hours out of a 24 hour window would be allocated 12 CPU hours. For pods with BestEffort quality of service (i.e. no requests) allocation is done solely on resource usage. 
 
-#### How do I set my AWS Spot bids for accurate allocation?
+#### How do I set my AWS Spot estimates for cost allocation?
 
-Modify [spotCPU](https://github.com/kubecost/cost-model/blob/master/configs/default.json#L5) and  [spotRAM](https://github.com/kubecost/cost-model/blob/master/configs/default.json#L7) in default.json to the price of your bid. Allocation will use these bid prices, but it does not take into account what you are actually charged by AWS. Alternatively, you can provide an AWS key to allow access to the Spot data feed. This will provide accurate Spot prices. 
+Modify [spotCPU](https://github.com/kubecost/cost-model/blob/master/configs/default.json#L5) and  [spotRAM](https://github.com/kubecost/cost-model/blob/master/configs/default.json#L7) in default.json to the level of recent market prices. Allocation will use these prices, but it does not take into account what you are actually charged by AWS. Alternatively, you can provide an AWS key to allow access to the Spot data feed. This will provide accurate Spot price reconciliation. 
 
 #### Do I need a GCP billing API key?
 

pkg/cloud/awsprovider.go

@@ -24,6 +24,7 @@ import (
 	"github.com/kubecost/cost-model/pkg/errors"
 	"github.com/kubecost/cost-model/pkg/log"
 	"github.com/kubecost/cost-model/pkg/util"
+	"github.com/kubecost/cost-model/pkg/util/cloudutil"
 	"github.com/kubecost/cost-model/pkg/util/json"
 
 	"github.com/aws/aws-sdk-go/aws"
@@ -1540,52 +1541,6 @@ func (a *AWS) GetDisks() ([]byte, error) {
 	})
 }
 
-// ConvertToGlueColumnFormat takes a string and runs through various regex
-// and string replacement statements to convert it to a format compatible
-// with AWS Glue and Athena column names.
-// Following guidance from AWS provided here ('Column Names' section):
-// https://docs.aws.amazon.com/awsaccountbilling/latest/aboutv2/run-athena-sql.html
-// It returns a string containing the column name in proper column name format and length.
-func ConvertToGlueColumnFormat(column_name string) string {
-	klog.V(5).Infof("Converting string \"%s\" to proper AWS Glue column name.", column_name)
-
-	// An underscore is added in front of uppercase letters
-	capital_underscore := regexp.MustCompile(`[A-Z]`)
-	final := capital_underscore.ReplaceAllString(column_name, `_$0`)
-
-	// Any non-alphanumeric characters are replaced with an underscore
-	no_space_punc := regexp.MustCompile(`[\s]{1,}|[^A-Za-z0-9]`)
-	final = no_space_punc.ReplaceAllString(final, "_")
-
-	// Duplicate underscores are removed
-	no_dup_underscore := regexp.MustCompile(`_{2,}`)
-	final = no_dup_underscore.ReplaceAllString(final, "_")
-
-	// Any leading and trailing underscores are removed
-	no_front_end_underscore := regexp.MustCompile(`(^\_|\_$)`)
-	final = no_front_end_underscore.ReplaceAllString(final, "")
-
-	// Uppercase to lowercase
-	final = strings.ToLower(final)
-
-	// Longer column name than expected - remove _ left to right
-	allowed_col_len := 128
-	undersc_to_remove := len(final) - allowed_col_len
-	if undersc_to_remove > 0 {
-		final = strings.Replace(final, "_", "", undersc_to_remove)
-	}
-
-	// If removing all of the underscores still didn't
-	// make the column name < 128 characters, trim it!
-	if len(final) > allowed_col_len {
-		final = final[:allowed_col_len]
-	}
-
-	klog.V(5).Infof("Column name being returned: \"%s\". Length: \"%d\".", final, len(final))
-
-	return final
-}
-
 func generateAWSGroupBy(lastIdx int) string {
 	sequence := []string{}
 	for i := 1; i < lastIdx+1; i++ {
@@ -1955,7 +1910,7 @@ func (a *AWS) ExternalAllocations(start string, end string, aggregators []string
 	formattedAggregators := []string{}
 	for _, agg := range aggregators {
 		aggregator_column_name := "resource_tags_user_" + agg
-		aggregator_column_name = ConvertToGlueColumnFormat(aggregator_column_name)
+		aggregator_column_name = cloudutil.ConvertToGlueColumnFormat(aggregator_column_name)
 		formattedAggregators = append(formattedAggregators, aggregator_column_name)
 	}
 	aggregatorNames := strings.Join(formattedAggregators, ",")
@@ -1963,7 +1918,7 @@ func (a *AWS) ExternalAllocations(start string, end string, aggregators []string
 	aggregatorOr = aggregatorOr + " <> ''"
 
 	filter_column_name := "resource_tags_user_" + filterType
-	filter_column_name = ConvertToGlueColumnFormat(filter_column_name)
+	filter_column_name = cloudutil.ConvertToGlueColumnFormat(filter_column_name)
 
 	var query string
 	var lastIdx int

pkg/costmodel/router.go

@@ -817,7 +817,10 @@ func Initialize(additionalConfigWatchers ...ConfigWatchers) *Accesses {
 	keepAlive := 120 * time.Second
 	scrapeInterval, _ := time.ParseDuration("1m")
 
-	promCli, _ := prom.NewPrometheusClient(address, timeout, keepAlive, queryConcurrency, "")
+	promCli, err := prom.NewPrometheusClient(address, timeout, keepAlive, queryConcurrency, "")
+	if err != nil {
+		klog.Fatalf("Failed to create prometheus client, Error: %v", err)
+	}
 
 	api := prometheusAPI.NewAPI(promCli)
 	pcfg, err := api.Config(context.Background())

pkg/kubecost/allocation.go

@@ -3,7 +3,6 @@ package kubecost
 import (
 	"bytes"
 	"fmt"
-	"sort"
 	"strings"
 	"sync"
 	"time"
@@ -715,13 +714,14 @@ func NewAllocationSet(start, end time.Time, allocs ...*Allocation) *AllocationSe
 // simple flag for sharing idle resources.
 type AllocationAggregationOptions struct {
 	FilterFuncs       []AllocationMatchFunc
-	SplitIdle         bool
 	IdleByNode        bool
+	LabelConfig       *LabelConfig
 	MergeUnallocated  bool
+	SharedHourlyCosts map[string]float64
 	ShareFuncs        []AllocationMatchFunc
 	ShareIdle         string
 	ShareSplit        string
-	SharedHourlyCosts map[string]float64
+	SplitIdle         bool
 }
 
 // AggregateBy aggregates the Allocations in the given AllocationSet by the given
@@ -754,11 +754,26 @@ func (as *AllocationSet) AggregateBy(aggregateBy []string, options *AllocationAg
 	// 10. If the merge idle option is enabled, merge any remaining idle
 	//     allocations into a single idle allocation
 
+	if as.IsEmpty() {
+		return nil
+	}
+
 	if options == nil {
 		options = &AllocationAggregationOptions{}
 	}
 
-	if as.IsEmpty() {
+	if options.LabelConfig == nil {
+		options.LabelConfig = NewLabelConfig()
+	}
+
+	// If aggregateBy is nil, we don't aggregate anything. On the other hand,
+	// an empty slice implies that we should aggregate everything. See
+	// generateKey for why that makes sense.
+	shouldAggregate := aggregateBy != nil
+	shouldFilter := len(options.FilterFuncs) > 0
+	shouldShare := len(options.SharedHourlyCosts) > 0 || len(options.ShareFuncs) > 0
+	if !shouldAggregate && !shouldFilter && !shouldShare {
+		// There is nothing for AggregateBy to do, so simply return nil
 		return nil
 	}
 
@@ -921,7 +936,7 @@ func (as *AllocationSet) AggregateBy(aggregateBy []string, options *AllocationAg
 			hours := as.Resolution().Hours()
 
 			// If set ends in the future, adjust hours accordingly
-			diff := time.Now().Sub(as.End())
+			diff := time.Since(as.End())
 			if diff < 0.0 {
 				hours += diff.Hours()
 			}
@@ -1022,7 +1037,7 @@ func (as *AllocationSet) AggregateBy(aggregateBy []string, options *AllocationAg
 		}
 
 		// (5) generate key to use for aggregation-by-key and allocation name
-		key := alloc.generateKey(aggregateBy)
+		key := alloc.generateKey(aggregateBy, options.LabelConfig)
 
 		alloc.Name = key
 		if options.MergeUnallocated && alloc.IsUnallocated() {
@@ -1153,7 +1168,7 @@ func (as *AllocationSet) AggregateBy(aggregateBy []string, options *AllocationAg
 			}
 		}
 		if !skip {
-			key := alloc.generateKey(aggregateBy)
+			key := alloc.generateKey(aggregateBy, options.LabelConfig)
 
 			alloc.Name = key
 			aggSet.Insert(alloc)
@@ -1198,7 +1213,7 @@ func computeShareCoeffs(aggregateBy []string, options *AllocationAggregationOpti
 
 		// Determine the post-aggregation key under which the allocation will
 		// be shared.
-		name := alloc.generateKey(aggregateBy)
+		name := alloc.generateKey(aggregateBy, options.LabelConfig)
 
 		// If the current allocation will be filtered out in step 3, contribute
 		// its share of the shared coefficient to a "__filtered__" bin, which
@@ -1383,11 +1398,15 @@ func (a *Allocation) getIdleId(options *AllocationAggregationOptions) (string, e
 	return idleId, nil
 }
 
-func (a *Allocation) generateKey(aggregateBy []string) string {
+func (a *Allocation) generateKey(aggregateBy []string, labelConfig *LabelConfig) string {
 	if a == nil {
 		return ""
 	}
 
+	if labelConfig == nil {
+		labelConfig = NewLabelConfig()
+	}
+
 	// Names will ultimately be joined into a single name, which uniquely
 	// identifies allocations.
 	names := []string{}
@@ -1429,7 +1448,7 @@ func (a *Allocation) generateKey(aggregateBy []string) string {
 			names = append(names, a.Properties.Container)
 		case agg == AllocationServiceProp:
 			services := a.Properties.Services
-			if services == nil || len(services) == 0 {
+			if len(services) == 0 {
 				// Indicate that allocation has no services
 				names = append(names, UnallocatedSuffix)
 			} else {
@@ -1442,61 +1461,93 @@ func (a *Allocation) generateKey(aggregateBy []string) string {
 		case strings.HasPrefix(agg, "label:"):
 			labels := a.Properties.Labels
 			if labels == nil {
-				// Indicate that allocation has no labels
 				names = append(names, UnallocatedSuffix)
 			} else {
-				labelNames := []string{}
-				aggLabels := strings.Split(strings.TrimPrefix(agg, "label:"), ";")
-				for _, labelName := range aggLabels {
-					if val, ok := labels[labelName]; ok {
-						labelNames = append(labelNames, fmt.Sprintf("%s=%s", labelName, val))
-					} else if indexOf(UnallocatedSuffix, labelNames) == -1 { // if UnallocatedSuffix not already in names
-						labelNames = append(labelNames, UnallocatedSuffix)
-					}
-				}
-				// resolve arbitrary ordering. e.g., app=app0/env=env0 is the same agg as env=env0/app=app0
-				if len(labelNames) > 1 {
-					sort.Strings(labelNames)
-				}
-				unallocatedSuffixIndex := indexOf(UnallocatedSuffix, labelNames)
-				// suffix should be at index 0 if it exists b/c of underscores
-				if unallocatedSuffixIndex != -1 {
-					labelNames = append(labelNames[:unallocatedSuffixIndex], labelNames[unallocatedSuffixIndex+1:]...)
-					labelNames = append(labelNames, UnallocatedSuffix) // append to end
+				labelName := strings.TrimPrefix(agg, "label:")
+				if labelValue, ok := labels[labelName]; ok {
+					names = append(names, fmt.Sprintf("%s=%s", labelName, labelValue))
+				} else {
+					names = append(names, UnallocatedSuffix)
 				}
-
-				names = append(names, labelNames...)
 			}
 		case strings.HasPrefix(agg, "annotation:"):
 			annotations := a.Properties.Annotations
 			if annotations == nil {
-				// Indicate that allocation has no annotations
 				names = append(names, UnallocatedSuffix)
 			} else {
-				annotationNames := []string{}
-				aggAnnotations := strings.Split(strings.TrimPrefix(agg, "annotation:"), ";")
-				for _, annotationName := range aggAnnotations {
-					if val, ok := annotations[annotationName]; ok {
-						annotationNames = append(annotationNames, fmt.Sprintf("%s=%s", annotationName, val))
-					} else if indexOf(UnallocatedSuffix, annotationNames) == -1 { // if UnallocatedSuffix not already in names
-						annotationNames = append(annotationNames, UnallocatedSuffix)
-					}
+				annotationName := strings.TrimPrefix(agg, "annotation:")
+				if annotationValue, ok := annotations[annotationName]; ok {
+					names = append(names, fmt.Sprintf("%s=%s", annotationName, annotationValue))
+				} else {
+					names = append(names, UnallocatedSuffix)
 				}
-				// resolve arbitrary ordering. e.g., app=app0/env=env0 is the same agg as env=env0/app=app0
-				if len(annotationNames) > 1 {
-					sort.Strings(annotationNames)
+			}
+		case agg == AllocationDepartmentProp:
+			labels := a.Properties.Labels
+			if labels == nil {
+				names = append(names, UnallocatedSuffix)
+			} else {
+				labelName := labelConfig.DepartmentLabel
+				if labelValue, ok := labels[labelName]; ok {
+					names = append(names, labelValue)
+				} else {
+					names = append(names, UnallocatedSuffix)
 				}
-				unallocatedSuffixIndex := indexOf(UnallocatedSuffix, annotationNames)
-				// suffix should be at index 0 if it exists b/c of underscores
-				if unallocatedSuffixIndex != -1 {
-					annotationNames = append(annotationNames[:unallocatedSuffixIndex], annotationNames[unallocatedSuffixIndex+1:]...)
-					annotationNames = append(annotationNames, UnallocatedSuffix) // append to end
+			}
+		case agg == AllocationEnvironmentProp:
+			labels := a.Properties.Labels
+			if labels == nil {
+				names = append(names, UnallocatedSuffix)
+			} else {
+				labelName := labelConfig.EnvironmentLabel
+				if labelValue, ok := labels[labelName]; ok {
+					names = append(names, labelValue)
+				} else {
+					names = append(names, UnallocatedSuffix)
+				}
+			}
+		case agg == AllocationOwnerProp:
+			labels := a.Properties.Labels
+			if labels == nil {
+				names = append(names, UnallocatedSuffix)
+			} else {
+				labelName := labelConfig.OwnerLabel
+				if labelValue, ok := labels[labelName]; ok {
+					names = append(names, labelValue)
+				} else {
+					names = append(names, UnallocatedSuffix)
+				}
+			}
+		case agg == AllocationProductProp:
+			labels := a.Properties.Labels
+			if labels == nil {
+				names = append(names, UnallocatedSuffix)
+			} else {
+				labelName := labelConfig.ProductLabel
+				if labelValue, ok := labels[labelName]; ok {
+					names = append(names, labelValue)
+				} else {
+					names = append(names, UnallocatedSuffix)
+				}
+			}
+		case agg == AllocationTeamProp:
+			labels := a.Properties.Labels
+			if labels == nil {
+				names = append(names, UnallocatedSuffix)
+			} else {
+				labelName := labelConfig.TeamLabel
+				if labelValue, ok := labels[labelName]; ok {
+					names = append(names, labelValue)
+				} else {
+					names = append(names, UnallocatedSuffix)
 				}
-
-				names = append(names, annotationNames...)
 			}
+		default:
+			// This case should never be reached, as input up until this point
+			// should be checked and rejected if invalid. But if we do get a
+			// value we don't recognize, log a warning.
+			log.Warningf("AggregateBy: illegal aggregation parameter: %s", agg)
 		}
-
 	}
 
 	return strings.Join(names, "/")
@@ -2299,7 +2350,7 @@ func (asr *AllocationSetRange) Length() int {
 // MarshalJSON JSON-encodes the range
 func (asr *AllocationSetRange) MarshalJSON() ([]byte, error) {
 	asr.RLock()
-	asr.RUnlock()
+	defer asr.RUnlock()
 	return json.Marshal(asr.allocations)
 }
 

pkg/kubecost/allocation_test.go

@@ -435,7 +435,7 @@ func TestAllocationSet_generateKey(t *testing.T) {
 		AllocationClusterProp,
 	}
 
-	key = alloc.generateKey(props)
+	key = alloc.generateKey(props, nil)
 	if key != "" {
 		t.Fatalf("generateKey: expected \"\"; actual \"%s\"", key)
 	}
@@ -449,7 +449,7 @@ func TestAllocationSet_generateKey(t *testing.T) {
 		},
 	}
 
-	key = alloc.generateKey(props)
+	key = alloc.generateKey(props, nil)
 	if key != "cluster1" {
 		t.Fatalf("generateKey: expected \"cluster1\"; actual \"%s\"", key)
 	}
@@ -460,7 +460,7 @@ func TestAllocationSet_generateKey(t *testing.T) {
 		"label:app",
 	}
 
-	key = alloc.generateKey(props)
+	key = alloc.generateKey(props, nil)
 	if key != "cluster1//app=app1" {
 		t.Fatalf("generateKey: expected \"cluster1//app=app1\"; actual \"%s\"", key)
 	}
@@ -473,10 +473,36 @@ func TestAllocationSet_generateKey(t *testing.T) {
 			"env": "env1",
 		},
 	}
-	key = alloc.generateKey(props)
+	key = alloc.generateKey(props, nil)
 	if key != "cluster1/namespace1/app=app1" {
 		t.Fatalf("generateKey: expected \"cluster1/namespace1/app=app1\"; actual \"%s\"", key)
 	}
+
+	props = []string{
+		AllocationDepartmentProp,
+		AllocationEnvironmentProp,
+		AllocationOwnerProp,
+		AllocationProductProp,
+		AllocationTeamProp,
+	}
+
+	labelConfig := NewLabelConfig()
+
+	alloc.Properties = &AllocationProperties{
+		Cluster:   "cluster1",
+		Namespace: "namespace1",
+		Labels: map[string]string{
+			labelConfig.DepartmentLabel:  "dept1",
+			labelConfig.EnvironmentLabel: "envt1",
+			labelConfig.OwnerLabel:       "ownr1",
+			labelConfig.ProductLabel:     "prod1",
+			labelConfig.TeamLabel:        "team1",
+		},
+	}
+	key = alloc.generateKey(props, nil)
+	if key != "dept1/envt1/ownr1/prod1/team1" {
+		t.Fatalf("generateKey: expected \"dept1/envt1/ownr1/prod1/team1\"; actual \"%s\"", key)
+	}
 }
 
 func TestNewAllocationSet(t *testing.T) {
@@ -920,17 +946,17 @@ func TestAllocationSet_AggregateBy(t *testing.T) {
 		// 2d AggregationProperties=(Label:app, Label:environment)
 		"2d": {
 			start:      start,
-			aggBy:      []string{"label:app;env"},
+			aggBy:      []string{"label:app", "label:env"},
 			aggOpts:    nil,
 			numResults: 3 + numIdle + numUnallocated,
 			totalCost:  activeTotalCost + idleTotalCost,
 			// sets should be {idle, unallocated, app1/env1, app2/env2, app2/unallocated}
 			results: map[string]float64{
-				"app=app1/env=env1":             16.00,
-				"app=app2/env=env2":             12.00,
-				"app=app2/" + UnallocatedSuffix: 12.00,
-				IdleSuffix:                      30.00,
-				UnallocatedSuffix:               42.00,
+				"app=app1/env=env1":                         16.00,
+				"app=app2/env=env2":                         12.00,
+				"app=app2/" + UnallocatedSuffix:             12.00,
+				IdleSuffix:                                  30.00,
+				UnallocatedSuffix + "/" + UnallocatedSuffix: 42.00,
 			},
 			windowStart: startYesterday,
 			windowEnd:   endYesterday,
@@ -939,17 +965,17 @@ func TestAllocationSet_AggregateBy(t *testing.T) {
 		// 2e AggregationProperties=(Cluster, Label:app, Label:environment)
 		"2e": {
 			start:      start,
-			aggBy:      []string{AllocationClusterProp, "label:app;env"},
+			aggBy:      []string{AllocationClusterProp, "label:app", "label:env"},
 			aggOpts:    nil,
 			numResults: 6,
 			totalCost:  activeTotalCost + idleTotalCost,
 			results: map[string]float64{
-				"cluster1/app=app2/env=env2":             12.00,
-				"__idle__":                               30.00,
-				"cluster1/app=app1/env=env1":             16.00,
-				"cluster1/" + UnallocatedSuffix:          18.00,
-				"cluster2/app=app2/" + UnallocatedSuffix: 12.00,
-				"cluster2/" + UnallocatedSuffix:          24.00,
+				"cluster1/app=app2/env=env2": 12.00,
+				"__idle__":                   30.00,
+				"cluster1/app=app1/env=env1": 16.00,
+				"cluster1/" + UnallocatedSuffix + "/" + UnallocatedSuffix: 18.00,
+				"cluster2/app=app2/" + UnallocatedSuffix:                  12.00,
+				"cluster2/" + UnallocatedSuffix + "/" + UnallocatedSuffix: 24.00,
 			},
 			windowStart: startYesterday,
 			windowEnd:   endYesterday,

pkg/kubecost/allocationprops.go

@@ -4,6 +4,8 @@ import (
 	"fmt"
 	"sort"
 	"strings"
+
+	"github.com/kubecost/cost-model/pkg/prom"
 )
 
 const (
@@ -23,6 +25,11 @@ const (
 	AllocationStatefulSetProp    string = "statefulset"
 	AllocationDaemonSetProp      string = "daemonset"
 	AllocationJobProp            string = "job"
+	AllocationDepartmentProp     string = "department"
+	AllocationEnvironmentProp    string = "environment"
+	AllocationOwnerProp          string = "owner"
+	AllocationProductProp        string = "product"
+	AllocationTeamProp           string = "team"
 )
 
 func ParseProperty(text string) (string, error) {
@@ -57,7 +64,28 @@ func ParseProperty(text string) (string, error) {
 		return AllocationStatefulSetProp, nil
 	case "job":
 		return AllocationJobProp, nil
+	case "department":
+		return AllocationDepartmentProp, nil
+	case "environment":
+		return AllocationEnvironmentProp, nil
+	case "owner":
+		return AllocationOwnerProp, nil
+	case "product":
+		return AllocationProductProp, nil
+	case "team":
+		return AllocationTeamProp, nil
+	}
+
+	if strings.HasPrefix(text, "label:") {
+		label := prom.SanitizeLabelName(strings.TrimSpace(strings.TrimPrefix(text, "label:")))
+		return fmt.Sprintf("label:%s", label), nil
 	}
+
+	if strings.HasPrefix(text, "annotation:") {
+		annotation := prom.SanitizeLabelName(strings.TrimSpace(strings.TrimPrefix(text, "annotation:")))
+		return fmt.Sprintf("annotation:%s", annotation), nil
+	}
+
 	return AllocationNilProp, fmt.Errorf("invalid allocation property: %s", text)
 }
 
@@ -72,8 +100,8 @@ type AllocationProperties struct {
 	Pod            string                `json:"pod,omitempty"`
 	Services       []string              `json:"services,omitempty"`
 	ProviderID     string                `json:"providerID,omitempty"`
-	Labels         AllocationLabels      `json:"allocationLabels,omitempty"`
-	Annotations    AllocationAnnotations `json:"allocationAnnotations,omitempty"`
+	Labels         AllocationLabels      `json:"labels,omitempty"`
+	Annotations    AllocationAnnotations `json:"annotations,omitempty"`
 }
 
 // AllocationLabels is a schema-free mapping of key/value pairs that can be

pkg/kubecost/asset.go

@@ -121,11 +121,16 @@ type Asset interface {
 //   => nil, err
 //
 // (See asset_test.go for assertions of these examples and more.)
-func AssetToExternalAllocation(asset Asset, aggregateBy []string, externalLabelsCfg map[string]string) (*Allocation, error) {
+func AssetToExternalAllocation(asset Asset, aggregateBy []string, labelConfig *LabelConfig) (*Allocation, error) {
 	if asset == nil {
 		return nil, fmt.Errorf("asset is nil")
 	}
 
+	// Use default label config if one is not provided.
+	if labelConfig == nil {
+		labelConfig = NewLabelConfig()
+	}
+
 	// names will collect the slash-separated names accrued by iterating over
 	// aggregateBy and checking the relevant labels.
 	names := []string{}
@@ -137,83 +142,67 @@ func AssetToExternalAllocation(asset Asset, aggregateBy []string, externalLabels
 	// props records the relevant Properties to set on the resultant Allocation
 	props := AllocationProperties{}
 
+	// For each aggregation parameter, try to find a match in the asset's
+	// labels, using the labelConfig to translate. For an aggregation parameter
+	// defined by a label (e.g. "label:app") this is simple: look for the label
+	// and use it (e.g. if "app" is a defined label on the asset, then use its
+	// value). For an aggregation parameter defined by a non-label property
+	// (e.g. "namespace") this requires using the labelConfig to look up the
+	// label name associated with that property and to use the value under that
+	// label, if set (e.g. if the aggregation property is "namespace" and the
+	// labelConfig is configured with "namespace_external_label" => "kubens"
+	// and the asset has label "kubens":"kubecost", then file the asset as an
+	// external cost under "kubecost").
 	for _, aggBy := range aggregateBy {
-		// labelName should be derived from the mapping of properties to
-		// label names, unless the aggBy is explicitly a label, in which
-		// case we should pull the label name from the aggBy string.
-		// Unless this matches a special aggregation, as we have that mapping already transformed...
-		labelName := aggBy
-		agglName := aggBy
-		if strings.HasPrefix(aggBy, "label:") {
-			labelName = strings.TrimPrefix(aggBy, "label:")
-			agglName = labelName
-			if v, ok := externalLabelsCfg[labelName]; ok {
-				agglName = v
-			}
-		}
-
-		if labelName == "" {
+		name := labelConfig.GetExternalAllocationName(asset.Labels(), aggBy)
+		if name == "" {
 			// No matching label has been defined in the cost-analyzer label config
 			// relating to the given aggregateBy property.
 			names = append(names, UnallocatedSuffix)
 			continue
-		}
-
-		if value := asset.Labels()[agglName]; value != "" {
-			// Valid label value was found for one of the aggregation properties,
-			// so add it to the name.
-			if strings.HasPrefix(aggBy, "label:") {
-				// Use naming convention labelName=labelValue for labels
-				// e.g. aggBy="label:env", value="prod" => "env=prod"
-				names = append(names, fmt.Sprintf("%s=%s", strings.TrimPrefix(aggBy, "label:"), value))
-				match = true
-
-				// Set the corresponding label in props
-				labels := props.Labels
-				if labels == nil {
-					labels = map[string]string{}
-				}
-
-				labels[labelName] = value
-				props.Labels = labels
-			} else {
-				names = append(names, value)
-				match = true
-
-				// Set the corresponding property on props
-				switch aggBy {
-				case AllocationClusterProp:
-					props.Cluster = value
-				case AllocationNodeProp:
-					props.Node = value
-				case AllocationNamespaceProp:
-					props.Namespace = value
-				case AllocationControllerKindProp:
-					props.ControllerKind = value
-				case AllocationControllerProp:
-					props.Controller = value
-				case AllocationPodProp:
-					props.Pod = value
-				case AllocationContainerProp:
-					props.Container = value
-				case AllocationServiceProp:
-					// TODO: external allocation: how to do this? multi-service?
-					props.Services = []string{value}
+		} else {
+			names = append(names, name)
+			match = true
+
+			// Set the corresponding property on props
+			switch aggBy {
+			case AllocationClusterProp:
+				props.Cluster = name
+			case AllocationNodeProp:
+				props.Node = name
+			case AllocationNamespaceProp:
+				props.Namespace = name
+			case AllocationControllerKindProp:
+				props.ControllerKind = name
+			case AllocationControllerProp:
+				props.Controller = name
+			case AllocationPodProp:
+				props.Pod = name
+			case AllocationContainerProp:
+				props.Container = name
+			case AllocationServiceProp:
+				props.Services = []string{name}
+			default:
+				if strings.HasPrefix(aggBy, "label:") {
+					// Set the corresponding label in props
+					if props.Labels == nil {
+						props.Labels = map[string]string{}
+					}
+					labelName := strings.TrimPrefix(aggBy, "label:")
+					labelValue := strings.TrimPrefix(name, labelName+"=")
+					props.Labels[labelName] = labelValue
 				}
 			}
-		} else {
-			// No value label value was found on the Asset; consider it
-			// unallocated. Note that this case is only truly relevant if at
-			// least one other property matches (e.g. case 3 in the examples)
-			// because if there are no matches, then an error is returned.
-			names = append(names, UnallocatedSuffix)
 		}
 	}
 
+	// If not a signle aggregation property generated a matching label property,
+	// then consider the asset ineligible to be treated as an external allocation.
 	if !match {
 		return nil, fmt.Errorf("asset does not qualify as an external allocation")
 	}
 
+	// Use naming to label as an external allocation. See IsExternal() for more.
 	names = append(names, ExternalSuffix)
 
 	// TODO: external allocation: efficiency?

pkg/kubecost/asset_test.go

@@ -923,9 +923,9 @@ func TestAssetToExternalAllocation(t *testing.T) {
 	var alloc *Allocation
 	var err error
 
-	alloc, err = AssetToExternalAllocation(asset, []string{"namespace"}, map[string]string{})
+	_, err = AssetToExternalAllocation(asset, []string{"namespace"}, nil)
 	if err == nil {
-		t.Fatalf("expected error due to nil asset")
+		t.Fatalf("expected error due to nil asset; no error returned")
 	}
 
 	// Consider this Asset:
@@ -938,20 +938,20 @@ func TestAssetToExternalAllocation(t *testing.T) {
 	//   }
 	cloud := NewCloud(ComputeCategory, "abc123", start1, start2, windows[0])
 	cloud.SetLabels(map[string]string{
-		"namespace": "monitoring",
-		"env":       "prod",
-		"product":   "cost-analyzer",
+		"kubernetes_namespace": "monitoring",
+		"env":                  "prod",
+		"app":                  "cost-analyzer",
 	})
 	cloud.Cost = 10.00
 	asset = cloud
 
-	alloc, err = AssetToExternalAllocation(asset, []string{"namespace"}, map[string]string{})
+	_, err = AssetToExternalAllocation(asset, []string{"namespace"}, nil)
 	if err != nil {
-		t.Fatalf("expected to not error")
+		t.Fatalf("unexpected error: %s", err)
 	}
-	alloc, err = AssetToExternalAllocation(asset, nil, map[string]string{})
+	_, err = AssetToExternalAllocation(asset, nil, nil)
 	if err == nil {
-		t.Fatalf("expected error due to nil aggregateBy")
+		t.Fatalf("expected error due to nil aggregateBy; no error returned")
 	}
 
 	// Given the following parameters, we expect to return:
@@ -977,7 +977,7 @@ func TestAssetToExternalAllocation(t *testing.T) {
 	//   => nil, err
 
 	// 1) single-prop full match
-	alloc, err = AssetToExternalAllocation(asset, []string{"namespace"}, map[string]string{})
+	alloc, err = AssetToExternalAllocation(asset, []string{"namespace"}, nil)
 	if err != nil {
 		t.Fatalf("unexpected error: %s", err)
 	}
@@ -985,7 +985,7 @@ func TestAssetToExternalAllocation(t *testing.T) {
 		t.Fatalf("expected external allocation with name '%s'; got '%s'", "monitoring/__external__", alloc.Name)
 	}
 	if ns := alloc.Properties.Namespace; ns != "monitoring" {
-		t.Fatalf("expected external allocation with AllocationProperties.Namespace '%s'; got '%s' (%s)", "monitoring", ns, err)
+		t.Fatalf("expected external allocation with AllocationProperties.Namespace '%s'; got '%s'", "monitoring", ns)
 	}
 	if alloc.ExternalCost != 10.00 {
 		t.Fatalf("expected external allocation with ExternalCost %f; got %f", 10.00, alloc.ExternalCost)
@@ -995,7 +995,7 @@ func TestAssetToExternalAllocation(t *testing.T) {
 	}
 
 	// 2) multi-prop full match
-	alloc, err = AssetToExternalAllocation(asset, []string{"namespace", "label:env"}, map[string]string{})
+	alloc, err = AssetToExternalAllocation(asset, []string{"namespace", "label:env"}, nil)
 	if err != nil {
 		t.Fatalf("unexpected error: %s", err)
 	}
@@ -1016,7 +1016,7 @@ func TestAssetToExternalAllocation(t *testing.T) {
 	}
 
 	// 3) multi-prop partial match
-	alloc, err = AssetToExternalAllocation(asset, []string{"namespace", "label:foo"}, map[string]string{})
+	alloc, err = AssetToExternalAllocation(asset, []string{"namespace", "label:foo"}, nil)
 	if err != nil {
 		t.Fatalf("unexpected error: %s", err)
 	}
@@ -1033,134 +1033,17 @@ func TestAssetToExternalAllocation(t *testing.T) {
 		t.Fatalf("expected external allocation with TotalCost %f; got %f", 10.00, alloc.TotalCost())
 	}
 
-	// 3) no match
-	alloc, err = AssetToExternalAllocation(asset, []string{"cluster"}, map[string]string{})
+	// 4) no match
+	alloc, err = AssetToExternalAllocation(asset, []string{"cluster"}, nil)
 	if err == nil {
 		t.Fatalf("expected 'no match' error")
 	}
 
-	alloc, err = AssetToExternalAllocation(asset, []string{"namespace", "label:app"}, map[string]string{"app": "product"})
+	alloc, err = AssetToExternalAllocation(asset, []string{"namespace", "label:app"}, nil)
 	if alloc.ExternalCost != 10.00 {
 		t.Fatalf("expected external allocation with ExternalCost %f; got %f", 10.00, alloc.ExternalCost)
 	}
 	if alloc.TotalCost() != 10.00 {
 		t.Fatalf("expected external allocation with TotalCost %f; got %f", 10.00, alloc.TotalCost())
 	}
-
 }
-
-// TODO merge conflict had this:
-
-// as.Each(func(key string, a Asset) {
-// 	if exp, ok := exps[key]; ok {
-// 		if math.Round(a.TotalCost()*100) != math.Round(exp*100) {
-// 			t.Fatalf("AssetSet.AggregateBy[%s]: key %s expected total cost %.2f, actual %.2f", msg, key, exp, a.TotalCost())
-// 		}
-// 		if !a.Window().Equal(window) {
-// 			t.Fatalf("AssetSet.AggregateBy[%s]: key %s expected window %s, actual %s", msg, key, window, as.Window)
-// 		}
-// 	} else {
-// 		t.Fatalf("AssetSet.AggregateBy[%s]: unexpected asset: %s", msg, key)
-// 	}
-// })
-// }
-
-// // GenerateMockAssetSet generates the following topology:
-// //
-// // | Asset                        | Cost |  Adj |
-// // +------------------------------+------+------+
-// //   cluster1:
-// //     node1:                        6.00   1.00
-// //     node2:                        4.00   1.50
-// //     node3:                        7.00  -0.50
-// //     disk1:                        2.50   0.00
-// //     disk2:                        1.50   0.00
-// //     clusterManagement1:           3.00   0.00
-// // +------------------------------+------+------+
-// //   cluster1 subtotal              24.00   2.00
-// // +------------------------------+------+------+
-// //   cluster2:
-// //     node4:                       12.00  -1.00
-// //     disk3:                        2.50   0.00
-// //     disk4:                        1.50   0.00
-// //     clusterManagement2:           0.00   0.00
-// // +------------------------------+------+------+
-// //   cluster2 subtotal              16.00  -1.00
-// // +------------------------------+------+------+
-// //   cluster3:
-// //     node5:                       17.00   2.00
-// // +------------------------------+------+------+
-// //   cluster3 subtotal              17.00   2.00
-// // +------------------------------+------+------+
-// //   total                          57.00   3.00
-// // +------------------------------+------+------+
-// func GenerateMockAssetSet(start time.Time) *AssetSet {
-// end := start.Add(day)
-// window := NewWindow(&start, &end)
-
-// hours := window.Duration().Hours()
-
-// node1 := NewNode("node1", "cluster1", "gcp-node1", *window.Clone().start, *window.Clone().end, window.Clone())
-// node1.CPUCost = 4.0
-// node1.RAMCost = 4.0
-// node1.GPUCost = 2.0
-// node1.Discount = 0.5
-// node1.CPUCoreHours = 2.0 * hours
-// node1.RAMByteHours = 4.0 * gb * hours
-// node1.SetAdjustment(1.0)
-// node1.SetLabels(map[string]string{"test": "test"})
-
-// node2 := NewNode("node2", "cluster1", "gcp-node2", *window.Clone().start, *window.Clone().end, window.Clone())
-// node2.CPUCost = 4.0
-// node2.RAMCost = 4.0
-// node2.GPUCost = 0.0
-// node2.Discount = 0.5
-// node2.CPUCoreHours = 2.0 * hours
-// node2.RAMByteHours = 4.0 * gb * hours
-// node2.SetAdjustment(1.5)
-
-// node3 := NewNode("node3", "cluster1", "gcp-node3", *window.Clone().start, *window.Clone().end, window.Clone())
-// node3.CPUCost = 4.0
-// node3.RAMCost = 4.0
-// node3.GPUCost = 3.0
-// node3.Discount = 0.5
-// node3.CPUCoreHours = 2.0 * hours
-// node3.RAMByteHours = 4.0 * gb * hours
-// node3.SetAdjustment(-0.5)
-
-// node4 := NewNode("node4", "cluster2", "gcp-node4", *window.Clone().start, *window.Clone().end, window.Clone())
-// node4.CPUCost = 10.0
-// node4.RAMCost = 6.0
-// node4.GPUCost = 0.0
-// node4.Discount = 0.25
-// node4.CPUCoreHours = 4.0 * hours
-// node4.RAMByteHours = 12.0 * gb * hours
-// node4.SetAdjustment(-1.0)
-
-// node5 := NewNode("node5", "cluster3", "aws-node5", *window.Clone().start, *window.Clone().end, window.Clone())
-// node5.CPUCost = 10.0
-// node5.RAMCost = 7.0
-// node5.GPUCost = 0.0
-// node5.Discount = 0.0
-// node5.CPUCoreHours = 8.0 * hours
-// node5.RAMByteHours = 24.0 * gb * hours
-// node5.SetAdjustment(2.0)
-
-// disk1 := NewDisk("disk1", "cluster1", "gcp-disk1", *window.Clone().start, *window.Clone().end, window.Clone())
-// disk1.Cost = 2.5
-// disk1.ByteHours = 100 * gb * hours
-
-// disk2 := NewDisk("disk2", "cluster1", "gcp-disk2", *window.Clone().start, *window.Clone().end, window.Clone())
-// disk2.Cost = 1.5
-// disk2.ByteHours = 60 * gb * hours
-
-// disk3 := NewDisk("disk3", "cluster2", "gcp-disk3", *window.Clone().start, *window.Clone().end, window.Clone())
-// disk3.Cost = 2.5
-// disk3.ByteHours = 100 * gb * hours
-
-// disk4 := NewDisk("disk4", "cluster2", "gcp-disk4", *window.Clone().start, *window.Clone().end, window.Clone())
-// disk4.Cost = 1.5
-// disk4.ByteHours = 100 * gb * hours
-
-// cm1 := NewClusterManagement("gcp", "cluster1", window.Clone())
-// cm1.Cost = 3.0

pkg/kubecost/assetprops.go

@@ -65,20 +65,6 @@ func ParseAssetProperty(text string) (AssetProperty, error) {
 	return AssetNilProp, fmt.Errorf("invalid asset property: %s", text)
 }
 
-func propsEqual(p1, p2 []AssetProperty) bool {
-	if len(p1) != len(p2) {
-		return false
-	}
-
-	for _, p := range p1 {
-		if !hasProp(p2, p) {
-			return false
-		}
-	}
-
-	return true
-}
-
 // Category options
 
 // ComputeCategory signifies the Compute Category

pkg/kubecost/config.go

@@ -3,6 +3,9 @@ package kubecost
 import (
 	"fmt"
 	"strings"
+
+	"github.com/kubecost/cost-model/pkg/prom"
+	"github.com/kubecost/cost-model/pkg/util/cloudutil"
 )
 
 // LabelConfig is a port of type AnalyzerConfig. We need to be more thoughtful
@@ -29,6 +32,30 @@ type LabelConfig struct {
 	TeamExternalLabel        string `json:"team_external_label"`
 }
 
+// NewLabelConfig creates a new LabelConfig instance with default values.
+func NewLabelConfig() *LabelConfig {
+	return &LabelConfig{
+		DepartmentLabel:          "department",
+		EnvironmentLabel:         "env",
+		OwnerLabel:               "owner",
+		ProductLabel:             "app",
+		TeamLabel:                "team",
+		ClusterExternalLabel:     "kubernetes_cluster",
+		NamespaceExternalLabel:   "kubernetes_namespace",
+		ControllerExternalLabel:  "kubernetes_controller",
+		DaemonsetExternalLabel:   "kubernetes_daemonset",
+		DeploymentExternalLabel:  "kubernetes_deployment",
+		StatefulsetExternalLabel: "kubernetes_statefulset",
+		ServiceExternalLabel:     "kubernetes_service",
+		PodExternalLabel:         "kubernetes_pod",
+		DepartmentExternalLabel:  "kubernetes_label_department",
+		EnvironmentExternalLabel: "kubernetes_label_env",
+		OwnerExternalLabel:       "kubernetes_label_owner",
+		ProductExternalLabel:     "kubernetes_label_app",
+		TeamExternalLabel:        "kubernetes_label_team",
+	}
+}
+
 // Map returns the config as a basic string map, with default values if not set
 func (lc *LabelConfig) Map() map[string]string {
 	// Start with default values
@@ -142,57 +169,104 @@ func (lc *LabelConfig) Map() map[string]string {
 	return m
 }
 
-// ExternalQueryLabels returns the config's external labels as a mapping of the
-// query column to the label it should set;
-// e.g. if the config stores "statefulset_external_label": "kubernetes_sset",
-//      then this would return "kubernetes_sset": "statefulset"
-func (lc *LabelConfig) ExternalQueryLabels() map[string]string {
-	queryLabels := map[string]string{}
+// 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
+// corresponding value under "app" and returns "app=thatvalue". If the
+// aggregation property is not a label but a Kubernetes concept
+// (e.g. namespace) then this function first finds the "external label"
+// configured to represent it (e.g. NamespaceExternalLabel: "kubens") and uses
+// that label to determine an external allocation name. If no label value can
+// be found, return an empty string.
+func (lc *LabelConfig) GetExternalAllocationName(labels map[string]string, aggregateBy string) string {
+	labelNames := []string{}
+	aggByLabel := false
+
+	// Determine if the aggregation property is, itself, a label or not. If
+	// not, determine the label associated with the given aggregation property.
+	if strings.HasPrefix(aggregateBy, "label:") {
+		labelNames = append(labelNames, prom.SanitizeLabelName(strings.TrimPrefix(aggregateBy, "label:")))
+		aggByLabel = true
+	} else {
+		// If lc is nil, use a default LabelConfig to do a best-effort match
+		if lc == nil {
+			lc = NewLabelConfig()
+		}
+
+		switch strings.ToLower(aggregateBy) {
+		case AllocationClusterProp:
+			labelNames = strings.Split(lc.ClusterExternalLabel, ",")
+		case AllocationControllerProp:
+			labelNames = strings.Split(lc.ControllerExternalLabel, ",")
+		case AllocationNamespaceProp:
+			labelNames = strings.Split(lc.NamespaceExternalLabel, ",")
+		case AllocationPodProp:
+			labelNames = strings.Split(lc.PodExternalLabel, ",")
+		case AllocationServiceProp:
+			labelNames = strings.Split(lc.ServiceExternalLabel, ",")
+		case AllocationDeploymentProp:
+			labelNames = strings.Split(lc.DeploymentExternalLabel, ",")
+		case AllocationStatefulSetProp:
+			labelNames = strings.Split(lc.StatefulsetExternalLabel, ",")
+		case AllocationDaemonSetProp:
+			labelNames = strings.Split(lc.DaemonsetExternalLabel, ",")
+		case AllocationDepartmentProp:
+			labelNames = strings.Split(lc.DepartmentExternalLabel, ",")
+		case AllocationEnvironmentProp:
+			labelNames = strings.Split(lc.EnvironmentExternalLabel, ",")
+		case AllocationOwnerProp:
+			labelNames = strings.Split(lc.OwnerExternalLabel, ",")
+		case AllocationProductProp:
+			labelNames = strings.Split(lc.ProductExternalLabel, ",")
+		case AllocationTeamProp:
+			labelNames = strings.Split(lc.TeamExternalLabel, ",")
+		}
 
-	for label, query := range lc.Map() {
-		if strings.HasSuffix(label, "external_label") && query != "" {
-			queryLabels[query] = label
+		for i, labelName := range labelNames {
+			labelNames[i] = prom.SanitizeLabelName(strings.TrimSpace(labelName))
 		}
 	}
 
-	return queryLabels
-}
+	// No label is set for the given aggregation property.
+	if len(labelNames) == 0 {
+		return ""
+	}
 
-// AllocationPropertyLabels returns the config's external resource labels
-// as a mapping from k8s resource-to-label name.
-// e.g. if the config stores "statefulset_external_label": "kubernetes_sset",
-//      then this would return "statefulset": "kubernetes_sset"
-// e.g. if the config stores "owner_label": "product_owner",
-//      then this would return "label:product_owner": "product_owner"
-func (lc *LabelConfig) AllocationPropertyLabels() map[string]string {
-	labels := map[string]string{}
-
-	for labelKind, labelName := range lc.Map() {
-		if labelName != "" {
-			switch labelKind {
-			case "namespace_external_label":
-				labels["namespace"] = labelName
-			case "cluster_external_label":
-				labels["cluster"] = labelName
-			case "controller_external_label":
-				labels["controller"] = labelName
-			case "product_external_label":
-				labels["product"] = labelName
-			case "service_external_label":
-				labels["service"] = labelName
-			case "deployment_external_label":
-				labels["deployment"] = labelName
-			case "statefulset_external_label":
-				labels["statefulset"] = labelName
-			case "daemonset_external_label":
-				labels["daemonset"] = labelName
-			case "pod_external_label":
-				labels["pod"] = labelName
-			default:
-				labels[fmt.Sprintf("label:%s", labelName)] = labelName
+	// The relevant label is not present in the set of labels provided.
+	labelName := ""
+	labelValue := ""
+	for _, ln := range labelNames {
+		if lv, ok := labels[ln]; ok {
+			// Match found for given label
+			labelName = ln
+			labelValue = lv
+			break
+		} else {
+			// Convert the label name to a format compatible with AWS Glue and
+			// Athena column naming and check again. If not found after that,
+			// then consider the label not present.
+			ln = cloudutil.ConvertToGlueColumnFormat(ln)
+			if lv, ok = labels[ln]; ok {
+				// Match found for given label after converting to AWS format
+				labelName = ln
+				labelValue = lv
+				break
 			}
 		}
 	}
 
-	return labels
+	// No match found
+	if labelName == "" {
+		return ""
+	}
+
+	// When aggregating by some label (i.e. not by a Kubernetes concept),
+	// prepend the label value with the label name (e.g. "app=cost-analyzer").
+	// This step is not necessary for Kubernetes concepts (e.g. for namespace,
+	// we do not need "namespace=kubecost"; just "kubecost" will do).
+	if aggByLabel {
+		return fmt.Sprintf("%s=%s", labelName, labelValue)
+	}
+
+	return labelValue
 }

pkg/kubecost/config_test.go

@@ -1,6 +1,10 @@
 package kubecost
 
-import "testing"
+import (
+	"testing"
+
+	"github.com/kubecost/cost-model/pkg/util/cloudutil"
+)
 
 func TestLabelConfig_Map(t *testing.T) {
 	var m map[string]string
@@ -32,63 +36,97 @@ func TestLabelConfig_Map(t *testing.T) {
 	}
 }
 
-func TestLabelConfig_ExternalQueryLabels(t *testing.T) {
-	var qls map[string]string
-	var lc *LabelConfig
+func TestLabelConfig_GetExternalAllocationName(t *testing.T) {
+	// Make sure that AWS's Glue/Athena column formatting is supported
+	glueFormattedLabel := cloudutil.ConvertToGlueColumnFormat("Non__GlueFormattedLabel")
 
-	qls = lc.ExternalQueryLabels()
-	if len(qls) != 13 {
-		t.Fatalf("ExternalQueryLabels: expected length %d; got length %d", 13, len(qls))
-	}
-	if val, ok := qls["kubernetes_deployment"]; !ok || val != "deployment_external_label" {
-		t.Fatalf("ExternalQueryLabels: expected %s; got %s", "deployment_external_label", val)
-	}
-	if val, ok := qls["kubernetes_namespace"]; !ok || val != "namespace_external_label" {
-		t.Fatalf("ExternalQueryLabels: expected %s; got %s", "namespace_external_label", val)
+	labels := map[string]string{
+		"kubens":                      "kubecost-staging",
+		"kubeowner":                   "kubecost-owner",
+		"env":                         "env1",
+		"app":                         "app1",
+		"team":                        "team1",
+		glueFormattedLabel:            "glue",
+		"prom_sanitization_test":      "pass",
+		"kubernetes_cluster":          "cluster-one",
+		"kubernetes_namespace":        "kubecost",
+		"kubernetes_controller":       "kubecost-controller",
+		"kubernetes_daemonset":        "kubecost-daemonset",
+		"kubernetes_deployment":       "kubecost-deployment",
+		"kubernetes_statefulset":      "kubecost-statefulset",
+		"kubernetes_service":          "kubecost-service",
+		"kubernetes_pod":              "kubecost-cost-analyzer-abc123",
+		"kubernetes_label_department": "kubecost-department",
+		"kubernetes_label_env":        "kubecost-env",
+		"kubernetes_label_owner":      "kubecost-owner",
+		"kubernetes_label_app":        "kubecost-app",
+		"kubernetes_label_team":       "kubecost-team",
 	}
 
-	lc = &LabelConfig{
-		DaemonsetExternalLabel: "kubernetes_ds",
-	}
-	qls = lc.ExternalQueryLabels()
-	if len(qls) != 13 {
-		t.Fatalf("ExternalQueryLabels: expected length %d; got length %d", 13, len(qls))
-	}
-	if val, ok := qls["kubernetes_ds"]; !ok || val != "daemonset_external_label" {
-		t.Fatalf("ExternalQueryLabels: expected %s; got %s", "daemonset_external_label", val)
-	}
-	if val, ok := qls["kubernetes_namespace"]; !ok || val != "namespace_external_label" {
-		t.Fatalf("ExternalQueryLabels: expected %s; got %s", "namespace_external_label", val)
+	testCases := []struct {
+		aggBy    string
+		expected string
+	}{
+		{"label:env", "env=env1"},
+		{"label:app", "app=app1"},
+		{"label:Non__GlueFormattedLabel", "non_glue_formatted_label=glue"},
+		{"cluster", "cluster-one"},
+		{"namespace", "kubecost"},
+		{"controller", "kubecost-controller"},
+		{"daemonset", "kubecost-daemonset"},
+		{"deployment", "kubecost-deployment"},
+		{"statefulset", "kubecost-statefulset"},
+		{"service", "kubecost-service"},
+		{"pod", "kubecost-cost-analyzer-abc123"},
+		{"pod", "kubecost-cost-analyzer-abc123"},
+		{"notathing", ""},
+		{"", ""},
 	}
-}
 
-func TestTestLabelConfig_AllocationPropertyLabels(t *testing.T) {
-	var labels map[string]string
 	var lc *LabelConfig
 
-	labels = lc.AllocationPropertyLabels()
-	if len(labels) != 18 {
-		t.Fatalf("AllocationPropertyLabels: expected length %d; got length %d", 18, len(labels))
-	}
-	if val, ok := labels["namespace"]; !ok || val != "kubernetes_namespace" {
-		t.Fatalf("AllocationPropertyLabels: expected %s; got %s", "kubernetes_namespace", val)
-	}
-	if val, ok := labels["label:env"]; !ok || val != "env" {
-		t.Fatalf("AllocationPropertyLabels: expected %s; got %s", "env", val)
+	// If lc is nil, everything should still work off of defaults.
+	for _, tc := range testCases {
+		actual := lc.GetExternalAllocationName(labels, tc.aggBy)
+		if actual != tc.expected {
+			t.Fatalf("GetExternalAllocationName failed; expected '%s'; got '%s'", tc.expected, actual)
+		}
 	}
 
-	lc = &LabelConfig{
-		NamespaceExternalLabel: "kubens",
-		EnvironmentLabel:       "kubeenv",
-	}
-	labels = lc.AllocationPropertyLabels()
-	if len(labels) != 18 {
-		t.Fatalf("AllocationPropertyLabels: expected length %d; got length %d", 18, len(labels))
+	// If lc is default, everything should work, just like the nil.
+	lc = NewLabelConfig()
+	for _, tc := range testCases {
+		actual := lc.GetExternalAllocationName(labels, tc.aggBy)
+		if actual != tc.expected {
+			t.Fatalf("GetExternalAllocationName failed; expected '%s'; got '%s'", tc.expected, actual)
+		}
 	}
-	if val, ok := labels["namespace"]; !ok || val != "kubens" {
-		t.Fatalf("AllocationPropertyLabels: expected %s; got %s", "kubens", val)
-	}
-	if val, ok := labels["label:kubeenv"]; !ok || val != "kubeenv" {
-		t.Fatalf("AllocationPropertyLabels: expected %s; got %s", "kubeenv", val)
+
+	// Change the external label for namespace and confirm it still works
+	lc.NamespaceExternalLabel = "kubens"
+	lc.ServiceExternalLabel = "prom-sanitization-test"
+	lc.PodExternalLabel = "Non__GlueFormattedLabel"
+	lc.OwnerExternalLabel = "kubeowner"
+	lc.DepartmentExternalLabel = "doesntexist,env"
+	lc.TeamExternalLabel = "team,env"
+
+	// TODO how is e.g. OwnerExternalLabel supposed to work?
+
+	testCases = []struct {
+		aggBy    string
+		expected string
+	}{
+		{"namespace", "kubecost-staging"},
+		{"service", "pass"},
+		{"pod", "glue"},
+		{"owner", "kubecost-owner"},
+		{"department", "env1"},
+		{"team", "team1"},
+	}
+	for _, tc := range testCases {
+		actual := lc.GetExternalAllocationName(labels, tc.aggBy)
+		if actual != tc.expected {
+			t.Fatalf("GetExternalAllocationName failed; expected '%s'; got '%s'", tc.expected, actual)
+		}
 	}
 }

pkg/kubecost/window.go

@@ -572,7 +572,7 @@ func (w Window) DurationOffset() (time.Duration, time.Duration, error) {
 	}
 
 	duration := w.Duration()
-	offset := time.Now().Sub(*w.End())
+	offset := time.Since(*w.End())
 
 	return duration, offset, nil
 }

pkg/kubecost/window_test.go

@@ -598,7 +598,7 @@ func TestWindow_DurationOffsetStrings(t *testing.T) {
 	if err != nil {
 		t.Fatalf(`unexpected error parsing "1589448338,1589534798": %s`, err)
 	}
-	dur, off = w.DurationOffsetStrings()
+	dur, _ = w.DurationOffsetStrings()
 	if dur != "1d" {
 		t.Fatalf(`expect: window to be "1d"; actual: "%s"`, dur)
 	}

pkg/log/log.go

@@ -63,7 +63,7 @@ func Profilef(format string, a ...interface{}) {
 }
 
 func Debugf(format string, a ...interface{}) {
-	klog.V(4).Infof(fmt.Sprintf("[Debug] %s", format), a...)
+	klog.V(5).Infof(fmt.Sprintf("[Debug] %s", format), a...)
 }
 
 func Profile(start time.Time, name string) {

pkg/util/cloudutil/aws.go

@@ -0,0 +1,54 @@
+package cloudutil
+
+import (
+	"regexp"
+	"strings"
+
+	"github.com/kubecost/cost-model/pkg/log"
+)
+
+// ConvertToGlueColumnFormat takes a string and runs through various regex
+// and string replacement statements to convert it to a format compatible
+// with AWS Glue and Athena column names.
+// Following guidance from AWS provided here ('Column Names' section):
+// https://docs.aws.amazon.com/awsaccountbilling/latest/aboutv2/run-athena-sql.html
+// It returns a string containing the column name in proper column name format and length.
+func ConvertToGlueColumnFormat(columnName string) string {
+	log.Debugf("Converting string \"%s\" to proper AWS Glue column name.", columnName)
+
+	// An underscore is added in front of uppercase letters
+	capitalUnderscore := regexp.MustCompile(`[A-Z]`)
+	final := capitalUnderscore.ReplaceAllString(columnName, `_$0`)
+
+	// Any non-alphanumeric characters are replaced with an underscore
+	noSpacePunc := regexp.MustCompile(`[\s]{1,}|[^A-Za-z0-9]`)
+	final = noSpacePunc.ReplaceAllString(final, "_")
+
+	// Duplicate underscores are removed
+	noDupUnderscore := regexp.MustCompile(`_{2,}`)
+	final = noDupUnderscore.ReplaceAllString(final, "_")
+
+	// Any leading and trailing underscores are removed
+	noFrontUnderscore := regexp.MustCompile(`(^\_|\_$)`)
+	final = noFrontUnderscore.ReplaceAllString(final, "")
+
+	// Uppercase to lowercase
+	final = strings.ToLower(final)
+
+	// Longer column name than expected - remove _ left to right
+	allowedColLen := 128
+	underscoreToRemove := len(final) - allowedColLen
+	if underscoreToRemove > 0 {
+		final = strings.Replace(final, "_", "", underscoreToRemove)
+	}
+
+	// If removing all of the underscores still didn't
+	// make the column name < 128 characters, trim it!
+	if len(final) > allowedColLen {
+		final = final[:allowedColLen]
+	}
+
+	log.Debugf("Column name being returned: \"%s\". Length: \"%d\".", final, len(final))
+
+	return final
+}