-
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
aa5adc6
29f26ef
f26eea3
efa4a81
d25661a
c413895
9962e03
feca4ed
8d84e7b
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. 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. @sjberman I don't see any other target in this Makefile that creates a kind cluster. There is one in the main Makefile, but none in 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. in the tests Makefile there is the line 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. Ah I see, thanks @bjee19
Suggested change
|
||||||
kubectl kustomize ../config/crd | kubectl apply -f - | ||||||
sarthyparty marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||
$(MAKE) test-cel-validation |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,220 @@ | ||
package cel | ||
|
||
import ( | ||
"context" | ||
"fmt" | ||
"testing" | ||
"time" | ||
|
||
. "github.com/onsi/gomega" | ||
"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) { | ||
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. Let's add |
||
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) | ||
Comment on lines
+142
to
+143
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 can also add t.Parallel() to each loop iteration. |
||
}) | ||
} | ||
} | ||
|
||
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) | ||
} | ||
Comment on lines
+180
to
+182
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. g.Expect(err).ToNot(HaveOccurred()) |
||
return | ||
} | ||
|
||
// We expected errors - validation should have failed | ||
if err == nil { | ||
t.Errorf("expected validation error but policy was accepted") | ||
return | ||
} | ||
Comment on lines
+187
to
+190
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. g.Expect(err).To(HaveOccurred()) |
||
g := NewWithT(t) | ||
// Check that we got the expected error messages | ||
for _, wantError := range tt.wantErrors { | ||
g.Expect(err.Error()).To(ContainSubstring(wantError), "Expected error '%s' not found in: %s", wantError, err.Error()) | ||
} | ||
} | ||
|
||
// 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) | ||
} | ||
Comment on lines
+202
to
+205
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. Let's return errors from this function so we can diagnose them. The caller should assert that an error didn't occur. |
||
|
||
// 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 | ||
} |
Uh oh!
There was an error while loading. Please reload this page.