@pini-gh after some thoughts, I think this PR in its current state might cause might issues than it solves.

First I don't think we should break the previous "If only 1 port exposed, use that" behaviour.

I agree on you that ideally that should have been the target behaviour from the start:

• VIRTUAL_PORT should always be honored (least surprise principle)
• the server shouldn't be marked down unless it is unreachable
• a comment could be added when VIRTUAL_PORT is not exposed (troubleshooting hint)

... but we don't know how many setup we might break by changing the first point because they relied implicitly on the previous behaviour, and I don't want to do something in the likes of moby/libnetwork#2607 that I criticised myself.

Regarding nginx the server 127.0.0.1 down entry, are we safe including it by default or should we try to include It only when required (ie when the proxy and proxied containers don't share any common networks) ?

I would also prefer not to loose the # Cannot connect to network of this container indication because that's a pretty common misconfiguration, especially when using Docker Compose.

I'll think a bit more about it and come back to you with a suggestion we can discuss, if that's okay with you.

And if completely misunderstood the way this PR works, please correct me.

... but we don't know how many setup we might break by changing the first point because they relied implicitly on the previous behaviour, and I don't want to do something in the likes of moby/libnetwork#2607 that I criticised myself.

Well, I really can't see any use case for defining a VIRTUAL_PORT variable which would not be the wanted port. This behavior (always honor VIRTUAL_PORT) is a kind of safety check: when only one port is exposed, either this is the same port, or the configuration is wrong on one side.

Regarding nginx the server 127.0.0.1 down entry, are we safe including it by default or should we try to include It only when required (ie when the proxy and proxied containers don't share any common networks) ?

I've been using this feature for several months with no problem so far. Do you think about a use case where it wouldn't be safe?

I would also prefer not to loose the # Cannot connect to network of this container indication because that's a pretty common misconfiguration, especially when using Docker Compose.

Agreed. How about printing the network's name as well then?

# Cannot connect to network '{{ $containerNetwork.Name }}' of this container

@pini-gh I'll come back to this PR by the end of the week

Looks good to me in this state and after clarification on VIRTUAL_PORT, but ideally I'd like tests to be adjusted to verify that behaviour.

The debug feature should also be tested, on the model of location per vhost test :

def test_custom_block_is_present_in_nginx_generated_conf(docker_compose, nginxproxy):
    assert b"include /etc/nginx/vhost.d/web1.nginx-proxy.local_location;" in nginxproxy.get_conf()

Please tell me if you need help with this.

Please tell me if you need help with this.

Yes, please bear with me. I'm new to this ci/cd thing. Is there a way to run these tests on my box, so I could proceed by try / error iterations?

@buchdag I've added a test to check that the server is unreachable when VIRTUAL_PORT is defined different from the single exposed port. Is that what you have in mind?

Yes, please bear with me. I'm new to this ci/cd thing. Is there a way to run these tests on my box, so I could proceed by try / error iterations?

Yes, instructions for running tests localy are in /tests's README , but since you added a test I guess your already found out.

In addition I'd suggest using a Python virtual environment (with something like virtualenvwrapper) to avoid clogging your local Python with unwanted packages.

I've added a test to check that the server is unreachable when VIRTUAL_PORT is defined different from the single exposed port.

Thanks, I think we're good with the VIRTUAL_PORT tests:

• when a single port is exposed, this is the default port
• when multiple port are exposed, the default port is 80
• VIRTUAL_PORT works
• VIRTUAL_PORT is always honnored even wen it's different from the default port

We need another test for the DEBUG feature and we're good to go.

Something like this for instance

/test/test_debug.yml

web:
  image: web
  expose:
    - "80"
    - "81"
  environment:
    WEB_PORTS: "80 81"
    VIRTUAL_HOST: "web.nginx-proxy.tld"
    VIRTUAL_PORT: "82"
    DEBUG: "true"

sut:
  image: nginxproxy/nginx-proxy:test
  volumes:
    - /var/run/docker.sock:/tmp/docker.sock:ro
    - ../lib/ssl/dhparam.pem:/etc/nginx/dhparam/dhparam.pem:ro

/test/test_debug.py

import pytest

def test_debug_info_is_present_in_nginx_generated_conf(docker_compose, nginxproxy):
    assert b"# Exposed ports: [{80 tcp} {81 tcp}]" in nginxproxy.get_conf()
    assert b"# Default virtual port: 80" in nginxproxy.get_conf()
    assert b"# VIRTUAL_PORT: 82" in nginxproxy.get_conf()

We want to test that DEBUG=true actually adds the expected comments in the generated nginx conf file.

I've added two tests:

• one with DEBUG set at the proxy level
• the other with DEBUG set at the webserver level

LGTM, thanks ! 👍

@pini-gh regarding the default inclusion of the server 127.0.0.1 down entry:

Do you think about a use case where it wouldn't be safe?

Well I found one: #1495

The fix proposed by @wlonkly in #1496 works and pass tests, but as stated in #1495 it would have the side effect of breaking load balancing behind the proxy. I know this isn't an advertised feature but looks like people are using it anyway (#1654).

Do you think there would be an easy-ish way to output the server 127.0.0.1 down entry only when required ?

It's not simple. Because the variables' scope is too limited with this template language. We need to be able to build a list from these nested loops:

        {{ range $knownNetwork := $CurrentContainer.Networks }}
                {{ range $containerNetwork := $container.Networks }}
                        {{ if (and (ne $containerNetwork.Name "ingress") (or (eq $knownNetwork.Name $containerNetwork.Name) (eq $knownNetwork.Name "host"))) }}                                                   

Then it's easy. But we need this list first, and I have no brilliant idea right now :/

I thought about using global variables (which is possible in the go templating language), but I could not find specifically if setting a global variables inside a template execution actions is possible or not.

The only solution I can come up with is something along these lines:

• dereference the upstream template and hardcode it inside the nested loop
• use a $server_found variable set to "false" before the loops
• set this variable to "true" when a server line is issued
• print the fallback server 127.0.0.1 down; line only when $server_found equals "false"