-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
🎉 allow users to access both the api and webapp from the same port #3603
Conversation
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
.env.dev
Outdated
@@ -19,3 +19,4 @@ TRACKING_STRATEGY=logging | |||
HACK_LOCAL_ROOT_PARENT=/tmp | |||
WEBAPP_URL=http://localhost:8000/ | |||
API_URL=http://localhost:8001/api/v1/ | |||
INTERNAL_API_URL=http://airbyte-server:8001/api/ |
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 line looks incorrect.
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.
ah yes, this should be INTERNAL_API_HOST
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's a detail here i'm not fully understanding. left the question below to clear it up. but it looks good!
@@ -29,4 +33,8 @@ server { | |||
location = /50x.html { | |||
root /usr/share/nginx/html; | |||
} | |||
|
|||
location /api/ { |
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.
Going to write out my understanding so that you can correct me if I'm misinterpreting...
- The UI is already (by default) hosted on port 8000. We want to make it so that the API is also accessible on port 8000 (instead of 80001).
- We do this by adding a rule in the nginx server that runs on port 8000 saying whenever you traffic to
/api
proxy that to the server. so now localhost:8000/api/foo gets forwarded to the server. right so far? - What I'm not fully understanding is this part of your explanation: "To "enable" this, all you need is to set API_URL=/api/v1/ in your .env file." It seems like as configured it already achieves what you want it to. Why would one need to set it
/api/v1/
in addition?
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 you don't set API_URL
it will query localhost:8001 still from the UI instead of querying the now forwarded address.
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.
We don't need to make this the default for now, but I could see it being the default in the future.
For SM all I'll need to do is update the .env after upgrading them to a release on a version with this PR.
Just realized I need to propagate this env to the kube manifest |
Btw @cgardens shouldn't using I think cookies should be sent correctly in such case, because webapp and API will be on the same domain. Probably it will need additional changes to nginx config to support it correctly, but I believe it is much easier than trying to set up CORS correctly |
In #3422 I was trying to make a single port the only method of using Airbyte.
In this PR, I'm simply allowing the usage. It always forwards /api to an upstream (which is more resilient to server problems than the original PR as well).
To "enable" this, all you need is to set
API_URL=/api/v1/
in your.env
file.There's one downside, which is that on startup instead of showing the server is starting message it fails until the server starts. We'll need to add 502 errors as an allowed error that causes the network boundary error.
I'll create issues to update our defaults/docs and for the 502 error handling.