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.

Sign up for GitHub

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

Jump to bottom

[THREESCALE-2540] Fix issues with uppercase upstreams. #1044

Merged
merged 3 commits into from
May 27, 2019
Merged

[THREESCALE-2540] Fix issues with uppercase upstreams. #1044

merged 3 commits into from
May 27, 2019
+66 −3

Conversation

eloycoto
Copy link
Contributor

If the proxy API backend is in uppercase it'll not work correctly and
Apicast exists with an execpetion [0]. This commit transform api_backend
always to lowercase string, so the system will not break even if
receives a uppercase string.

[0] Exception

2019/05/21 05:59:30 [error] 1037#1037: *3 lua entry thread aborted: runtime error: /home/centos/gateway/src/apicast/policy/apicast/apicast.lua:101: missing upstream
stack traceback:
coroutine 0:
        [C]: in function 'assert'
        /home/centos/gateway/src/apicast/policy/apicast/apicast.lua:101: in function </home/centos/gateway/src/apicast/policy/apicast/apicast.lua:100>
        /home/centos/gateway/src/apicast/policy_chain.lua:190: in function </home/centos/gateway/src/apicast/policy_chain.lua:187>
        /home/centos/gateway/src/apicast/policy_chain.lua:190: in function 'content'
        content_by_lua(lua_JOJems:485):1: in function <content_by_lua(lua_JOJems:485):1> while sending to client, client: 127.0.0.1, server: _, request: "GET /some-path?user_key=4e5944f11f575fd50bd6624c8d441f13 HTTP/1.1", host: "localhost:8080"

Fix THREESCALE-2540

Signed-off-by: Eloy Coto [email protected]

@eloycoto eloycoto requested a review from a team as a code owner May 21, 2019 06:18
@eloycoto eloycoto added this to the 3.6 milestone May 21, 2019
@eloycoto eloycoto force-pushed the THREESCALE-2540 branch 4 times, most recently from 1ff991f to 15dfaaa Compare May 21, 2019 12:24
eloycoto
eloycoto commented May 21, 2019
View reviewed changes
gateway/src/apicast/policy/apicast/apicast.lua
@@ -98,6 +102,11 @@ function _M:access(context)
end

function _M:content(context)
if not context[self].upstream then
ngx.log(ngx.WARN, "Upstream server not found for this request")
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure here, but I think that something like this need to be in place, but I'm not sure.

  ngx.status = 504
  ngx.print('')
  return exit()

Copy link
Contributor

Choose a reason for hiding this comment

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

Why is this if needed? Shouldn't we treat this case like we did up until now with the assert below?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The thing is if you have 3 services and one is not correctly set, APICast will die and other services will be affected.

With this solution, the current service will not work but others will be not affected at all.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think that we need a test case showing that apicast no longer crashes in that case.

mikz
mikz reviewed May 22, 2019
View reviewed changes
gateway/src/apicast/policy/apicast/apicast.lua Outdated
context[self].upstream = p.get_upstream(service)
context[self].upstream, err = p.get_upstream(service)
if err then
ngx.log(ngx.WARN, "Backend api for the service=", service.id, " is invalid, error:", err)
Copy link
Contributor

Choose a reason for hiding this comment

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

Usually, the logs start with a lower case letter, also it is more common to use : rather than =

Also, it is not really "backend api", but rather "upstream"

eloycoto added 3 commits May 27, 2019 11:48
@eloycoto
THREESCALE-2540 [Dependencies] Bump lua-resty-url to 0.3.5-1
Loading
Loading status checks…
56e70de 
Signed-off-by: Eloy Coto <[email protected]>
@eloycoto
Upstream: Log errors in case of invalid upstream.
670d80c 
In case of a invalid upstream url log a warning error, and don't kill
all process.

Signed-off-by: Eloy Coto <[email protected]>
@eloycoto
[Test]: Validate that uppercase upstream works correctly
732d22f 
Integration test to validate that a upstream with uppercase do not crash
and works correctly.

Signed-off-by: Eloy Coto <[email protected]>
@eloycoto eloycoto requested review from davidor and mikz May 27, 2019 10:09
mikz
mikz approved these changes May 27, 2019
View reviewed changes
Copy link
Contributor

@mikz mikz left a comment

Choose a reason for hiding this comment

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

Excellent 👍

@davidor davidor merged commit ee32f84 into 3scale:master May 27, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mikz mikz

@davidor davidor

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
3.6
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@eloycoto @mikz @davidor