[upstream policy] use the default port (by scheme) for the upstream url #647
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
mayorova
changed the title
mayorova
force-pushed
the
default-port-for-upstream-policy
branch
from
3907d1d to
f877522
Compare
mayorova
force-pushed
the
default-port-for-upstream-policy
branch
from
f877522 to
80f00d9
Compare
mayorova
changed the title
mikz
approved these changes
Mar 7, 2018
davidor
reviewed
Mar 8, 2018
| @@ -27,7 +27,7 @@ end | |||
|
|
|||
| local function change_upstream(url) | |||
| ngx.ctx.upstream = resty_resolver:instance():get_servers( | |||
| url.host, { port = url.port }) | |||
| url.host, { port = url.port or resty_url.default_port(url.scheme) }) | |||
There was a problem hiding this comment.
I think this can't be here. When a port is passed here, it overwrites the one set by the resolver.
That's why the sanity.t integration test fails. That test uses the upstream policy:
"policy_chain": [
{ "name": "apicast.policy.upstream",
"configuration": { "rules": [ { "regex": "/", "url": "http://echo" } ] } }
]In that case, when resty_resolver resolves echo, it includes a port (randomly generated by the test framework in this case), and your solution overwrites it with 80.
I think the port should be set only when it's not set by the resolver.
|
Closing in favor of #662 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This fixes the following issue:
when the URL in the
upstreampolicy is set tohttps://something, the port isnil, and so https://github.com/3scale/apicast/blob/2affe4e40e268841b489cf06c229a66ac9aa9749/gateway/src/resty/balancer.lua#L33 will set it to80. As a result APIcast will try to reachhttps://something:80and fail.Couldn't figure out how to test this yet... some help would be appreciated @davidor @mikz 🙏