-
Notifications
You must be signed in to change notification settings - Fork 129
Add CEL validation test for targetRef in ClientSettingsPolicy #3623
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
dbb5b5c
48ec86b
c7129bd
87a4e0e
29f201e
d921ed2
f22269a
7fddffa
dd66a79
d664f35
cd9bada
7d9989d
9274e57
506b01c
f99ab29
ad80b70
e5479fb
1789701
aa4a6b2
8f74214
4486bd5
1adf168
7c6dba4
69f485b
1ad24c4
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -17,6 +17,7 @@ STANDARD_CONFORMANCE_PROFILES = GATEWAY-HTTP,GATEWAY-GRPC | |
EXPERIMENTAL_CONFORMANCE_PROFILES = GATEWAY-TLS | ||
CONFORMANCE_PROFILES = $(STANDARD_CONFORMANCE_PROFILES) # by default we use the standard conformance profiles. If experimental is enabled we override this and add the experimental profiles. | ||
SKIP_TESTS = | ||
CEL_TEST_TARGET = | ||
|
||
# Check if ENABLE_EXPERIMENTAL is true | ||
ifeq ($(ENABLE_EXPERIMENTAL),true) | ||
|
@@ -186,3 +187,22 @@ uninstall-ngf: ## Uninstall NGF on configured kind cluster | |
-make uninstall-gateway-crds | ||
-kubectl delete namespace nginx-gateway | ||
-kubectl kustomize ../config/crd | kubectl delete -f - | ||
|
||
# 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 | ||
|
||
|
||
# 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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We already have a Makefile target to create kind cluster, so shouldn't need this. |
||
kubectl kustomize ../config/crd | kubectl apply -f - | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm not sure if its just my machine There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. not just your machine, it should be --server-side apply, see #3589 |
||
$(MAKE) test-cel-validation |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,226 @@ | ||
package cel | ||
|
||
import ( | ||
"context" | ||
"fmt" | ||
"strings" | ||
"testing" | ||
"time" | ||
|
||
"k8s.io/apimachinery/pkg/runtime" | ||
controllerruntime "sigs.k8s.io/controller-runtime" | ||
"sigs.k8s.io/controller-runtime/pkg/client" | ||
gatewayv1alpha2 "sigs.k8s.io/gateway-api/apis/v1alpha2" | ||
|
||
ngfAPIv1alpha1 "github.com/nginx/nginx-gateway-fabric/apis/v1alpha1" | ||
) | ||
|
||
const ( | ||
GatewayKind = "Gateway" | ||
HTTPRouteKind = "HTTPRoute" | ||
GRPCRouteKind = "GRPCRoute" | ||
TCPRouteKind = "TCPRoute" | ||
InvalidKind = "InvalidKind" | ||
) | ||
|
||
const ( | ||
GatewayGroup = "gateway.networking.k8s.io" | ||
InvalidGroup = "invalid.networking.k8s.io" | ||
DiscoveryGroup = "discovery.k8s.io/v1" | ||
) | ||
|
||
const ( | ||
ExpectedTargetRefKindError = `TargetRef Kind must be one of: Gateway, HTTPRoute, or GRPCRoute` | ||
ExpectedTargetRefGroupError = `TargetRef Group must be gateway.networking.k8s.io.` | ||
) | ||
|
||
const ( | ||
PolicyName = "test-policy" | ||
TargetRefFormat = "targetRef-name-%d" | ||
) | ||
|
||
func TestClientSettingsPoliciesTargetRefKind(t *testing.T) { | ||
tests := []struct { | ||
policySpec ngfAPIv1alpha1.ClientSettingsPolicySpec | ||
name string | ||
wantErrors []string | ||
}{ | ||
{ | ||
name: "Validate TargetRef of kind Gateway is allowed", | ||
policySpec: ngfAPIv1alpha1.ClientSettingsPolicySpec{ | ||
TargetRef: gatewayv1alpha2.LocalPolicyTargetReference{ | ||
Kind: GatewayKind, | ||
Group: GatewayGroup, | ||
}, | ||
}, | ||
}, | ||
{ | ||
name: "Validate TargetRef of kind HTTPRoute is allowed", | ||
policySpec: ngfAPIv1alpha1.ClientSettingsPolicySpec{ | ||
TargetRef: gatewayv1alpha2.LocalPolicyTargetReference{ | ||
Kind: HTTPRouteKind, | ||
Group: GatewayGroup, | ||
}, | ||
}, | ||
}, | ||
{ | ||
name: "Validate TargetRef of kind GRPCRoute is allowed", | ||
policySpec: ngfAPIv1alpha1.ClientSettingsPolicySpec{ | ||
TargetRef: gatewayv1alpha2.LocalPolicyTargetReference{ | ||
Kind: GRPCRouteKind, | ||
Group: GatewayGroup, | ||
}, | ||
}, | ||
}, | ||
{ | ||
name: "Validate Invalid TargetRef Kind is not allowed", | ||
wantErrors: []string{ExpectedTargetRefKindError}, | ||
policySpec: ngfAPIv1alpha1.ClientSettingsPolicySpec{ | ||
TargetRef: gatewayv1alpha2.LocalPolicyTargetReference{ | ||
Kind: InvalidKind, | ||
Group: GatewayGroup, | ||
}, | ||
}, | ||
}, | ||
{ | ||
name: "Validate TCPRoute TargetRef Kind is not allowed", | ||
wantErrors: []string{ExpectedTargetRefKindError}, | ||
policySpec: ngfAPIv1alpha1.ClientSettingsPolicySpec{ | ||
TargetRef: gatewayv1alpha2.LocalPolicyTargetReference{ | ||
Kind: TCPRouteKind, | ||
Group: GatewayGroup, | ||
}, | ||
}, | ||
}, | ||
} | ||
|
||
for _, tt := range tests { | ||
t.Run(tt.name, func(t *testing.T) { | ||
validateClientSettingsPolicy(t, tt) | ||
}) | ||
} | ||
} | ||
|
||
func TestClientSettingsPoliciesTargetRefGroup(t *testing.T) { | ||
tests := []struct { | ||
policySpec ngfAPIv1alpha1.ClientSettingsPolicySpec | ||
name string | ||
wantErrors []string | ||
}{ | ||
{ | ||
name: "Validate gateway.networking.k8s.io TargetRef Group is allowed", | ||
policySpec: ngfAPIv1alpha1.ClientSettingsPolicySpec{ | ||
TargetRef: gatewayv1alpha2.LocalPolicyTargetReference{ | ||
Kind: GatewayKind, | ||
Group: GatewayGroup, | ||
}, | ||
}, | ||
}, | ||
{ | ||
name: "Validate invalid.networking.k8s.io TargetRef Group is not allowed", | ||
wantErrors: []string{ExpectedTargetRefGroupError}, | ||
policySpec: ngfAPIv1alpha1.ClientSettingsPolicySpec{ | ||
TargetRef: gatewayv1alpha2.LocalPolicyTargetReference{ | ||
Kind: GatewayKind, | ||
Group: InvalidGroup, | ||
}, | ||
}, | ||
}, | ||
{ | ||
name: "Validate discovery.k8s.io/v1 TargetRef Group is not allowed", | ||
wantErrors: []string{ExpectedTargetRefGroupError}, | ||
policySpec: ngfAPIv1alpha1.ClientSettingsPolicySpec{ | ||
TargetRef: gatewayv1alpha2.LocalPolicyTargetReference{ | ||
Kind: GatewayKind, | ||
Group: DiscoveryGroup, | ||
}, | ||
}, | ||
}, | ||
} | ||
|
||
for _, tt := range tests { | ||
t.Run(tt.name, func(t *testing.T) { | ||
validateClientSettingsPolicy(t, tt) | ||
}) | ||
} | ||
} | ||
|
||
func validateClientSettingsPolicy(t *testing.T, tt struct { | ||
policySpec ngfAPIv1alpha1.ClientSettingsPolicySpec | ||
name string | ||
wantErrors []string | ||
}, | ||
) { | ||
t.Helper() | ||
sjberman marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
// Get Kubernetes client from test framework | ||
// This should be set up by your test framework to connect to a real cluster | ||
k8sClient := getKubernetesClient(t) | ||
|
||
policySpec := tt.policySpec | ||
policySpec.TargetRef.Name = gatewayv1alpha2.ObjectName(fmt.Sprintf(TargetRefFormat, time.Now().UnixNano())) | ||
|
||
clientSettingsPolicy := &ngfAPIv1alpha1.ClientSettingsPolicy{ | ||
ObjectMeta: controllerruntime.ObjectMeta{ | ||
Name: PolicyName, | ||
Namespace: "default", | ||
}, | ||
Spec: policySpec, | ||
} | ||
|
||
err := k8sClient.Create(context.Background(), clientSettingsPolicy) | ||
sarthyparty marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
// Clean up after test | ||
defer func() { | ||
_ = k8sClient.Delete(context.Background(), clientSettingsPolicy) | ||
}() | ||
|
||
// Check if we expected errors | ||
if len(tt.wantErrors) == 0 { | ||
if err != nil { | ||
t.Errorf("expected no error but got: %v", err) | ||
} | ||
return | ||
} | ||
|
||
// We expected errors - validation should have failed | ||
if err == nil { | ||
t.Errorf("expected validation error but policy was accepted") | ||
return | ||
} | ||
|
||
// Check that we got the expected error messages | ||
var missingErrors []string | ||
for _, wantError := range tt.wantErrors { | ||
if !strings.Contains(err.Error(), wantError) { | ||
missingErrors = append(missingErrors, wantError) | ||
} | ||
} | ||
if len(missingErrors) != 0 { | ||
t.Errorf("missing expected errors: %v, got: %v", missingErrors, err) | ||
} | ||
} | ||
|
||
// getKubernetesClient returns a client connected to a real Kubernetes cluster. | ||
func getKubernetesClient(t *testing.T) client.Client { | ||
t.Helper() | ||
// Use controller-runtime to get cluster connection | ||
k8sConfig, err := controllerruntime.GetConfig() | ||
if err != nil { | ||
t.Skipf("Cannot connect to Kubernetes cluster: %v", err) | ||
} | ||
|
||
// Set up scheme with NGF types | ||
scheme := runtime.NewScheme() | ||
if err := ngfAPIv1alpha1.AddToScheme(scheme); err != nil { | ||
t.Fatalf("Failed to add NGF schema: %v", err) | ||
} | ||
|
||
// Create client | ||
k8sClient, err := client.New(k8sConfig, client.Options{Scheme: scheme}) | ||
if err != nil { | ||
t.Skipf("Cannot create k8s client: %v", err) | ||
} | ||
|
||
return k8sClient | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We're also going to need a job in our CI pipeline to run these.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's fair. I found this pretty helpful for debugging specific tests. It might be good to keep just for that purpose