moved context reset "after" request execution: keeps the correct valu…

…e of original_request and takes care of THREESCALE-8252 too.
3scale · Feb 26, 2022 · b7c134f · b7c134f
1 parent 9e61256
commit b7c134f
Show file tree

Hide file tree

Showing 4 changed files with 120 additions and 67 deletions.
diff --git a/gateway/src/apicast/policy/clear_context/clear_context.lua b/gateway/src/apicast/policy/clear_context/clear_context.lua
@@ -2,23 +2,14 @@ local _M = require('apicast.policy').new('Clear Context')
 local new = _M.new
 
 function _M.new(...)
-  ssl_ctx_reset = false
   return new(...)
 end
 
-function _M:ssl_certificate(context)
+function _M:log(context)
+  --resetting the context after every other policy has 
+  --been executed, to avoid reusing any information in
+  --the next request(s).
   reset_context(context)
-  ssl_ctx_reset = true
-end
-
-function _M:rewrite(context)
-  --Do not reset the context again if the ssl_certificate phase
-  --was executed for this request. This way other policies that edited 
-  --the context in their ssl_* phases won't have their changes cleared
-  if not ssl_ctx_reset then
-    reset_context(context)
-  end
-  ssl_ctx_reset = false
 end
 
 function reset_context(context)

diff --git a/gateway/src/apicast/policy_chain.lua b/gateway/src/apicast/policy_chain.lua
@@ -67,11 +67,11 @@ end
 
 
 local DEFAULT_POLICIES = {
-    'apicast.policy.clear_context',
     'apicast.policy.load_configuration',
     'apicast.policy.find_service',
     'apicast.policy.local_chain',
-    'apicast.policy.nginx_metrics'
+    'apicast.policy.nginx_metrics',
+    'apicast.policy.clear_context'
 }
 
 --- Return new policy chain with default policies.

diff --git a/spec/policy/clear_context/clear_context_spec.lua b/spec/policy/clear_context/clear_context_spec.lua
@@ -5,67 +5,18 @@ local ngx_variable = require('apicast.policy.ngx_variable')
 describe('Clear Context policy', function()
   local current_ctx = {some_key = 'some_value'}
 
-  describe('.ssl_certificate + .rewrite', function()
+  describe('log phase context reset', function()
     before_each(function()
       ctx = {}
       stub(ngx_variable, 'available_context', function(context) return context end)
     end)
 
-    context('.ssl_certificate', function()
+    context('.log', function()
       local clear_context_policy = ClearContextPolicy.new()
 
       it('clears the context', function()
         ctx.current = current_ctx
-        clear_context_policy:ssl_certificate(ctx)
-        assert.are.same({}, ctx.current)
-      end)
-    end)
-
-    context('.rewrite', function()
-      local clear_context_policy = ClearContextPolicy.new()
-
-      it('clears the context', function()
-        ctx.current = current_ctx
-        clear_context_policy:rewrite(ctx)
-        assert.are.same({}, ctx.current)
-      end)
-    end)
-
-    context('.ssl_certificate and .rewrite', function()
-      local clear_context_policy = ClearContextPolicy.new()
-
-      it('clears the context once', function()
-        ctx.current = current_ctx
-        clear_context_policy:ssl_certificate(ctx)
-        assert.are.same({}, ctx.current)
-        ctx.current = current_ctx
-        clear_context_policy:rewrite(ctx)
-        assert.are.same(current_ctx, ctx.current)
-      end)
-    end)
-
-    context('.ssl_certificate and .ssl_certificate', function()
-      local clear_context_policy = ClearContextPolicy.new()
-
-      it('clears the context twice', function()
-        ctx.current = current_ctx
-        clear_context_policy:ssl_certificate(ctx)
-        assert.are.same({}, ctx.current)
-        ctx.current = current_ctx
-        clear_context_policy:ssl_certificate(ctx)
-        assert.are.same({}, ctx.current)
-      end)
-    end)
-
-    context('.rewrite and .rewrite', function()
-      local clear_context_policy = ClearContextPolicy.new()
-
-      it('clears the context twice', function()
-        ctx.current = current_ctx
-        clear_context_policy:rewrite(ctx)
-        assert.are.same({}, ctx.current)
-        ctx.current = current_ctx
-        clear_context_policy:rewrite(ctx)
+        clear_context_policy:log(ctx)
         assert.are.same({}, ctx.current)
       end)
     end)

diff --git a/t/apicast-policy-logging.t b/t/apicast-policy-logging.t
@@ -7,6 +7,7 @@ our $public_key = `cat t/fixtures/rsa.pub`;
 # Test::Nginx does not allow to grep access logs, so we redirect them to
 # stderr to be able to use "grep_error_log" by setting APICAST_ACCESS_LOG_FILE
 $ENV{APICAST_ACCESS_LOG_FILE} = "$Test::Nginx::Util::ErrLogFile";
+$ENV{APICAST_HTTPS_RANDOM_PORT} = Test::APIcast::get_random_port();
 check_accum_error_log();
 run_tests();
 
@@ -643,3 +644,113 @@ OK
 --- error_code: 200
 --- no_error_log eval
 [qr/^Status\:\:200 USER_KEY\:\:123 42/]
+
+=== TEST 15: original_request contains the right information for requests after the first
+when the same TCP connection is reused (THREESCALE-8252)
+--- env eval
+(
+    'APICAST_HTTPS_PORT' => $ENV{APICAST_HTTPS_RANDOM_PORT},
+    'HTTP_KEEPALIVE_TIMEOUT' => '5',
+    'APICAST_PATH_ROUTING' => '1'
+)
+--- backend
+location /transactions/authrep.xml {
+  content_by_lua_block {
+    ngx.exit(200)
+  }
+}
+--- configuration
+{
+  "services": [
+    {
+      "id": 42,
+      "backend_version": 1,
+      "proxy": {
+        "api_backend": "http://test:$TEST_NGINX_SERVER_PORT/api-backend/foo/",
+        "hosts": ["127.0.0.1"],
+        "policy_chain": [
+          {
+            "name": "apicast.policy.apicast"
+          },
+          {
+            "name": "apicast.policy.logging",
+            "configuration": {
+              "custom_logging": "Status::{{ status }} {{service.id}} {{original_request.path}}",
+              "enable_json_logs": false
+            }
+          }
+        ],
+        "proxy_rules": [
+          {
+            "pattern": "/one",
+            "http_method": "GET",
+            "metric_system_name": "hits",
+            "delta": 1
+          }
+        ]
+      }
+    },
+    {
+      "id": 21,
+      "backend_version": 1,
+      "proxy": {
+        "api_backend": "http://test:$TEST_NGINX_SERVER_PORT/api-backend/bar/",
+        "hosts": ["127.0.0.1"],
+        "policy_chain": [
+          {
+            "name": "apicast.policy.apicast"
+          },
+          {
+            "name": "apicast.policy.logging",
+            "configuration": {
+              "custom_logging": "Status::{{ status }} {{service.id}} {{original_request.path}}",
+              "enable_json_logs": false
+            }
+          }
+        ],
+        "proxy_rules": [
+          {
+            "pattern": "/two",
+            "http_method": "GET",
+            "metric_system_name": "hits",
+            "delta": 1
+          }
+        ]
+      }
+    }
+  ]
+}
+--- upstream
+location ~ /api-backend(/.+) {
+  echo 'yay, api backend: $1';
+}
+--- test
+content_by_lua_block {
+  local http = require "resty.http"
+  local resty_env = require 'resty.env'
+
+  local https_port = resty_env.get('APICAST_HTTPS_PORT')
+  local uri_one = "https://127.0.0.1:".. https_port .."/one?user_key=foo"
+  local uri_two = "https://127.0.0.1:".. https_port .."/two?user_key=foo"
+
+  local httpc = http.new()
+  local res1, err1 = httpc:request_uri(uri_one, {
+    method = "GET",
+    ssl_verify = false,
+    version = 1.1
+  })
+  assert(res1, "Request failed"..(err1 or ""))
+
+  local res2, err2 = httpc:request_uri(uri_two, {
+    method = "GET",
+    ssl_verify = false, 
+    version = 1.1
+  })
+  assert(res2, "Request failed"..(err2 or ""))
+  httpc:close()
+}
+--- no_error_log
+[error]
+--- error_log eval
+[qr/^Status\:\:200 42 \/one/, qr/^Status\:\:200 21 \/two/]
+