jjethwa / rundeck

GNU General Public License v3.0
123 stars 137 forks source link

allow omission of EXTERNAL_RUNDECK_URL; formatting code #93

Closed ddavison closed 6 years ago

ddavison commented 6 years ago

Summation of changes:

Problem description:

When launching the jordan/rundeck container from an orchestration tool like Kubernetes, OpenShift or AppOrbit; The image is given a dynamic IP of hosts to host from. Rundeck's grails.serverURL controls how grails will redirect the application. When this property is commented out, rundeck refuses to forcibly redirect and leaves it to relative pathing.

Examples:

Before change

$ docker run -p 4440:4440 -e SERVER_URL=http://localhost:4440

After logging in, a header is sent from rundeck to redirect to "http://localhost:4440. fine. If this container was launched from AppOrbit, or another orchestration tool that dynamically assigns the IP, the IP can be impossible to obtain. Therefore, rundeck would attempt to redirect to an invalid ip/host.

After change

$ docker run -p 4440:4440 -e SERVER_URL=http://localhost:4440
$ docker exec -it ... cat /etc/rundeck/rundeck-config.properties
...
#grails.serverURL=
...

# With grails.serverURL commented out, rundeck will no longer forcibly redirect and fall back to relative pathing.
$ docker run -p 4440:4440 -e SERVER_URL=http://localhost:4440 -e EXTERNAL_SERVER_URL=http://test:4440
$ docker exec -it ... cat /etc/rundeck/rundeck-config.properties
...
grails.serverURL=http://test:4440
...

# We are able to provide EXTERNAL_SERVER_URL when applicable

I've tested this thoroughly and validates that this indeed works.

Source material:

There could be unknown side-effects of this change, however the ability for the explicit setting of EXTERNAL_SERVER_URL should bypass any of those cases.

jjethwa commented 6 years ago

Thanks so much, @ddavison

Is there any way you can update the PR to include only the necessary changes? The space/tab changes are making it a bit hard to sort through. Really appreciate it πŸ‘

ddavison commented 6 years ago

sure thing! unfortunately i forgot to separate the commits.. so give me a bit :face_with_head_bandage:

ddavison commented 6 years ago

should be a bit more readable now. a couple spacing fixes were unintentionally left in there, as well as blank lines.

jjethwa commented 6 years ago

Thanks, @ddavison

Looks good! πŸ˜„

jjethwa commented 6 years ago

Hi @ddavison

I received an issue and some offline complaints about configs getting replaced due to the /etc/rundeck check being removed, so I've added it back in for now. Do you see any problems with having it back in that would affect your work? Thanks πŸ˜„

ddavison commented 6 years ago

yea no, that's fine! honestly, in retrospect - it was kind of a destructive change that i probably should not have put in. no worries though. all is well.

jjethwa commented 6 years ago

Great! Thanks for confirming, @ddavison I had missed it during the PR merge πŸ˜›