-
-
Notifications
You must be signed in to change notification settings - Fork 7.7k
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
enhancement!: use server.globalProxy value for creating a global http/https proxy #20416
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
Building exp release: https://github.com/strapi/strapi/actions/runs/9322440851 |
@innerdvations looks like we are getting build failures with |
Ugh, very sorry! I missed a last minute version bump on undici. It builds now. |
Can you run a new exp version build Ben and I'll at in the morning |
exp version: |
I haven't looked into how they're loaded. If they're essentially an img src so it would make sense not to pass through the proxy. If we have something where the server is pulling them from the remote bucket and then sending them to the user, that should go through the proxy, but I'll consider it a separate issue and address it later. |
Ah yeah your probably right ignore me |
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.
The code LGTM, one small comment.
I couldn't test the actual behavior as I don't have the correct setup.
@@ -17,7 +17,7 @@ const healthCheck: Core.MiddlewareHandler = async (ctx) => { | |||
|
|||
const createServer = (strapi: Core.Strapi): Modules.Server.Server => { | |||
const app = createKoaApp({ | |||
proxy: strapi.config.get('server.proxy'), |
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 change have a dedicated codemod?
If it's meant for another PR, could we add a TODO so that we don't forget?
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.
yep, already created a jira ticket but I'll mention it here too
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.
Hum if we just have strapi.proxy set can't we consider that it's should be the proxy applied for everything like a shorthand version ? that could be super convenient 🤔
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 don't have time to write tests for it before I'm off, but I did this as a follow-up convenience feature, and someone can hopefully finish it and update the docs: #20537
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.
Oh wait, I think I completely misunderstood. You mean the strapi.proxy could be like string | { fetch, http, https, koa }
and if it's a string it is treated as global?
You can test it generally using For the more advanced stuff like testing that SSO works through a proxy, I'm hoping @derrickmehaffy will be able to give it one final QA once the code has settled before we merge. |
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 code wise, just left a small suggestion
const globalProxy = | ||
strapi.config.get<ConstructorParameters<typeof ProxyAgent>[0]>('server.globalProxy'); | ||
const proxy = | ||
strapi.config.get<ConstructorParameters<typeof ProxyAgent>[0]>('server.proxy.fetch') || |
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 wondering if it will be clearer to add a type alias for this ConstructorParameters<typeof ProxyAgent>[0]
will leave it for you to decided otherwise looks good
What does it do?
server.proxy
toserver.proxy.koa
server.globalProxy
toserver.proxy.global
server.proxy.fetch
to act only on Fetch requests (defaults to proxy.global if set)server.proxy.http
to act onhttp
requests (defaults to proxy.global if set)server.proxy.https
to act onhttps
requests (defaults to proxy.global if set)This
Why is it needed?
Allows proxy of all http and https requests globally (including anything that uses http/https internally, such as axios, passport-oauth2, etc, giving Strapi an actual global proxy to solve issues like proxied SSO
How to test it?
npx http-echo-server
to create an echo proxyRelated issue(s)/PR(s)
Fixes #17317
DX-989
Documentation PR: strapi/documentation#2124
TODO: