-
-
Notifications
You must be signed in to change notification settings - Fork 5.2k
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
Comments
@timoreimann Thanks a lot for your detailed description :)
Reverting a PR is possible but I would prefer to find a consensus. @diegooliveira any idea ? |
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. |
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. |
Hi @diegooliveira, did you have a chance to dig deeper yet? |
@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. |
@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. |
Appreciated, @diegooliveira! Left an approving comment on the PR. |
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:
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-levelports
JSON array) over task-level ones (from thetasks.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 ifportMappings
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:
Given 2., my suggestion would be to expose two new functions
getServicePort
andgetHostPort
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
The text was updated successfully, but these errors were encountered: