Add CEL validation test for targetRef in ClientSettingsPolicy #3623

shaun-nx · 2025-07-14T09:56:57Z

Proposed changes

Add tests for CEL validation test for targetRef in ClientSettingsPolicy

Resolves targetRef CEL validation requirement of 3621

Checklist

Before creating a PR, run through this checklist and mark each as complete.

I have read the CONTRIBUTING doc
I have added tests that prove my fix is effective or that my feature works
I have checked that all unit tests pass after adding my changes
I have updated necessary documentation
I have rebased my branch onto main
I will ensure my PR is targeting the main branch and pulling from my branch from my own fork

nginx-bot · 2025-07-14T09:57:02Z

Hi @shaun-nx! Welcome to the project! 🎉

Thanks for opening this pull request!
Be sure to check out our Contributing Guidelines while you wait for someone on the team to review this.

Please make sure to include the issue number in the PR description to automatically close the issue when the PR is merged.
See Linking a pull request to an issue and our Pull Request Guidelines for more information.

codecov · 2025-07-14T09:59:40Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 87.05%. Comparing base (19fb47b) to head (1adf168).

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3623      +/-   ##
==========================================
+ Coverage   87.02%   87.05%   +0.02%     
==========================================
  Files         127      127              
  Lines       15579    15579              
  Branches       62       62              
==========================================
+ Hits        13558    13562       +4     
+ Misses       1861     1858       -3     
+ Partials      160      159       -1

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

ciarams87

I think what we want is to make sure our CEL validation is correct - this test file is testing Go logic rather than our actual validation (that is, we need to test the CEL expressions themselves, not the logic we hope they are enforcing).

There is a package we could probably use for cel validation k8s.io/apiserver/pkg/cel which might be useful.

ciarams87 · 2025-07-14T13:37:18Z

Or, now that I've read the parent issue, we could follow the pattern set in the Gateway API and simply run some functional tests where we actually create the resources and test the validation is working that way.

bjee19 · 2025-07-14T17:03:20Z

@ciarams87, though looking at the CEL tests in the Gateway API, https://github.com/kubernetes-sigs/gateway-api/blob/main/pkg/test/cel/httproute_test.go, doesn't this PR follow the same standard set by them?

EDIT: oh actually nvm i see where in the gateway api they create the resource.

bjee19 · 2025-07-15T06:06:01Z

Also regards to file structure, i would support just doing a single test file per CRD similar to how the gateway api repo does it. Thus the tests would just be like tests/cel/clientsettingspolicy_test.go and so on.

And i would kinda agree with @ciarams87, though not too sure if a functional test is needed, but would instead follow whats done in the gateway api and create the actual crd object through k8sclient and validate the errors like this:

func validateHTTPRoute(t *testing.T, route *gatewayv1.HTTPRoute, wantErrors []string) {
	t.Helper()

	ctx := context.Background()
	err := k8sClient.Create(ctx, route)

	if (len(wantErrors) != 0) != (err != nil) {
		t.Fatalf("Unexpected response while creating HTTPRoute %q; got err=\n%v\n;want error=%v", fmt.Sprintf("%v/%v", route.Namespace, route.Name), err, wantErrors)
	}

	var missingErrorStrings []string
	for _, wantError := range wantErrors {
		if !celErrorStringMatches(err.Error(), wantError) {
			missingErrorStrings = append(missingErrorStrings, wantError)
		}
	}
	if len(missingErrorStrings) != 0 {
		t.Errorf("Unexpected response while creating HTTPRoute %q; got err=\n%v\n;missing strings within error=%q", fmt.Sprintf("%v/%v", route.Namespace, route.Name), err, missingErrorStrings)
	}
}

shaun-nx · 2025-07-15T07:43:24Z

Thanks @bjee19 and @ciarams87 for the inputs! I didn't notice originally that the cel tests in the in the gateway api created resources in the test. I'll update my tests now to better follow that pattern.

I'll also adjust the filename as per your suggestion @bjee19 :)

ciarams87

I probably should have called it an integration test maybe, but yes that's what I meant @bjee19 - create the actual crd object through k8sclient. That way, we are testing the actual CEL expressions in the CRD. @bjee19 gave you an example from the Gateway API already, but here is one specific to ClientSettingsPolicy for clarity:

func TestClientSettingsPolicyValidation(t *testing.T) {
    ctx := context.Background()
    
    testCases := []struct {
        desc       string
        policy     *ngfAPIv1alpha1.ClientSettingsPolicy
        wantErrors []string
    }{
        {
            desc: "valid Gateway targetRef",
            policy: &ngfAPIv1alpha1.ClientSettingsPolicy{
                ObjectMeta: metav1.ObjectMeta{
                    Name:      "test-policy",
                    Namespace: "default",
                },
                Spec: ngfAPIv1alpha1.ClientSettingsPolicySpec{
                    TargetRef: gatewayv1alpha2.LocalPolicyTargetReference{
                        Kind:  "Gateway",
                        Group: "gateway.networking.k8s.io",
                        Name:  "test-gateway",
                    },
                },
            },
            // No wantErrors = should succeed
        },
        {
            desc: "invalid targetRef kind",
            policy: &ngfAPIv1alpha1.ClientSettingsPolicy{
                ObjectMeta: metav1.ObjectMeta{
                    Name:      "test-policy-invalid",
                    Namespace: "default",
                },
                Spec: ngfAPIv1alpha1.ClientSettingsPolicySpec{
                    TargetRef: gatewayv1alpha2.LocalPolicyTargetReference{
                        Kind:  "Service", // Invalid
                        Group: "gateway.networking.k8s.io",
                        Name:  "test-service",
                    },
                },
            },
            wantErrors: []string{"TargetRef Kind must be one of: Gateway, HTTPRoute, or GRPCRoute"},
        },
    }

    for _, tc := range testCases {
        t.Run(tc.desc, func(t *testing.T) {
            err := k8sClient.Create(ctx, tc.policy)
            
            if (len(tc.wantErrors) != 0) != (err != nil) {
                t.Fatalf("got err=%v; want error=%v", err, tc.wantErrors != nil)
            }
            
            if err != nil {
                for _, wantError := range tc.wantErrors {
                    if !strings.Contains(err.Error(), wantError) {
                        t.Errorf("missing expected error %q in %v", wantError, err)
                    }
                }
            }
        })
    }
}

Notice we are calling err := k8sClient.Create(ctx, tc.policy) to create the actual Policy object, and then asserting on that creation - if we are creating an invalid policy, we expect an error, and we assert the error is one that we expect. Otherwise, we are testing Go logic in the test file instead of the actual CEL validation rules that Kubernetes will enforce.

Your current approach creates a ClientSettingsPolicy struct in memory and then checks it against a Go map, which doesn't invoke the actual CEL validation. When users apply the CRD to a Kubernetes cluster, it's the CEL expressions defined in the CRD spec that validate the resource.

By using k8sClient.Create(), we're testing the exact same validation path that real users will experience: the Kubernetes API server will evaluate your CRD's CEL rules against the incoming resource and either accept or reject it with the actual error messages that CEL produces.

Hopefully this makes sense!

tataruty · 2025-07-17T10:08:58Z

@sjberman is this about those policies with limit of 16? 🤪 Sorry, I guess i will ask till i understand what are those 🥹 Anyway, if they are then maybe there should be a test about limit as well or do we have it in some other test cases?

tataruty · 2025-07-17T16:14:45Z

@sjberman is this about those policies with limit of 16? 🤪 Sorry, I guess i will ask till i understand what are those 🥹 Anyway, if they are then maybe there should be a test about limit as well or do we have it in some other test cases?

or @bjee19 maybe you know the answer to my question?

tests/cel/clientsettingspolicy_test.go

sarthyparty · 2025-07-17T16:31:24Z

tests/cel/clientsettingspolicy_test.go

+	}{
+		{
+			name:       "Validate gateway.networking.k8s.io TargetRef Group is allowed",
+			wantErrors: []string{"TargetRef Group must be gateway.networking.k8s.io."},


Why would there be errors in a validTargetRefGroup

Great point, I didn't notice I had that there. I'll update that

How did this pass in the pipeline? is it actually running? I'm assuming not since our unit tests only run on the internal and cmd directories. Our make unit-test target will need to be updated to also run in tests/cel.

Ah, nice catch @sjberman !
Something must be off in my logic if that's the case. I'll re-review that today

@salonichf5
It looks like the make unit-test target doesn't cover anything in the tests/ directory from what I can see

.PHONY: unit-test unit-test: ## Run unit tests for the go code go test ./cmd/... ./internal/... -buildvcs -race -shuffle=on -coverprofile=coverage.out -covermode=atomic go tool cover -html=coverage.out -o cover.html

I know we also have a Makefile in tests/ as well. Does something need to be updated in there?

The make unit-test target should be updated to include tests/cel/.... The Makefile in the tests/ directory right now controls functional/ngf/conformance tests.

@sjberman this should be resolved now.

There is a new Makefile target for the cel test. I do still need to create a new workflow for these test so that they run as part of the pipeline for other PRs

sarthyparty · 2025-07-17T16:35:42Z

tests/cel/clientsettingspolicy_test.go

+	testInvalidTargetRefGroup(t)
+}
+
+func testValidTargetRefKind(t *testing.T) {


Can you combine testValid…, testInvalid…, etc. into one test block? It seems redundant to have multiple functions that are doing the same thing basically, the valid and invalid can be determined by the name of the the test right?

I like to do this to avoid having a single test function with a massive set of table tests. I find it makes the tests easier to read a digest.

Perhaps, a helper function that runs the tests would be useful for readability

where each of the functions like testValidTargetRefKind just defines the tests and calls the helper

I also agree with @sarthyparty on combining the invalid/valid tests mainly to maintain consistency with the existing tests in our repo and because it follows standards set in the gateway api, example of their test structure: https://github.com/kubernetes-sigs/gateway-api/blob/main/pkg/test/cel/httproute_test.go

@bjee19 that's a fair point. I'll make that update today as well then. Thanks :)

@bjee19 @sarthyparty this should be updated now.

sjberman · 2025-07-17T16:53:03Z

@tataruty The max items is handled by the OpenAPI Schema validation in the CRD definition, which we don't need to test. This is for validation that can't be done with basic json schema, and is instead done with CEL in the CRD definition (which are comments that have additional validation for certain fields). Good observation though.

sjberman · 2025-07-17T16:57:03Z

tests/go.mod

@@ -5,7 +5,7 @@ go 1.24.2
 replace github.com/nginx/nginx-gateway-fabric => ../

 require (
-	github.com/nginx/nginx-gateway-fabric v0.0.0
+	github.com/nginx/nginx-gateway-fabric v1.6.2


This should stay as-is.

Thanks @sjberman
I'm not sure why that updated originally. Just so I know, what is the impact of this version being v1.6.2 vs v0.0.0?

Functionally it may not matter since we perform the replace above, but it's a little confusing IMO versus just pointing to an empty version to show that we're just importing whatever the same version is that we have checked out.

@sjberman This should be fixed now

sjberman · 2025-07-17T16:57:15Z

tests/cel/clientsettingspolicy_test.go

+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"k8s.io/apimachinery/pkg/runtime"
+	"sigs.k8s.io/controller-runtime/pkg/client/fake"
+


Suggested change

can keep the gateway API import with the other 3rd party imports.

From what I remember, this affected the linter. I will double check that though.

sjberman · 2025-07-17T16:58:54Z

tests/cel/clientsettingspolicy_test.go

+			policySpec: ngfAPIv1alpha1.ClientSettingsPolicySpec{
+				TargetRef: gatewayv1alpha2.LocalPolicyTargetReference{
+					Kind:  "Gateway",
+					Group: "gateway.networking.k8s.io",


We could make this group value a constant

Great point.

@sjberman This change has been made in the latest push

sjberman · 2025-07-17T17:00:40Z

tests/cel/clientsettingspolicy_test.go

+	}{
+		{
+			name:       "Validate gateway.networking.k8s.io TargetRef Group is allowed",
+			wantErrors: []string{"TargetRef Group must be gateway.networking.k8s.io."},


How did this pass in the pipeline? is it actually running? I'm assuming not since our unit tests only run on the internal and cmd directories. Our make unit-test target will need to be updated to also run in tests/cel.

tests/cel/clientsettingspolicy_test.go

sjberman · 2025-07-17T17:02:44Z

tests/cel/clientsettingspolicy_test.go

+	if err != nil {
+		t.Logf("Error creating ClientSettingsPolicy %q: %v",
+			fmt.Sprintf("%v/%v", clientSettingsPolicy.Namespace, clientSettingsPolicy.Name), err)
+	}
+
+	if err != nil {
+		for _, wantError := range wantErrors {
+			if !strings.Contains(err.Error(), wantError) {
+				t.Errorf("missing expected error %q in %v", wantError, err)
+			}
+		}
+	}


We typically use the gomega library to do assertion matching. See our other unit tests for examples, but you can basically do:

g := NewWithT(T) g.Expect(err).ToNot(HaveOccurred())

We also use t.Parallel() at the beginning of each unit test and in each tt.Run loop iteration to parrallelize the tests.

Good to know, thanks @sjberman !

@sjberman I still need to make this update to the test 📝

salonichf5 · 2025-07-17T20:32:05Z

tests/cel/clientsettingspolicy_test.go

+			wantErrors: []string{"TargetRef Group must be gateway.networking.k8s.io."},
+			policySpec: ngfAPIv1alpha1.ClientSettingsPolicySpec{
+				TargetRef: gatewayv1alpha2.LocalPolicyTargetReference{
+					Kind:  "Gateway",


we can make kind Gateway constant too, i see multiple uses.

we do have the various resource Kinds as constants in a kinds package.

nit: on that note, we can use a different kind for one of these tests, just to try all combinations for invalid targetRef group

@salonichf5 @sjberman great suggestions here. I'll be sure to make those updates!

…PoliciesTargetRefGroup validate each test correctly

tests/cel/clientsettingspolicy_test.go

sarthyparty · 2025-07-22T15:58:16Z

tests/Makefile

+.PHONY: setup-and-test-cel-validation
+setup-and-test-cel-validation:
+	kind create cluster --name $(CLUSTER_NAME) || true
+	kubectl kustomize ../config/crd | kubectl apply -f -


When I ran this target, I got this error: The CustomResourceDefinition "nginxproxies.gateway.nginx.org" is invalid: metadata.annotations: Too long: may not be more than 262144 bytes. I was able to fix it by adding changing the apply command to kubectl apply --server-side -f -. Can you check this out?

I'm not sure if its just my machine

sjberman · 2025-07-22T16:02:12Z

tests/Makefile

+# Set up a kind cluster, install NGF CRDs, and run CEL validation tests
+.PHONY: setup-and-test-cel-validation
+setup-and-test-cel-validation:
+	kind create cluster --name $(CLUSTER_NAME) || true


We already have a Makefile target to create kind cluster, so shouldn't need this.

sjberman · 2025-07-22T16:07:19Z

tests/Makefile

+# Run CEL validation integration tests against a real cluster
+.PHONY: test-cel-validation
+test-cel-validation:
+	@if [ -z "$(CEL_TEST_TARGET)" ]; then \
+		echo "Running all tests in ./cel"; \
+		go test -v ./cel; \
+	else \
+		echo "Running test: $(CEL_TEST_TARGET) in ./cel"; \
+		go test -v ./cel -run "$(CEL_TEST_TARGET)"; \
+	fi


Not sure how important this is to have a separate section for running a specific test. We don't do it for our other tests, and it's pretty straightforward for the one-off cases to just specify the test directly in the command line.

We're also going to need a job in our CI pipeline to run these.

Add CEL validation test for targetRef in ClientSettingsPolicy

dbb5b5c

shaun-nx requested a review from a team as a code owner July 14, 2025 09:56

github-project-automation bot added this to NGINX Gateway Fabric Jul 14, 2025

github-project-automation bot moved this to 🆕 New in NGINX Gateway Fabric Jul 14, 2025

nginx-bot bot added the community label Jul 14, 2025

shaun-nx added 3 commits July 14, 2025 11:06

gofumpt

48ec86b

Add tests for targetRefGroup

c7129bd

Rename tests

87a4e0e

shaun-nx added the tests Pull requests that update tests label Jul 14, 2025

ciarams87 requested changes Jul 14, 2025

View reviewed changes

github-project-automation bot moved this from 🆕 New to 🏗 In Progress in NGINX Gateway Fabric Jul 14, 2025

Merge branch 'main' into tests/cel-clientsettingspolicies

29f201e

github-actions bot removed the tests Pull requests that update tests label Jul 15, 2025

shaun-nx added 2 commits July 15, 2025 08:49

Move tests into clientsettingspolicy_test.go

d921ed2

Update tests to create a ClientSettingsPolicy resource during tests

f22269a

shaun-nx requested review from ciarams87 and bjee19 July 15, 2025 10:34

make lint in tests

7fddffa

ciarams87 requested changes Jul 15, 2025

View reviewed changes

shaun-nx added 3 commits July 17, 2025 10:45

Update tests to create a ClientSettingsPolicy object during validation

dd66a79

Merge branch 'main' into tests/cel-clientsettingspolicies

d664f35

Update TargetRegGroup tests

cd9bada

shaun-nx requested a review from ciarams87 July 17, 2025 09:53

Fix lint errors

7d9989d

sarthyparty reviewed Jul 17, 2025

View reviewed changes

sjberman reviewed Jul 17, 2025

View reviewed changes

salonichf5 reviewed Jul 17, 2025

View reviewed changes

shaun-nx added 5 commits July 21, 2025 13:18

Group valid and invalid test cases into single test function

9274e57

Merge branch 'main' into tests/cel-clientsettingspolicies

506b01c

Revert dependency version

f99ab29

Move imports

ad80b70

Define constants for Kinds and Groups

e5479fb

github-actions bot added the tests Pull requests that update tests label Jul 21, 2025

shaun-nx linked an issue Jul 22, 2025 that may be closed by this pull request

Add CEL test for ClientSettingsPolicy Policy #3621

Open

2 tasks

shaun-nx added 5 commits July 22, 2025 09:41

Merge branch 'main' into tests/cel-clientsettingspolicies

1789701

Use controller-runtime library to get cluster information

aa4a6b2

Add Makefile targets to run CEL test and re format test

8f74214

Ensure TestClientSettingsPoliciesTargetRefKind and TestClientSettings…

4486bd5

…PoliciesTargetRefGroup validate each test correctly

Merge branch 'main' into tests/cel-clientsettingspolicies

1adf168

shaun-nx requested review from sjberman, sarthyparty and salonichf5 July 22, 2025 14:55

sjberman reviewed Jul 22, 2025

View reviewed changes

tests/cel/clientsettingspolicy_test.go Outdated Show resolved Hide resolved

sarthyparty reviewed Jul 22, 2025

View reviewed changes

sjberman reviewed Jul 22, 2025

View reviewed changes

Add CEL validation test for targetRef in ClientSettingsPolicy #3623

Are you sure you want to change the base?

Add CEL validation test for targetRef in ClientSettingsPolicy #3623

Conversation

shaun-nx commented Jul 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Proposed changes

Checklist

Uh oh!

nginx-bot bot commented Jul 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov bot commented Jul 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

ciarams87 left a comment

Choose a reason for hiding this comment

Uh oh!

ciarams87 commented Jul 14, 2025

Uh oh!

bjee19 commented Jul 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bjee19 commented Jul 15, 2025

Uh oh!

shaun-nx commented Jul 15, 2025

Uh oh!

ciarams87 left a comment

Choose a reason for hiding this comment

Uh oh!

tataruty commented Jul 17, 2025

Uh oh!

tataruty commented Jul 17, 2025

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sjberman commented Jul 17, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

shaun-nx commented Jul 14, 2025 •

edited

Loading

nginx-bot bot commented Jul 14, 2025 •

edited

Loading

codecov bot commented Jul 14, 2025 •

edited

Loading

bjee19 commented Jul 14, 2025 •

edited

Loading