From c835e839033eddc6d4ded698cd467f219607cfff Mon Sep 17 00:00:00 2001 From: Mattia Lavacca Date: Mon, 28 Oct 2024 11:52:54 +0100 Subject: [PATCH] [release-1.1] Backport CI improvements (#3417) * Fix CEL tests for v1.28+ (#3325) * feat: validate CRDs on specific k8s versions (#3316) Signed-off-by: Mattia Lavacca * finalize verify->crds-validation migration (#3330) Signed-off-by: Mattia Lavacca * Fixing kind v1.30 image ref (#3329) --------- Signed-off-by: Mattia Lavacca Co-authored-by: Rob Scott --- Makefile | 5 +++ ...y-crds-kind.sh => test-crds-validation.sh} | 38 ++++++++++++++++++- hack/verify-all.sh | 6 ++- pkg/test/cel/backendlbpolicy_test.go | 3 +- pkg/test/cel/backendtlspolicy_test.go | 3 +- pkg/test/cel/gateway_test.go | 3 +- pkg/test/cel/gatewayclass_test.go | 3 +- pkg/test/cel/grpcroute_test.go | 3 +- pkg/test/cel/httproute_test.go | 23 +++++------ pkg/test/cel/main_test.go | 24 ++++++++++++ 10 files changed, 86 insertions(+), 25 deletions(-) rename hack/{verify-crds-kind.sh => test-crds-validation.sh} (71%) diff --git a/Makefile b/Makefile index 39cd316631..8bcd4fbff2 100644 --- a/Makefile +++ b/Makefile @@ -83,6 +83,11 @@ test: cd "conformance/echo-basic" && go test -race -cover ./... cd "gwctl" && go test -race -cover ./... +# Run tests for CRDs validation +.PHONY: test.crds-validation +test.crds-validation: + ./hack/test-crds-validation.sh $(VERSION) + # Run conformance tests against controller implementation .PHONY: conformance conformance: diff --git a/hack/verify-crds-kind.sh b/hack/test-crds-validation.sh similarity index 71% rename from hack/verify-crds-kind.sh rename to hack/test-crds-validation.sh index 319224f1ac..eda98d2d7e 100755 --- a/hack/verify-crds-kind.sh +++ b/hack/test-crds-validation.sh @@ -41,15 +41,49 @@ cleanup() { trap cleanup INT TERM EXIT +# TODO(mlavacca): find a good way to keep this dependency up to date. +KIND_VERSION="v0.24.0" + +# list of kind images taken from https://github.com/kubernetes-sigs/kind/releases/tag/v0.24.0. +# they need to be updated when kind is updated. +KIND_IMAGES=( + "kindest/node:v1.27.17@sha256:3fd82731af34efe19cd54ea5c25e882985bafa2c9baefe14f8deab1737d9fabe" + "kindest/node:v1.28.13@sha256:45d319897776e11167e4698f6b14938eb4d52eb381d9e3d7a9086c16c69a8110" + "kindest/node:v1.29.8@sha256:d46b7aa29567e93b27f7531d258c372e829d7224b25e3fc6ffdefed12476d3aa" + "kindest/node:v1.30.4@sha256:976ea815844d5fa93be213437e3ff5754cd599b040946b5cca43ca45c2047114" + "kindest/node:v1.31.0@sha256:53df588e04085fd41ae12de0c3fe4c72f7013bba32a20e7325357a1ac94ba865" +) + +if [ "$#" -gt 1 ]; then + echo "Error: Too many arguments provided. Only 1 argument is allowed." + exit 1 +fi + +DEFAULT_INDEX=$((1)) + +if [ "$#" -eq 1 ]; then + # Check if the argument is a valid number between 1 and 5 + if ! [[ "$1" =~ ^[1-5] ]]; then + echo "Error: Argument is not a valid integer between 1 and 5." + exit 1 + fi + INDEX=$(($1)) +else + INDEX=$((DEFAULT_INDEX)) +fi + +K8S_IMAGE=${KIND_IMAGES[$((INDEX-1))]} +echo "Using Kubernetes image: ${K8S_IMAGE}" + # For exit code res=0 # Install kind -(cd $GOPATH && go install sigs.k8s.io/kind@v0.20.0) || res=$? +(cd "${GOPATH}" && go install sigs.k8s.io/kind@${KIND_VERSION}) || res=$? # Create cluster KIND_CREATE_ATTEMPTED=true -kind create cluster --name "${CLUSTER_NAME}" +kind create cluster --name "${CLUSTER_NAME}" --image "${K8S_IMAGE}" || res=$? # Verify CEL validation for CHANNEL in experimental standard; do diff --git a/hack/verify-all.sh b/hack/verify-all.sh index 2f67b64c17..e275a8236a 100755 --- a/hack/verify-all.sh +++ b/hack/verify-all.sh @@ -53,10 +53,12 @@ fi EXCLUDE="verify-all.sh" +SCRIPTS=$(find "${SCRIPT_ROOT}"/hack -name "verify-*.sh") + ret=0 -for t in `ls $SCRIPT_ROOT/hack/verify-*.sh` +for t in $SCRIPTS; do - if is-excluded $t ; then + if is-excluded "${t}" ; then echo "Skipping $t" continue fi diff --git a/pkg/test/cel/backendlbpolicy_test.go b/pkg/test/cel/backendlbpolicy_test.go index ed83b2e7f3..f0392fd20a 100644 --- a/pkg/test/cel/backendlbpolicy_test.go +++ b/pkg/test/cel/backendlbpolicy_test.go @@ -22,7 +22,6 @@ package main import ( "context" "fmt" - "strings" "testing" "time" @@ -122,7 +121,7 @@ func validateBackendLBPolicy(t *testing.T, lbPolicy *gatewayv1a2.BackendLBPolicy var missingErrorStrings []string for _, wantError := range wantErrors { - if !strings.Contains(strings.ToLower(err.Error()), strings.ToLower(wantError)) { + if !celErrorStringMatches(err.Error(), wantError) { missingErrorStrings = append(missingErrorStrings, wantError) } } diff --git a/pkg/test/cel/backendtlspolicy_test.go b/pkg/test/cel/backendtlspolicy_test.go index 1740a002db..5d23278428 100644 --- a/pkg/test/cel/backendtlspolicy_test.go +++ b/pkg/test/cel/backendtlspolicy_test.go @@ -22,7 +22,6 @@ package main import ( "context" "fmt" - "strings" "testing" "time" @@ -144,7 +143,7 @@ func validateBackendTLSPolicy(t *testing.T, route *gatewayv1a3.BackendTLSPolicy, var missingErrorStrings []string for _, wantError := range wantErrors { - if !strings.Contains(strings.ToLower(err.Error()), strings.ToLower(wantError)) { + if !celErrorStringMatches(err.Error(), wantError) { missingErrorStrings = append(missingErrorStrings, wantError) } } diff --git a/pkg/test/cel/gateway_test.go b/pkg/test/cel/gateway_test.go index ac6ba4e601..4c24020ead 100644 --- a/pkg/test/cel/gateway_test.go +++ b/pkg/test/cel/gateway_test.go @@ -19,7 +19,6 @@ package main import ( "context" "fmt" - "strings" "testing" "time" @@ -608,7 +607,7 @@ func TestValidateGateway(t *testing.T) { var missingErrorStrings []string for _, wantError := range tc.wantErrors { - if !strings.Contains(strings.ToLower(err.Error()), strings.ToLower(wantError)) { + if !celErrorStringMatches(err.Error(), wantError) { missingErrorStrings = append(missingErrorStrings, wantError) } } diff --git a/pkg/test/cel/gatewayclass_test.go b/pkg/test/cel/gatewayclass_test.go index 8c8225906e..d984beb53f 100644 --- a/pkg/test/cel/gatewayclass_test.go +++ b/pkg/test/cel/gatewayclass_test.go @@ -19,7 +19,6 @@ package main import ( "context" "fmt" - "strings" "testing" "time" @@ -72,7 +71,7 @@ func TestValidateGatewayClassUpdate(t *testing.T) { if (tc.wantError != "") != (err != nil) { t.Fatalf("Unexpected error while updating GatewayClass; got err=\n%v\n;want error=%v", err, tc.wantError != "") } - if tc.wantError != "" && !strings.Contains(strings.ToLower(err.Error()), strings.ToLower(tc.wantError)) { + if tc.wantError != "" && !celErrorStringMatches(err.Error(), tc.wantError) { t.Fatalf("Unexpected error while updating GatewayClass; got err=\n%v\n;want substring within error=%q", err, tc.wantError) } }) diff --git a/pkg/test/cel/grpcroute_test.go b/pkg/test/cel/grpcroute_test.go index 4c76e9fa12..85f9e98097 100644 --- a/pkg/test/cel/grpcroute_test.go +++ b/pkg/test/cel/grpcroute_test.go @@ -22,7 +22,6 @@ package main import ( "context" "fmt" - "strings" "testing" "time" @@ -425,7 +424,7 @@ func validateGRPCRoute(t *testing.T, route *gatewayv1.GRPCRoute, wantErrors []st var missingErrorStrings []string for _, wantError := range wantErrors { - if !strings.Contains(strings.ToLower(err.Error()), strings.ToLower(wantError)) { + if !celErrorStringMatches(err.Error(), wantError) { missingErrorStrings = append(missingErrorStrings, wantError) } } diff --git a/pkg/test/cel/httproute_test.go b/pkg/test/cel/httproute_test.go index 9dcc022b03..86ba4deebf 100644 --- a/pkg/test/cel/httproute_test.go +++ b/pkg/test/cel/httproute_test.go @@ -19,7 +19,6 @@ package main import ( "context" "fmt" - "strings" "testing" "time" @@ -75,7 +74,7 @@ func TestHTTPPathMatch(t *testing.T) { }, { name: "invalid type", - wantErrors: []string{"type must be one of ['Exact', 'PathPrefix', 'RegularExpression']"}, + wantErrors: []string{"must be one of ['Exact', 'PathPrefix', 'RegularExpression']"}, path: &gatewayv1.HTTPPathMatch{ Type: ptrTo(gatewayv1.PathMatchType("FooBar")), Value: ptrTo("/path"), @@ -739,13 +738,13 @@ func TestHTTPRouteRule(t *testing.T) { }}, }, { - name: "invalid URLRewrite filter because too many path matches", + name: "invalid URLRewrite filter because wrong path match type", wantErrors: []string{"When using URLRewrite filter with path.replacePrefixMatch, exactly one PathPrefix match must be specified"}, rules: []gatewayv1.HTTPRouteRule{{ Matches: []gatewayv1.HTTPRouteMatch{ { Path: &gatewayv1.HTTPPathMatch{ - Type: ptrTo(gatewayv1.PathMatchType(gatewayv1.FullPathHTTPPathModifier)), // Incorrect Patch match Type for URLRewrite filter with ReplacePrefixMatch. + Type: ptrTo(gatewayv1.PathMatchRegularExpression), // Incorrect Path match Type for URLRewrite filter with ReplacePrefixMatch. Value: ptrTo("/foo"), }, }, @@ -813,13 +812,13 @@ func TestHTTPRouteRule(t *testing.T) { }}, }, { - name: "invalid RequestRedirect filter because path match has type ReplaceFullPath", + name: "invalid RequestRedirect filter because path match has type RegularExpression", wantErrors: []string{"When using RequestRedirect filter with path.replacePrefixMatch, exactly one PathPrefix match must be specified"}, rules: []gatewayv1.HTTPRouteRule{{ Matches: []gatewayv1.HTTPRouteMatch{ { Path: &gatewayv1.HTTPPathMatch{ - Type: ptrTo(gatewayv1.PathMatchType(gatewayv1.FullPathHTTPPathModifier)), // Incorrect Patch match Type for RequestRedirect filter with ReplacePrefixMatch. + Type: ptrTo(gatewayv1.PathMatchRegularExpression), // Incorrect Path match Type for RequestRedirect filter with ReplacePrefixMatch. Value: ptrTo("/foo"), }, }, @@ -907,13 +906,13 @@ func TestHTTPRouteRule(t *testing.T) { }}, }, { - name: "invalid URLRewrite filter (within backendRefs) because path match has type ReplaceFullPath", + name: "invalid URLRewrite filter (within backendRefs) because path match has type RegularExpression", wantErrors: []string{"Within backendRefs, When using URLRewrite filter with path.replacePrefixMatch, exactly one PathPrefix match must be specified"}, rules: []gatewayv1.HTTPRouteRule{{ Matches: []gatewayv1.HTTPRouteMatch{ { Path: &gatewayv1.HTTPPathMatch{ - Type: ptrTo(gatewayv1.PathMatchType(gatewayv1.FullPathHTTPPathModifier)), // Incorrect Patch match Type for URLRewrite filter with ReplacePrefixMatch. + Type: ptrTo(gatewayv1.PathMatchRegularExpression), // Incorrect Path match Type for URLRewrite filter with ReplacePrefixMatch. Value: ptrTo("/foo"), }, }, @@ -1011,13 +1010,13 @@ func TestHTTPRouteRule(t *testing.T) { }}, }, { - name: "invalid RequestRedirect filter (within backendRefs) because path match has type ReplaceFullPath", + name: "invalid RequestRedirect filter (within backendRefs) because path match has type RegularExpression", wantErrors: []string{"Within backendRefs, when using RequestRedirect filter with path.replacePrefixMatch, exactly one PathPrefix match must be specified"}, rules: []gatewayv1.HTTPRouteRule{{ Matches: []gatewayv1.HTTPRouteMatch{ { Path: &gatewayv1.HTTPPathMatch{ - Type: ptrTo(gatewayv1.PathMatchType(gatewayv1.FullPathHTTPPathModifier)), // Incorrect Patch match Type for RequestRedirect filter with ReplacePrefixMatch. + Type: ptrTo(gatewayv1.PathMatchRegularExpression), // Incorrect Path match Type for RequestRedirect filter with ReplacePrefixMatch. Value: ptrTo("/foo"), }, }, @@ -1318,6 +1317,7 @@ func TestHTTPPathModifier(t *testing.T) { name: "type must be 'ReplaceFullPath' when replaceFullPath is set", wantErrors: []string{"type must be 'ReplaceFullPath' when replaceFullPath is set"}, pathModifier: gatewayv1.HTTPPathModifier{ + Type: gatewayv1.PrefixMatchHTTPPathModifier, ReplaceFullPath: ptrTo("foo"), }, }, @@ -1339,6 +1339,7 @@ func TestHTTPPathModifier(t *testing.T) { name: "type must be 'ReplacePrefixMatch' when replacePrefixMatch is set", wantErrors: []string{"type must be 'ReplacePrefixMatch' when replacePrefixMatch is set"}, pathModifier: gatewayv1.HTTPPathModifier{ + Type: gatewayv1.FullPathHTTPPathModifier, ReplacePrefixMatch: ptrTo("/foo"), }, }, @@ -1384,7 +1385,7 @@ func validateHTTPRoute(t *testing.T, route *gatewayv1.HTTPRoute, wantErrors []st var missingErrorStrings []string for _, wantError := range wantErrors { - if !strings.Contains(strings.ToLower(err.Error()), strings.ToLower(wantError)) { + if !celErrorStringMatches(err.Error(), wantError) { missingErrorStrings = append(missingErrorStrings, wantError) } } diff --git a/pkg/test/cel/main_test.go b/pkg/test/cel/main_test.go index c08187a2e6..9bc48c7c52 100644 --- a/pkg/test/cel/main_test.go +++ b/pkg/test/cel/main_test.go @@ -20,6 +20,7 @@ import ( "fmt" "os" "path" + "strings" "testing" v1 "sigs.k8s.io/gateway-api/apis/v1" @@ -59,3 +60,26 @@ func TestMain(m *testing.M) { func ptrTo[T any](a T) *T { return &a } + +func celErrorStringMatches(got, want string) bool { + gotL := strings.ToLower(got) + wantL := strings.ToLower(want) + + // Starting in k8s v1.28, CEL error messages stopped adding spec and status prefixes to path names + wantLAdjusted := strings.ReplaceAll(wantL, "spec.", "") + wantLAdjusted = strings.ReplaceAll(wantLAdjusted, "status.", "") + + // Enum validation messages changed in k8s v1.28: + // Before: must be one of ['Exact', 'PathPrefix', 'RegularExpression'] + // After: supported values: "Exact", "PathPrefix", "RegularExpression" + if strings.Contains(wantLAdjusted, "must be one of") { + r := strings.NewReplacer( + "must be one of", "supported values:", + "[", "", + "]", "", + "'", "\"", + ) + wantLAdjusted = r.Replace(wantLAdjusted) + } + return strings.Contains(gotL, wantL) || strings.Contains(gotL, wantLAdjusted) +}