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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Configure proxy prefix #127

Merged
merged 3 commits into from
Apr 23, 2017
Merged

Configure proxy prefix #127

merged 3 commits into from
Apr 23, 2017

Conversation

abretaud
Copy link
Contributor

Hej!
I'm running galaxy dockers behind a collection of proxies, with url prefixes and it was a bit complicated to configure until now. So I'm preparing a PR for https://github.com/bgruening/docker-galaxy-stable where I introduce a new env var (GALAXY_CONFIG_PROXY_PREFIX) which will magically make all changes necessary to make all components aware of the url prefix.
The main logic is in this PR, most of it is working on my test setup, though I'll do more tests next week.
Open to any suggestions of course!
Happy easter! 馃 馃 馃

@bgruening
Copy link
Member

@abretaud great! Thanks for working on this! Can we change GALAXY_CONFIG_PROXY_PREFIX to something else. Usually GALAXY_CONFIG_* indicates that it is a variable from galaxy.ini. Mabye just PROXY_PREFIX?

@abretaud
Copy link
Contributor Author

Right, I just changed its name. From what I have tested it works ok now.

I had a problem accessing reports when DISABLE_REPORTS_AUTH was true, it looks like there was a bug in the nginx conf (because of the strange if behavior in nginx). I reworked it to avoid using the nginx if, it looks a bit hacky, but I see no better way...

I have problems running docker-in-docker at the moment, so I couldn't test IE. (Yet)

rm /etc/nginx/htpasswd
echo "" > /etc/nginx/conf.d/reports_auth.conf
else
# enable authentification by deleting the htpasswd file
Copy link
Member

Choose a reason for hiding this comment

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

This comment looks wrong.

@bgruening
Copy link
Member

@jmchilton @mvdbeek any comment to this PR. Looks good to me.

# disable authentification by deleting the htpasswd file
echo "Disable Galaxy reports authentification "
rm /etc/nginx/htpasswd
echo "" > /etc/nginx/conf.d/reports_auth.conf
Copy link
Member

Choose a reason for hiding this comment

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

Why not keep

            if (!-f /etc/nginx/htpasswd) {		
                set $auth off;		
            }

in templates/nginx_reports_auth.conf.j2 ?
That would be better for people not using the docker image / these startup scripts.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'd prefer to, but it doesn't work unfortunately... When nginx enters into the if {}, the proxy_pass is ignored and you get a 404 error. I had a hard time finding out why, there are some examples explaining why there: http://agentzh.blogspot.fr/2011/03/how-nginx-location-if-works.html

@bgruening
Copy link
Member

@mvdbeek what do you think?

@mvdbeek
Copy link
Member

mvdbeek commented Apr 22, 2017

This does now imperatively required a htpwassd file to use reports , and the role is not providing a way to set the htpasswd file. I'd prefer this to be optional, so that you can run the majority of the role without additional bash action.
You could even remove the bash scripting part in this PR and run ansible to apply the new settings at runtime, like we already do for letsencrypt (lines 332-360 in startup.sh.j2).
The PR is good otherwise, and setting proxy-prefix at runtime is definitely important. If this will bite me in the future I can always open another PR.

@mvdbeek mvdbeek merged commit 1e42fb2 into galaxyproject:master Apr 23, 2017
@drosofff drosofff mentioned this pull request Apr 23, 2017
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 this pull request may close these issues.

None yet

3 participants