New validation webhook for route uniqueness

This adds a validating webhook to ensure the uniqueness of routes across all orgs and spaces.

Since the name registry is used now in 3 webhooks: subnamespaceanchor,
app and route, we factored out the common logic in a new
DuplicateValidator helper.

Route uniqueness is checked using a combination of host, path, domain
guid and domain namespace within the cf root namespace. Note we are not
using the actual domain name, as this should be immutable, and avoid an
extra lookup.

We have introduced a config property to set the root CF namespace in the
controllers config, and the various config yamls, as we need to use a
name registry in the root namespace to guarantee route uniqueness
throughout all orgs and spaces.

We have also moved the duplicate validator and error messages from
workloads up a package level to webhooks, so networking and workloads
can share the code.

There is no way to update route host/path/domain via the API, so we have
an e2e test to trigger the update hook via the add destinations
endpoint. This will exit the hook early as new and old route
host/path/domain remain unchanged.

The code also fixes e2e flakiness by setting the non-propagation
annotation on the cfdomain permissions workaround role binding. See #728

Issue: #178
Co-authored-by: Danail Branekov <danailster@gmail.com>
Co-authored-by: Giuseppe Capizzi <gcapizzi@pivotal.io>
Co-authored-by: Georgi Sabev <georgethebeatle@gmail.com>

api/apis/app_handler.go

@@ -11,7 +11,7 @@ import (
 
 	"github.com/gorilla/schema"
 
-	"code.cloudfoundry.org/cf-k8s-controllers/controllers/webhooks/workloads"
+	"code.cloudfoundry.org/cf-k8s-controllers/controllers/webhooks"
 
 	"code.cloudfoundry.org/cf-k8s-controllers/api/authorization"
 	"code.cloudfoundry.org/cf-k8s-controllers/api/payloads"
@@ -147,7 +147,7 @@ func (h *AppHandler) appCreateHandler(authInfo authorization.Info, w http.Respon
 
 	appRecord, err := h.appRepo.CreateApp(ctx, authInfo, payload.ToAppCreateMessage())
 	if err != nil {
-		if workloads.HasErrorCode(err, workloads.DuplicateAppError) {
+		if webhooks.HasErrorCode(err, webhooks.DuplicateAppError) {
 			errorDetail := fmt.Sprintf("App with the name '%s' already exists.", payload.Name)
 			h.logger.Error(err, errorDetail, "App Name", payload.Name)
 			writeUniquenessError(w, errorDetail)

api/apis/org_handler.go

@@ -15,7 +15,7 @@ import (
 	"code.cloudfoundry.org/cf-k8s-controllers/api/payloads"
 	"code.cloudfoundry.org/cf-k8s-controllers/api/presenter"
 	"code.cloudfoundry.org/cf-k8s-controllers/api/repositories"
-	"code.cloudfoundry.org/cf-k8s-controllers/controllers/webhooks/workloads"
+	"code.cloudfoundry.org/cf-k8s-controllers/controllers/webhooks"
 )
 
 const (
@@ -62,7 +62,7 @@ func (h *OrgHandler) orgCreateHandler(info authorization.Info, w http.ResponseWr
 
 	record, err := h.orgRepo.CreateOrg(r.Context(), info, org)
 	if err != nil {
-		if workloads.HasErrorCode(err, workloads.DuplicateOrgNameError) {
+		if webhooks.HasErrorCode(err, webhooks.DuplicateOrgNameError) {
 			errorDetail := fmt.Sprintf("Organization '%s' already exists.", org.Name)
 			h.logger.Info(errorDetail)
 			writeUnprocessableEntityError(w, errorDetail)

api/apis/org_handler_test.go

@@ -19,7 +19,7 @@ import (
 	"code.cloudfoundry.org/cf-k8s-controllers/api/apis/fake"
 	"code.cloudfoundry.org/cf-k8s-controllers/api/authorization"
 	"code.cloudfoundry.org/cf-k8s-controllers/api/repositories"
-	"code.cloudfoundry.org/cf-k8s-controllers/controllers/webhooks/workloads"
+	"code.cloudfoundry.org/cf-k8s-controllers/controllers/webhooks"
 )
 
 const (
@@ -111,7 +111,7 @@ var _ = Describe("OrgHandler", func() {
 			BeforeEach(func() {
 				var err error = &k8serrors.StatusError{
 					ErrStatus: metav1.Status{
-						Reason: metav1.StatusReason(fmt.Sprintf(`{"code":%d}`, workloads.DuplicateOrgNameError)),
+						Reason: metav1.StatusReason(fmt.Sprintf(`{"code":%d}`, webhooks.DuplicateOrgNameError)),
 					},
 				}
 				orgRepo.CreateOrgReturns(repositories.OrgRecord{}, err)

api/apis/route_handler.go

@@ -237,7 +237,17 @@ func (h *RouteHandler) routeCreateHandler(authInfo authorization.Info, w http.Re
 			h.logger.Error(err, "no auth to create route")
 			writeNotAuthenticatedErrorResponse(w)
 			return
-		default: // TODO: Catch the error from the (unwritten) validating webhook
+		case repositories.DuplicateError:
+			pathDetails := ""
+			if createRouteMessage.Path != "" {
+				pathDetails = fmt.Sprintf(" and path '%s'", createRouteMessage.Path)
+			}
+			errorDetail := fmt.Sprintf("Route already exists with host '%s'%s for domain '%s'.",
+				createRouteMessage.Host, pathDetails, domain.Name)
+			h.logger.Info(errorDetail)
+			writeUnprocessableEntityError(w, errorDetail)
+			return
+		default:
 			h.logger.Error(err, "Failed to create route", "Route Host", payload.Host)
 			writeUnknownErrorResponse(w)
 			return

api/apis/route_handler_test.go

@@ -958,6 +958,42 @@ var _ = Describe("RouteHandler", func() {
 			})
 		})
 
+		When("CreateRoute returns an already exists error", func() {
+			BeforeEach(func() {
+				routeRepo.CreateRouteReturns(repositories.RouteRecord{}, repositories.NewDuplicateError(repositories.RouteResourceType, nil))
+			})
+
+			It("returns a duplicate resource error", func() {
+				Expect(rr).To(HaveHTTPBody(MatchJSON(`{
+                    "errors": [
+                        {
+                            "title": "CF-UnprocessableEntity",
+                            "detail": "Route already exists with host 'test-route-host' and path '/test-route-path' for domain 'test-domain-name'.",
+                            "code": 10008
+                        }
+                    ]
+                }`)))
+			})
+
+			When("the route path is not set", func() {
+				BeforeEach(func() {
+					requestBody = initializeCreateRouteRequestBody(testRouteHost, "", testSpaceGUID, testDomainGUID, nil, nil)
+				})
+
+				It("returns a duplicate resource error", func() {
+					Expect(rr).To(HaveHTTPBody(MatchJSON(`{
+                    "errors": [
+                        {
+                            "title": "CF-UnprocessableEntity",
+                            "detail": "Route already exists with host 'test-route-host' for domain 'test-domain-name'.",
+                            "code": 10008
+                        }
+                    ]
+                }`)))
+				})
+			})
+		})
+
 		When("CreateRoute returns an unknown error", func() {
 			BeforeEach(func() {
 				routeRepo.CreateRouteReturns(repositories.RouteRecord{},

api/apis/space_handler.go

@@ -17,7 +17,7 @@ import (
 	"code.cloudfoundry.org/cf-k8s-controllers/api/payloads"
 	"code.cloudfoundry.org/cf-k8s-controllers/api/presenter"
 	"code.cloudfoundry.org/cf-k8s-controllers/api/repositories"
-	"code.cloudfoundry.org/cf-k8s-controllers/controllers/webhooks/workloads"
+	"code.cloudfoundry.org/cf-k8s-controllers/controllers/webhooks"
 )
 
 const (
@@ -70,7 +70,7 @@ func (h *SpaceHandler) SpaceCreateHandler(info authorization.Info, w http.Respon
 
 	record, err := h.spaceRepo.CreateSpace(ctx, info, space)
 	if err != nil {
-		if workloads.HasErrorCode(err, workloads.DuplicateSpaceNameError) {
+		if webhooks.HasErrorCode(err, webhooks.DuplicateSpaceNameError) {
 			errorDetail := fmt.Sprintf("Space '%s' already exists.", space.Name)
 			h.logger.Info(errorDetail)
 			writeUnprocessableEntityError(w, errorDetail)

api/apis/space_handler_test.go

@@ -16,7 +16,7 @@ import (
 	"code.cloudfoundry.org/cf-k8s-controllers/api/apis/fake"
 	"code.cloudfoundry.org/cf-k8s-controllers/api/authorization"
 	"code.cloudfoundry.org/cf-k8s-controllers/api/repositories"
-	"code.cloudfoundry.org/cf-k8s-controllers/controllers/webhooks/workloads"
+	"code.cloudfoundry.org/cf-k8s-controllers/controllers/webhooks"
 )
 
 var _ = Describe("Spaces", func() {
@@ -223,7 +223,7 @@ var _ = Describe("Spaces", func() {
 			BeforeEach(func() {
 				var err error = &k8serrors.StatusError{
 					ErrStatus: metav1.Status{
-						Reason: metav1.StatusReason(fmt.Sprintf(`{"code":%d}`, workloads.DuplicateSpaceNameError)),
+						Reason: metav1.StatusReason(fmt.Sprintf(`{"code":%d}`, webhooks.DuplicateSpaceNameError)),
 					},
 				}
 				spaceRepo.CreateSpaceReturns(repositories.SpaceRecord{}, err)

api/repositories/errors.go

@@ -36,16 +36,20 @@ type NotFoundError struct {
 	repoError
 }
 
-type ForbiddenError struct {
-	repoError
-}
-
 func NewNotFoundError(resourceType string, baseError error) NotFoundError {
 	return NotFoundError{
 		repoError: repoError{err: baseError, resourceType: resourceType, message: "not found"},
 	}
 }
 
+func IsNotFoundError(err error) bool {
+	return errors.As(err, &NotFoundError{})
+}
+
+type ForbiddenError struct {
+	repoError
+}
+
 func NewForbiddenError(resourceType string, baseError error) ForbiddenError {
 	return ForbiddenError{
 		repoError: repoError{err: baseError, resourceType: resourceType, message: "forbidden"},
@@ -56,6 +60,16 @@ func IsForbiddenError(err error) bool {
 	return errors.As(err, &ForbiddenError{})
 }
 
-func IsNotFoundError(err error) bool {
-	return errors.As(err, &NotFoundError{})
+type DuplicateError struct {
+	repoError
+}
+
+func NewDuplicateError(resourceType string, baseError error) DuplicateError {
+	return DuplicateError{
+		repoError: repoError{err: baseError, resourceType: resourceType, message: "duplicate"},
+	}
+}
+
+func IsDuplicateError(err error) bool {
+	return errors.As(err, &DuplicateError{})
 }

api/repositories/route_repository.go

@@ -6,6 +6,7 @@ import (
 
 	"code.cloudfoundry.org/cf-k8s-controllers/api/authorization"
 	networkingv1alpha1 "code.cloudfoundry.org/cf-k8s-controllers/controllers/apis/networking/v1alpha1"
+	"code.cloudfoundry.org/cf-k8s-controllers/controllers/webhooks"
 
 	"github.com/google/uuid"
 	v1 "k8s.io/api/core/v1"
@@ -333,6 +334,9 @@ func (f *RouteRepo) CreateRoute(ctx context.Context, authInfo authorization.Info
 	cfRoute := message.toCFRoute()
 	err := f.privilegedClient.Create(ctx, &cfRoute)
 	if err != nil {
+		if webhooks.HasErrorCode(err, webhooks.DuplicateRouteError) {
+			return RouteRecord{}, NewDuplicateError(RouteResourceType, err)
+		}
 		return RouteRecord{}, err
 	}
 

api/tests/e2e/e2e_suite_test.go

@@ -74,14 +74,21 @@ type relationship struct {
 type resourceList struct {
 	Resources []resource `json:"resources"`
 }
+
 type resourceListWithInclusion struct {
 	Resources []resource    `json:"resources"`
 	Included  *includedApps `json:",omitempty"`
 }
+
 type includedApps struct {
 	Apps []resource `json:"apps"`
 }
 
+type bareResource struct {
+	GUID string `json:"guid,omitempty"`
+	Name string `json:"name,omitempty"`
+}
+
 type appResource struct {
 	resource `json:",inline"`
 	State    string `json:"state,omitempty"`
@@ -127,8 +134,17 @@ type manifestRouteResource struct {
 
 type routeResource struct {
 	resource `json:",inline"`
-	Host     string
-	Path     string
+	Host     string `json:"host"`
+	Path     string `json:"path"`
+	URL      string `json:"url,omitempty"`
+}
+
+type destinationsResource struct {
+	Destinations []destination `json:"destinations"`
+}
+
+type destination struct {
+	App bareResource `json:"app"`
 }
 
 type cfErrs struct {
@@ -725,6 +741,12 @@ func createDomain(name string) string {
 	return domain.Name
 }
 
+func deleteDomain(guid string) {
+	Expect(k8sClient.Delete(context.Background(), &networkingv1alpha1.CFDomain{
+		ObjectMeta: metav1.ObjectMeta{Namespace: rootNamespace, Name: guid},
+	})).To(Succeed())
+}
+
 func createRoute(host, path string, spaceGUID, domainGUID string) string {
 	var route resource
 
@@ -742,8 +764,8 @@ func createRoute(host, path string, spaceGUID, domainGUID string) string {
 		SetResult(&route).
 		Post("/v3/routes")
 
-	Expect(err).NotTo(HaveOccurred())
-	Expect(resp).To(HaveRestyStatusCode(http.StatusCreated))
+	ExpectWithOffset(1, err).NotTo(HaveOccurred())
+	ExpectWithOffset(1, resp).To(HaveRestyStatusCode(http.StatusCreated))
 
 	return route.GUID
 }