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

[upstream policy] use the default port (by scheme) for the upstream url #647

Closed
wants to merge 1 commit into from
Closed

[upstream policy] use the default port (by scheme) for the upstream url #647

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ and this project adheres to [Semantic Versioning](http://semver.org/).
## Added

- New property `summary` in the policy manifests [PR #633](https://github.com/3scale/apicast/pull/633)
- OAuth2.0 Token Introspection policy [PR #619](https://github.com/3scale/apicast/pull/619)
- OAuth 2.0 Token Introspection policy [PR #619](https://github.com/3scale/apicast/pull/619)
- New `metrics` phase that runs when prometheus is collecting metrics [PR #629](https://github.com/3scale/apicast/pull/629)
- Validation of policy configs both in integration and unit tests [PR #646](https://github.com/3scale/apicast/pull/646)

Expand All @@ -20,6 +20,7 @@ and this project adheres to [Semantic Versioning](http://semver.org/).
- Avoid `nameserver` repetion from `RESOLVER` variable and `resolv.conf` file [PR #636](https://github.com/3scale/apicast/pull/636)
- Bug in URL rewriting policy that ignored the `commands` attribute in the policy manifest [PR #641](https://github.com/3scale/apicast/pull/641)
- Skip comentaries after `search` values in resolv.conf [PR #635](https://github.com/3scale/apicast/pull/635)
- Add the default port based on scheme in the `upstream` policy if it is missing in the policy configuration [PR #647](https://github.com/3scale/apicast/pull/647)

## [3.2.0-beta1] - 2018-02-20

Expand Down
2 changes: 1 addition & 1 deletion gateway/src/apicast/policy/upstream/upstream.lua
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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) })
Copy link
Contributor

@davidor davidor Mar 8, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@davidor how about #662?


ngx.var.proxy_pass = proxy_pass(url)
ngx.req.set_header('Host', url.host)
Expand Down
20 changes: 20 additions & 0 deletions spec/policy/upstream/upstream_spec.lua
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ describe('Upstream policy', function()

-- ngx functions and vars
ngx.var = { uri = test_req_uri }
ngx.ctx = {}
stub(ngx.req, 'set_header')
stub(ngx, 'exec')
end)
Expand Down Expand Up @@ -98,6 +99,25 @@ describe('Upstream policy', function()
assert.is_falsy(context.upstream_changed)
end)
end)

describe('when the port is not specified for the url', function()
local url_without_port = 'https://127.0.0.1/a_path'
local config_with_a_matching_rule = {
rules = {
{ regex = '/', url = url_without_port }
}
}

local upstream = UpstreamPolicy.new(config_with_a_matching_rule)

it('sets a default port by scheme', function()
upstream:content(context)

assert.is_truthy(context.upstream_changed)
assert.equals(443, ngx.ctx.upstream[1].port)
end)

end)
end)

describe('.balancer', function()
Expand Down