diff --git a/pkg/route/apis/route/validation/validation.go b/pkg/route/apis/route/validation/validation.go index e7bc6a3922..55208d265b 100644 --- a/pkg/route/apis/route/validation/validation.go +++ b/pkg/route/apis/route/validation/validation.go @@ -2,6 +2,7 @@ package validation import ( "context" + "strings" "k8s.io/apimachinery/pkg/util/validation/field" corev1client "k8s.io/client-go/kubernetes/typed/core/v1" @@ -21,6 +22,16 @@ func toRouteV1(internal *routeapi.Route) (*routev1.Route, field.ErrorList) { return &external, nil } +func validatePath(pathValue string, fldPath *field.Path) field.ErrorList { + result := field.ErrorList{} + + if strings.ContainsAny(pathValue, "# ") { + result = append(result, field.Invalid(fldPath, pathValue, "cannot contain # or spaces")) + } + + return result +} + // ValidateRoute tests if required fields in the route are set. func ValidateRoute(ctx context.Context, route *routeapi.Route, sarClient routecommon.SubjectAccessReviewCreator, secretsGetter corev1client.SecretsGetter, opts routecommon.RouteValidationOptions) field.ErrorList { external, errs := toRouteV1(route) @@ -28,7 +39,12 @@ func ValidateRoute(ctx context.Context, route *routeapi.Route, sarClient routeco return errs } - return routevalidation.ValidateRoute(ctx, external, sarClient, secretsGetter, opts) + allErrs := routevalidation.ValidateRoute(ctx, external, sarClient, secretsGetter, opts) + + pathFieldPath := field.NewPath("spec", "path") + allErrs = append(allErrs, validatePath(external.Spec.Path, pathFieldPath)...) + + return allErrs } func ValidateRouteUpdate(ctx context.Context, route *routeapi.Route, oldRoute *routeapi.Route, sarClient routecommon.SubjectAccessReviewCreator, secretsGetter corev1client.SecretsGetter, opts routecommon.RouteValidationOptions) field.ErrorList { @@ -42,7 +58,14 @@ func ValidateRouteUpdate(ctx context.Context, route *routeapi.Route, oldRoute *r return errs } - return routevalidation.ValidateRouteUpdate(ctx, external, oldExternal, sarClient, secretsGetter, opts) + allErrs := routevalidation.ValidateRouteUpdate(ctx, external, oldExternal, sarClient, secretsGetter, opts) + + if external.Spec.Path != oldExternal.Spec.Path { + pathFieldPath := field.NewPath("spec", "path") + allErrs = append(allErrs, validatePath(external.Spec.Path, pathFieldPath)...) + } + + return allErrs } // ValidateRouteStatusUpdate validates status updates for routes. diff --git a/pkg/route/apiserver/registry/route/strategy_test.go b/pkg/route/apiserver/registry/route/strategy_test.go index 6670fc34fe..1996b92f66 100644 --- a/pkg/route/apiserver/registry/route/strategy_test.go +++ b/pkg/route/apiserver/registry/route/strategy_test.go @@ -17,6 +17,7 @@ import ( routev1 "github.com/openshift/api/route/v1" "github.com/openshift/library-go/pkg/route" routeapi "github.com/openshift/openshift-apiserver/pkg/route/apis/route" + routev1conversion "github.com/openshift/openshift-apiserver/pkg/route/apis/route/v1" corev1client "k8s.io/client-go/kubernetes/typed/core/v1" ) @@ -1112,3 +1113,154 @@ func TestExternalCertRemoval(t *testing.T) { } } } + +func TestValidateRoute(t *testing.T) { + ctx := apirequest.NewContext() + strategy := NewStrategy(testAllocator{}, &testSAR{allow: true}, &testSecretGetter{}, true) + + tests := []struct { + name string + path string + expectedErrors int + }{ + { + name: "spec.path should not contain hash symbol in the spec.path", + path: "/new-path#", + expectedErrors: 1, + }, + { + name: "spec.path should not contain whitespace in the spec.path", + path: "/new path", + expectedErrors: 1, + }, + { + name: "spec.path should not contain whitespace or # in the spec.path", + path: "/invalid-path # ", + expectedErrors: 1, + }, + { + name: "spec.path is valid", + path: "/valid-path", + expectedErrors: 0, + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + // Create a route with the specified path + route := &routev1.Route{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-route", + Namespace: "foo", + }, + Spec: routev1.RouteSpec{ + Path: tc.path, + To: routev1.RouteTargetReference{ + Name: "serviceName", + Kind: "Service", + }, + }, + } + + // Convert to internal API type + var routeInternal routeapi.Route + if err := routev1conversion.Convert_v1_Route_To_route_Route(route, &routeInternal, nil); err != nil { + t.Fatalf("failed to convert route: %v", err) + } + + // Validate the route creation + errs := strategy.Validate(ctx, &routeInternal) + + // Check if the number of errors matches expectations + if len(errs) != tc.expectedErrors { + t.Errorf("expected %d errors, but got %d: %v", tc.expectedErrors, len(errs), errs) + } + }) + } +} + +func TestValidateRouteUpdate(t *testing.T) { + ctx := apirequest.NewContext() + strategy := NewStrategy(testAllocator{}, &testSAR{allow: true}, &testSecretGetter{}, true) + + tests := []struct { + name string + route *routev1.Route + change func(*routev1.Route) + expectedErrors int + }{ + { + name: "Reject update to spec.path containing #", + route: &routev1.Route{ + ObjectMeta: metav1.ObjectMeta{ + Name: "valid-route", + Namespace: "foo", + ResourceVersion: "1", + }, + Spec: routev1.RouteSpec{ + Path: "/valid-path", // Initial valid path + To: routev1.RouteTargetReference{ + Name: "serviceName", + Kind: "Service", + }, + }, + }, + change: func(route *routev1.Route) { + route.Spec.Path = "/path#invalid" // Introduce # + }, + expectedErrors: 1, + }, + { + name: "Reject update to spec.path containing whitespace", + route: &routev1.Route{ + ObjectMeta: metav1.ObjectMeta{ + Name: "valid-route", + Namespace: "foo", + ResourceVersion: "1", + }, + Spec: routev1.RouteSpec{ + Path: "/valid-path", + To: routev1.RouteTargetReference{ + Name: "serviceName", + Kind: "Service", + }, + }, + }, + change: func(route *routev1.Route) { + route.Spec.Path = "/invalid path" // Introduce whitespace + }, + expectedErrors: 1, + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + // Create a copy of the original route to use as oldRoute + oldRoute := tc.route.DeepCopy() + + // Convert to internal API type + var oldRouteInternal routeapi.Route + if err := routev1conversion.Convert_v1_Route_To_route_Route(oldRoute, &oldRouteInternal, nil); err != nil { + t.Fatalf("failed to convert old route: %v", err) + } + + // Apply the change function to create the new route + newRoute := tc.route.DeepCopy() + tc.change(newRoute) + + // Convert new route to internal API type + var newRouteInternal routeapi.Route + if err := routev1conversion.Convert_v1_Route_To_route_Route(newRoute, &newRouteInternal, nil); err != nil { + t.Fatalf("failed to convert new route: %v", err) + } + + // Validate the update + errs := strategy.ValidateUpdate(ctx, &newRouteInternal, &oldRouteInternal) + + // Check if the number of errors matches expectations + if len(errs) != tc.expectedErrors { + t.Errorf("expected %d errors, but got %d: %v", tc.expectedErrors, len(errs), errs) + } + }) + } +}