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

feat: multiport support #2434

Merged
merged 15 commits into from
May 9, 2024
Merged

feat: multiport support #2434

merged 15 commits into from
May 9, 2024

Conversation

buchdag
Copy link
Member

@buchdag buchdag commented May 3, 2024

This PR adds the ability to proxy to multiple ports on a single container using a new VIRTUAL_HOST_MULTIPORTS variable that take a YAML (or JSON) representation of the desired hostname / path / port / dest setup and completely override the VIRTUAL_HOST, VIRTUAL_PORT, VIRTUAL_PATH and VIRTUAL_DEST variables.

See #1504 for the previous discussion on this feature.

Thanks to @pini-gh for the actual feature's code.

Close #560
Close #911
Close #939
Close #1042
Close #1066
Close #1112
Close #1382
Close #1463
Close #1504
Close #2050

Supersede #259, #1405 and #2130

@buchdag buchdag added type/feat PR for a new feature scope/multiport Issue or PR related to multi port proxying labels May 3, 2024
@buchdag buchdag self-assigned this May 3, 2024
@buchdag buchdag mentioned this pull request May 3, 2024
nginx.tmpl Show resolved Hide resolved
@buchdag buchdag force-pushed the multiport-support branch 2 times, most recently from 89c69ec to ef4a4a9 Compare May 3, 2024 20:33
@buchdag
Copy link
Member Author

buchdag commented May 4, 2024

Merging VIRTUAL_HOST and VIRTUAL_HOST_MULTIPORT seems to stop working when we start mixing VIRTUAL_HOST configs with and without VIRTUAL_PORT.

This work (no virtual path):
services:
  merged-singleport-1: # 172.18.0.2
    image: nginx:alpine
    environment:
      VIRTUAL_HOST: merged.nginx-proxy.tld

  merged-singleport-2: # 172.18.0.3
    image: nginx:alpine
    environment:
      VIRTUAL_HOST: merged.nginx-proxy.tld
      VIRTUAL_PORT: "81"

  merged-multiports: # 172.18.0.4
    image: nginx:alpine
    environment:
      VIRTUAL_HOST_MULTIPORTS: |-
        merged.nginx-proxy.tld:
          "/":
            port: 82
# merged.nginx-proxy.tld/
upstream merged.nginx-proxy.tld {
    # Container: nginx-proxy-merged-multiports-1
    # ...
    # Container: nginx-proxy-merged-singleport-1-1
    # ...
    # Container: nginx-proxy-merged-singleport-2-1
    # ...
    server 172.18.0.4:82;
    server 172.18.0.2:80;
    server 172.18.0.3:81;
}
server {
    server_name merged.nginx-proxy.tld;
    # ...
    location / {
        proxy_pass http://merged.nginx-proxy.tld;
        set $upstream_keepalive false;
    }
}
This work too (only virtual path):
services:
  merged-singleport-1: # 172.18.0.2
    image: nginx:alpine
    environment:
      VIRTUAL_HOST: merged.nginx-proxy.tld
      VIRTUAL_PATH: "/foo"

  merged-singleport-2: # 172.18.0.3
    image: nginx:alpine
    environment:
      VIRTUAL_HOST: merged.nginx-proxy.tld
      VIRTUAL_PATH: "/foo"
      VIRTUAL_PORT: "81"

  merged-multiports: # 172.18.0.4
    image: nginx:alpine
    environment:
      VIRTUAL_HOST_MULTIPORTS: |-
        merged.nginx-proxy.tld:
          "/foo":
            port: 82
# merged.nginx-proxy.tld/foo
upstream merged.nginx-proxy.tld-6dbd548cc03e44b8b44b6e68e56255ce4273ae49 {
    # Container: nginx-proxy-merged-multiports-1
    # ...
    # Container: nginx-proxy-merged-singleport-1-1
    # ...
    # Container: nginx-proxy-merged-singleport-2-1
    # ...
    server 172.18.0.4:82;
    server 172.18.0.2:80;
    server 172.18.0.3:81;
}
server {
    server_name merged.nginx-proxy.tld;
    # ...
    location /foo {
        proxy_pass http://merged.nginx-proxy.tld-6dbd548cc03e44b8b44b6e68e56255ce4273ae49;
        set $upstream_keepalive false;
    }
    location / {
        return 404;
    }
}
This work too (root path on multiport container only):
services:
  merged-singleport-2: # 172.18.0.3
    image: nginx:alpine
    environment:
      VIRTUAL_HOST: merged.nginx-proxy.tld
      VIRTUAL_PATH: "/foo"
      VIRTUAL_PORT: "81"

  merged-multiports: # 172.18.0.4
    image: nginx:alpine
    environment:
      VIRTUAL_HOST_MULTIPORTS: |-
        merged.nginx-proxy.tld:
          "/":
            port: 82
          "/foo":
            port: 83
# merged.nginx-proxy.tld/
upstream merged.nginx-proxy.tld {
    # Container: nginx-proxy-merged-multiports-1
    # ...
    server 172.18.0.4:82;
}
# merged.nginx-proxy.tld/foo
upstream merged.nginx-proxy.tld-6dbd548cc03e44b8b44b6e68e56255ce4273ae49 {
    # Container: nginx-proxy-merged-multiports-1
    # ...
    # Container: nginx-proxy-merged-singleport-2-1
    # ...
    server 172.18.0.4:83;
    server 172.18.0.3:81;
}
server {
    server_name merged.nginx-proxy.tld;
    # ...
    location / {
        proxy_pass http://merged.nginx-proxy.tld;
        set $upstream_keepalive false;
    }
    location /foo {
        proxy_pass http://merged.nginx-proxy.tld-6dbd548cc03e44b8b44b6e68e56255ce4273ae49;
        set $upstream_keepalive false;
    }
}
This doesn't (the first container's config is seemingly discarded):
services:
  merged-singleport-1: # 172.18.0.2
    image: nginx:alpine
    environment:
      VIRTUAL_HOST: merged.nginx-proxy.tld

  merged-singleport-2: # 172.18.0.3
    image: nginx:alpine
    environment:
      VIRTUAL_HOST: merged.nginx-proxy.tld
      VIRTUAL_PATH: "/foo"
      VIRTUAL_PORT: "81"

  merged-multiports: # 172.18.0.4
    image: nginx:alpine
    environment:
      VIRTUAL_HOST_MULTIPORTS: |-
        merged.nginx-proxy.tld:
          "/":
            port: 82
          "/foo":
            port: 84
# merged.nginx-proxy.tld/
upstream merged.nginx-proxy.tld {
    # Container: nginx-proxy-merged-multiports-1
    # ...
    # /!\ nginx-proxy-singleport-1-1 should be here /!\
    server 172.18.0.4:82;
}
# merged.nginx-proxy.tld/foo
upstream merged.nginx-proxy.tld-6dbd548cc03e44b8b44b6e68e56255ce4273ae49 {
    # Container: nginx-proxy-merged-multiports-1
    # ...
    # Container: nginx-proxy-merged-singleport-2-1
    # ...
    server 172.18.0.4:83;
    server 172.18.0.3:81;
}
server {
    server_name merged.nginx-proxy.tld;
    # ...
    location / {
        proxy_pass http://merged.nginx-proxy.tld;
        set $upstream_keepalive false;
    }
    location /foo {
        proxy_pass http://merged.nginx-proxy.tld-6dbd548cc03e44b8b44b6e68e56255ce4273ae49;
        set $upstream_keepalive false;
    }
}

@pini-gh
Copy link
Contributor

pini-gh commented May 5, 2024

This doesn't (the first container's config is seemingly discarded):

It is the case on the branch main as well.

Because this block:

    {{- $tmp_paths := groupBy $containers "Env.VIRTUAL_PATH" }}
    {{- $has_virtual_paths := gt (len $tmp_paths) 0}}
    {{- if not $has_virtual_paths }}
        {{- $tmp_paths = dict "/" $containers }}
    {{- end }}

silently discards containers with no VIRTUAL_PATH variable when at least one exists with.

@pini-gh
Copy link
Contributor

pini-gh commented May 5, 2024

this block:

    {{- $tmp_paths := groupBy $containers "Env.VIRTUAL_PATH" }}
    {{- $has_virtual_paths := gt (len $tmp_paths) 0}}
    {{- if not $has_virtual_paths }}
        {{- $tmp_paths = dict "/" $containers }}
    {{- end }}

silently discards containers with no VIRTUAL_PATH variable when at least one exists with.

Could be replaced with:

    {{- $tmp_paths := dict }}
    {{- range $path, $path_containers := groupBy $containers "Env.VIRTUAL_PATH" }}
        {{ $_ := set $tmp_paths $path $path_containers }}
    {{- end }}
    {{- $has_virtual_paths := gt (len $tmp_paths) 0}}
    {{- $_ := unset $tmp_paths "/" }}
    {{- $slash_containers := $containers }}
    {{- range $path, $path_containers := $tmp_paths }}
        {{ range $container := $path_containers }}
            {{ $slash_containers = without $slash_containers $container }}
        {{- end }}
    {{- end }}
    {{- if (gt (len $slash_containers) 0) }}
        {{- $_ := set $tmp_paths "/" $slash_containers }}
    {{- end }}

I couldn't find a simpler way because docker-gen map type is not compatible with sprig dict.

@buchdag
Copy link
Member Author

buchdag commented May 5, 2024

I've tried that earlier an ran into the same dockergen map / sprig dict incompatibility.

I think a cleaner approach might be to add a groupByWithDefault function to docker-gen that would work like this :

{{- $tmp_paths := groupByWithDefault $containers "Env.VIRTUAL_PATH" "/" }}

I have it ready on a local branch and tested it with a modified template, seems to work like a charm.

While I'm at it, maybe groupByMultiWithDefault / groupByLabelWithDefault would be useful to clean the template too ?

@pini-gh
Copy link
Contributor

pini-gh commented May 5, 2024

While I'm at it, maybe groupByMultiWithDefault / groupByLabelWithDefault would be useful to clean the template too ?

Sure.

pini-gh and others added 11 commits May 5, 2024 16:15
(See #1504)

Using variable VIRTUAL_HOST_MULTIPORTS as a dictionnary:

key: hostname
value: dictionnary:
  key: path
  value: struct
    port
    dest

When the dictionnary associated with a hostname is empty, default values
apply:
  path = "/"
  port = default port
  dest = ""

For each path entry, port and dest are optionnal and are assigned default
values when missing.

Example:
      VIRTUAL_HOST_MULTIPORTS: |
        host1.example.org:
          "/":
            port: 8000
          "/somewhere":
            port: 9000
            dest: "/elsewhere"
        host2.example.org:
        host3.example.org:
          "/inner/path":
For containers grouped by identical VIRTUAL_HOST,
those with no VIRTUAL_PATH variable were silently discarded
when at least one container with VIRTUAL_PATH existed.
@buchdag

This comment was marked as resolved.

@buchdag buchdag marked this pull request as ready for review May 5, 2024 19:15
@buchdag
Copy link
Member Author

buchdag commented May 5, 2024

I think we're pretty much done in term of documentation and tests here, marking the PR as ready for review.

@buchdag buchdag requested a review from pini-gh May 5, 2024 19:16
docs/README.md Outdated Show resolved Hide resolved
@pini-gh
Copy link
Contributor

pini-gh commented May 6, 2024

LGTM. Please go ahead 👍

docs/README.md Outdated Show resolved Hide resolved
Copy link
Contributor

@SchoNie SchoNie left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very nice work @pini-gh & @buchdag 👍

docs/README.md Outdated Show resolved Hide resolved
Co-authored-by: Niek <100143256+SchoNie@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment