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

[hotfix/resolver] avoid URL decoding in URI proxying #691

Merged
merged 1 commit into from Nov 10, 2015
Merged

Conversation

subnetmarco
Copy link
Member

This should fix issue #688.

@yoanisgil to test this do you need a distribution package? If yes, just tell me your system and I can create it for you.

@yoanisgil
Copy link

@thefosk We're on Docker :) so I guess something like mashape/kong:hostfix_uri will work :D

@yoanisgil
Copy link

BTW looks like the build system is broke :(

@subnetmarco
Copy link
Member Author

Yes, the TravisCI and CircleCI integrations are not working properly, but it works locally if you execute make test-all. I have done a rewrite to the CLI in the feat/invalidations branch (which includes clustering support), which is going to be my starting point to fix this once and for all.

I am building the docker package, please give me a few minutes.

@subnetmarco
Copy link
Member Author

The Docker image is kong:hotfixuri, you can now execute:

docker run -p 8000:8000 -p 8001:8001 -d --name kong --link cassandra:cassandra mashape/kong:hotfixuri

@yoanisgil
Copy link

Thanks ! I'll give a try and let you know. BTW aren't you guys using automated builds from Docker Hub with dynamic branches? That seems a pretty neat though I haven't had the time to test it myself ...

@subnetmarco
Copy link
Member Author

Nope, we are not using automated builds - would be cool to spend some time doing that (also #219).

Let me know if this hotfix addresses the issue.

@thibaultcha thibaultcha changed the title hotfix(resolver) avoid URL decoding in URI proxying [hotfix/resolver] avoid URL decoding in URI proxying Nov 7, 2015
@yoanisgil
Copy link

@thefosk Just wanted to confirm that your fix works. Thanks!!

@@ -194,7 +194,8 @@ local function find_api(uri)
end

function _M.execute(conf)
local uri = ngx.var.uri
local uri = stringy.split(ngx.var.request_uri, "?")[1]

Choose a reason for hiding this comment

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

Just curious, why the split on "?" ?

Copy link
Member Author

Choose a reason for hiding this comment

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

Some plugins, like the Request Transformations, can update the querystring. Here we are calculating the base URI, and then later in kong.lua after all the "access" handlers in the plugins have been executed, we append the possibly updated querystring at the end. So here, since I just want the base URI for resolving the API using the path, I remove the querystring.

I personally don't like the split on every request, but I can't see how we can do this differently. On the other side the split function should be fast because it invokes stringy, which invokes strchr under the hood.

@subnetmarco subnetmarco merged commit 5164bc4 into master Nov 10, 2015
@subnetmarco
Copy link
Member Author

Manually merged into both master and next

@yoanisgil
Copy link

@thefosk Can you please let me know once the new tag is available on Docker Hub?

@subnetmarco subnetmarco deleted the hotfix/uri branch November 10, 2015 02:43
@subnetmarco
Copy link
Member Author

@yoanisgil it's going to happen in the next version - we will announce it on Gitter and on our blog.

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 this pull request may close these issues.

None yet

2 participants