Skip to content

fix: nohttp(s) shouldn't disable fallback server #2475

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Jun 9, 2024
Merged

fix: nohttp(s) shouldn't disable fallback server #2475

merged 1 commit into from
Jun 9, 2024

Conversation

pini-gh
Copy link
Contributor

@pini-gh pini-gh commented Jun 6, 2024

Say we have two containers:

  • app1 with HTTPS_METHOD=redirect
  • app2 with HTTPS_METHOD=nohttps

Without this change the fallback answer on an HTTPS request to an unknown server would change depending on whether app1 is up (503) or not (connection refused). This is not wanted.

In case someone doesn't want HTTPS at all, they just have to not bind port 443.

@pini-gh
fix: nohttp(s) shouldn't disable fallback server
4606b15 
Say we have two containers:
- `app1` with `HTTPS_METHOD=redirect`
- `app2` with `HTTPS_METHOD=nohttps`

Without this change the fallback answer on an HTTPS request to an unknown
server would change depending on whether `app1` is up (503) or not
(connection refused). This is not wanted.

In case someone doesn't want HTTPS at all, they just have to not bind
port 443.
@buchdag
Copy link
Member

buchdag commented Jun 9, 2024

LGTM, unknown host behaviour after PR:

default certificate at least one HTTP vhost at least one HTTPS vhost HTTPS Method (on proxy) HTTP response HTTPS response Certificate served
❌ ❌ ❌ not set 503 444 n/a
❌ ❌ ❌ redirect 503 444 n/a
❌ ❌ ❌ noredirect 503 444 n/a
❌ ❌ ❌ nohttp 503 444 n/a
❌ ❌ ❌ nohttps 503 444 n/a
🟢 ❌ ❌ not set 503 503 default
🟢 ❌ ❌ redirect 503 503 default
🟢 ❌ ❌ noredirect 503 503 default
🟢 ❌ ❌ nohttp 503 503 default
🟢 ❌ ❌ nohttps 503 503 default
❌ 🟢 ❌ not set 503 444 n/a
❌ 🟢 ❌ redirect 503 444 n/a
❌ 🟢 ❌ noredirect 503 444 n/a
❌ 🟢 ❌ nohttp 503 444 n/a
❌ 🟢 ❌ nohttps 503 444 n/a
🟢 🟢 ❌ not set 503 503 default
🟢 🟢 ❌ redirect 503 503 default
🟢 🟢 ❌ noredirect 503 503 default
🟢 🟢 ❌ nohttp 503 503 default
🟢 🟢 ❌ nohttps 503 503 default
❌ 🟢 🟢 not set 503 444 n/a
❌ 🟢 🟢 redirect 503 444 n/a
❌ 🟢 🟢 noredirect 503 444 n/a
❌ 🟢 🟢 nohttp 503 444 n/a
❌ 🟢 🟢 nohttps 503 444 n/a
🟢 🟢 🟢 not set 503 503 default
🟢 🟢 🟢 redirect 503 503 default
🟢 🟢 🟢 noredirect 503 503 default
🟢 🟢 🟢 nohttp 503 503 default
🟢 🟢 🟢 nohttps 503 503 default

@buchdag buchdag merged commit 477366d into nginx-proxy:main Jun 9, 2024
@pini-gh pini-gh deleted the pini-fallback-https branch June 9, 2024 16:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@buchdag buchdag buchdag approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@pini-gh @buchdag