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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

WebPrefix does not update API_URL #225

Closed
lawrenceong opened this issue Feb 27, 2019 · 12 comments · Fixed by #234
Closed

WebPrefix does not update API_URL #225

lawrenceong opened this issue Feb 27, 2019 · 12 comments · Fixed by #234

Comments

@lawrenceong
Copy link
Contributor

When we add a WebPrefix by defining FLAGR_WEB_PREFIX, we are expecting the entire app to use the prefix. We found that the APIs are not using the prefix, resulting in calls to /api/v1 instead of /webprefix/api/v1.

Expected Behavior

If we set up flagr locally, calls to the APIs should include the prefix. For example:

The following should return a 200 OK:

curl -v http://localhost:18000/WEBPREFIX/api/v1/health

The following should return a 404 NOT FOUND:

curl -v http://localhost:18000/api/v1/health

Instead, we get the opposite behavior, ie. works without prefix, but does not work with a prefix.

Current Behavior

See Expected Behavior above.

Possible Solution

May be able to fix this by ensuring that API_URL is set based on the provided WebPrefix (ie. FLAGR_WEB_PREFIX).

Steps to Reproduce (for bugs)

Run up an instance of flagr locally and set FLAGR_WEB_PREFIX to /flags. Run the curl commands as per expected behavior.

Context

We are trying to run flagr with other applications and this issue prevents it from working as we are proxying only a particular webprefix to flagr.

Your Environment

  • Flagr version used: 1.0.14
  • Running on docker
@zhouzhuojie
Copy link
Collaborator

zhouzhuojie commented Feb 27, 2019

FLAGR_WEB_PREFIX is for the UI prefix route only, we currently don't have a configurable API prefix route. PRs are welcome to add it.

@lawrenceong
Copy link
Contributor Author

lawrenceong commented Feb 27, 2019

Thanks @zhouzhuojie for the quick reply! We are using the following in nginx as a temporary workaround:

    set $flagr "http://flagr:18000";
    location /flagr {
        proxy_pass $flagr;
    }
    location /api/v1/flags {
        proxy_pass $flagr;
    }
    location /api/v1/evaluation {
        proxy_pass $flagr;
    }

Yes, I understand this does not include every possible endpoints, eg. /api/v1/health.

Will take a look at providing a PR when able.

@lawrenceong
Copy link
Contributor Author

lawrenceong commented Feb 27, 2019

@zhouzhuojie regarding a PR to allow the API URL to have a prefix.

I believe the parameter FLAGR_WEB_PREFIX should change the API prefix as well. From what I understand, webpack pre-generate the js file that contains the parameter API_URL. This means the only way we can change API_URL is to use relative pathing as how you have done in #223 with the static assets.

If we make API_URL relative, it will immediately start going to /webprefix/api/v1/*. To me, this means the API path should just be updated to make use of FLAGR_WEB_PREFIX. What do you think?

@zhouzhuojie
Copy link
Collaborator

That is a breaking change. For example, people may have API runing at /api/v1 and web UI running at /ui, and then the change of reusing FLAGR_WEB_PREFIX for API will break their client SDK calls.

How about not setting FLAGR_WEB_PREFIX, and just proxy via nginx or any api-gateway with url strip (e.g. rewrite or proxy_pass), for example

example.com/webprefix/ => flagr.localhost:18000/, and you can get exactly what you want.

@lawrenceong
Copy link
Contributor Author

lawrenceong commented Feb 28, 2019

example.com/webprefix/ => flagr.localhost:18000/, and you can get exactly what you want.

@zhouzhuojie That does not actually work because the reverse proxy mechanism will only modify the headers. It does not touch the body which is where API_URL is located (redirecting to a hardcoded /api/v1/*).

This is why I have to revert to the above method to make it work in Nginx. ie. 3 different paths. This is regardless of whether we use FLAGR_WEB_PREFIX or not.

@zhouzhuojie
Copy link
Collaborator

I see. We can make API_URL in the vue app as an environment variable when it's built, and then in Dockerfile, we pass that as a build arg. The downside is that you will have to build the docker image yourself.

@lawrenceong
Copy link
Contributor Author

Yup, I took a look at that solution too, but it's not elegant due to the need to rebuild. Basically looking for a solution where we do not have to rebuild the docker image.

Is it bad to have FLAGR_WEB_PREFIX move API path as well? I understand that it can be a breaking change if someone sets FLAGR_WEB_PREFIX and wants the API to remain at /api/v1. What is the chance that someone needs this?

Based on #223 (that was fixed less than 6 days ago), it looks like FLAGR_WEB_PREFIX may not be in use extensively yet. Would you be willing to accept this breaking change and move API based on FLAGR_WEB_PREFIX as well? This would allow us to map the entire app to a different path using a reverse proxy easily.

@zhouzhuojie
Copy link
Collaborator

Yup, agreed, FLAGR_WEB_PREFIX was broken before #223

@SebastianOsuna, since you implemented #170, are you ok with using FLAGR_WEB_PREFIX to prefix both API and UI?

@SebastianOsuna
Copy link
Contributor

I agree it's a better solution!

@lawrenceong
Copy link
Contributor Author

So, I've been taking a look at submitting a PR for this and it looks like it's not all that simple. We generate the API using go-swagger and what's being generated does not allow us to overwrite the basePath. It looks like OpenAPI v3 allows us to override basePath, but that would mean an upgrade from v2 to v3 and go-swagger itself does not support v3.

  • For the frontend, we can just change /api/v1 to api/v2 without the leading /
  • For the backend, switch to another library to generate the API from swagger config?

@zhouzhuojie
Copy link
Collaborator

@lawrenceong How about #234?

@lawrenceong
Copy link
Contributor Author

@zhouzhuojie Looks good to me. Thank you.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants