unit tests for unmounted allocations bug (#1876)

* unit tests for unmounted allocations bug

Signed-off-by: nickcurie <nick.curie64@gmail.com>

* fix for unmounted services showing + updated unit test

Signed-off-by: nickcurie <nick.curie64@gmail.com>

* uncommented fix

Signed-off-by: nickcurie <nick.curie64@gmail.com>

* cleaned up test, PR feedback

Signed-off-by: nickcurie <nick.curie64@gmail.com>

---------

Signed-off-by: nickcurie <nick.curie64@gmail.com>

pkg/kubecost/allocation_test.go

@@ -4,9 +4,11 @@ import (
 	"fmt"
 	"math"
 	"reflect"
+	"strings"
 	"testing"
 	"time"
 
+	"github.com/davecgh/go-spew/spew"
 	"github.com/opencost/opencost/pkg/util"
 	"github.com/opencost/opencost/pkg/util/json"
 )
@@ -2767,3 +2769,82 @@ func TestAllocationSet_Accumulate_Equals_AllocationSetRange_Accumulate(t *testin
 		}
 	}
 }
+
+func Test_AggregateByService_UnmountedLBs(t *testing.T) {
+	end := time.Now().UTC().Truncate(day)
+	start := end.Add(-day)
+
+	normalProps := &AllocationProperties{
+		Cluster:        "cluster-one",
+		Container:      "nginx-plus-nginx-ingress",
+		Controller:     "nginx-plus-nginx-ingress",
+		ControllerKind: "deployment",
+		Namespace:      "nginx-plus",
+		Pod:            "nginx-plus-nginx-ingress-123a4b5678-ab12c",
+		ProviderID:     "test",
+		Node:           "testnode",
+		Services: []string{
+			"nginx-plus-nginx-ingress",
+		},
+	}
+
+	problematicProps := &AllocationProperties{
+		Cluster:    "cluster-one",
+		Container:  UnmountedSuffix,
+		Namespace:  UnmountedSuffix,
+		Pod:        UnmountedSuffix,
+		ProviderID: "test",
+		Node:       "testnode",
+		Services: []string{
+			"nginx-plus-nginx-ingress",
+			"ingress-nginx-controller",
+			"pacman",
+		},
+	}
+
+	idle := NewMockUnitAllocation(fmt.Sprintf("cluster-one/%s", IdleSuffix), start, day, &AllocationProperties{
+		Cluster: "cluster-one",
+	})
+	// this allocation is the main point of the test; an unmounted LB that has services
+	problematicAllocation := NewMockUnitAllocation("cluster-one//__unmounted__/__unmounted__/__unmounted__", start, day, problematicProps)
+
+	two := NewMockUnitAllocation("cluster-one//nginx-plus/nginx-plus-nginx-ingress-123a4b5678-ab12c/nginx-plus-nginx-ingress", start, day, normalProps)
+	three := NewMockUnitAllocation("cluster-one//nginx-plus/nginx-plus-nginx-ingress-123a4b5678-ab12c/nginx-plus-nginx-ingress", start, day, normalProps)
+	four := NewMockUnitAllocation("cluster-one//nginx-plus/nginx-plus-nginx-ingress-123a4b5678-ab12c/nginx-plus-nginx-ingress", start, day, normalProps)
+
+	problematicAllocation.ExternalCost = 2.35
+	two.ExternalCost = 1.35
+	three.ExternalCost = 2.60
+	four.ExternalCost = 4.30
+	set := NewAllocationSet(start, start.Add(day), problematicAllocation, two, three, four)
+
+	set.Insert(idle)
+
+	set.AggregateBy([]string{AllocationServiceProp}, &AllocationAggregationOptions{
+		Filter: AllocationFilterCondition{Field: FilterServices, Op: FilterContains, Value: "nginx-plus-nginx-ingress"},
+	})
+
+	for _, alloc := range set.Allocations {
+		if !strings.Contains(UnmountedSuffix, alloc.Name) {
+			props := alloc.Properties
+			if props.Cluster == UnmountedSuffix {
+				t.Error("cluster unmounted")
+			}
+			if props.Container == UnmountedSuffix {
+				t.Error("container unmounted")
+			}
+			if props.Namespace == UnmountedSuffix {
+				t.Error("namespace unmounted")
+			}
+			if props.Pod == UnmountedSuffix {
+				t.Error("pod unmounted")
+			}
+			if props.Controller == UnmountedSuffix {
+				t.Error("controller unmounted")
+			}
+		}
+	}
+
+	spew.Config.DisableMethods = true
+	t.Logf("%s", spew.Sdump(set.Allocations))
+}

pkg/kubecost/allocationprops.go

@@ -288,10 +288,18 @@ func (p *AllocationProperties) GenerateKey(aggregateBy []string, labelConfig *La
 				// Indicate that allocation has no services
 				names = append(names, UnallocatedSuffix)
 			} else {
-				// This just uses the first service
-				for _, service := range services {
-					names = append(names, service)
-					break
+				// Unmounted load balancers lead to __unmounted__ Allocations whose
+				// services field is populated. If we don't have a special case, the
+				// __unmounted__ Allocation will be transformed into a regular Allocation,
+				// causing issues with AggregateBy and drilldown
+				if p.Pod == UnmountedSuffix || p.Namespace == UnmountedSuffix || p.Container == UnmountedSuffix {
+					names = append(names, UnmountedSuffix)
+				} else {
+					// This just uses the first service
+					for _, service := range services {
+						names = append(names, service)
+						break
+					}
 				}
 			}
 		case strings.HasPrefix(agg, "label:"):

pkg/kubecost/allocationprops_test.go

@@ -96,6 +96,32 @@ func TestAllocationPropsIntersection(t *testing.T) {
 				Annotations:        map[string]string{"key2": "val2"},
 			},
 		},
+		"test services are nulled when intersecting": {
+			allocationProps1: &AllocationProperties{
+				AggregatedMetadata: false,
+				Container:          UnmountedSuffix,
+				Namespace:          "ns1",
+				Services: []string{
+					"cool",
+				},
+				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 {