Uses both binded HostIP and HostPort when useBindPortIP=true #3638

geraldcroes · 2018-07-18T11:22:42Z

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
          }
        },
      }
    },
  }
}

ldez · 2018-07-18T11:52:35Z

provider/docker/config.go

@@ -322,22 +311,49 @@ func getPort(container dockerData) string {
 	return ""
 }

+func (p *Provider) getPortBinding(container dockerData) (nat.PortBinding, error) {


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

ldez · 2018-07-18T11:53:54Z

provider/docker/config.go

+		}
+	}
+
+	return nat.PortBinding{HostIP: "", HostPort: ""}, fmt.Errorf("Unable to find the external IP:Port for the container %q", container.Name)


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)

ldez · 2018-07-18T11:55:12Z

provider/docker/config.go

+			portBinding, err := p.getPortBinding(container)
+			if err != nil {
+				log.Warnf("Unable to find a binding for the container %q: ignoring server", container.Name)
+				continue


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

ldez · 2018-07-18T11:56:49Z

provider/docker/config.go

+				log.Warnf("Unable to find a binding for the container %q: ignoring server", container.Name)
+				continue
+			}
+			if portBinding.HostIP != "0.0.0.0" {


fail fast: could you invert the condition:

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

The else is not needed.

ldez · 2018-07-18T11:57:25Z

provider/docker/config.go

@@ -347,6 +363,9 @@ func (p *Provider) getServers(containers []dockerData) map[string]types.Server {
 			continue
 		}

+		if servers == nil {
+			servers = make(map[string]types.Server)


the map initialization must be done before.

mmatur · 2018-07-19T08:58:11Z

provider/docker/deprecated_config.go

-		"getPort":      getPortV1,
-		"getWeight":    getFuncIntLabelV1(label.TraefikWeight, label.DefaultWeight),
-		"getProtocol":  getFuncStringLabelV1(label.TraefikProtocol, label.DefaultProtocol),
+		"getDeprecatedIPAddress": p.getDeprecatedIPAddress,


Should be "getIPAddress" instead of "getDeprecatedIPAddress"

mmatur · 2018-07-19T08:58:18Z

provider/docker/config.go

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


Should be "getIPAddress" instead of "getDeprecatedIPAddress"

ldez

LGTM

mmatur

LGTM

nmengin

LGTM 🐳 👏

geraldcroes requested a review from a team as a code owner July 18, 2018 11:22

traefiker added area/provider/docker size/S status/0-needs-triage labels Jul 18, 2018

mmatur added kind/bug/fix a bug fix status/2-needs-review and removed status/0-needs-triage labels Jul 18, 2018

mmatur changed the base branch from master to v1.7 July 18, 2018 11:25

mmatur added this to the 1.7 milestone Jul 18, 2018

ldez suggested changes Jul 18, 2018

View reviewed changes

traefiker added the contributor/waiting-for-corrections label Jul 18, 2018

geraldcroes force-pushed the issue-3622 branch from e8ab53b to 21fabc8 Compare July 18, 2018 12:58

geraldcroes added 8 commits July 18, 2018 14:59

Uses the binded IP AND port when UseBindedPortIP=true

0580ba3

Replaced the deprecated getPortV1 by getPort

92a8c34

Reformated

6423fbc

updated the error message

bdc8e1c

Ignores the server when HostIP is 0.0.0.0

b78cd0e

code review

f8d38a9

code review - method extract

a3c5601

code review - missunderstanding about the return nil

f7953ea

geraldcroes force-pushed the issue-3622 branch from 21fabc8 to f7953ea Compare July 18, 2018 12:59

geraldcroes added 8 commits July 18, 2018 15:05

code review - capitalization

37d8ce3

code review - keeps the current behaviour in the templates

d9f4f58

code review - indentation

4d527b9

code review - deprecated comment

30e85b5

code review - comments

19f4485

code review - comments

182c929

code review - comments

86bbded

Correctly renamed getIPAddress

0cbfa97

mmatur requested changes Jul 19, 2018

View reviewed changes

geraldcroes and others added 5 commits July 19, 2018 11:43

Added unit tests

ed9db71

Fixed test name

a56c4ae

Fixed map entry in the buildConfigurationV2

598234b

Kept a getIPAddressV1 for buildConfigurationV1 full compatibility

238bfda

review: small changes.

1f036f0

ldez removed the contributor/waiting-for-corrections label Jul 19, 2018

ldez approved these changes Jul 19, 2018

View reviewed changes

mmatur approved these changes Jul 19, 2018

View reviewed changes

nmengin approved these changes Jul 19, 2018

View reviewed changes

nmengin added status/3-needs-merge and removed status/2-needs-review labels Jul 19, 2018

traefiker merged commit d50b6a3 into traefik:v1.7 Jul 19, 2018

traefiker removed the status/3-needs-merge label Jul 19, 2018

traefiker mentioned this pull request Jul 19, 2018

docker.UseBindPortIP ignored #3622

Closed

ldez deleted the issue-3622 branch August 2, 2018 08:41

ldez mentioned this pull request Sep 27, 2018

Error on Docker back-ends with 1.7 not present in 1.6.6 #3943

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uses both binded HostIP and HostPort when useBindPortIP=true #3638

Uses both binded HostIP and HostPort when useBindPortIP=true #3638

geraldcroes commented Jul 18, 2018 •

edited

Loading

ldez Jul 18, 2018

ldez Jul 18, 2018

ldez Jul 18, 2018

ldez Jul 18, 2018

ldez Jul 18, 2018

mmatur Jul 19, 2018

mmatur Jul 19, 2018

ldez left a comment

mmatur left a comment

nmengin left a comment

Uses both binded HostIP and HostPort when useBindPortIP=true #3638

Uses both binded HostIP and HostPort when useBindPortIP=true #3638

Conversation

geraldcroes commented Jul 18, 2018 • edited Loading

What does this PR do?

Motivation

More

Examples

ldez Jul 18, 2018

Choose a reason for hiding this comment

ldez Jul 18, 2018

Choose a reason for hiding this comment

ldez Jul 18, 2018

Choose a reason for hiding this comment

ldez Jul 18, 2018

Choose a reason for hiding this comment

ldez Jul 18, 2018

Choose a reason for hiding this comment

mmatur Jul 19, 2018

Choose a reason for hiding this comment

mmatur Jul 19, 2018

Choose a reason for hiding this comment

ldez left a comment

Choose a reason for hiding this comment

mmatur left a comment

Choose a reason for hiding this comment

nmengin left a comment

Choose a reason for hiding this comment

geraldcroes commented Jul 18, 2018 •

edited

Loading