code review comments - additional documentation, add unit test

Signed-off-by: Alex Meijer <ameijer@kubecost.com>

pkg/kubecost/allocationprops.go

@@ -92,18 +92,20 @@ func ParseProperty(text string) (string, error) {
 
 // AllocationProperties describes a set of Kubernetes objects.
 type AllocationProperties struct {
-	Cluster            string                `json:"cluster,omitempty"`
-	Node               string                `json:"node,omitempty"`
-	Container          string                `json:"container,omitempty"`
-	Controller         string                `json:"controller,omitempty"`
-	ControllerKind     string                `json:"controllerKind,omitempty"`
-	Namespace          string                `json:"namespace,omitempty"`
-	Pod                string                `json:"pod,omitempty"`
-	Services           []string              `json:"services,omitempty"`
-	ProviderID         string                `json:"providerID,omitempty"`
-	Labels             AllocationLabels      `json:"labels,omitempty"`
-	Annotations        AllocationAnnotations `json:"annotations,omitempty"`
-	AggregatedMetadata bool                  `json:"-"`
+	Cluster        string                `json:"cluster,omitempty"`
+	Node           string                `json:"node,omitempty"`
+	Container      string                `json:"container,omitempty"`
+	Controller     string                `json:"controller,omitempty"`
+	ControllerKind string                `json:"controllerKind,omitempty"`
+	Namespace      string                `json:"namespace,omitempty"`
+	Pod            string                `json:"pod,omitempty"`
+	Services       []string              `json:"services,omitempty"`
+	ProviderID     string                `json:"providerID,omitempty"`
+	Labels         AllocationLabels      `json:"labels,omitempty"`
+	Annotations    AllocationAnnotations `json:"annotations,omitempty"`
+	// When set to true, maintain the intersection of all labels + annotations
+	// in the aggregated AllocationProperties object
+	AggregatedMetadata bool `json:"-"`
 }
 
 // AllocationLabels is a schema-free mapping of key/value pairs that can be
@@ -445,6 +447,15 @@ func (p *AllocationProperties) Intersection(that *AllocationProperties) *Allocat
 		// ignore the incoming labels from unallocated or unmounted special case pods
 		if p.AggregatedMetadata || that.AggregatedMetadata {
 			intersectionProps.AggregatedMetadata = true
+
+			// When aggregating by metadata, we maintain the intersection of the labels/annotations
+			// of the two AllocationProperties objects being intersected here.
+			// Special case unallocated/unmounted Allocations never have any labels or annotations.
+			// As a result, they have the effect of always clearing out the intersection,
+			// regardless if all the other actual allocations/etc have them.
+			// This logic is designed to effectively ignore the unmounted/unallocated objects
+			// and just copy over the labels from the other object - we only take the intersection
+			// of 'legitimate' allocations.
 			if p.Container == UnmountedSuffix || p.Container == UnallocatedSuffix {
 				intersectionProps.Annotations = that.Annotations
 				intersectionProps.Labels = that.Labels

pkg/kubecost/allocationprops_test.go

@@ -5,6 +5,110 @@ import (
 	"testing"
 )
 
+func TestAllocationPropsIntersection(t *testing.T) {
+	cases := map[string]struct {
+		allocationProps1 *AllocationProperties
+		allocationProps2 *AllocationProperties
+		expected         *AllocationProperties
+	}{
+		"intersection two allocation properties with empty labels/annotations": {
+			allocationProps1: &AllocationProperties{
+				Labels:      map[string]string{},
+				Annotations: map[string]string{},
+			},
+			allocationProps2: &AllocationProperties{
+				Labels:      map[string]string{},
+				Annotations: map[string]string{},
+			},
+			expected: &AllocationProperties{
+				Labels:      nil,
+				Annotations: nil,
+			},
+		},
+		"nil intersection": {
+			allocationProps1: nil,
+			allocationProps2: nil,
+			expected:         nil,
+		},
+		"intersection, with labels/annotations, no aggregated metdata": {
+			allocationProps1: &AllocationProperties{
+				AggregatedMetadata: false,
+				Node:               "node1",
+				Labels:             map[string]string{"key1": "val1"},
+				Annotations:        map[string]string{"key2": "val2"},
+			},
+			allocationProps2: &AllocationProperties{
+				AggregatedMetadata: false,
+				Node:               "node1",
+				Labels:             map[string]string{"key3": "val3"},
+				Annotations:        map[string]string{"key4": "val4"},
+			},
+			expected: &AllocationProperties{
+				AggregatedMetadata: false,
+				Node:               "node1",
+				Labels:             nil,
+				Annotations:        nil,
+			},
+		},
+		"intersection, with labels/annotations, with aggregated metdata": {
+			allocationProps1: &AllocationProperties{
+				AggregatedMetadata: false,
+				ControllerKind:     "controller1",
+				Namespace:          "ns1",
+				Labels:             map[string]string{"key1": "val1"},
+				Annotations:        map[string]string{"key2": "val2"},
+			},
+			allocationProps2: &AllocationProperties{
+				AggregatedMetadata: true,
+				ControllerKind:     "controller2",
+				Namespace:          "ns1",
+				Labels:             map[string]string{"key1": "val1"},
+				Annotations:        map[string]string{"key2": "val2"},
+			},
+			expected: &AllocationProperties{
+				AggregatedMetadata: true,
+				Namespace:          "ns1",
+				ControllerKind:     "",
+				Labels:             map[string]string{"key1": "val1"},
+				Annotations:        map[string]string{"key2": "val2"},
+			},
+		},
+		"intersection, with labels/annotations, special case container": {
+			allocationProps1: &AllocationProperties{
+				AggregatedMetadata: false,
+				Container:          UnallocatedSuffix,
+				Namespace:          "ns1",
+				Labels:             map[string]string{},
+				Annotations:        map[string]string{},
+			},
+			allocationProps2: &AllocationProperties{
+				AggregatedMetadata: true,
+				Container:          "container3",
+				Namespace:          "ns1",
+				Labels:             map[string]string{"key1": "val1"},
+				Annotations:        map[string]string{"key2": "val2"},
+			},
+			expected: &AllocationProperties{
+				AggregatedMetadata: true,
+				Namespace:          "ns1",
+				ControllerKind:     "",
+				Labels:             map[string]string{"key1": "val1"},
+				Annotations:        map[string]string{"key2": "val2"},
+			},
+		},
+	}
+
+	for name, tc := range cases {
+		t.Run(name, func(t *testing.T) {
+
+			actual := tc.allocationProps1.Intersection(tc.allocationProps2)
+
+			if !reflect.DeepEqual(actual, tc.expected) {
+				t.Fatalf("test case %s: expected %+v; got %+v", name, tc.expected, actual)
+			}
+		})
+	}
+}
 func TestGenerateKey(t *testing.T) {
 
 	customOwnerLabelConfig := NewLabelConfig()