[retry] Move context export to rewrite phase

3scale · Jul 25, 2024 · 8946c81 · 8946c81
1 parent b8748a7
commit 8946c81
Show file tree

Hide file tree

Showing 4 changed files with 157 additions and 5 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -17,6 +17,8 @@ and this project adheres to [Semantic Versioning](http://semver.org/).
 
 - Fixed confusing log display when APIcast listens on HTTPS and path routing is enabled [PR #1486](https://github.com/3scale/APIcast/pull/1486/files) [THREESCALE #8486](https://issues.redhat.com/browse/THREESCALE-8486)
 
+- Fixed Conditional policy evaluating incorrectly: second policy in policy chain that implement export() always triggers [PR #1485](https://github.com/3scale/APIcast/pull/1485) [THREESCALE-9320](https://issues.redhat.com/browse/THREESCALE-9320)
+
 ### Added
 
 - Bump openresty to 1.21.4.3 [PR #1461](https://github.com/3scale/APIcast/pull/1461) [THREESCALE-10601](https://issues.redhat.com/browse/THREESCALE-10601)

diff --git a/gateway/src/apicast/policy/retry/retry.lua b/gateway/src/apicast/policy/retry/retry.lua
@@ -13,8 +13,8 @@ function _M.new(config)
   return self
 end
 
-function _M:export()
-  return { upstream_retries = self.retries }
+function _M:rewrite(context)
+  context.upstream_retries = self.retries
 end
 
 return _M
diff --git a/spec/policy/retry/retry_spec.lua b/spec/policy/retry/retry_spec.lua
@@ -5,10 +5,11 @@ describe('Retry policy', function()
     it('returns the the number of retries', function()
       local policy_config = { retries = 5, }
       local policy = RetryPolicy.new(policy_config)
+      local context = {}
 
-      local exported = policy:export()
+      policy:rewrite(context)
 
-      assert.same(policy_config.retries, exported.upstream_retries)
+      assert.same(policy_config.retries, context.upstream_retries)
     end)
   end)
 end)
diff --git a/t/apicast-policy-retry.t b/t/apicast-policy-retry.t
@@ -456,4 +456,153 @@ retried.
 GET /
 --- error_code: 504
 --- error_log
-upstream timed out
+upstream timed out
+
+
+
+=== TEST 9: works also with the conditional policy
+--- configuration
+{
+  "services": [
+    {
+      "id": 42,
+      "backend_version": 1,
+      "backend_authentication_type": "service_token",
+      "backend_authentication_value": "token-value",
+      "proxy": {
+        "policy_chain": [
+          {
+            "name": "apicast.policy.conditional",
+            "configuration": {
+              "condition": {
+                "operations": [
+                  {
+                    "left": "{{ uri }}",
+                    "left_type": "liquid",
+                    "op": "==",
+                    "right": "/test",
+                    "right_type": "plain"
+                  }
+                ]
+              },
+              "policy_chain": [
+                {
+                  "name": "apicast.policy.retry",
+                  "configuration": {
+                    "retries": 5
+                  }
+                }
+              ]
+            }
+          },
+          {
+            "name": "apicast.policy.apicast"
+          }
+        ],
+        "api_backend": "http://test:$TEST_NGINX_SERVER_PORT/",
+        "proxy_rules": [
+          { "pattern": "/", "http_method": "GET", "metric_system_name": "hits", "delta": 2 }
+        ]
+      }
+    }
+  ]
+}
+--- backend
+  location /transactions/authrep.xml {
+    content_by_lua_block {
+      ngx.exit(200)
+    }
+  }
+--- upstream
+ location /test {
+    content_by_lua_block {
+     local test_counter = ngx.shared.test_counter or 1
+     if test_counter <= 2 then
+       ngx.shared.test_counter = test_counter + 1
+       ngx.exit(503)
+     else
+       ngx.say('yay, api backend');
+     end
+    }
+ }
+--- request
+GET /test?user_key=foo
+--- response_body
+yay, api backend
+--- error_code: 200
+--- no_error_log
+[error]
+
+
+
+=== TEST 10: should not retry when inside conditional policy and with fail condition
+--- ONLY
+--- configuration
+{
+  "services": [
+    {
+      "id": 42,
+      "backend_version": 1,
+      "backend_authentication_type": "service_token",
+      "backend_authentication_value": "token-value",
+      "proxy": {
+        "policy_chain": [
+          {
+            "name": "apicast.policy.conditional",
+            "configuration": {
+              "condition": {
+                "operations": [
+                  {
+                    "left": "{{ uri }}",
+                    "left_type": "liquid",
+                    "op": "==",
+                    "right": "/invalid",
+                    "right_type": "plain"
+                  }
+                ]
+              },
+              "policy_chain": [
+                {
+                  "name": "apicast.policy.retry",
+                  "configuration": {
+                    "retries": 5
+                  }
+                }
+              ]
+            }
+          },
+          {
+            "name": "apicast.policy.apicast"
+          }
+        ],
+        "api_backend": "http://test:$TEST_NGINX_SERVER_PORT/",
+        "proxy_rules": [
+          { "pattern": "/", "http_method": "GET", "metric_system_name": "hits", "delta": 2 }
+        ]
+      }
+    }
+  ]
+}
+--- backend
+  location /transactions/authrep.xml {
+    content_by_lua_block {
+      ngx.exit(200)
+    }
+  }
+--- upstream
+ location /test {
+    content_by_lua_block {
+     local test_counter = ngx.shared.test_counter or 1
+     if test_counter <= 2 then
+       ngx.shared.test_counter = test_counter + 1
+       ngx.exit(503)
+     else
+       ngx.say('yay, api backend');
+     end
+    }
+ }
+--- request
+GET /test?user_key=foo
+--- error_code: 503
+--- no_error_log
+[error]