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

Remove deny rule, enforce cluster checks when adding frontends and backends #997

Closed
wants to merge 5 commits into from

Conversation

Keksoj
Copy link
Contributor

@Keksoj Keksoj commented Sep 22, 2023

The deny rule, added at 5dc5c00 seems of little value today. It returns a 401 HTTP response when a frontend without cluster has received a connection.

In conformity with the spirit of #996 , this PR removes the possibility of creating a frontend without a cluster. cluster_id is no longer an option, and the cluster is checked for existence when adding a frontend to the state.

This PR adds checks to be sure that backends, too, will not be added to the state if the corresponding cluster does not exist.

Last but not least, this pull requests adds this feature: removing a cluster will remove its associated frontends and backends. (but a cluster can still be edited with the AddFrontend request)

@Keksoj Keksoj added this to the v0.16.0 milestone Sep 22, 2023
@Keksoj Keksoj self-assigned this Sep 22, 2023
@Keksoj Keksoj requested a review from Wonshtrum as a code owner September 22, 2023 13:41
@Geal
Copy link
Member

Geal commented Sep 22, 2023

Chesterton's fence 🫠 instead of restricting the features to only what you're using, ask yourself what would be useful generally for this tool

The context you're missing here: someday you"ll want to prevent some traffic pattern from reaching the cluster (web vuln, DoS where the proxy can handle the traffic but the cluster can't). This has already been useful for production issues at Clever Cloud.

If you want to avoid orphan clusters and frontend, this is better handled in your control plane

@FlorentinDUBOIS
Copy link
Collaborator

Hey @Geal,

Firstly, thank you for given us the context and the expression, I really like it. Do you know, if there are others reasons that motivate this choice of implementation?
We took a look at the git history, but we did not see more information about this implementation choice.

Besides, it seems that what you are describing is a shortcut for creating a "black hole" cluster without backend and the frontend with a deny route that match everything. Am I missing something ?

To me, a frontend (with or without a cluster) without backend (so no one available) should answer a 503 according to https://developer.mozilla.org/en-US/docs/Web/HTTP/Status#server_error_responses.

Thank you and have a nice weekend!

@Geal
Copy link
Member

Geal commented Sep 22, 2023

It is done that way because a HTTP proxy is first a web server, second a proxy. If a route is not allowed by that server, it should return a 401, not a 503. The long term goal was to allow more options to short circuit requests, like default pages (as is already done with 404) or tarpitting requests. The way it was done was not to deny everything, but to allow varying precision levels, like detecting a specific attack on an app and stopping its traffic while letting the rest through, or matching on traffic from a bot across all apps. I think I left some info on that in the roadmap docs I wrote before I left.
One advantage of handling this directly after matching a frontend instead of going to the cluster level is that it was extremely cheap. In particular, default answers cost almost nothing to send back to the client because we won't need a buffer

@Keksoj
Copy link
Contributor Author

Keksoj commented Oct 19, 2023

Let's close this in favor of #1003 then.

@Keksoj Keksoj closed this Oct 19, 2023
@Keksoj Keksoj deleted the remove-deny-rule branch December 1, 2023 13:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

3 participants