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

Should nginx.tmpl's https_method=redirect block try to include vhost.d files? #1613

Open
dakotahawkins opened this issue May 5, 2021 · 3 comments · May be fixed by #1618
Open

Should nginx.tmpl's https_method=redirect block try to include vhost.d files? #1613

dakotahawkins opened this issue May 5, 2021 · 3 comments · May be fixed by #1618
Labels
kind/feature-request Issue requesting a new feature

Comments

@dakotahawkins
Copy link

I'm trying to add a header to all possible responses (disabling FLoC), and I can't gracefully add it for the default https redirects.

For other (www.domain.com > domain.com) redirects, adding the header in the appropriate vhost.d works correctly with their return 301s.

It seems like it may work if vhost.d includes are added somewhere around nginx.tmpl#L266.

Thoughts?

@buchdag buchdag added the kind/question Issue that might be transferred to Discussions label May 5, 2021
dakotahawkins added a commit to dakotahawkins/nginx-proxy that referenced this issue May 6, 2021
Adds customization points to https redirect responses that are analagous
to other response handling. This gives you the opportunity to add
response headers or etc. before returning the 301 redirect.

If `$host` or `default` exist in `/etc/nginx/vhost.d/`, we rely on
nginx-proxy/acme-companion to add the Let's Encrypt acme challenge
location block there, so it's only included here if those files don't
exist.

Fixes nginx-proxy#1613
@dakotahawkins
Copy link
Author

dakotahawkins commented May 6, 2021

See this change for what I think may be the correct behavior. I'm still working to get the desired effect, and it needs some tests.

Edit: @buchdag that change relies on nginx-proxy/acme-companion to add the location block to vhost files. Is that reasonable or not really possible here (in this repo)? You may not want to assume that users have that set up, but I don't think there's a clean way to check whether the location block exists in a vhost file we can include.

dakotahawkins added a commit to dakotahawkins/nginx-proxy that referenced this issue May 7, 2021
Adds customization points to https redirect responses that are analagous
to other response handling. This gives you the opportunity to add
response headers or etc. before returning the 301 redirect.

If `$host` or `default` exist in `/etc/nginx/vhost.d/`, we rely on
nginx-proxy/acme-companion to add the Let's Encrypt acme challenge
location block there, so it's only included here if those files don't
exist.

New tests in `test_ssl` are similar to tests in `test_custom`, except they
expect 301 responses along with custom configs.

Fixes nginx-proxy#1613
dakotahawkins added a commit to dakotahawkins/nginx-proxy that referenced this issue May 7, 2021
Adds customization points to https redirect responses that are analogous
to other response handling. This gives you the opportunity to add
response headers or etc. before returning the 301 redirect.

If `$host` or `default` exist in `/etc/nginx/vhost.d/`, we rely on
nginx-proxy/acme-companion to add the Let's Encrypt acme challenge
location block there, so it's only included here if those files don't
exist.

New tests in `test_ssl` are similar to tests in `test_custom`, except they
expect 301 responses along with custom configs.

Fixes nginx-proxy#1613
@buchdag
Copy link
Member

buchdag commented May 10, 2021

Edit: @buchdag that change relies on nginx-proxy/acme-companion to add the location block to vhost files. Is that reasonable or not really possible here (in this repo)? You may not want to assume that users have that set up, but I don't think there's a clean way to check whether the location block exists in a vhost file we can include.

That's intersecting with something I had in mind for a while.

nginx-proxy should be able to detect if the acme-companion is in use for a specific container and automatically add the location ^~ /.well-known/acme-challenge/ block to server based on this. Conversely, acme-companion should attempt to manipulate files in /etc/nginx/vhost.d only when nginx-proxy does not use an nginx.tmpl with this feature.

@buchdag buchdag added kind/feature-request Issue requesting a new feature and removed kind/question Issue that might be transferred to Discussions labels May 10, 2021
@dakotahawkins
Copy link
Author

dakotahawkins commented May 11, 2021

Do you have a way of detecting that in mind? I could add it to #1618 if you let me know how you'd like to go about it. I haven't looked in to how to do that yet but it was on my list of things to try.

buchdag pushed a commit to dakotahawkins/nginx-proxy that referenced this issue Aug 3, 2021
Adds customization points to https redirect responses that are analogous
to other response handling. This gives you the opportunity to add
response headers or etc. before returning the 301 redirect.

If `$host` or `default` exist in `/etc/nginx/vhost.d/`, we rely on
nginx-proxy/acme-companion to add the Let's Encrypt acme challenge
location block there, so it's only included here if those files don't
exist.

New tests in `test_ssl` are similar to tests in `test_custom`, except they
expect 301 responses along with custom configs.

Fixes nginx-proxy#1613
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/feature-request Issue requesting a new feature
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants