[KEP-0009] feat: add expression based assertions #576

kumar-mallikarjuna · 2024-11-23T08:45:22Z

What this PR does / why we need it:
This PR adds CEL-expression based assertions to TestAsserts. See https://github.com/kudobuilder/kuttl/blob/main/keps/0009-expression-based-assertions.md for more details.

Fixes #562

porridge

Nice, but needs a few changes.

pkg/apis/testharness/v1beta1/test_types.go

pkg/test/utils/kubernetes.go

eddycharly · 2024-11-25T11:05:05Z

pkg/test/utils/kubernetes.go

+		variables[resourceRef.Id] = referencedResource.Object
+	}
+
+	env, err := cel.NewEnv()


Calling cel.NewEnv() may not be enough, you probably want to enable a couple of options/libs.

@eddycharly any hints on what might be useful? 🤔

pkg/test/utils/kubernetes.go

pkg/apis/testharness/v1beta1/expression.go

pkg/test/step.go

pkg/test/utils/kubernetes.go

pkg/test/utils/expression.go

pkg/test/utils/kubernetes.go

kumar-mallikarjuna · 2024-12-03T17:24:36Z

Thanks for the quick review @porridge . I'll add the tests tomorrow.

porridge

Thanks, this looks much better, but some more changes are needed. 🙏🏻
Please see inline.

pkg/test/step.go

pkg/test/expression_integration_test.go

pkg/expressions/cel.go

pkg/test/step.go

pkg/apis/testharness/v1beta1/expression.go

pkg/expressions/cel.go

pkg/test/expression_integration_test.go

porridge

Almost there!
Just a bunch of nitpicks for messages and identifier names, and one issue in the test..

pkg/expressions/cel.go

pkg/test/expression_integration_test.go

...gration_test_data/assert_expressions/check_expression_for_ephemeral_namespace/01-assert.yaml

...gration_test_data/assert_expressions/check_expression_for_ephemeral_namespace/00-create.yaml

porridge · 2024-12-19T12:26:41Z

pkg/test/utils/kubernetes.go

+		variables[resourceRef.Id] = referencedResource.Object
+	}
+
+	env, err := cel.NewEnv()


@eddycharly any hints on what might be useful? 🤔

porridge

A few more things.

pkg/test/expression_integration_test.go

pkg/expressions/cel.go

pkg/test/expression_integration_test.go

...gration_test_data/assert_expressions/check_expression_for_ephemeral_namespace/01-assert.yaml

This PR adds CEL-expression based assertions to `TestAsserts`. See https://github.com/kudobuilder/kuttl/blob/main/keps/0009-expression-based-assertions.md for more details. Signed-off-by: Kumar Mallikarjuna <[email protected]>

Signed-off-by: Kumar Mallikarjuna <[email protected]>

…re present Signed-off-by: Kumar Mallikarjuna <[email protected]>

Signed-off-by: Kumar Mallikarjuna <[email protected]>

…pace Signed-off-by: Kumar Mallikarjuna <[email protected]>

Signed-off-by: Kumar Mallikarjuna <[email protected]>

porridge

LGTM now, just the debugging leftovers need to go :-)

pkg/test/step.go

pkg/test/expression_integration_test.go

Signed-off-by: Kumar Mallikarjuna <[email protected]>

eddycharly · 2025-01-08T20:32:25Z

Shouldn't you verify that the program return type is a boolean ?

Something like:

...
if !ast.OutputType().IsExactType(types.BoolType) ...
...

eddycharly · 2025-01-08T20:35:55Z

pkg/expressions/cel.go

+		return nil, fmt.Errorf("failed to load resource reference(s): %w", errors.Join(errs...))
+	}
+
+	env, err := cel.NewEnv()


Are you extending the env somewhere ? Looks like you will want to register a bunch of CEL libs to enable features.

Any suggestions on what libs are a must-have @eddycharly ?

Something like this is a good start IMHO. it enables important language features, register comme libs and kube specific ones.

return cel.NewEnv( // configure env cel.HomogeneousAggregateLiterals(), cel.EagerlyValidateDeclarations(true), cel.DefaultUTCTimeZone(true), cel.CrossTypeNumericComparisons(true), // register common libs cel.OptionalTypes(), ext.Bindings(), ext.Encoders(), ext.Lists(), ext.Math(), ext.Protos(), ext.Sets(), ext.Strings(), // register kubernetes libs library.CIDR(), library.Format(), library.IP(), library.Lists(), library.Regex(), library.URLs(), )

eddycharly · 2025-01-08T20:42:17Z

pkg/expressions/cel.go

+	}
+
+	for _, resourceRef := range resourceRefs {
+		env, err = env.Extend(cel.Variable(resourceRef.Ref, cel.DynType))


Would be nice to use a type provider instead of DynType.

eddycharly · 2025-01-08T20:46:19Z

pkg/expressions/cel.go

+}
+
+func evaluateExpression(expr string,
+	programs map[string]cel.Program,


Why create a map to lookup the program corresponding to an expression ? 🤔

@eddycharly programs are compiled early at YAML loading time, so here we just access them. We use a map in order to mention the problematic expression in the error message in case it fails.

I don't get it, if programs are compiled at load time you shouldn't even reach evaluateExpression right ?
I didn't look at it deeply but i would expect the focus is on the expression path, not the expression itself.

An expression can compile OK (and its good to do it ASAP to catch syntax errors, etc), but evaluation can fail later (depending on resources in the cluster). An error message about the evaluation should mention the expression itself, so the user knows which expression was the problematic one.

eddycharly · 2025-01-08T20:49:29Z

pkg/apis/testharness/v1beta1/test_types.go

@@ -157,6 +157,11 @@ type TestAssert struct {
 	Collectors []*TestCollector `json:"collectors,omitempty"`
 	// Commands is a set of commands to be run as assertions for the current step
 	Commands []TestAssertCommand `json:"commands,omitempty"`
+
+	ResourceRefs []TestResourceRef `json:"resourceRefs,omitempty"`


You should consider two TestResourceRef with the same id an error.

eddycharly · 2025-01-08T20:52:53Z

pkg/expressions/cel.go

+	var anyExprErrors, allExprErrors []error
+	for _, expr := range assertAny {
+		if err := evaluateExpression(expr.CELExpression, programs, variables); err != nil {
+			anyExprErrors = append(anyExprErrors, err)
+		}
+	}
+
+	for _, expr := range assertAll {
+		if err := evaluateExpression(expr.CELExpression, programs, variables); err != nil {
+			allExprErrors = append(allExprErrors, err)
+		}
+	}
+
+	if len(assertAny) != 0 && len(anyExprErrors) == len(assertAny) {
+		errs = append(errs, fmt.Errorf("no expression evaluated to true: %w", errors.Join(anyExprErrors...)))
+	}
+
+	if len(allExprErrors) > 0 {
+		errs = append(errs, fmt.Errorf("not all assertAll expressions evaluated to true: %w", errors.Join(allExprErrors...)))
+	}
+
+	return errs


This would make sense to return early here instead of evaluating all expressions before making a decision.

eddycharly · 2025-01-08T20:54:17Z

pkg/test/step.go

+	variables := make(map[string]interface{})
+	for _, resourceRef := range s.Assert.ResourceRefs {
+		if resourceRef.Namespace == "" {
+			resourceRef.Namespace = namespace
+		}
+		namespacedName, referencedResource := resourceRef.BuildResourceReference()
+		if err := client.Get(context.TODO(), namespacedName, referencedResource); err != nil {
+			return []error{fmt.Errorf("failed to get referenced resource '%v': %w", namespacedName, err)}
+		}
+
+		variables[resourceRef.Ref] = referencedResource.Object
+	}


This should be lazy loaded.

@eddycharly you mean as a performance improvement? Or do you see a correctness issue?

Somehow both. If the first condition is met in any why bother evaluating other conditions ?
I can imagine cases where the first condition returns true but the second condition throws an error. Returning early makes the evaluation succeed.

(this may or may not be what you want though)

porridge

Sorry for the delay in reviewing.

I think @eddycharly made a bunch of great suggestions. I know too little about CEL to judge with certainty, but I don't think any of these are really blocking. Perhaps we can do them as followup improvements, as the code looks OK in general, and this review is already taking waaaay longer than I expected. And it would feel good to get something usable merged already even if it's not perfect from the start. Unless you feel like addressing at least some of them straight away.

However one thing I completely forgot to mention earlier is documentation. Please at least update the TestAssert schema description and throw in a really short one-pager about this feature somewhere under docs/testing (probably a new file and link to it where appropriate). We can iterate on improving it later.

kumar-mallikarjuna · 2025-01-14T17:31:40Z

I think @eddycharly made a bunch of great suggestions. I know too little about CEL to judge with certainty, but I don't think any of these are really blocking. Perhaps we can do them as followup improvements, as the code looks OK in general, and this review is already taking waaaay longer than I expected. And it would feel good to get something usable merged already even if it's not perfect from the start. Unless you feel like addressing at least some of them straight away.

Thanks for the suggestions, @eddycharly ! I'd vote to merge this as well and include improvements in a separate PR.

However one thing I completely forgot to mention earlier is documentation. Please at least update the TestAssert schema description and throw in a really short one-pager about this feature somewhere under docs/testing (probably a new file and link to it where appropriate). We can iterate on improving it later.

Let me add this tomorrow during my day.

Thanks again @porridge , @eddycharly for the thorough review! :)

porridge requested changes Nov 25, 2024

View reviewed changes

eddycharly reviewed Nov 25, 2024

View reviewed changes

pkg/test/utils/kubernetes.go Outdated Show resolved Hide resolved

kumar-mallikarjuna force-pushed the KEP-0009 branch 2 times, most recently from 0fe748e to a85752a Compare December 3, 2024 07:56

kumar-mallikarjuna requested a review from porridge December 3, 2024 09:33

porridge requested changes Dec 3, 2024

View reviewed changes

kumar-mallikarjuna mentioned this pull request Dec 4, 2024

Refactor pkg/test/utils/kubernetes.go #589

Closed

kumar-mallikarjuna marked this pull request as ready for review December 4, 2024 17:47

kumar-mallikarjuna requested a review from kensipe as a code owner December 4, 2024 17:47

kumar-mallikarjuna requested a review from porridge December 5, 2024 13:46

porridge requested changes Dec 10, 2024

View reviewed changes

kumar-mallikarjuna force-pushed the KEP-0009 branch from 7044f54 to a7122a6 Compare December 19, 2024 07:44

porridge requested changes Dec 19, 2024

View reviewed changes

kumar-mallikarjuna force-pushed the KEP-0009 branch from 1752619 to 32bcb9c Compare December 19, 2024 17:04

kumar-mallikarjuna requested review from eddycharly and porridge January 2, 2025 06:06

porridge requested changes Jan 2, 2025

View reviewed changes

kumar-mallikarjuna requested a review from porridge January 7, 2025 12:36

kumar-mallikarjuna added 10 commits January 7, 2025 18:09

[KEP-0009] feat: add expression based assertions

bd77825

This PR adds CEL-expression based assertions to `TestAsserts`. See https://github.com/kudobuilder/kuttl/blob/main/keps/0009-expression-based-assertions.md for more details. Signed-off-by: Kumar Mallikarjuna <[email protected]>

chore: rename Id->Ref and make linter happy

68c60e3

Signed-off-by: Kumar Mallikarjuna <[email protected]>

chore: add validation for resourceRefs

9a94a6b

Signed-off-by: Kumar Mallikarjuna <[email protected]>

chore: make assertion syntax consistent with the KEP

38819ef

Signed-off-by: Kumar Mallikarjuna <[email protected]>

refactor: rename Validate method for TestResourceRef

3a6b02e

Signed-off-by: Kumar Mallikarjuna <[email protected]>

chore: pre-build environment and program for expressions

bd362f2

Signed-off-by: Kumar Mallikarjuna <[email protected]>

chore: make linter happy and initialize Programs only if assertions a…

14ad7a8

…re present Signed-off-by: Kumar Mallikarjuna <[email protected]>

chore: incorporate review comments

6df2e32

Signed-off-by: Kumar Mallikarjuna <[email protected]>

chore: move RunAssertExpressions() to pkg/expressions

aa6cb65

Signed-off-by: Kumar Mallikarjuna <[email protected]>

refactor: move CEL program loading to a dedicated function

832bf9e

Signed-off-by: Kumar Mallikarjuna <[email protected]>

kumar-mallikarjuna added 17 commits January 7, 2025 18:09

fix: correct evaluation for assertAll

f08a27e

Signed-off-by: Kumar Mallikarjuna <[email protected]>

chore: add integration tests for expressions

7cb27fc

Signed-off-by: Kumar Mallikarjuna <[email protected]>

chore: make linter happy

3fcf018

Signed-off-by: Kumar Mallikarjuna <[email protected]>

chore: incorporate review comments

c6c3e8a

Signed-off-by: Kumar Mallikarjuna <[email protected]>

fix: change array reference for all-asserts

51e3deb

Signed-off-by: Kumar Mallikarjuna <[email protected]>

chore: change signature for CheckAssertExpressions()

32330e9

Signed-off-by: Kumar Mallikarjuna <[email protected]>

chore: use envtest in expression_integration_test.go

5e252f5

Signed-off-by: Kumar Mallikarjuna <[email protected]>

fix: remove redundant definitions for integration test tools

56c1779

Signed-off-by: Kumar Mallikarjuna <[email protected]>

chore: use testenv in expression_integration_test.go

2c58b63

Signed-off-by: Kumar Mallikarjuna <[email protected]>

fix: run target expressions test within a Kuttl owned ephemeral names…

aa2fa55

…pace Signed-off-by: Kumar Mallikarjuna <[email protected]>

chore: remove redundant file and update go.mod

9e77060

Signed-off-by: Kumar Mallikarjuna <[email protected]>

chore: incorporate review comments

f824fb9

Signed-off-by: Kumar Mallikarjuna <[email protected]>

chore: update error messages

965b7eb

Signed-off-by: Kumar Mallikarjuna <[email protected]>

chore: update error message

5173e6c

Signed-off-by: Kumar Mallikarjuna <[email protected]>

chore: incorporate review comments

4f358b0

Signed-off-by: Kumar Mallikarjuna <[email protected]>

fix: update context creation

60b818d

Signed-off-by: Kumar Mallikarjuna <[email protected]>

chore: aggregate test steps into one

dd85a98

Signed-off-by: Kumar Mallikarjuna <[email protected]>

kumar-mallikarjuna force-pushed the KEP-0009 branch from dd2131a to dd85a98 Compare January 7, 2025 12:40

porridge requested changes Jan 8, 2025

View reviewed changes

pkg/test/step.go Outdated Show resolved Hide resolved

pkg/test/expression_integration_test.go Outdated Show resolved Hide resolved

kumar-mallikarjuna added 2 commits January 8, 2025 22:21

chore: remove debug statements

d510656

Signed-off-by: Kumar Mallikarjuna <[email protected]>

chore(tests): remove prefixes for expression integration test steps

7c164ba

Signed-off-by: Kumar Mallikarjuna <[email protected]>

kumar-mallikarjuna requested a review from porridge January 8, 2025 17:17

eddycharly reviewed Jan 8, 2025

View reviewed changes

porridge requested changes Jan 13, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[KEP-0009] feat: add expression based assertions #576

[KEP-0009] feat: add expression based assertions #576

kumar-mallikarjuna commented Nov 23, 2024

porridge left a comment

eddycharly Nov 25, 2024

porridge Dec 19, 2024

kumar-mallikarjuna commented Dec 3, 2024

porridge left a comment

porridge left a comment

porridge Dec 19, 2024

porridge left a comment

porridge left a comment

eddycharly commented Jan 8, 2025 •

edited

Loading

eddycharly Jan 8, 2025

porridge Jan 13, 2025

eddycharly Jan 13, 2025

eddycharly Jan 8, 2025

eddycharly Jan 8, 2025

porridge Jan 13, 2025

eddycharly Jan 13, 2025

porridge Jan 13, 2025

eddycharly Jan 8, 2025

eddycharly Jan 8, 2025

eddycharly Jan 8, 2025

porridge Jan 13, 2025

eddycharly Jan 13, 2025

eddycharly Jan 13, 2025

porridge left a comment

kumar-mallikarjuna commented Jan 14, 2025

[KEP-0009] feat: add expression based assertions #576

Are you sure you want to change the base?

[KEP-0009] feat: add expression based assertions #576

Conversation

kumar-mallikarjuna commented Nov 23, 2024

porridge left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kumar-mallikarjuna commented Dec 3, 2024

porridge left a comment

Choose a reason for hiding this comment

porridge left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

porridge left a comment

Choose a reason for hiding this comment

porridge left a comment

Choose a reason for hiding this comment

eddycharly commented Jan 8, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

porridge left a comment

Choose a reason for hiding this comment

kumar-mallikarjuna commented Jan 14, 2025

eddycharly commented Jan 8, 2025 •

edited

Loading