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

Enable path-based interactive tools #1256

Merged
merged 2 commits into from
Jul 11, 2024
Merged

Conversation

kysrpex
Copy link
Contributor

@kysrpex kysrpex commented Jul 11, 2024

Enable access via path (e.g. https://usegalaxy.eu/interactivetool/ep/3qg8nstfnl44r/fsjbqy66kln5) to interactive tools that support it (those for which requires_domain="False" in the tool's xml file).

More information:

Related commit: usegalaxy-eu/galaxy@e216b45

@kysrpex kysrpex self-assigned this Jul 11, 2024
Upgrade role `usegalaxy_eu.gie_proxy` from version 0.0.2 to version 0.1.0. The upgrade is required to use the variable`gie_proxy_path_prefix`.
@kysrpex kysrpex marked this pull request as ready for review July 11, 2024 13:26
@kysrpex
Copy link
Contributor Author

kysrpex commented Jul 11, 2024

Ping @sveinugu (just FYI)

kysrpex referenced this pull request in usegalaxy-eu/galaxy Jul 11, 2024
@sveinugu
Copy link

Great!

Could you also update the ansible playbook example you mentioned?

Copy link
Member

@sanjaysrikakulam sanjaysrikakulam left a comment

Choose a reason for hiding this comment

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

It looks good from my side. With this change, both subdomain and path-based ITs will work.

For path-based, the tool XML wrapper should set requires_domain="False" requires_path_in_url="True", right and rest would automagically work.

Let's wait for @bgruening's review.

proxy_set_header X-Real-IP $remote_addr;
proxy_set_header Upgrade $http_upgrade;
proxy_set_header Connection "upgrade";
proxy_pass http://127.0.0.1:{{ gie_proxy_port }};
Copy link
Member

Choose a reason for hiding this comment

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

I have a question: I see on the main nginx conf that the path /gie_proxy is suffixed after the port here. Is that necessary for this proxy_pass as well?

Copy link
Contributor Author

@kysrpex kysrpex Jul 11, 2024

Choose a reason for hiding this comment

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

Actually, I do not know what the path

	# Global GIE configuration
	location /gie_proxy {
		proxy_pass http://127.0.0.1:8800/gie_proxy;
		proxy_redirect off;
	}

does (maybe @hexylena has a clue? <- git blame). As far as I know, the configuration that is making domain-based interactive tools work is this one.

	server_name ~^(?<key>[0-9a-z-]*)\.ep\.interactivetool\.(?<domain>[a-z0-9.-]*)usegalaxy\.eu$;

	[...]

	location / {
		proxy_pass http://127.0.0.1:8800;
		proxy_redirect off;
		proxy_http_version 1.1;
		proxy_set_header Host $host;
		proxy_set_header Upgrade $http_upgrade;
		proxy_set_header Connection $connection_upgrade;
	}

See also the Galaxy docs. Both in the excerpt above and the Galaxy docs you can see that it is not necessary to add the prefix again, regardless of whether the tool is domain-based or path-based. I assume it's needed only if you want special behavior (e.g. alter the prefix).

Edit: testing galaxyproject/galaxy#18481 involved configuring nginx for both types of interactive tools, and they work fine, no extra prefix needed.

Choose a reason for hiding this comment

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

True. Adding an extra prefix would OTOH mess up path-based ITs.

Also the solution for supporting path-based ITs might differ from tool to tool. The config @sanjaysrikakulam refers to above will work if all paths in the web server in the IT are relative. If not, other solutions for injecting the path are available, as
documented in the Galaxy admin docs.

@bgruening
Copy link
Member

Interesting! Thanks @kysrpex! If this gets merged I will remove my commit in our Galaxy fork and we can try.

@kysrpex
Copy link
Contributor Author

kysrpex commented Jul 11, 2024

Great!

Could you also update the ansible playbook example you mentioned?

@sveinugu Commit a97941b would not be valid without having updated the Ansible role, see the latest release from 2h ago and the comment on the new README.

# configuring this path prefix enables path-based interactive tools
# (https://docs.galaxyproject.org/en/master/admin/special_topics/interactivetools.html#nginx-proxy-server-configuration-in-production)
gie_proxy_path_prefix: /interactivetool/ep  

@bgruening
Copy link
Member

So if someone presses merge here, I will remove the commit in the galaxy repo and redeploy :)

@kysrpex kysrpex merged commit 3fb1ef9 into master Jul 11, 2024
2 checks passed
@kysrpex kysrpex deleted the path-based_interactive_tools branch July 11, 2024 16:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants