-
-
Notifications
You must be signed in to change notification settings - Fork 9.2k
Fix/#4513/ability to use a sub path behind a proxy #5724
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/#4513/ability to use a sub path behind a proxy #5724
Conversation
Codecov Report
@@ Coverage Diff @@
## features/media-lib #5724 +/- ##
======================================================
- Coverage 19.16% 19.14% -0.02%
======================================================
Files 856 856
Lines 11885 11897 +12
Branches 1883 1889 +6
======================================================
Hits 2278 2278
- Misses 8072 8078 +6
- Partials 1535 1541 +6
Continue to review full report at Codecov.
|
|
I think I will move the guide part of this as I am currently writing deployment guides that include a proxy Can merge with it like it is, and I'll just move it in my PR. Will test this in a bit. Edit: Side note, please never ever recommend editing the default |
|
So here are the 3 more popular use cases I see:
|
|
Thanks for your feedback @derrickmehaffy, I will try to change the config file to only use 2 variables. @alexandrebodin proposes: |
Yup np and I agree this solution seems a bit more clear, once we get the structure in place I can assist in making some example configurations for Nginx on at least some of these. We will want to do some testing especially with something like:
|
|
@derrickmehaffy using server.url we can remove the proxy btw * |
I'm fine with that, again I'd need to test this extensively (especially with the one-clicks) Assuming since you would just prefix with http(s) and suffix with the port it should be fine, IP addresses should also work as well. And should be no problems with using environment variables as to not have to statically define it for source control. This of course would be a breaking change again. (Which I'm not against either, just need to document that) |
|
@petersg83 don't forget we also need to remove the proxy docs from the server config https://strapi.io/documentation/3.0.0-beta.x/concepts/configurations.html#server (and naturally move some documentation around, for now I'd say avoid putting any nginx proxy stuff directly in the docs as my PR: #5571 can supplement these) Once I can get around to testing this I will start building some sample configs for the usecases |
|
Yes no problem, it's still WIP :) |
Signed-off-by: Pierre Noël <pierre.noel@strapi.io>
258b490 to
d1bb84a
Compare
Signed-off-by: Pierre Noël <pierre.noel@strapi.io>
…BehindAProxy Signed-off-by: Pierre Noël <pierre.noel@strapi.io>
|
I removed the guides I wrote so we can use yours :) |
|
I'd say since you work for Strapi, signing off should be implied 😆 @alexandrebodin you da boss there. |
|
Yeah I'm setting it to pass I don't think there will be any issue here 😂 |
| - `admin` | ||
| - `autoOpen` (boolean): Enable or disabled administration opening on start. Default value: `true`. | ||
| - `path` (string): Allow to change the URL to access the admin panel. Default value: `/admin`. | ||
| - `url` (string): Url of your admin panel. Default value: `/admin`. Note: If the url is relative, it will be concatenate with `server.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.
Combined with previous note
/adminhttp://${host}:${port}/admin(to not havehttp://${host}:${port}/api/admin)
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 of what you mean, especialy with to not have http://${host}:${port}/api/admin. Can you be more specific?
|
I will go over the proxy documentation in my PR with greater detail but the two notes I added should suffice for now. |
Signed-off-by: Pierre Noël <pierre.noel@strapi.io>
6e55458 to
76267a5
Compare
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.
LGTM, we just need to keep a reminder that whatever version this is included in that the migration guide needs to reflect the changes to the proxy block.
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.
Globally looks good. a few questions
| email_confirmation: false, | ||
| email_confirmation_redirection: `http://${strapi.config.currentEnvironment.server.host}:${strapi.config.currentEnvironment.server.port}/admin`, | ||
| email_reset_password: `http://${strapi.config.currentEnvironment.server.host}:${strapi.config.currentEnvironment.server.port}/admin`, | ||
| email_confirmation_redirection: `${strapi.config.server.url}/admin`, |
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.
Shouldn't this be the admin url instead ?
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.
Yes that would make sense
| hostname, | ||
| port: strapi.config.port, | ||
| }); | ||
| let serverUrl = _.trim( |
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.
This is kinda hard to read can split the logic and add some error messages when the urls are invalid. something like this
if (_.has(conf, 'server.url') {
try {
return _.trim(new URL(conf.server.uril).toString())
} catch (error) {
throw new Error('Invalid server.url config. Make sure ....')
}
}
---
|
@alexandrebodin ? Making a new PR? |
Nope this was juste autoclosed because we delete the base branch. pierre is going to reopen it soon |
|
Mmmh It looks like I'm stuck :p |
|
This pull request has been mentioned on Strapi Community Forum. There might be relevant details there: |
fix #4513
Description of what you did:
server.admin.pathin 2 varsserver.admin.build.publicPathandserver.admin.server.pathserver.admin.backendmywebsite.com/mystrapiI tried to spot everywhere it could break. Do not hesitate to heavily test, especially in the case you setup strapi on a non-root url.