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

Threescale 7967 http proxy patch #1323

Merged
merged 5 commits into from
Apr 20, 2022
Merged

Threescale 7967 http proxy patch #1323

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
2fc0df2
removes resolve of hostname and adds host/port to target server conne…
kevprice83 Feb 2, 2022
1fa7dd8
fixes integration tests 1-10
kevprice83 Apr 18, 2022
d9b6f30
updates CHANGELOG entry, reverts some unwanted changes introduced in …
kevprice83 Apr 20, 2022
dc752e9
fixes broken http proxy policy integration test
kevprice83 Apr 20, 2022
4b47c81
fixed camel policy tests
samugi Apr 20, 2022
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
43 changes: 27 additions & 16 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,11 +1,13 @@
# Change Log

Copy link
Contributor

@samugi samugi Feb 4, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

were all these newlines and formatting changes in the CHANGELOG meant to be committed?

Copy link
Member Author

Choose a reason for hiding this comment

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

no I didn't make those changes, not sure where they came from to be honest. I only added a CHANGELOG entry, I assume you are referring to the CHANGELOG and not README though right?

Copy link
Contributor

@samugi samugi Feb 9, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

yes I was :) - fixed my comment

All notable changes to this project will be documented in this file.

The format is based on [Keep a Changelog](http://keepachangelog.com/)
and this project adheres to [Semantic Versioning](http://semver.org/).


## [3.11.0] 2021-09-03
## [Unreleased]

### Fixed

Expand Down Expand Up @@ -34,6 +36,8 @@ and this project adheres to [Semantic Versioning](http://semver.org/).
- Fixed warning messages [PR #1318](https://github.com/3scale/APIcast/pull/1318) [THREESCALE-7906](https://issues.redhat.com/browse/THREESCALE-7906)
- Fixed dirty context [PR #1328](https://github.com/3scale/APIcast/pull/1328) [THREESCALE-8000](https://issues.redhat.com/browse/THREESCALE-8000) [THREESCALE-8007](https://issues.redhat.com/browse/THREESCALE-8007)
- Fixed jwk alg confusion [PR #1329](https://github.com/3scale/APIcast/pull/1329) [THREESCALE-8249](https://issues.redhat.com/browse/THREESCALE-8249)
- Fixed issue with resolving target server hostnames to IP when using CONNECT method [PR #1323](https://github.com/3scale/APIcast/pull/1323) [THREESCALE-7967](https://issues.redhat.com/browse/THREESCALE-7967)
- Fixed issue with resolving target server hostnames to IPs when forwarding requests through http/s proxy [PR #1323](https://github.com/3scale/APIcast/pull/1323) [THREESCALE-7967](https://issues.redhat.com/browse/THREESCALE-7967)

### Added

Expand All @@ -54,7 +58,6 @@ and this project adheres to [Semantic Versioning](http://semver.org/).
- Added on_failed policy [PR#1286](https://github.com/3scale/APIcast/pull/1286) [THREESCALE-6705](https://issues.redhat.com/browse/THREESCALE-6705)
- Master branch containers builds are now latest tag on quay.io [PR#1289](https://github.com/3scale/APIcast/pull/1289) [THREESCALE-7251](https://issues.redhat.com/browse/THREESCALE-7251)


## [3.10.0] 2021-01-04

Beta1 is stable and moved to final release.
Expand All @@ -74,6 +77,7 @@ Beta1 is stable and moved to final release.
## [3.10.0-alpha1] 2020-10-13

### Added

- Support Proxy Protocol [PR #1211](https://github.com/3scale/APIcast/pull/1211) [THREESCALE-5366](https://issues.redhat.com/browse/THREESCALE-5366)
- Enable support to log credentials on logging policy [PR #1217](https://github.com/3scale/APIcast/pull/1217) [THREESCALE-5273](https://issues.redhat.com/browse/THREESCALE-5273)
- Add a way to support more than 1000 services in a single instance [PR #1222](https://github.com/3scale/APIcast/pull/1222) [THREESCALE-5308](https://issues.redhat.com/browse/THREESCALE-5308)
Expand All @@ -82,21 +86,21 @@ Beta1 is stable and moved to final release.
- Add response/request content size limits [PR #1227](https://github.com/3scale/APIcast/pull/1227) [THREESCALE-5244](https://issues.redhat.com/browse/THREESCALE-5244)
- Add HTTP codes policy [PR #1236](https://github.com/3scale/APIcast/pull/1236) [THREESCALE-6255](https://issues.redhat.com/browse/THREESCALE-6255)



### Fixed

- Fixed issues with allow caching mode and 3scale batcher [PR #1216](https://github.com/3scale/APIcast/pull/1216) [THREESCALE-5753](https://issues.redhat.com/browse/THREESCALE-5753)
- Fixed issues when Auth Caching is disabled [PR #1225](https://github.com/3scale/APIcast/pull/1225) [THREESCALE-4464](https://issues.redhat.com/browse/THREESCALE-4464)
- Fixed issues with service filter and OIDC [PR #1229](https://github.com/3scale/APIcast/pull/1229) [THREESCALE-6042](https://issues.redhat.com/browse/THREESCALE-6042)
- Increased size of dictionaries used by the Batching policy to 20 MB. Users
with many services might have experienced issues with this policy because the
size of those dictionaries was not enough to store everything the policy needs
to function correctly. [PR #1231](https://github.com/3scale/APIcast/pull/1231)
with many services might have experienced issues with this policy because the
size of those dictionaries was not enough to store everything the policy needs
to function correctly. [PR #1231](https://github.com/3scale/APIcast/pull/1231)
- Fixed issue with Camel service over HTTPs when Routing Policy [PR #1230](https://github.com/3scale/APIcast/pull/1230) [THREESCALE-5891](https://issues.redhat.com/browse/THREESCALE-5891)
- Fixed doc issue on SERVICES_FILTER parameter [PR #1233](https://github.com/3scale/APIcast/pull/1233) [THREESCALE-5421](https://issues.redhat.com/browse/THREESCALE-5421)
- Non-alphanumeric metric name in 3scale-batcher policy [PR #1234](https://github.com/3scale/APIcast/pull/1234) [THREESCALE-4913](https://issues.redhat.com/browse/THREESCALE-4913)

## [3.9.1] 2020-10-13

- Fixed issues when using fully qualified DNS query [PR #1235](https://github.com/3scale/APIcast/pull/1235) [THREESCALE-4752](https://issues.redhat.com/browse/THREESCALE-4752)
- Fixed issues with OIDC validation [PR #1239](https://github.com/3scale/APIcast/pull/1239) [THREESCALE-6313](https://issues.redhat.com/browse/THREESCALE-6313)
- Fixed issues with Liquid body size [PR #1240](https://github.com/3scale/APIcast/pull/1240) [THREESCALE-6315](https://issues.redhat.com/browse/THREESCALE-6315)
Expand Down Expand Up @@ -125,7 +129,6 @@ No issues found on beta1,so becames final release.
- Fixed issues with path routing and query args [THREESCALE-5149](https://issues.redhat.com/browse/THREESCALE-5149) [PR #1190](https://github.com/3scale/APIcast/pull/1190)
- Fixed issue with IPCheck policy when forwarder-for value contains port [THREESCALE-5258](https://issues.redhat.com/browse/THREESCALE-5258) [PR #1192](https://github.com/3scale/APIcast/pull/1192)


### Added

- Added upstream Mutual TLS policy [THREESCALE-672](https://issues.jboss.org/browse/THREESCALE-672) [PR #1182](https://github.com/3scale/APIcast/pull/1182)
Expand All @@ -138,23 +141,23 @@ No issues found on beta1,so becames final release.
- New content_caching Prometheus metric [THREESCALE-5439](https://issues.jboss.org/browse/THREESCALE-5439) [PR #1203](https://github.com/3scale/APIcast/pull/1203)
- Added Camel policy [PR #1193](https://github.com/3scale/APIcast/pull/1193) [THREESCALE-4867](https://issues.jboss.org/browse/THREESCALE-4867)


## [3.8.0] - 2020-03-24

`3.8.0-cr1` was considered final and became `3.8.0`.

## [3.8.0-cr1] - 2020-03-07

### Fixed

- Fixed naming issues in policies [THREESCALE-4150](https://issues.jboss.org/browse/THREESCALE-4150) [PR #1167](https://github.com/3scale/APIcast/pull/1167)
- Fixed issues on invalid config in logging policy [THREESCALE-4605](https://issues.jboss.org/browse/THREESCALE-4605) [PR #1168](https://github.com/3scale/APIcast/pull/1168)
- Fixed issues with routing policy and GRPC one [THREESCALE-4684](https://issues.jboss.org/browse/THREESCALE-4684) [PR #1177](https://github.com/3scale/APIcast/pull/1177) [PR #1179](https://github.com/3scale/APIcast/pull/1179)

## [3.8.0-alpha2] - 2020-02-18

### Fixed
- Check status is bigger than zero on caching policy [THREESCALE-4471](https://issues.jboss.org/browse/THREESCALE-4471) [PR #1163](https://github.com/3scale/APIcast/pull/1163)

- Check status is bigger than zero on caching policy [THREESCALE-4471](https://issues.jboss.org/browse/THREESCALE-4471) [PR #1163](https://github.com/3scale/APIcast/pull/1163)

## [3.8.0-alpha1] - 2020-01-31

Expand All @@ -167,7 +170,6 @@ No issues found on beta1,so becames final release.
- Added Request_id on ngx.log function. [THREESCALE-3644](https://issues.jboss.org/browse/THREESCALE-3644) [PR #1156](https://github.com/3scale/APIcast/pull/1156)
- Logging policy add the option to log JWT claims [THREESCALE-4326](https://issues.jboss.org/browse/THREESCALE-4326) [PR #1160](https://github.com/3scale/APIcast/pull/1160)


### Fixed

- When PATH routing was enabled the URL was not correctly escaped [THREESCALE-3468](https://issues.jboss.org/browse/THREESCALE-3468) [PR #1150](https://github.com/3scale/APIcast/pull/1150)
Expand All @@ -176,7 +178,6 @@ No issues found on beta1,so becames final release.
- Fix issues with non-alphanumeric variables in liquid [THREESCALE-3968](https://issues.jboss.org/browse/THREESCALE-3968) [PR #1158](https://github.com/3scale/APIcast/pull/1158)
- Fix issues with double mapping rules [THREESCALE-3950](https://issues.jboss.org/browse/THREESCALE-3950) [PR #1159](https://github.com/3scale/APIcast/pull/1159)


## [3.7.0] - 2019-11-27

`3.7.0-rc2` was considered final and became `3.7.0`.
Expand All @@ -203,7 +204,6 @@ No issues found on beta1,so becames final release.

- Fix issues with escaped characters in URI [THREESCALE-3468](https://issues.jboss.org/browse/THREESCALE-3468) [PR #1123](https://github.com/3scale/APIcast/pull/1123)


## [3.7.0-beta1]- 2019-09-13

### Added
Expand Down Expand Up @@ -243,7 +243,6 @@ No issues found on beta1,so becames final release.

- Extended variables in Liquid template operations [PR #1081](https://github.com/3scale/APIcast/pull/1081), [THREESCALE-2927](https://issues.jboss.org/browse/THREESCALE-2927)


## [3.6.0-beta1] - 2019-06-18

### Added
Expand Down Expand Up @@ -374,11 +373,11 @@ Apart from the changes mentioned in this section, this version also includes the
`3.3.0-cr2` was considered final and became `3.3.0`.

- The configuration schema of the rate-limit policy has changed from `3.2.0` so
if you were using it, please adapt your configuration file accordingly.
if you were using it, please adapt your configuration file accordingly.
- The Native OAuth 2.0 flow is deprecated. Please consider using the OIDC
integration instead.
integration instead.
- The new conditional policy is considered experimental. The way conditions are
expressed might change in future releases.
expressed might change in future releases.

## [3.3.0-cr2] - 2018-09-25

Expand Down Expand Up @@ -670,6 +669,7 @@ expressed might change in future releases.
- Live and ready endpoints now set correct Content-Type header in the response[PR #441](https://github.com/3scale/apicast/pull/441), [THREESCALE-377](https://issues.jboss.org/browse/THREESCALE-377)

## [3.1.0] - 2017-10-27

- 3.1.0-rc2 was considered final and became 3.1.0.

## [3.1.0-rc2] - 2017-09-29
Expand Down Expand Up @@ -794,6 +794,7 @@ expressed might change in future releases.
- Ability to Authenticate against API using RHSSO and OpenID Connect [PR #283](https://github.com/3scale/apicast/pull/283)

### Fixed

- `http_ng` client supports auth passsed in the url, and default client options if the request options are missing for methods with body (POST, PUT, etc.) [PR #310](https://github.com/3scale/apicast/pull/310)
- Fixed lazy configuration loader to recover from failures [PR #313](https://github.com/3scale/apicast/pull/313)
- Fixed undefined variable `p` in post\_action [PR #316](https://github.com/3scale/apicast/pull/316)
Expand All @@ -818,6 +819,7 @@ expressed might change in future releases.
## [3.0.0-beta1] - 2017-03-03

### Changed

- Lazy load DNS resolver to improve performance [PR #251](https://github.com/3scale/apicast/pull/251)
- Execute queries to all defined nameservers in parallel [PR #260](https://github.com/3scale/apicast/pull/260)
- `RESOLVER` ENV variable overrides all other nameservers detected from `/etc/resolv.conf` [PR #260](https://github.com/3scale/apicast/pull/260)
Expand Down Expand Up @@ -849,34 +851,43 @@ expressed might change in future releases.
## [3.0.0-alpha2] - 2017-02-06

### Added

- A way to override backend endpoint [PR #248](https://github.com/3scale/apicast/pull/248)

### Changed

- Cache all calls to `os.getenv` via custom module [PR #231](https://github.com/3scale/apicast/pull/231)
- Bump s2i-openresty to 1.11.2.2-1 [PR #239](https://github.com/3scale/apicast/pull/239)
- Use resty-resolver over nginx resolver for HTTP [PR #237](https://github.com/3scale/apicast/pull/237)
- Use resty-resolver over nginx resolver for Redis [PR #237](https://github.com/3scale/apicast/pull/237)
- Internal change to reduce global state [PR #233](https://github.com/3scale/apicast/pull/233)

### Fixed

- [OAuth] Return correct state value back to client

### Removed

- Nginx resolver directive auto detection. Rely on internal DNS resolver [PR #237](https://github.com/3scale/apicast/pull/237)

## [3.0.0-alpha1] - 2017-01-16

### Added

- A CHANGELOG.md to track important changes
- User-Agent header with APIcast version and system information [PR #214](https://github.com/3scale/apicast/pull/214)
- Try to load configuration from V2 API [PR #193](https://github.com/3scale/apicast/pull/193)

### Changed

- Require openresty 1.11.2 [PR #194](https://github.com/3scale/apicast/pull/194)
- moved development from `v2` branch to `master` [PR #209](https://github.com/3scale/apicast/pull/209)
- `X-3scale-Debug` HTTP header now uses Service Token [PR #217](https://github.com/3scale/apicast/pull/217)

## [2.0.0] - 2016-11-29

### Changed

- Major rewrite using JSON configuration instead of code generation.

[Unreleased]: https://github.com/3scale/apicast/compare/v3.10.0...HEAD
Expand Down
9 changes: 5 additions & 4 deletions gateway/src/apicast/http_proxy.lua
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,7 @@ function _M.resolve(uri)
return ip, port
end

-- #TODO: This local function is no longer called as of PR#1323 and should be removed
local function resolve(uri)
local host = uri.host
local port = uri.port
Expand All @@ -70,12 +71,12 @@ local function resolve(uri)
end

local function absolute_url(uri)
local host, port = resolve(uri)

-- target server requires hostname not IP and DNS resolution is left to the proxy itself as specified in the RFC #7231
-- https://httpwg.org/specs/rfc7231.html#CONNECT
return format('%s://%s:%s%s',
uri.scheme,
host,
port,
uri.host,
uri.port,
uri.path or '/'
)
end
Expand Down
17 changes: 10 additions & 7 deletions gateway/src/resty/http/proxy.lua
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ local function connect_direct(httpc, request)
local uri = request.uri
local host = uri.host
local ip, port = httpc:resolve(host, nil, uri)

-- #TODO: This logic may no longer be needed as of PR#1323 and should be reviewed as part of a refactor
local options = { pool = format('%s:%s', host, port) }
local ok, err = httpc:connect(ip, port or default_port(uri), options)

Expand Down Expand Up @@ -72,8 +72,9 @@ local function _connect_proxy_https(httpc, request, host, port)
end

local function connect_proxy(httpc, request, skip_https_connect)
-- target server requires hostname not IP and DNS resolution is left to the proxy itself as specified in the RFC #7231
-- https://httpwg.org/specs/rfc7231.html#CONNECT
local uri = request.uri
local host, port = httpc:resolve(uri.host, uri.port, uri)
local proxy_uri = request.proxy

if proxy_uri.scheme ~= 'http' then
Expand All @@ -94,15 +95,17 @@ local function connect_proxy(httpc, request, skip_https_connect)
ngx.log(ngx.DEBUG, 'connection to ', proxy_uri.host, ':', proxy_uri.port, ' established',
', pool: ', options.pool, ' reused times: ', httpc:get_reused_times())

ngx.log(ngx.DEBUG, 'targeting server ', uri.host, ':', uri.port)

if uri.scheme == 'http' then
-- http proxy needs absolute URL as the request path
request.path = format('%s://%s:%s%s', uri.scheme, host, port, uri.path or '/')
request.path = format('%s://%s:%s%s', uri.scheme, uri.host, uri.port, uri.path or '/')
return httpc
elseif uri.scheme == 'https' and skip_https_connect then
request.path = format('%s://%s:%s%s', uri.scheme, host, port, request.path or '/')
return _connect_tls_direct(httpc, request, host, port)
request.path = format('%s://%s:%s%s', uri.scheme, uri.host, uri.port, request.path or '/')
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 this will work, but please test this with routing policy, I remember that we had issues with that. Also, with Camel policy

Copy link
Member Author

Choose a reason for hiding this comment

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

I will check with the routing policy yep! However, the camel policy I might need to get some help with because I don't understand how that works and the tests for that specific policy are not passing even without my changes. It seems like something is broken there or maybe it's expected and we need to test in another way?

return _connect_tls_direct(httpc, request, uri.host, uri.port)
elseif uri.scheme == 'https' then
return _connect_proxy_https(httpc, request, host, port)
return _connect_proxy_https(httpc, request, uri.host, uri.port)

else
return nil, 'invalid scheme'
Expand Down Expand Up @@ -176,4 +179,4 @@ end

_M.new = connect

return _M:reset()
return _M:reset()
46 changes: 2 additions & 44 deletions spec/resty/http/proxy_spec.lua
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,55 +50,13 @@ describe('resty.http.proxy', function()
it('connects to the #http_proxy', function()
_M:reset({ http_proxy = 'http://127.0.0.1:1984' })

local request = { url = 'http://127.0.0.1:1984/request', method = 'GET' }
local request = { url = 'http://upstream:8091/request', method = 'GET' }
local proxy = assert(_M.new(request))

local res = assert(proxy:request(request))

assert.same(200, res.status)
assert.match('GET http://127.0.0.1:1984/request HTTP/1.1', res:read_body())
end)

-- Regression test. Ref: https://issues.jboss.org/browse/THREESCALE-2205
context('when different subdomains resolve to the same IP', function()
local request_domain_1 = { url = 'http://test.example.com/', method = 'GET' }
local request_domain_2 = { url = 'http://prod.example.com/', method = 'GET' }

before_each(function()
-- Make everything resolve to the same IP
local resty_resolver = require 'resty.resolver.http'
stub(resty_resolver, 'resolve', function() return "1.1.1.1", 80 end)
end)

context('and it uses a http proxy', function()
before_each(function()
_M:reset({ http_proxy = 'http://127.0.0.1:1984' })
end)

it('does not reuse the connection', function()
local proxy = _M.new(request_domain_1)
proxy:request(request_domain_1):read_body()
proxy:set_keepalive()

proxy = _M.new(request_domain_2)
assert.same(0, proxy:get_reused_times())
end)
end)

context('and it does not use an http proxy', function()
before_each(function()
_M:reset({})
end)

it('does not reuse the connection', function()
local proxy = _M.new(request_domain_1)
proxy:request(request_domain_1):read_body()
proxy:set_keepalive()

proxy = _M.new(request_domain_2)
assert.same(0, proxy:get_reused_times())
end)
end)
assert.match('GET http://upstream:8091/request HTTP/1.1', res:read_body())
end)
end)
end)
Loading