Skip to content

Conversation

@geraldcroes
Copy link
Contributor

@geraldcroes geraldcroes commented Jul 18, 2018

What does this PR do?

Context: docker provider with usebindportip

Currently:

  • Traefik doesn't find the port bindings when the API responds in lowercase.
  • When the API responds in uppercase, Traefik uses the external IP only, and not the port
  • When no IP is specified, Traefik creates a route using the IP 0.0.0.0
  • When the label traefik.portis specified, it is used both for selecting the binding and in the route

This PR:

  • ignores the case when looking for port bindings
  • takes the external IP and port into account to create the routes
  • excludes servers with no binding
  • excludes servers with no IP specified in the binding (0.0.0.0)
  • the label traefik.port is used to select the binding (which enables the use of segments)

To sum it up:

label Binding Routes to
traefik.port not set IP_E1:P_E1:P_I1 IP_E1:P_E1
traefik.port not set P_E1:P_I1 Warning & Ignored (cannot route to 0.0.0.0)
traefik.port not set None Warning & Ignored (Binding is missing)
traefik.port=P_I1 IP_E1:P_E1:P_I1 IP_E1:P_E1
traefik.port=P_I2 IP_E1:P_E1:P_I1 Warning & Ignored (cannot find binding for XX:XX:P_I2)
traefik.port=P_I2 IP_E1:P_E1:P_I1 & IP_E2:P_E2:P_I2 IP_E2:P_E2
traefik.port=P_I1 None Warning & Ignored (Binding is missing)

Motivation

Fixes #3622

(kudos to @systemmonkey42 for the awesome description in the issue)

More

  • Added/updated tests
  • Added/updated documentation

Examples

docker-compose.yml
version: '3'

services:
  whoami-1:
    image: emilevauge/whoami
    ports:
      -  xx.xx.xx.xx:8082:80 # Routes to XX.XX.XX.XX:8082
    labels: 
      - traefik.enable=true
      - traefik.backend=whoami
      - "traefik.frontend.rule=PathPrefix:/whoami-1"

  whoami-2:
    image: emilevauge/whoami
    ports:
      -  8083:80 # Warning Cannot determine the IP address (got 0.0.0.0) for the container "/issue_3622_whoami-2_1": ignoring server 
    labels: 
      - traefik.enable=true
      - traefik.backend=whoami-2
      - "traefik.frontend.rule=PathPrefix:/whoami-2"

  whoami-3:
    image: emilevauge/whoami
    # Warning Unable to find a binding for the container "/issue_3622_whoami-3_1": ignoring server
    labels: 
      - traefik.enable=true
      - traefik.backend=whoami-3
      - "traefik.frontend.rule=PathPrefix:/whoami-3"
  
  whoami-4:
    image: emilevauge/whoami
    ports:
      -  xx.xx.xx.xx:8085:80 # routes to XX:XX:XX:XX:8085
    labels:
      - traefik.port=80
      - traefik.enable=true
      - traefik.backend=whoami-4
      - "traefik.frontend.rule=PathPrefix:/whoami-4"

  whoami-5:
    image: emilevauge/whoami
    ports:
      -  xx.xx.xx.xx:8086:80 # Warning Unable to find a binding for the container "/issue_3622_whoami-5_1": ignoring server
    labels:
      - traefik.port=81
      - traefik.enable=true
      - traefik.backend=whoami-5
      - "traefik.frontend.rule=PathPrefix:/whoami-5"

  whoami-6:
    image: emilevauge/whoami
    ports:
      -  xx.xx.xx.xx:8087:80
      -  xx.xx.xx.xx:8088:81 # routes to xx.xx.xx.xx:8088
    labels:
      - traefik.port=81
      - traefik.enable=true
      - traefik.backend=whoami-6
      - "traefik.frontend.rule=PathPrefix:/whoami-6"

  whoami-7:
    image: emilevauge/whoami
    # Warning Unable to find a binding for the container "/issue_3622_whoami-7_1": ignoring server
    labels:
      - traefik.port=81
      - traefik.enable=true
      - traefik.backend=whoami-7
      - "traefik.frontend.rule=PathPrefix:/whoami-7"
traefik/api
{
  "docker": {
    "backends": {
      "backend-whoami": {
        "servers": {
          "server-issue-3622-whoami-1-1-7c60b6958b968de9998659c11d7406a0": {
            "url": "http://xx.xx.xx.xx:8082",
            "weight": 1
          }
        },
      },
      "backend-whoami-4": {
        "servers": {
          "server-issue-3622-whoami-4-1-5dcf432359cd6ad04e1779a85d45314a": {
            "url": "http://xx.xx.xx.xx:8085",
            "weight": 1
          }
        },
      },
      "backend-whoami-6": {
        "servers": {
          "server-issue-3622-whoami-6-1-554f0078ab3c259e14fc03848f9f8ce4": {
            "url": "http://xx.xx.xx.xx:8088",
            "weight": 1
          }
        },
      }
    },
  }
}

@geraldcroes geraldcroes requested a review from a team as a code owner July 18, 2018 11:22
@mmatur mmatur changed the base branch from master to v1.7 July 18, 2018 11:25
@mmatur mmatur added this to the 1.7 milestone Jul 18, 2018
Copy link
Contributor

Choose a reason for hiding this comment

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

could you return a pointer (*nat.PortBinding) instead of a struct?

Copy link
Contributor

Choose a reason for hiding this comment

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

return nil.

https://github.com/golang/go/wiki/CodeReviewComments#error-strings

fmt.Errorf("unable to find the external IP:Port for the container %q", container.Name)

Copy link
Contributor

Choose a reason for hiding this comment

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

could you create a function that returns an error to replace the continue

Copy link
Contributor

Choose a reason for hiding this comment

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

fail fast: could you invert the condition:

if portBinding.HostIP == "0.0.0.0" {
// ...
}
// ...

The else is not needed.

Copy link
Contributor

Choose a reason for hiding this comment

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

the map initialization must be done before.

"getPort": getPortV1,
"getWeight": getFuncIntLabelV1(label.TraefikWeight, label.DefaultWeight),
"getProtocol": getFuncStringLabelV1(label.TraefikProtocol, label.DefaultProtocol),
"getDeprecatedIPAddress": p.getDeprecatedIPAddress,
Copy link
Member

Choose a reason for hiding this comment

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

Should be "getIPAddress" instead of "getDeprecatedIPAddress"

"getBuffering": label.GetBuffering,
"getCircuitBreaker": label.GetCircuitBreaker,
"getLoadBalancer": label.GetLoadBalancer,
"getDeprecatedIPAddress": p.getDeprecatedIPAddress, // TODO: Should we expose getIPPort instead?
Copy link
Member

Choose a reason for hiding this comment

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

Should be "getIPAddress" instead of "getDeprecatedIPAddress"

Copy link
Contributor

@ldez ldez left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Member

@mmatur mmatur left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@nmengin nmengin left a comment

Choose a reason for hiding this comment

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

LGTM 🐳 👏

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.

5 participants