From 7b0071d71b9bdbd921ad157a4d2642aa596aa923 Mon Sep 17 00:00:00 2001
From: Patrick Ohly <patrick.ohly@intel.com>
Date: Thu, 17 Oct 2024 17:52:05 +0200
Subject: [PATCH] DRA CEL: disable runtime cost check

In DRA, the cost check is done only at validation time.  At runtime, any
expression that passed validation gets executed without interrupting it. The
advantage is that it becomes easier to change the limit because stored
expression do not suddenly fail after an up- or downgrade. The limit could
even become a configuration parameter of the apiserver because that is the only
place where the limit gets checked
---
 .../cel/compile.go                            | 24 +++++++++++++++----
 .../cel/compile_test.go                       |  7 +++---
 2 files changed, 22 insertions(+), 9 deletions(-)

diff --git a/staging/src/k8s.io/dynamic-resource-allocation/cel/compile.go b/staging/src/k8s.io/dynamic-resource-allocation/cel/compile.go
index 53418314a47..663e550be92 100644
--- a/staging/src/k8s.io/dynamic-resource-allocation/cel/compile.go
+++ b/staging/src/k8s.io/dynamic-resource-allocation/cel/compile.go
@@ -20,6 +20,7 @@ import (
 	"context"
 	"errors"
 	"fmt"
+	"math"
 	"reflect"
 	"strings"
 	"sync"
@@ -130,12 +131,25 @@ func (c compiler) CompileCELExpression(expression string, envType environment.Ty
 		return resultError("unexpected compilation error: "+err.Error(), apiservercel.ErrorTypeInternal)
 	}
 	prog, err := env.Program(ast,
-		// cel.CostLimit is also set to the VAP PerCallLimit as part of
-		// the base environment.
+		// The Kubernetes CEL base environment sets the VAP cost limit.
 		//
-		// This call here should override that. In practice it shouldn't
-		// matter because the limits are the same.
-		cel.CostLimit(resourceapi.CELSelectorExpressionMaxCost),
+		// In DRA, the cost check is done only at validation time.  At
+		// runtime, any expression that passed validation gets executed
+		// without interrupting it. The advantage is that it becomes
+		// easier to change the limit because stored expression do not
+		// suddenly fail after an up- or downgrade.  The limit could
+		// even become a configuration parameter of the apiserver
+		// because that is the only place where the limit gets checked
+		//
+		// We cannot unset the cost limit
+		// (https://github.com/kubernetes/kubernetes/blob/9568a2ac145cf9be930e3da835f86c1e61f7f7c1/vendor/github.com/google/cel-go/cel/options.go#L512-L518). But
+		// we can set a high value that then never triggers
+		// (https://github.com/kubernetes/kubernetes/blob/9568a2ac145cf9be930e3da835f86c1e61f7f7c1/vendor/github.com/google/cel-go/interpreter/runtimecost.go#L104-L106).
+		//
+		// Even better would be to enable cost tracking only during
+		// validation, but for that k8s.io/apiserver/pkg/cel/environment would
+		// have to be changed to not set the limit.
+		cel.CostLimit(math.MaxInt64),
 		cel.InterruptCheckFrequency(celconfig.CheckFrequency),
 	)
 	if err != nil {
diff --git a/staging/src/k8s.io/dynamic-resource-allocation/cel/compile_test.go b/staging/src/k8s.io/dynamic-resource-allocation/cel/compile_test.go
index ee697a0495b..b0fa08f30f9 100644
--- a/staging/src/k8s.io/dynamic-resource-allocation/cel/compile_test.go
+++ b/staging/src/k8s.io/dynamic-resource-allocation/cel/compile_test.go
@@ -227,10 +227,9 @@ device.attributes["dra.example.com"]["version"].isGreaterThan(semver("0.0.1"))
 				}
 				return attributes
 			}(),
-			expression:       `device.attributes["dra.example.com"].map(s, s.lowerAscii()).map(s, s.size()).sum() == 0`,
-			driver:           "dra.example.com",
-			expectMatchError: "actual cost limit exceeded",
-			expectCost:       18446744073709551615, // Exceeds limit!
+			expression: `device.attributes["dra.example.com"].map(s, s.lowerAscii()).map(s, s.size()).sum() == 0`,
+			driver:     "dra.example.com",
+			expectCost: 18446744073709551615, // Exceeds limit!
 		},
 	} {
 		t.Run(name, func(t *testing.T) {