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

Environment variables for published ports changed when using --link #9900

Closed
jyrkiput opened this issue Jan 5, 2015 · 18 comments · Fixed by #10061
Closed

Environment variables for published ports changed when using --link #9900

jyrkiput opened this issue Jan 5, 2015 · 18 comments · Fixed by #10061
Milestone

Comments

@jyrkiput
Copy link
Contributor

jyrkiput commented Jan 5, 2015

I have multiple ports published from command line, ie.

docker run --name backend -p 7101:7101 -p 7102:7102 -p 7103:7103 ....
docker run --name frontedn --link backend:backend ...

There used to be environment variable available for each port, ie.

BACKEND_PORT_7101_TCP_PORT=7101

But now there's only

BACKEND_PORT_7101_TCP_PORT_END=7103
BACKEND_PORT_7101_TCP_PORT_START=7101

I think that fd774a8 changed the behaviour, and it requires at least three ports to be defined

This can be seen with

docker run -d --name backend -p 7101:7101 -p 7102:7102 -p 7103:7103 busybox /bin/sh -c "while true; do sleep 5; done"

and then

docker run -t -i --rm --name frontend --link backend:backend busybox /bin/sh -c set
@thaJeztah
Copy link
Member

Sounds like it sees the ports as a "range" because they are consecutive ports yes. Nice catch!
Think @brahmaroutu or @jfrazelle should have a look?

@brahmaroutu
Copy link
Contributor

@jyrkiput @thaJeztah You are right, the behavior is changed as mentioned above, when creating port ranges, the ports assigned are contiguous and creating the env variable to publish the start and end of the range is better than having many env variables created (as the range can be very large).

@SvenDowideit
Copy link
Contributor

mmm, this was changed in #8167 - and wasn't documented. I'm also not 100% sure that it was discussed enough, as its a UX change :/

@brahmaroutu
Copy link
Contributor

@SvenDowideit I agree with you, this is a debatable issue. Given the code, we can salvage couple ways. 1. to carry information from input params and only create appropriate env variables based on whether range is specified in the input or not. 2. create env variable for each port if the port range is less that 10 numbers. If not create a env variables to specify range (looks a bit hacky).

@thaJeztah
Copy link
Member

I'd opt for option 1;

If a user specifies single ports, multiple times (-p 1 -p 2 -p3), then always keep the "old" behavior (even if they form a range).

If a user explicitly specifies a range (--expose 1-3), then give it the new behavior.

I do see various edge cases, and this might be a hard one to solve; For example, --expose 1-3 -p 2 -p 3 -p 4 what takes precedence here? expose or publish?

If not create a env variables to specify range

Perhaps, create both? One for range and one for individual ports. I do find the naming of the "range" env a bit odd (BACKEND_PORT_7101_TCP_PORT_END); will it always contain the first port in the range?

@brahmaroutu
Copy link
Contributor

thanks @thaJeztah Option 1 is a bit harder as I need to keep that information about the incoming request in some place. Another way to address/thought is by creating env for all ports individually and not have this range concept. In theory no one probably expose 65K ports?

@thaJeztah
Copy link
Member

In theory no one probably expose 65K ports?

Probably not, but still.

Wondering; are the new style PORT_XXX_START / PORT_XXX_END env variables actually documented anywhere? IIRC, plans are to phase out the environment variables in future. If they were not documented so far, I suggest;

  • Only create env-variables if individual ports were exposed by the user
  • Don't create env-variables for port-ranges.

This will preserve the old behavior and not introduce new env-variables. Since port-ranges are a new feature, I think it's reasonable to say that env-variables are not part of that new feature.

But.. I am not a maintainer :)

(Of course, this still requires to implement logic to differentiate between "ranges" and "individual" ports.)

@SvenDowideit
Copy link
Contributor

at minimum, that PR needed to have the documentation updated - right now, users don't know that they can't use the original env vars. IMO, the range one is much harder to script for, but the old ones were pretty painful already.

mmm, for example, http://docs.docker.com/articles/ambassador_pattern_linking/#the-svendowideitambassador-dockerfile won't work with the new ENV.

@crosbymichael can you advise - are we stuck with the new ENV vars, or do we have wiggle room?

if we're stuck with it, I'll work with @brahmaroutu to get the docs updated.

@crosbymichael
Copy link
Contributor

looking

@crosbymichael crosbymichael modified the milestone: 1.5.0 Jan 7, 2015
@crosbymichael
Copy link
Contributor

This needs to be changed back to support the old env vars in addition to these range values.

Sorry but this is a breaking change and a regression.

@crosbymichael crosbymichael added this to the 1.5.0 milestone Jan 7, 2015
@brahmaroutu
Copy link
Contributor

I agree, will work on it. Only issue now is that code has to know that the incoming request has range format or not to distinguish (I may have to add addl. info into links.go struct)

@crosbymichael
Copy link
Contributor

@brahmaroutu no, I don't htink that is needed, we will just have to add both types of env vars

@brahmaroutu
Copy link
Contributor

That will be a simple fix.
Issue is that if I see 100 consecutive ports then I will have 100 * 4 = 400 of the following env variables apart from 4 range env variables.
BACKEND_PORT_7101_TCP='tcp://172.17.0.5:7101'
BACKEND_PORT_7101_TCP_ADDR='172.17.0.5'
BACKEND_PORT_7101_TCP_PORT='7101'
BACKEND_PORT_7101_TCP_PROTO='tcp'

@thaJeztah
Copy link
Member

@crosbymichael do we need the new environment variables? Also see my comment; #9900 (comment)

@brahmaroutu
Copy link
Contributor

I have made changes to create both env variables for each of the ports as well for ranges when ranges are detected. I will issue PR once we determine about env variables for ranges(which is superfluous now).

@SvenDowideit
Copy link
Contributor

@brahmaroutu if you don't remove them, can you please document them, including advice on how to consume them from a bash script - ala the ambassador example?

@brahmaroutu
Copy link
Contributor

@SvenDowideit I cannot think of a good use case to show case in a example.
But like mentioned in #1834, the need is to expose range as in one example, Hadoop MR(YARN cluster) where in the config file will specify port range as values 5000-5100.
yarn.app.mapreduce.am.job.client.port-range
50100-50200
But that is XML and not CLI to substitute env variables. Couchbase needs hardcoded port range and do not need env variables.
I am not an expert I need a concrete example to document and looks like this is not captured in #1834 @jhorey may be able to chip in?

@brahmaroutu
Copy link
Contributor

I have left the new ENV variables in place as I was not sure of the original use case.

brahmaroutu added a commit to brahmaroutu/docker that referenced this issue Jan 15, 2015
…les for port ranges, regression from moby#1834

Closes moby#9900
Signed-off-by: Srini Brahmaroutu <srbrahma@us.ibm.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants