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

deny_traffic field in frontends #1003

Closed
wants to merge 1 commit into from
Closed

Conversation

Keksoj
Copy link
Contributor

@Keksoj Keksoj commented Oct 16, 2023

This field deny_traffic replaces cluster_id being an option, when None meant DENY.

Related to #997 and #996

@Keksoj Keksoj added this to the v0.16.0 milestone Oct 16, 2023
@Keksoj Keksoj self-assigned this Oct 16, 2023
it replaces cluster_id being an option, with None meaning DENY
@Geal
Copy link
Member

Geal commented Oct 19, 2023

I think my point from #997 (comment) was not clear.
There are very good reasons to separate the matching rule (frontends) from the routing decision (ClusterId), and you'll see most web servers and proxys clearly separate them (example: ACLs in haproxy).

When you start putting the routing decision in the matching rule, then that means that the rules or routing decisions become much less reusable. And when you want to add more routing decisions, then you end up stuffing the matching rule struct with more and more fields, until you want to break them out in a separate structure, but now removing fields from this public structure breaks the public API and existing config.

Also, I'm not sure about the state of the code right now, but there was a clear semantic difference between:

  • cluster_id = None => we know about this frontend (like, an app declares that domain) but there are no servers to associate to it (the app has not started yet), which results in a 503 response, for which Clever Cloud has a specific page to show
  • cluster_id = Some(Deny): you are not allowed to make this request, so 401. Easy way to shut out scanning bots, very different from the 503 response
  • cluster_id = Some(id): we have a set of servers to associate here (but might still return a 503 if none of them answer)

@Keksoj
Copy link
Contributor Author

Keksoj commented Jan 22, 2024

You're right. After reviewing the issue several time, there is no concrete improvement to be made here.

@Keksoj Keksoj closed this Jan 22, 2024
@Keksoj Keksoj deleted the deny-traffic-field-in-frontends branch February 8, 2024 13:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants