Skip to content
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

[Feature]: API Client cannot create a duplicate route #178

Closed
davewalter opened this issue Sep 23, 2021 · 3 comments
Closed

[Feature]: API Client cannot create a duplicate route #178

davewalter opened this issue Sep 23, 2021 · 3 comments

Comments

@davewalter
Copy link
Member

davewalter commented Sep 23, 2021

Blockers/Dependencies

No response

Background

As a client of the API shim
I want to receive an error when I attempt to create a duplicate route based on its FQDN + context path
So that I am prevented from creating a route if it already exists

Acceptance Criteria

With context path

GIVEN I have an existing route (e.g. foo.example.com/bar )
WHEN I try to make another route with that same host, domain, and context path
THEN receive a 422 error

{
  "errors": [
    {
      "code": 10008,
      "detail": "Route already exists with host 'foo' and path '/bar' for domain 'atom-kitten.capi.land'.",
      "title": "CF-UnprocessableEntity"
    }
  ]
}

Without context path

GIVEN I have an existing route (e.g. foo.example.com )
WHEN I try to make another route with that same host and domain
THEN receive a 422 error

{
  "errors": [
    {
      "code": 10008,
      "detail": "Route already exists with host 'foo' for domain 'atom-kitten.capi.land'.",
      "title": "CF-UnprocessableEntity"
    }
  ]
}

Dev Notes

  • Start by implementing this the same way as we do for app names (with leases)
    • We recognize that the direct kubectl apply use case may not have sufficient guards against tampering, but the main API use case remains covered
@Birdrock Birdrock transferred this issue from cloudfoundry/cf-k8s-api Nov 2, 2021
kieron-dev pushed a commit that referenced this issue Feb 25, 2022
Checks combination of host, path, domain guid and namespace is unique
within the route namespace.

Also moves the duplicate validator and error messages from workloads up
a package level to webhooks

Issue: #178
kieron-dev pushed a commit that referenced this issue Feb 28, 2022
StatusOK -> StatusCreated as per spec

Issue: #178
Co-authored-by: Kieron Browne <kbrowne@vmware.com>
kieron-dev pushed a commit that referenced this issue Feb 28, 2022
This introduces a duplicate name helper for validating webhooks, to
factor out common code between the app, org/space and new route
webhooks.

Route uniqueness is checked using a combination of host, path, domain
guid and namespace within the route namespace. Note we are not
using actual domain name as this could feasbily be changed on the
domain, invalidating the name registry entry for the route.

Also moves 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.

Co-authored-by: Danail Branekov <danailster@gmail.com>
Co-authored-by: Giuseppe Capizzi <gcapizzi@pivotal.io>
Co-authored-by: Kieron Browne <kbrowne@vmware.com>
Issue: #178
@kieron-dev
Copy link
Contributor

@davewalter, I have a couple of questions:

  1. Does a route need a unique FQDN+path within the space where it is created, or throughout CF - i.e. in the root namespace?
  2. How should the associated domain be included in the uniqueness checking? We could use the domain name from the domain resource, but then there is the worry that the domain name could be modified there directly, making the route name stored in the registry out of date. Or we could use the domain's name and namespace as part of the uniqueness check, which relies on the domain name being unique throughout CF.

@tcdowney
Copy link
Member

Food for thought: Unlike most resources, Domains in CF can't be updated. I know the CRs can right now, but if we found ways to make them "immutable-ish" that could help with those questions.

https://v3-apidocs.cloudfoundry.org/version/3.115.0/index.html#update-a-domain

@kieron-dev
Copy link
Contributor

It also seems impossible to update host, path and domain on a route via the API. So maybe they should be made immutable too.

kieron-dev pushed a commit that referenced this issue Mar 1, 2022
This introduces a duplicate name helper for validating webhooks, to
factor out common code between the app, org/space and new route
webhooks.

Route uniqueness is checked using a combination of host, path, domain
guid and namespace within the cf root namespace. Note we are not
using actual domain name as this could feasbily be changed on the
domain, invalidating the name registry entry for the route.

We have introduced a config property to set the root CF namespace in the
controllers config, and the various config yamls.

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.

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>
kieron-dev pushed a commit that referenced this issue Mar 1, 2022
This introduces a duplicate name helper for validating webhooks, to
factor out common code between the app, org/space and new route
webhooks.

Route uniqueness is checked using a combination of host, path, domain
guid and namespace within the cf root namespace. Note we are not
using actual domain name as this could feasbily be changed on the
domain, invalidating the name registry entry for the route.

We have introduced a config property to set the root CF namespace in the
controllers config, and the various config yamls.

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.

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>
kieron-dev pushed a commit that referenced this issue Mar 1, 2022
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>
davewalter added a commit that referenced this issue Mar 1, 2022
New validation webhook for route uniqueness
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Archived in project
Development

No branches or pull requests

6 participants