Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Enable overriding the backend url to enable standalone mode #347

Merged
merged 10 commits into from
Jul 29, 2019

Conversation

chetanmeh
Copy link
Member

@chetanmeh chetanmeh commented Jul 26, 2019

This PR enables running api gateway with standalone server (apache/openwhisk#4571).

It also fixes current broken build by updating the opm version

Implementation Details

Api Gateway uses the backend url as computed by makeWebActionBackendUrl in createApi call. This backend url is based on url passed by wsk api command

{"apidoc":{"namespace":"_","gatewayBasePath":"/hello","gatewayPath":"/world","gatewayMethod":"GET","id":"API:_:/hello","action":{"name":"hello","namespace":"_","backendMethod":"GET","backendUrl":"http://localhost:3233/api/v1/web/_/default/hello.http","authkey":"23bc46b1-71f6-4ed5-8c54-816aa4f8c502:123zO3xZCLrMN6v2BKK1dXYFpXlPkccOFqm12CdAsMgRU4VrNZ9lyGVCGuMDGIwP"}}}

With standalone case this url becomes http://localhost:3233 and is used as is by api gateway routing logic. So to enable this flow to work the backendRouting logic now exposes a env variable BACKEND_HOST. If this variable is found to be set then it supersedes the backend url as specified in createApi call

@@ -39,6 +40,11 @@ function _M.setRoute(backendUrl, gatewayPath)
ngx.req.set_uri(getUriPath(u.path))
end
ngx.var.backendUrl = backendUrl
Copy link
Member

Choose a reason for hiding this comment

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

it's ok to not also change ngx.var?

Copy link
Member Author

Choose a reason for hiding this comment

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

Changed the ngx.var also

@chetanmeh
Copy link
Member Author

@mrutkows @dgrove-oss The build is failing due to scancode issue reporting lots of trailing whitespace in code from downloaded lua modules. Anyway to fix scancode logic to exclude those files?

Copy link
Contributor

@ddragosd ddragosd left a comment

Choose a reason for hiding this comment

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

LGTM as well. Many thanks for moving this ahead @chetanmeh !

@dgrove-oss
Copy link
Member

@chetanmeh -- we can reorder travis to do the scancode before it installs lua. Do the scancode in preinstall stage. Do you want to do it as part of this PR, or do you want me to do a separate PR just for that?

@chetanmeh
Copy link
Member Author

@dgrove-oss Please review the travis and build related changes once

@chetanmeh chetanmeh requested a review from dgrove-oss July 29, 2019 11:33
Copy link
Member

@dgrove-oss dgrove-oss left a comment

Choose a reason for hiding this comment

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

LGTM

@chetanmeh chetanmeh merged commit c064b29 into apache:master Jul 29, 2019
@chetanmeh chetanmeh deleted the standalone-support branch July 29, 2019 11:54
@mhamann
Copy link
Member

mhamann commented Jul 29, 2019

@chetanmeh can you write some tests for this? we need to make sure that this doesn't break production use cases. I'm assuming it does not, but there are no test assertions proving that.

@rabbah
Copy link
Member

rabbah commented Jul 29, 2019

a visual inspection shows all the changes are guarded by if backendOverride ~= nil then whose value is derived from a new environment variable, making this low risk in general.

of course, adding tests ensures this new feature isn't broken in the future.

@mhamann
Copy link
Member

mhamann commented Jul 29, 2019

Positive and negative tests are both needed (proving that both cases work as intended).

While I agree that the code looks correct, I've been bitten plenty of times by code that looked fine visually, but had some nasty side-effect nobody noticed.

@chetanmeh
Copy link
Member Author

@mhamann Added few tests in #350. Please review

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

Successfully merging this pull request may close these issues.

5 participants