Policy: HTTP proxy #1080

eloycoto · 2019-06-28T14:16:18Z

This policy adds the option to set a service to use a http proxy (As is
in use for HTTP_PROXY parameter)

This service will allow Apicast users to send the traffic over a proxy
and do request modifications in 3-party proxies.

Traffic flow:

	   ,-.
	   `-'
	   /|\
		|            ,-------.          ,---------.          ,----------.
	   / \           |Apicast|          |HTTPPROXY|          |APIBackend|
	  User           `---+---'          `----+----'          `----------'
	   |  GET /resource  |                   |                    |
	   | --------------->|                   |                    |
	   |                 |                   |                    |
	   |                 |  Get /resource    |                    |
	   |                 |------------------>|                    |
	   |                 |                   |                    |
	   |                 |                   |  Get /resource/    |
	   |                 |                   | - - - - - - - - - >|
	   |                 |                   |                    |
	   |                 |                   |     response       |
	   |                 |                   |<- - - - - - - - - -|
	   |                 |                   |                    |
	   |                 |     response      |                    |
	   |                 |<------------------|                    |
	   |                 |                   |                    |
	   |                 |                   |                    |
	   | <---------------|                   |                    |
	  User           ,---+---.          ,----+----.          ,----------.
	   ,-.           |Apicast|          |HTTPPROXY|          |APIBackend|
	   `-'           `-------'          `---------'          `----------'
	   /|\
		|
	   / \

Configuration:

{
  "services": [
	{
	  "id": 200,
	  "backend_version":  1,
	  "backend_authentication_type": "service_token",
	  "backend_authentication_value": "token-value",
	  "proxy": {
		"hosts": [
		  "one"
		],
		"api_backend": "https://echo-api.3scale.net/",
		"proxy_rules": [
		  {
			"pattern": "/",
			"http_method": "GET",
			"metric_system_name": "hits",
			"delta": 1
		  }
		],
		"policy_chain": [
		  {
			"name": "apicast.policy.apicast"
		  },
		  {
			"name": "apicast.policy.http_proxy",
			"configuration": {
				"all_proxy": "http://192.168.15.103:8888/",
				"https_proxy": "https://192.168.15.103:8888/",
				"http_proxy": "https://192.168.15.103:8888/"
			}
		  }
		]
	  }
	}
  ]
}

davidor · 2019-07-24T13:36:26Z

@eloycoto The commit with title Policy: Add http_proxy integration test appears as empty here, and I can't see any integration tests in other commits, so I guess something went wrong.

mikz · 2019-07-24T13:38:16Z

Also, the feature + tests should be in the same commit anyway :)

davidor · 2019-07-24T12:56:30Z

gateway/src/apicast/policy/http_proxy/Readme.md

+      "name": "apicast.policy.apicast"
+    },
+    {
+      "name": "apicast.policy.http_proxy",


I think it's important to clarify that this only applies to the API backend and not the 3scale backend (apisonator).

davidor · 2019-07-24T12:57:20Z

gateway/src/apicast/policy/http_proxy/Readme.md

+and terminated, traffic for this policy will be: 
+
+```
+@startuml


This does not look good when I open the file with the View file option in GitHub.

davidor · 2019-07-24T12:58:14Z

gateway/src/apicast/policy/http_proxy/Readme.md

+# HTTP proxy policy
+
+This policy allows users to define a HTTP proxy where the traffic will be send
+and terminated, traffic for this policy will be: 


What do you mean by terminated here?

davidor · 2019-07-24T13:01:13Z

gateway/src/apicast/policy/http_proxy/Readme.md

+
+## Caveats
+
+- This policy will disable all load-balancing policies and traffic will be


Asking just in case. Does the order in the chain matter?

davidor · 2019-07-24T13:32:33Z

gateway/src/apicast/policy/http_proxy/apicast-policy.json

+          "type": "string"
+        },
+        "https_proxy": {
+          "description": "Defines a HTTP proxy to be used for connecting to HTTP services. Authentication is not supported",


Should be HTTPS instead of HTTP.

davidor · 2019-07-24T16:02:42Z

gateway/src/apicast/policy/http_proxy/proxy.lua

+    end
+  end
+
+  for _, proto in ipairs(proxies) do


local ipairs = ipairs ?

davidor · 2019-07-24T16:04:57Z

gateway/src/resty/http/proxy.lua

@@ -108,6 +108,12 @@ local function find_proxy_url(request)
    local uri = parse_request_uri(request)
    if not uri then return end

+    -- request can have a local proxy defined and env variables have lower
+    -- priority, if the proxy is defined in the request that will be used.
+    if request.proxy_uri then


This could be simplified:

return request.proxy_uri or _M.find(uri)

davidor · 2019-07-24T16:18:11Z

spec/policy/http_proxy/http_proxy_spec.lua

+    local proxy = proxy_policy.new({
+      all_proxy = all_proxy_val
+    })
+    assert.same(proxy:find_proxy("http"), resty_url.parse(all_proxy_val))


In all these tests, I think we should check what export returns instead of find_proxy, because export is what should be exposed to other modules.

davidor · 2019-07-24T16:25:29Z

t/apicast-policy-http-proxy.t

+using proxy: $TEST_NGINX_HTTP_PROXY
+
+=== TEST 3: using HTTPS proxy for backend
+--- ONLY


davidor · 2019-07-24T16:29:57Z

t/apicast-policy-http-proxy.t

+location /transactions/authrep.xml {
+  access_by_lua_block {
+    assert = require('luassert')
+    assert.equal('https', ngx.var.scheme)


Shouldn't all this be in upstream instead of backend ?

davidor · 2019-07-26T14:02:53Z

gateway/src/apicast/policy/http_proxy/Readme.md

+
+An example HTTP proxy transformation can be the following project; this project
+transforms the response given by and endpoint and converts the body to
+uppercaint and convert the body to uppercase.


This sentence does not sound right.

davidor · 2019-07-26T14:09:44Z

CHANGELOG.md

@@ -10,6 +10,7 @@ and this project adheres to [Semantic Versioning](http://semver.org/).

 - Introduce possibility of specifying policy order restrictions in their schemas. APIcast now shows a warning when those restrictions are not respected [#1088](https://github.com/3scale/APIcast/pull/1088), [THREESCALE-2896](https://issues.jboss.org/browse/THREESCALE-2896)
 - Added new parameters to logging policy to allow custom access log [PR #1081](https://github.com/3scale/APIcast/pull/1089) [THREESCALE-1234](https://issues.jboss.org/browse/THREESCALE-1234)[THREESCALE-2876](https://issues.jboss.org/browse/THREESCALE-2876)
+- Added http_proxy policy to use an HTTP proxy in api_backed calls. [THREESCALE-2696](https://issues.jboss.org/browse/THREESCALE-2696)


Would be good to add the link to this PR as well.

davidor · 2019-07-26T14:12:59Z

gateway/src/apicast/policy/http_proxy/proxy.lua

@@ -0,0 +1,48 @@
+local policy = require('apicast.policy')
+local _M = policy.new('http_proxy')


Can you please add the version? It's needed in order to make the feature of checking the order of policies work.
I needed to add the version in all the policies here: https://github.com/3scale/APIcast/pull/1088/files
In the future we might want to see if there's a better way to do this.

davidor · 2019-07-26T14:24:48Z

gateway/src/apicast/policy/http_proxy/proxy.lua

+
+function _M:export()
+  return  {
+    get_http_proxy = function(_, uri)


I don't see why this function needs the first param.
Can't we get rid of it and then just call context.get_http_proxy(self.uri) from the upstream module instead of context:get_http_proxy(self.uri) ?

This policy adds the option to set a service to use a http proxy (As is in use for HTTP_PROXY parameter) This service will allow Apicast users to send the traffic over a proxy and do request modifications in 3-party proxies. Traffic flow: ``` ,-. `-' /|\ | ,-------. ,---------. ,----------. / \ |Apicast| |HTTPPROXY| |APIBackend| User `---+---' `----+----' `----------' | GET /resource | | | | --------------->| | | | | | | | | Get /resource | | | |------------------>| | | | | | | | | Get /resource/ | | | | - - - - - - - - - >| | | | | | | | response | | | |<- - - - - - - - - -| | | | | | | response | | | |<------------------| | | | | | | | | | | <---------------| | | User ,---+---. ,----+----. ,----------. ,-. |Apicast| |HTTPPROXY| |APIBackend| `-' `-------' `---------' `----------' /|\ | / \ ``` Configuration: ``` { "services": [ { "id": 200, "backend_version": 1, "backend_authentication_type": "service_token", "backend_authentication_value": "token-value", "proxy": { "hosts": [ "one" ], "api_backend": "https://echo-api.3scale.net/", "proxy_rules": [ { "pattern": "/", "http_method": "GET", "metric_system_name": "hits", "delta": 1 } ], "policy_chain": [ { "name": "apicast.policy.apicast" }, { "name": "apicast.policy.http_proxy", "configuration": { "all_proxy": "http://192.168.15.103:8888/", "https_proxy": "https://192.168.15.103:8888/", "http_proxy": "https://192.168.15.103:8888/" } } ] } } ] } ``` Signed-off-by: Eloy Coto <[email protected]>

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

This commit adds the integration test for the http proxy policy. Signed-off-by: Eloy Coto <[email protected]>

Add readme for http_proxy configuration. Signed-off-by: Eloy Coto <[email protected]>

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

eloycoto changed the title ~~Policy: HTTP proxy~~ WIP: Policy: HTTP proxy Jun 28, 2019

eloycoto force-pushed the HTTPProxyPolicy branch 3 times, most recently from 4d35291 to 5a64c3b Compare June 28, 2019 14:53

eloycoto force-pushed the HTTPProxyPolicy branch 3 times, most recently from c6e5abb to e8ae68a Compare July 22, 2019 09:22

eloycoto changed the title ~~WIP: Policy: HTTP proxy~~ Policy: HTTP proxy Jul 22, 2019

eloycoto requested a review from davidor July 22, 2019 09:43

eloycoto marked this pull request as ready for review July 22, 2019 15:03

eloycoto requested a review from a team as a code owner July 22, 2019 15:03

eloycoto force-pushed the HTTPProxyPolicy branch from 3fed5d5 to 84da86b Compare July 24, 2019 13:46

davidor reviewed Jul 24, 2019

View reviewed changes

eloycoto force-pushed the HTTPProxyPolicy branch 4 times, most recently from 8fa98a0 to c947504 Compare July 26, 2019 12:50

eloycoto requested a review from davidor July 26, 2019 13:04

davidor reviewed Jul 26, 2019

View reviewed changes

eloycoto added 6 commits July 26, 2019 16:41

Policy: added JsonSchema for proxy

1a1d15a

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

Policy: Add proxy policy unit test

0a44919

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

Policy: Add integration test for http-proxy test

45f0d02

This commit adds the integration test for the http proxy policy. Signed-off-by: Eloy Coto <[email protected]>

Policy: Proxy add readme

aff0477

Add readme for http_proxy configuration. Signed-off-by: Eloy Coto <[email protected]>

Update Changelog

4515570

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

eloycoto force-pushed the HTTPProxyPolicy branch from c947504 to 4515570 Compare July 26, 2019 15:05

eloycoto requested a review from davidor July 26, 2019 15:12

davidor approved these changes Jul 26, 2019

View reviewed changes

davidor merged commit 9ef6b4c into 3scale:master Jul 26, 2019

eloycoto mentioned this pull request Aug 20, 2019

Investigate delegation of upstream call via http proxy #1016

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Policy: HTTP proxy #1080

Policy: HTTP proxy #1080

eloycoto commented Jun 28, 2019

davidor commented Jul 24, 2019

mikz commented Jul 24, 2019

davidor Jul 24, 2019

davidor Jul 24, 2019

davidor Jul 24, 2019

davidor Jul 24, 2019

davidor Jul 24, 2019

davidor Jul 24, 2019

davidor Jul 24, 2019

davidor Jul 24, 2019

davidor Jul 24, 2019

davidor Jul 24, 2019

davidor Jul 26, 2019

davidor Jul 26, 2019

davidor Jul 26, 2019

davidor Jul 26, 2019


		## Caveats

		- This policy will disable all load-balancing policies and traffic will be

		@@ -0,0 +1,48 @@
		local policy = require('apicast.policy')
		local _M = policy.new('http_proxy')

Policy: HTTP proxy #1080

Policy: HTTP proxy #1080

Conversation

eloycoto commented Jun 28, 2019

davidor commented Jul 24, 2019

mikz commented Jul 24, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment