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

Allow binding ECS container port #3533

Merged
merged 4 commits into from
Jul 4, 2018
Merged

Allow binding ECS container port #3533

merged 4 commits into from
Jul 4, 2018

Conversation

andrewstucki
Copy link
Contributor

@andrewstucki andrewstucki commented Jun 26, 2018

What does this PR do?

Implements #2854. Makes Traefik look for an ECS label called containerPort to allow for dynamic mapping.

Motivation

See #2854 for a longer description, but basically there's no current way to specify routing behavior for ECS containers with more than one dynamically exposed port. This means without this feature:

  1. In order to make Traefik route correctly you must either:
    a. expose only a single port
    b. statically bind the container port to a well-known host port
  2. 1b. breaks the ability to schedule multiple containers of the same type on a single EC2 host.

More

  • Added/updated tests
  • Added/updated documentation

Additional Notes

In the final solution, we use only the traefik.port label

label Docker Host Container Value used
traefik.port not set 8080 8080 8080
traefik.port not set DYNAMIC_PORT 8080 DYNAMIC_PORT
traefik.port=8080 8080 8080 8080
traefik.port=8080 DYNAMIC_PORT 8080 DYNAMIC_PORT
traefik.port=80 DYNAMIC_PORT 8080 80

@andrewstucki
Copy link
Contributor Author

Also, for clarity, as per #2896 the only way to do mapping prior to this is something along these lines in a task definition:

	"portMappings": [
            {
		"containerPort": 80
            },
            {
		"hostPort": 8080,
		"protocol": "tcp",
		"containerPort": 8080
            }
	],
	"dockerLabels" :           {
            "traefik.enable" : "true",
            "traefik.frontend.rule" : "Host:XXXX",
            "traefik.port" : "8080"
        }

The static port binding clearly means you can only have one of these tasks running on a given host. The PR allows for something like:

	"portMappings": [
            {
		"containerPort": 80
            },
            {
		"containerPort": 8080
            }
	],
	"dockerLabels" :           {
            "traefik.enable" : "true",
            "traefik.frontend.rule" : "Host:XXXX",
            "traefik.containerPort" : "8080"
        }

Which allows you to do bin-packing and removes the necessity for static port binding.

@ldez ldez added the kind/enhancement a new or improved feature. label Jun 26, 2018
@mmatur mmatur self-assigned this Jun 27, 2018
@mmatur
Copy link
Member

mmatur commented Jun 29, 2018

Hi @andrewstucki,

Thanks for your interest in Træfik.

Why do you need a new label containerPort?
I think that you can reuse traefik.port label and adapt getPort function

WDYT ?

@andrewstucki
Copy link
Contributor Author

@mmatur well, the current traefik.port label (which the ECS provider supports) just sets up the routes so that Traefik assumes something is listening on that port on the host (it assumes you know what port the container is going to bind to, which in the case of dynamic port binding, you don't).

I wouldn't mind changing the functionality of the label so that it resolves dynamic port binding instead and treats the port specified as the container's port, but figured that making a breaking change to existing behavior was probably something we didn't want to do, hence the implementation of a new label. Off the top of my head I can't think of a way of implementing the new behavior while keeping the old behavior without a new label.

Let me know if you have any ideas.

@andrewstucki
Copy link
Contributor Author

So, hopefully to illustrate the point better, here's how things currently work.

Assuming static port binding a setup can look like this:

label Docker Host Container
traefik.port 8081 8080
traefik.containerPort 8081 8080

Where traefik.port is a reference to the host-level port. The new label references the container-level port. With static binding either would work.

However, when you have dynamic binding it looks like more this:

label Docker Host Container
traefik.port ? 8080
traefik.containerPort ? 8080

In the dynamic binding scenario we have no way of using the behavior traefik.port implements because we don't know which host-level port will get bound to. The only way you can choose the right port is to reference the port on the container side of things, in other words, the new traefik.containerPort label.

So, in most scenarios this probably won't be a huge deal since many containers only expose a single port. As such, Traefik will just choose the first port that it sees and route to it. However, In the case where you have multiple dynamically bound ports, then this becomes an issue because we currently have no way of specifying which of the exposed ports is the one we care about.

My particular use case is co-locating a bunch of services that expose two ports dynamically, a port that's internally scraped by Prometheus, and a port that a web-server serving up and API listens on. Since I want to co-locate all of the services, I can't dynamically bind either the Prometheus or the web-server ports, but need a way to tell Traefik to only route to the port that the web-server is bound on, meaning I need to tell Traefik to only route to the port exposed as port 8080 in my container and to ignore whatever translates to port 9000. What that maps to at the host level, I have no way of knowing due to the dynamic nature of the port binding.

@mmatur
Copy link
Member

mmatur commented Jun 30, 2018

@andrewstucki Thanks for you feedback.

I was thinking about a solution like that

func getPort(i ecsInstance) string {
	if value := label.GetStringValue(i.TraefikLabels, label.TraefikPort, ""); len(value) > 0 {
		port, err := strconv.ParseInt(value, 10, 64)
		for _, mapping := range i.machine.ports {
			if err == nil {
				if port == mapping.hostPort || port == mapping.containerPort {
					return strconv.FormatInt(mapping.hostPort, 10)
				}
			}
		}
		return value
	}
	return strconv.FormatInt(i.machine.ports[0].hostPort, 10)
}

If traefik.port match the hostPort we return the hostPort value
If traefik.port match the containerPort we return the hostPort value
If traefik.port does not match the hostPort or the containerPort we return the traefik.port value
if traefik.port match hostPort and containerPort we return hostPort value

With an example

label Docker Host Container Value used
traefik.port not set 8080 8080 8080
traefik.port not set DYNAMIC_PORT 8080 DYNAMIC_PORT
traefik.port=8080 8080 8080 8080
traefik.port=8080 DYNAMIC_PORT 8080 DYNAMIC_PORT
traefik.port=80 DYNAMIC_PORT 8080 80

WDYT ?

@mmatur
Copy link
Member

mmatur commented Jul 4, 2018

Hi @andrewstucki,

The code freeze for the next 1.7 release will happen at the end of this week, on Friday 6th July 2018.
So I will do the change on your PR because we want to be able to merge your PR into the next release

@mmatur mmatur added this to the 1.7 milestone Jul 4, 2018
@mmatur mmatur changed the title Add containerPort label for ECS Allow binding ECS container port Jul 4, 2018
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
Member

@juliens juliens left a comment

Choose a reason for hiding this comment

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

LGTM

@shanthakumarf22

This comment has been minimized.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants