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鈥檒l 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

Merged
merged 5 commits into from
May 26, 2021

Conversation

jrhizor
Copy link
Contributor

@jrhizor jrhizor commented May 25, 2021

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.

Copy link
Contributor

@michel-tricot michel-tricot left a 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/
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This line looks incorrect.

Copy link
Contributor Author

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

@jrhizor jrhizor requested review from cgardens and removed request for davinchia May 26, 2021 15:57
Copy link
Contributor

@cgardens cgardens left a 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/ {
Copy link
Contributor

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?

Copy link
Contributor Author

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.

Copy link
Contributor Author

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.

@jrhizor
Copy link
Contributor Author

jrhizor commented May 26, 2021

Just realized I need to propagate this env to the kube manifest

@jamakase
Copy link
Contributor

Btw @cgardens shouldn't using /api as API route also fix this issue? #2805

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

@jrhizor jrhizor merged commit 581b273 into master May 26, 2021
@jrhizor jrhizor deleted the jrhizor/single-port-v2 branch May 26, 2021 20:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants