Merge pull request #859 from cloudfoundry/issue-755

Validate CFRoute path

Closes #755

api/apis/shared.go

@@ -11,13 +11,15 @@ import (
 	"code.cloudfoundry.org/bytefmt"
 	"code.cloudfoundry.org/cf-k8s-controllers/api/apierrors"
 	"code.cloudfoundry.org/cf-k8s-controllers/api/payloads"
-	"gopkg.in/yaml.v3"
 
 	"github.com/go-http-utils/headers"
 	"github.com/go-playground/locales/en"
 	ut "github.com/go-playground/universal-translator"
 	"github.com/go-playground/validator/v10"
 	en_translations "github.com/go-playground/validator/v10/translations/en"
+	"golang.org/x/text/cases"
+	"golang.org/x/text/language"
+	"gopkg.in/yaml.v3"
 	ctrl "sigs.k8s.io/controller-runtime"
 )
 
@@ -51,8 +53,9 @@ func (dv *DecoderValidator) DecodeAndValidateJSONPayload(r *http.Request, object
 		var unmarshalTypeError *json.UnmarshalTypeError
 		switch {
 		case errors.As(err, &unmarshalTypeError):
-			Logger.Error(err, fmt.Sprintf("Request body contains an invalid value for the %q field (should be of type %v)", strings.Title(unmarshalTypeError.Field), unmarshalTypeError.Type))
-			return apierrors.NewUnprocessableEntityError(err, fmt.Sprintf("%v must be a %v", strings.Title(unmarshalTypeError.Field), unmarshalTypeError.Type))
+			titler := cases.Title(language.AmericanEnglish)
+			Logger.Error(err, fmt.Sprintf("Request body contains an invalid value for the %q field (should be of type %v)", titler.String(unmarshalTypeError.Field), unmarshalTypeError.Type))
+			return apierrors.NewUnprocessableEntityError(err, fmt.Sprintf("%v must be a %v", titler.String(unmarshalTypeError.Field), unmarshalTypeError.Type))
 		case strings.HasPrefix(err.Error(), "json: unknown field"):
 			// check whether the message matches an "unknown field" error. If so, 422. Else, 400
 			Logger.Error(err, fmt.Sprintf("Unknown field in JSON body: %T: %q", err, err.Error()))

api/repositories/route_repository.go

@@ -313,14 +313,8 @@ func (f *RouteRepo) CreateRoute(ctx context.Context, authInfo authorization.Info
 
 	err = userClient.Create(ctx, &cfRoute)
 	if err != nil {
-		if webhooks.HasErrorCode(err, webhooks.DuplicateRouteError) {
-			pathDetails := ""
-			if message.Path != "" {
-				pathDetails = fmt.Sprintf(" and path '%s'", message.Path)
-			}
-			errorDetail := fmt.Sprintf("Route already exists with host '%s'%s for domain '%s'.",
-				message.Host, pathDetails, message.DomainName)
-			return RouteRecord{}, apierrors.NewUnprocessableEntityError(err, errorDetail)
+		if webhooks.IsValidationError(err) {
+			return RouteRecord{}, apierrors.NewUnprocessableEntityError(err, webhooks.GetErrorMessage(err))
 		}
 		return RouteRecord{}, apierrors.FromK8sError(err, RouteResourceType)
 	}

controllers/config/samples/cfroute.yaml

@@ -9,6 +9,7 @@ spec:
   protocol: http
   domainRef:
     name: 5b5032ab-7fc8-4da5-b853-821fd1879201
+    namespace: cf
   # This array of destinations starts empty and is filled by CF Shim endpoints for /v3/routes/:guid/destinations
   destinations:
     - guid: 8ad77ef4-53d5-117a-b640-0ae227a13f35

controllers/webhooks/cf_validation_errors.go

@@ -23,8 +23,18 @@ const (
 	DuplicateDomainError
 	DuplicateServiceInstanceNameError
 	RouteDestinationNotInSpace
+	HostNameIsInvalidError
+	PathValidationError
 )
 
+func (v ValidationError) Marshal() string {
+	bytes, err := json.Marshal(v)
+	if err != nil {
+		return err.Error()
+	}
+	return string(bytes)
+}
+
 func (w ValidationErrorCode) Marshal() string {
 	bytes, err := json.Marshal(ValidationError{
 		Code:    w,
@@ -61,6 +71,8 @@ func (w ValidationErrorCode) GetMessage() string {
 		return "CFServiceInstance with same spec.name exists"
 	case RouteDestinationNotInSpace:
 		return "Route destination app not found in space"
+	case HostNameIsInvalidError:
+		return "Missing or Invalid host - Routes in shared domains must have a valid host defined"
 	default:
 		return "An unknown error has occurred"
 	}
@@ -79,3 +91,32 @@ func HasErrorCode(err error, code ValidationErrorCode) bool {
 	}
 	return false
 }
+
+func IsValidationError(err error) bool {
+	if statusError := new(k8serrors.StatusError); errors.As(err, &statusError) {
+		reason := statusError.Status().Reason
+
+		val := new(ValidationErrorCode)
+		val.Unmarshall(string(reason))
+
+		if *val != UnknownError {
+			return true
+		}
+	}
+	return false
+}
+
+func GetErrorMessage(err error) string {
+	if statusError := new(k8serrors.StatusError); errors.As(err, &statusError) {
+		reason := statusError.Status().Reason
+
+		val := new(ValidationError)
+		err := json.Unmarshal([]byte(reason), val)
+		if err != nil {
+			return ""
+		}
+
+		return val.Message
+	}
+	return ""
+}

controllers/webhooks/networking/cfroute_validation.go

@@ -3,6 +3,8 @@ package networking
 import (
 	"context"
 	"errors"
+	"fmt"
+	"net/url"
 	"regexp"
 	"strings"
 
@@ -22,6 +24,11 @@ import (
 
 const (
 	RouteEntityType = "route"
+
+	InvalidURIError          = "Invalid Route URI"
+	PathIsSlashError         = "Path cannot be a single slash"
+	PathHasQuestionMarkError = "Path cannot contain a question mark"
+	PathLengthExceededError  = "Path cannot exceed 128 characters"
 )
 
 //go:generate go run github.com/maxbrunsfeld/counterfeiter/v6 -generate
@@ -60,6 +67,7 @@ func (v *CFRouteValidation) SetupWebhookWithManager(mgr ctrl.Manager) error {
 
 func (v *CFRouteValidation) Handle(ctx context.Context, req admission.Request) admission.Response {
 	var route, oldRoute networkingv1alpha1.CFRoute
+	var domain networkingv1alpha1.CFDomain
 	if req.Operation == admissionv1.Create || req.Operation == admissionv1.Update {
 		err := v.decoder.Decode(req, &route)
 		if err != nil {
@@ -68,6 +76,14 @@ func (v *CFRouteValidation) Handle(ctx context.Context, req admission.Request) a
 
 			return admission.Denied(errMessage)
 		}
+
+		err = v.Client.Get(ctx, types.NamespacedName{Name: route.Spec.DomainRef.Name, Namespace: route.Spec.DomainRef.Namespace}, &domain)
+		if err != nil {
+			errMessage := "Error while retrieving CFDomain object"
+			logger.Error(err, errMessage)
+
+			return admission.Denied(errMessage)
+		}
 	}
 	if req.Operation == admissionv1.Update || req.Operation == admissionv1.Delete {
 		err := v.decoder.DecodeRaw(req.OldObject, &oldRoute)
@@ -86,7 +102,11 @@ func (v *CFRouteValidation) Handle(ctx context.Context, req admission.Request) a
 		if err != nil {
 			return admission.Denied(err.Error())
 		}
-		_, err = v.isFQDN(route)
+		_, err = IsFQDN(route.Spec.Host, domain.Spec.Name)
+		if err != nil {
+			return admission.Denied(err.Error())
+		}
+		err = validatePath(route)
 		if err != nil {
 			return admission.Denied(err.Error())
 		}
@@ -105,7 +125,11 @@ func (v *CFRouteValidation) Handle(ctx context.Context, req admission.Request) a
 		if err != nil {
 			return admission.Denied(err.Error())
 		}
-		_, err = v.isFQDN(route)
+		_, err = IsFQDN(route.Spec.Host, domain.Spec.Name)
+		if err != nil {
+			return admission.Denied(err.Error())
+		}
+		err = validatePath(route)
 		if err != nil {
 			return admission.Denied(err.Error())
 		}
@@ -127,7 +151,18 @@ func (v *CFRouteValidation) Handle(ctx context.Context, req admission.Request) a
 		logger.Info("duplicate validation failed", "error", validatorErr)
 
 		if errors.Is(validatorErr, webhooks.ErrorDuplicateName) {
-			return admission.Denied(webhooks.DuplicateRouteError.Marshal())
+			pathDetails := ""
+			if route.Spec.Path != "" {
+				pathDetails = fmt.Sprintf(" and path '%s'", route.Spec.Path)
+			}
+			errorDetail := fmt.Sprintf("Route already exists with host '%s'%s for domain '%s'.",
+				route.Spec.Host, pathDetails, domain.Spec.Name)
+
+			ve := webhooks.ValidationError{
+				Code:    webhooks.DuplicateRouteError,
+				Message: errorDetail,
+			}
+			return admission.Denied(ve.Marshal())
 		}
 
 		return admission.Denied(webhooks.UnknownError.Marshal())
@@ -140,20 +175,44 @@ func uniqueName(route networkingv1alpha1.CFRoute) string {
 	return strings.Join([]string{route.Spec.Host, route.Spec.DomainRef.Namespace, route.Spec.DomainRef.Name, route.Spec.Path}, "::")
 }
 
-func (v *CFRouteValidation) InjectDecoder(d *admission.Decoder) error {
-	v.decoder = d
-	return nil
-}
+func validatePath(route networkingv1alpha1.CFRoute) error {
+	var errStrings []string
 
-func (v *CFRouteValidation) isFQDN(cfRoute networkingv1alpha1.CFRoute) (bool, error) {
-	var cfDomain networkingv1alpha1.CFDomain
-	ctx := context.Background()
-	err := v.Client.Get(ctx, types.NamespacedName{Name: cfRoute.Spec.DomainRef.Name, Namespace: cfRoute.Spec.DomainRef.Namespace}, &cfDomain)
+	if route.Spec.Path == "" {
+		return nil
+	}
+
+	_, err := url.ParseRequestURI(route.Spec.Path)
 	if err != nil {
-		return false, err
+		errStrings = append(errStrings, InvalidURIError)
+	}
+	if route.Spec.Path == "/" {
+		errStrings = append(errStrings, PathIsSlashError)
+	}
+	if strings.Contains(route.Spec.Path, "?") {
+		errStrings = append(errStrings, PathHasQuestionMarkError)
+	}
+	if len(route.Spec.Path) > 128 {
+		errStrings = append(errStrings, PathLengthExceededError)
+	}
+	if len(errStrings) == 0 {
+		return nil
 	}
 
-	return IsFQDN(cfRoute.Spec.Host, cfDomain.Spec.Name)
+	if len(errStrings) > 0 {
+		ve := webhooks.ValidationError{
+			Code:    webhooks.PathValidationError,
+			Message: strings.Join(errStrings, ", "),
+		}
+		return errors.New(ve.Marshal())
+	}
+
+	return nil
+}
+
+func (v *CFRouteValidation) InjectDecoder(d *admission.Decoder) error {
+	v.decoder = d
+	return nil
 }
 
 func isHost(hostname string) (bool, error) {