-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
fix: make rest-explorer aware of openapi server root path #2293
fix: make rest-explorer aware of openapi server root path #2293
Conversation
this.openApiSpecUrl = this.getOpenApiSpecUrl(restConfig); | ||
} | ||
|
||
indexRedirect() { | ||
this.response.redirect(301, this.request.url + '/'); | ||
this.response.redirect(301, this.serverRootPath + this.request.url + '/'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure if inferring from openapi.servers[0].url
is a feasible solution. Please note that LoopBack app is not aware how its endpoints are proxies/exposed. The same endpoint can be possibly exposed as many different urls, such as:
- https://a.example.com/api/customer
- https://b.example.com/api/user
- http://internal.example.com:8080/customer
When it comes to redirect via Location
header, my understanding is that proxy/load balancer is responsible for rewriting Location
to match its clients.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The redirect could be made to depend on the proxy (though that requires config on the proxy). But the same question would then apply to the /openapi.json
reference too.
I was concerned about this in my original bug report, but @bajtos had a different perspective.
A brainstorm: I think LB2 dealt with this by having the explorer be the one that serves the openapi.json
, at least when "talking to itself". This could be replicated within rest-explorer
with a little work I think, though it would require the RestServer
to expose _serveOpenApiSpec
as a public method. But I think then the explorer could be made to use exclusively relative URLs, avoiding the path rewriting issue in a more general way?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we should set Location
to a url based on the request - https://github.com/strongloop/loopback-next/blob/master/packages/rest/src/rest.server.ts#L429.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we set Location
to a relative url, would it prevent reverse proxies from rewriting it correctly?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was hoping that using a relative Location
url would mean the proxy doesn't need to rewrite it :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe that this.request.url
contains only the path portion of the requested URL as seen by the LB4 server, after it has been rewritten by the reverse proxy. IMO, the current implementation is correct, it's redirecting from /explorer
to /explorer/index.html
.
I am not able to fully understand implications of the change proposed here. More importantly, I don't understand why this particular change is needed.
Can we revert it please?
@raymondfeng @bajtos -- What do you think about having the explorer serve its own copy of the |
Can you elaborate? |
Currently the explorer tries to figure out the URL it should send to the client for it to fetch the So, instead of trying to find the URL it's being served on for other purposes, what if it adds its own URL on which it serves the OAS material. I.e. if you have the explorer mounted at This is (roughly) how it seems to have worked in LB2 -- the OAS file was at |
I think it's worth exploring. Maybe we should expose an API from |
I see what you did there 😉
Indeed ... I was thinking of exposing a version of If this sounds sensible, I can update this PR to go down that path. |
+1 to modify our REST API explorer to use a relative URL for the OpenAPI spec if that's going to fix the problem. I am not convinced that we need a new explorer-specific endpoint for that though. A typical LoopBack 4 application comes with the following endpoints:
IMO, it should be enough to figure out what's the relative path from
Can we assume that the base path for openapi and explorer endpoints will be always the same? I believe the answer is true when LB4 is called directly. I am not sure about reverse proxies. How likely it is that somebody will configure the reverse proxy to expose I think our implementation should:
That way we can keep our implementation simple and easy to maintain and still support advanced use cases. |
@mgabeler-lee-6rs I feel that I may not fully understand what's happening under the hood when a reverse proxy is configured in front of a LB4 app and REST API Explorer is loaded. Could you please create a setup we could use to reproduce the problem you are encountering on our machines? I'd like to investigate this more in depth myself to build a better understanding on how individual parts interact together. A reverse proxy configuration for our todo example would be ideal. |
@mgabeler-lee-6rs It would be great to create such scenario based on https://loopback.io/doc/en/lb4/deploying-with-pm2-and-nginx.html to illustrate how the |
This seems to be simple way to reproduce the problem too - #2306 (comment) |
In #2330, we have moved test files from Sorry for the inconvenience. |
@mgabeler-lee-6rs ping 👋 what's the status of this pull request? Are you still keen to get it finished? |
I am still keen to finish it up, yes. I've just been diverted by some higher priorities temporarily. |
I created #2387 to address the issue when rest-server is mounted to express using a path such as |
@raymondfeng when do you see that one landing ? |
Is this pull request still relevant, considering the work done in #2856 and previous pull requests? |
I'd need to do some testing. I think @dkrantsberg's reverse-proxy setup described in the comments on #2554 is not dissimilar to mine, though simpler. I'm curious if he has a working setup for comparison. The workaround I'm still monkey-patching into my apps is to sub-class the explorer and give it an extra config setting for the "external" base path, which is built based on known patterns for our reverse proxy when running in production. This is ... not pretty to say the least, but it gets the job done for me. I think the simplest way to be able to handle this in a portable fashion is to make the rest explorer exclusively use relative URLs as described earlier -- have the explorer serve its own copy of the openapi spec using the format it needs, at a path relative to the explorer. Then we never need to worry about absolute paths in any of the requests, and most of the path logic should go away? |
@mgabeler-lee-6rs thank you for the response.
That sounds like a reasonable solution to me. Would you like to open a new pull request for this proposal? Another alternative that comes to my mind: modify Explorer to use a relative path to access OpenAPI spec. I.e. when serving |
Yes, I'll give that a shot 🙂
I thought about that, but then the problem space of correctly computing the relative path to the OAS for all the possible ways users might modify it starts to feel ... complicated. Much more so than having our own copy that is always at a fixed path relative to the explorer. |
I've started a draft of the relative path hosting in #3133. That PR is definitely in a WIP state right now, but there is enough there to take a look at the idea at least. I'll be fleshing it out with tests and whatnot of course. Closing this PR in favor of that new one. |
The
rest-explorer
component needs to be aware of any "external" base paths that are in use. This PR is at least a start to that by making it prefix any paths it sends to clients with the first path from theopenApiSpec.servers
list (if any).Fixes: #2285
Checklist
npm test
passes on your machinepackages/cli
were updatedexamples/*
were updatedHaving some problems with
test
andlint
, mainly related tonpm install
not having installedprettier
for some reason. I have no reason to believe that this is caused by my changes, and I added tests for my changes, which pass, but I've been unable to run the linter or get the full test suite to pass on my system yet.