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

Marathon integration changed default backend server port from task-level to application-level #1072

Closed
timoreimann opened this issue Jan 26, 2017 · 7 comments · Fixed by #1090

Comments

@timoreimann
Copy link
Contributor

timoreimann commented Jan 26, 2017

What version of Traefik are you using (traefik version)?

latest master as of now (commit 18cf497).

What is your environment & configuration (arguments, toml...)?

Running native Mac OS X build with Marathon integration as in:

./traefik --web --web.readonly --marathon --marathon.endpoint=http://my.marathon.endpoint:8080 --marathon.domain="my.domain" --checknewversion=false --debug

What did you do?

Start Traefik as documented above with a working Marathon cluster hosting a few applications.

What did you expect to see?

Backend URLs using the Marathon task ports (normally in the 32000+ range).

What did you see instead?

Backend URLs using Marathon service ports (normally in the 10000+ range).

The change in behavior was introduced by PR #841 (IP-per-task feature), specifically this part of the newly added processPorts() function which started to prefer application-level ports (from the top-level ports JSON array) over task-level ones (from the tasks.ports JSON array). For an explanation of the difference between the two port types, please see the Marathon documentation.

We don't use the IP-per-task feature, so I'm not sure if service ports came into the PR as a true requirement or rather a convenience to have. What I know is that we only rely on task ports (i.e., host ports) which is also the default you get in the latest stable release of Traefik (1.12). Mostly for historical reasons, configuration of ports in Marathon is a fairly complicated matter as there are multiple ways to define them. (See the documentation I linked to in the previous section.) We use the portMappings field on the docker specification, which is the recommended way when running containers in bridged mode according to the docs.

I also tried to tweak our application definition in order to prevent Marathon from exposing a service port at all, namely by setting different values to the port field (null, empty array, concrete value). Unfortunately, I did not succeed -- from what I can tell, Marathon always wants to set one if portMappings is given. I'm open for suggestions in case I overlooked something. However, even if we find something, it would still require existing Traefik/Marathon users to potentially make the same change in case the feature makes it into the next release of Traefik as-is.

Questions that I think we should answer in order to come to a good solution:

  1. Was the change to prefer application-level ports deliberate?
  2. If so: How can we manage to support both types of ports in Traefik?

Given 2., my suggestion would be to expose two new functions getServicePort and getHostPort that Traefik users can leverage in their custom Marathon template according to their needs. I also think we should revert Traefik's behavior (with regards to which ports it selects) to the state prior to the PR as I think it constitutes as a breaking API change (presuming here that the next release won't be a major one). At least, it broke our setup without notice.

Looking forward to your feedback!

@emilevauge @diegooliveira @errm

@emilevauge
Copy link
Member

@timoreimann Thanks a lot for your detailed description :)

I also think we should revert Traefik's behavior with regards to which ports it selects) to the state prior to the PR as I think it constitutes as a breaking API change (presuming here that the next release won't be a major one). At least, it broke our setup without notice.

Reverting a PR is possible but I would prefer to find a consensus.
Could you propose a solution that would also work with IP-per-task ?

@diegooliveira any idea ?

@timoreimann
Copy link
Contributor Author

Sorry, I should have been more clear: I didn't want to advocate reverting the entire PR, that would indeed be overkill. What I meant was restoring the behavior regarding the default server port only, that is, return to using the host port but provide a means to use the service port when desired. My idea was to provide two template functions with the one fetching the host port to be used in the default Marathon template.

Maybe there are smarter approaches that automatically pick the right port. For instance, I believe Marathon populates some fields only when the IP-per-task is enabled, so maybe we could pick the appropriate ports if those fields are there and otherwise go with the host ports? Ideally this would require no explicit user action but to set the desired mode in Marathon.

I really know too little about the IP-per-task usage to make a sophisticated recommendation, however. @diegooliveira can hopefully provide some insights as to what's best from the perspective of an IP-per-task user.

Looking forward to hear your suggestions.

@diegooliveira
Copy link
Contributor

diegooliveira commented Jan 26, 2017

The PR are a little old, but as far as I remember the pointed lines was added after a rebase, don't remember exactly for what purpose. I will take a look as soon as I arrive home. If it is a bug I'll send a PR to fix it and some test case to cover any regression.

@timoreimann
Copy link
Contributor Author

Hi @diegooliveira, did you have a chance to dig deeper yet?

@diegooliveira
Copy link
Contributor

@timoreimann Yes, had time to try the configuration with and without IP-Per-Task. When using a IP-Per-Task configuration marathon never configure an application port but does so otherwise. Per see it's a bug in the PR. The application port is a configuration to avoid port conflict in the proxy server but the proxy should connect in the task port. The fix will be just to remove the application port validation altogether.

@emilevauge I'll add some tests and submite a PR to fix the problem.

@diegooliveira
Copy link
Contributor

@timoreimann I just did a PR #1090 to fix the problem. Tested in a env with and without calico, the documentation about ports is a little confusing. The change was exactly what you pointed out, thanks.

@timoreimann
Copy link
Contributor Author

Appreciated, @diegooliveira! Left an approving comment on the PR.

@traefik traefik locked and limited conversation to collaborators Sep 1, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants