fix: Apply allocation filters before aggregation to ensure AND operator order independence (#3394)

Signed-off-by: Sparsh <sparsh.raj30@gmail.com>

core/pkg/opencost/allocationfilter_test.go

@@ -736,6 +736,89 @@ func Test_AllocationFilterAnd_Matches(t *testing.T) {
 	}
 }
 
+// Test_AllocationFilterAnd_OrderIndependence tests that AND filters work correctly
+// regardless of filter order and that all allocation properties are available for
+// matching before aggregation. This ensures filters like "namespace+cluster" produce
+// the same result as "cluster+namespace" and that filtering on properties like cluster
+// works even when results will later be aggregated by namespace.
+// Addresses: https://github.com/opencost/opencost/issues/3371
+func Test_AllocationFilterAnd_OrderIndependence(t *testing.T) {
+	cases := []struct {
+		name         string
+		a            *Allocation
+		filterString string
+		expected     bool
+	}{
+		{
+			name: "namespace matches, cluster does not -> should be false",
+			a: &Allocation{
+				Properties: &AllocationProperties{
+					Namespace: "ns1",
+					Cluster:   "cluster-one",
+				},
+			},
+			filterString: `namespace:"ns1"+cluster:"non-existent-uuid"`,
+			expected:     false,
+		},
+		{
+			name: "cluster first, namespace second (reversed order) -> should be false",
+			a: &Allocation{
+				Properties: &AllocationProperties{
+					Namespace: "ns1",
+					Cluster:   "cluster-one",
+				},
+			},
+			filterString: `cluster:"non-existent-uuid"+namespace:"ns1"`,
+			expected:     false,
+		},
+		{
+			name: "both namespace and cluster match -> should be true",
+			a: &Allocation{
+				Properties: &AllocationProperties{
+					Namespace: "ns1",
+					Cluster:   "cluster-one",
+				},
+			},
+			filterString: `namespace:"ns1"+cluster:"cluster-one"`,
+			expected:     true,
+		},
+		{
+			name: "namespace only filter -> should match",
+			a: &Allocation{
+				Properties: &AllocationProperties{
+					Namespace: "ns1",
+					Cluster:   "cluster-one",
+				},
+			},
+			filterString: `namespace:"ns1"`,
+			expected:     true,
+		},
+	}
+
+	parser := afilter.NewAllocationFilterParser()
+	compiler := NewAllocationMatchCompiler(nil)
+
+	for _, c := range cases {
+		t.Run(c.name, func(t *testing.T) {
+			filterNode, err := parser.Parse(c.filterString)
+			if err != nil {
+				t.Fatalf("Failed to parse filter '%s': %s", c.filterString, err)
+			}
+
+			matcher, err := compiler.Compile(filterNode)
+			if err != nil {
+				t.Fatalf("Failed to compile filter '%s': %s", c.filterString, err)
+			}
+
+			result := matcher.Matches(c.a)
+			if result != c.expected {
+				t.Errorf("Filter '%s' expected %t, got %t. AST: %s",
+					c.filterString, c.expected, result, ast.ToPreOrderShortString(filterNode))
+			}
+		})
+	}
+}
+
 func Test_AllocationFilterOr_Matches(t *testing.T) {
 	cases := []struct {
 		name   string

pkg/costmodel/aggregation.go

@@ -227,7 +227,11 @@ func (a *Accesses) ComputeAllocationHandler(w http.ResponseWriter, r *http.Reque
 	// Get allocation filter if provided
 	allocationFilter := qp.Get("filter", "")
 
-	asr, err := a.Model.QueryAllocation(window, step, aggregateBy, includeIdle, idleByNode, includeProportionalAssetResourceCosts, includeAggregatedMetadata, sharedLoadBalancer, accumulateBy, shareIdle)
+	// Query allocations with filtering, aggregation, and accumulation.
+	// Filtering is done BEFORE aggregation inside QueryAllocation to ensure
+	// filters can match on all allocation properties (like cluster, node, etc.)
+	// before they are potentially lost or merged during aggregation.
+	asr, err := a.Model.QueryAllocation(window, step, aggregateBy, includeIdle, idleByNode, includeProportionalAssetResourceCosts, includeAggregatedMetadata, sharedLoadBalancer, accumulateBy, shareIdle, allocationFilter)
 	if err != nil {
 		if strings.Contains(strings.ToLower(err.Error()), "bad request") {
 			proto.WriteError(w, proto.BadRequest(err.Error()))
@@ -238,34 +242,5 @@ func (a *Accesses) ComputeAllocationHandler(w http.ResponseWriter, r *http.Reque
 		return
 	}
 
-	// Apply allocation filter if provided
-	if allocationFilter != "" {
-		parser := allocation.NewAllocationFilterParser()
-		filterNode, err := parser.Parse(allocationFilter)
-		if err != nil {
-			proto.WriteError(w, proto.BadRequest(fmt.Sprintf("Invalid filter: %s", err)))
-			return
-		}
-		compiler := opencost.NewAllocationMatchCompiler(nil)
-		matcher, err := compiler.Compile(filterNode)
-		if err != nil {
-			proto.WriteError(w, proto.BadRequest(fmt.Sprintf("Failed to compile filter: %s", err)))
-			return
-		}
-		filteredASR := opencost.NewAllocationSetRange()
-		for _, as := range asr.Slice() {
-			filteredAS := opencost.NewAllocationSet(as.Start(), as.End())
-			for _, alloc := range as.Allocations {
-				if matcher.Matches(alloc) {
-					filteredAS.Set(alloc)
-				}
-			}
-			if filteredAS.Length() > 0 {
-				filteredASR.Append(filteredAS)
-			}
-		}
-		asr = filteredASR
-	}
-
 	WriteData(w, asr, nil)
 }

pkg/costmodel/costmodel.go

@@ -13,6 +13,7 @@ import (
 	"github.com/opencost/opencost/core/pkg/clustercache"
 	"github.com/opencost/opencost/core/pkg/clusters"
 	coreenv "github.com/opencost/opencost/core/pkg/env"
+	"github.com/opencost/opencost/core/pkg/filter/allocation"
 	"github.com/opencost/opencost/core/pkg/log"
 	"github.com/opencost/opencost/core/pkg/opencost"
 	"github.com/opencost/opencost/core/pkg/source"
@@ -1554,7 +1555,7 @@ func measureTime(start time.Time, threshold time.Duration, name string) {
 	}
 }
 
-func (cm *CostModel) QueryAllocation(window opencost.Window, step time.Duration, aggregate []string, includeIdle, idleByNode, includeProportionalAssetResourceCosts, includeAggregatedMetadata, sharedLoadBalancer bool, accumulateBy opencost.AccumulateOption, shareIdle bool) (*opencost.AllocationSetRange, error) {
+func (cm *CostModel) QueryAllocation(window opencost.Window, step time.Duration, aggregate []string, includeIdle, idleByNode, includeProportionalAssetResourceCosts, includeAggregatedMetadata, sharedLoadBalancer bool, accumulateBy opencost.AccumulateOption, shareIdle bool, filterString string) (*opencost.AllocationSetRange, error) {
 	// Validate window is legal
 	if window.IsOpen() || window.IsNegative() {
 		return nil, fmt.Errorf("illegal window: %s", window)
@@ -1624,6 +1625,33 @@ func (cm *CostModel) QueryAllocation(window opencost.Window, step time.Duration,
 		stepEnd = stepStart.Add(step)
 	}
 
+	// Apply allocation filter BEFORE aggregation if provided
+	if filterString != "" {
+		parser := allocation.NewAllocationFilterParser()
+		filterNode, err := parser.Parse(filterString)
+		if err != nil {
+			return nil, fmt.Errorf("invalid filter: %w", err)
+		}
+		compiler := opencost.NewAllocationMatchCompiler(nil)
+		matcher, err := compiler.Compile(filterNode)
+		if err != nil {
+			return nil, fmt.Errorf("failed to compile filter: %w", err)
+		}
+		filteredASR := opencost.NewAllocationSetRange()
+		for _, as := range asr.Slice() {
+			filteredAS := opencost.NewAllocationSet(as.Start(), as.End())
+			for _, alloc := range as.Allocations {
+				if matcher.Matches(alloc) {
+					filteredAS.Set(alloc)
+				}
+			}
+			if filteredAS.Length() > 0 {
+				filteredASR.Append(filteredAS)
+			}
+		}
+		asr = filteredASR
+	}
+
 	// Set aggregation options and aggregate
 	var shareIdleOpt string
 	if shareIdle {