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

Doesn't work with API served on sub-path #237

Closed
danh-fissara opened this issue Aug 16, 2018 · 5 comments · Fixed by #241
Closed

Doesn't work with API served on sub-path #237

danh-fissara opened this issue Aug 16, 2018 · 5 comments · Fixed by #241

Comments

@danh-fissara
Copy link

Hi,
I noticed that any path as part of the the base URL gets ignored, because the (/run, /minions etc.) paths passed to URI.resolve() are absolute and so replace whatever was specified in ClientConfig.URL:

URI uri = config.get(ClientConfig.URL).resolve(endpoint);

It looks like trivial fix, but I see the code's moved on a fair bit since 0.14.0, and am not sure if it's still an issue.

@renner
Copy link
Member

renner commented Aug 23, 2018

Hey @danh-fissara, we are about to release version 0.15.0 soon and indeed many things are going to change also regarding the client configuration. I don't really understand though what is your question: the API should always be running on a specific URL and those endpoints like /run should actually be appended by the call to resolve(). Maybe @lucidd can help on this topic, too.

@danh-fissara
Copy link
Author

Hey, thanks for the response. To clarify, we're running the API behind a proxy, so a request to https://example.com:443/api/run gets proxied to the salt-api as http://localhost:8000/run.

Anyway, even without the proxy, the docs seem to say running at sub-paths is supported via the root_prefix config:
https://docs.saltstack.com/en/latest/ref/netapi/all/salt.netapi.rest_cherrypy.html#a-rest-api-for-salt

root_prefix
: /
A URL path to the main entry point for the application. This is useful for serving multiple applications from the same URL.

@lucidd
Copy link
Member

lucidd commented Aug 23, 2018

@danh-fissara hey yeah you are right the problem is the leading / we added everywhere. I think it makes sense to support your use case and it should be easy to find all the endpoints strings in the code and remove the / there.

If you want to contribute a fix to this we are happy to include it in the 1.0 release 😄

@renner renner added this to the Version 1.0.0 milestone Aug 23, 2018
renner added a commit that referenced this issue Oct 18, 2018
renner added a commit that referenced this issue Oct 18, 2018
@renner
Copy link
Member

renner commented Oct 18, 2018

@danh-fissara: It would be great if you could try out the patch from #241 so we know if it actually fixes your issue. Thanks in advance!

@renner renner modified the milestone: Version 0.15.0 Oct 18, 2018
renner added a commit that referenced this issue Oct 22, 2018
renner added a commit that referenced this issue Oct 22, 2018
@renner
Copy link
Member

renner commented Oct 22, 2018

Okay, #241 was merged so I expect this issue to be fixed with the next release (or current master). Please let us know if it works for you or if there is any further issues, @danh-fissara.

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.

3 participants