Kong / kong

🦍 The Cloud-Native API Gateway and AI Gateway.
https://konghq.com/install/#kong-community
Apache License 2.0
38.81k stars 4.77k forks source link

Custom Plugin in Response Phase has sideeffect to if-match/etag handling #13430

Open TGeb-EV opened 1 month ago

TGeb-EV commented 1 month ago

Is there an existing issue for this?

Kong version ($ kong version)

3.7.1

Current Behavior

A custom plugin in response phase has sideeffect to if-match/etag handling. A global configured plugin (enabled or not) leads to 412 responses if Requests with "If-Match) header are incoming

Expected Behavior

The if-match header should be ignored, with and without the configured plugin. Without everything works as intended

Steps To Reproduce

  1. Create a Test Plugin for an empty kong with default config

`local TestPlugin = { PRIORITY = 850, VERSION = '0.0.1' }

function TestPlugin:response(conf)

end

return TestPlugin `

  1. Create a global plugin configuration for this plugin
  2. configure some service connected directly to host (no Kong upstream/LB functionality)
  3. make a request with a "if-match" header to the service trough kong
  4. the response is a 412
  5. if the plugin is configured and deactivated the response is 412
  6. if the plugin configuration is deleted everything works as intended
  7. disabling proxy caching changes nothing

Anything else?

No response

xianghai2 commented 1 month ago

Hi @TGeb-EV ,

Could you provide more details about your custom plugin? thank you.

And I could not reproduce this, it looks like that Kong 3.7.1 code base works smoothly for my custom plugin. I did following things:

============================ My custom plugin:

haix@kong:~/workspace/371$ git status On branch release/3.7.1 Your branch is up to date with 'origin/release/3.7.1'.

Changes not staged for commit: (use "git add ..." to update what will be committed) (use "git restore ..." to discard changes in working directory) modified: kong.conf.default

Untracked files: (use "git add ..." to include in what will be committed) kong/plugins/my-plugin/

no changes added to commit (use "git add" and/or "git commit -a")

diff --git a/kong.conf.default b/kong.conf.default index fac253492..2aa817864 100644 --- a/kong.conf.default +++ b/kong.conf.default @@ -138,7 +138,7 @@ #

Example: 0.25, this should account for 25% of all traces.

- +plugins = bundled,my-plugin

plugins = bundled # Comma-separated list of plugins this node

                              # should load. By default, only plugins
                              # bundled in official distributions are

haix@kong:~/workspace/371$ ls -l kong/plugins/my-plugin/ total 8 -rw-rw-r-- 1 haix haix 578 Jul 29 17:17 handler.lua -rw-rw-r-- 1 haix haix 209 Jul 29 17:17 schema.lua

haix@kong:~/workspace/371$ cat kong/plugins/my-plugin/handler.lua local MyPluginHandler = { PRIORITY = 1000, VERSION = "0.0.1", }

function MyPluginHandler:response(config) -- Implement logic for the access phase here (http) kong.response.set_header("X-MyPlugin", "response") end

function MyPluginHandler:access(config) -- Implement logic for the access phase here (http) kong.log("MyPlugin: access") end

-- function MyPluginHandler:header_filter(config) -- Implement logic for the header_filter phase here (http) -- kong.log("MyPlugin header_filter: " .. kong.response.get_header("Server")) -- end

return MyPluginHandler

haix@kong:~/workspace/371$ cat kong/plugins/my-plugin/schema.lua local PLUGIN_NAME = "my-plugin"

local schema = { name = PLUGIN_NAME, fields = { { config = { type = "record", fields = { }, }, }, }, }

return schema

================================== Start upstream web server listening on port 8888

build nginx from source and install: auto/configure --with-http_dav_module --prefix="/home/haix/oss/ngx/build/test" make make install cd build/test ./sbin/nginx -c ./conf/nginx.conf

check and verify ETag:

haix@kong:~$ curl -i localhost:8888 HTTP/1.1 200 OK Server: nginx/1.27.0 Date: Mon, 29 Jul 2024 08:47:54 GMT Content-Type: text/html Content-Length: 615 Last-Modified: Mon, 29 Jul 2024 08:44:30 GMT Connection: keep-alive ETag: "66a7566e-267" Accept-Ranges: bytes

<!DOCTYPE html>
<html>
<head>
<title>Welcome to nginx!</title>
<style>
html { color-scheme: light dark; }
body { width: 35em; margin: 0 auto;
font-family: Tahoma, Verdana, Arial, sans-serif; }
</style>
</head>
<body>
<h1>Welcome to nginx!</h1>
<p>If you see this page, the nginx web server is successfully installed and
working. Further configuration is required.</p>

<p>For online documentation and support please refer to
<a href="http://nginx.org/">nginx.org</a>.<br/>
Commercial support is available at
<a href="http://nginx.com/">nginx.com</a>.</p>

<p><em>Thank you for using nginx.</em></p>
</body>
</html>

haix@kong:~$

haix@kong:~$ curl --header 'If-Match: "66a7566e-267"' -i localhost:8888 HTTP/1.1 200 OK Server: nginx/1.27.0 Date: Mon, 29 Jul 2024 08:51:54 GMT Content-Type: text/html Content-Length: 615 Last-Modified: Mon, 29 Jul 2024 08:44:30 GMT Connection: keep-alive ETag: "66a7566e-267" Accept-Ranges: bytes

<!DOCTYPE html>
<html>
<head>
<title>Welcome to nginx!</title>
<style>
html { color-scheme: light dark; }
body { width: 35em; margin: 0 auto;
font-family: Tahoma, Verdana, Arial, sans-serif; }
</style>
</head>
<body>
<h1>Welcome to nginx!</h1>
<p>If you see this page, the nginx web server is successfully installed and
working. Further configuration is required.</p>

<p>For online documentation and support please refer to
<a href="http://nginx.org/">nginx.org</a>.<br/>
Commercial support is available at
<a href="http://nginx.com/">nginx.com</a>.</p>

<p><em>Thank you for using nginx.</em></p>
</body>
</html>

haix@kong:~$ curl --header 'If-None-Match: "66a7566e-267"' -i localhost:8888 HTTP/1.1 304 Not Modified Server: nginx/1.27.0 Date: Mon, 29 Jul 2024 08:52:13 GMT Last-Modified: Mon, 29 Jul 2024 08:44:30 GMT Connection: keep-alive ETag: "66a7566e-267"

haix@kong:~$

=============================== start kong with my custom plugin:

(kong-dev) haix@kong:~/workspace/371$ kong version 3.7.1

kong start -c ./kong.conf.default

curl -s localhost:8001 | jq '.plugins.available_on_server."my-plugin"'

curl -i -s -X POST http://localhost:8001/services \ --data name=example_service \ --data url='http://localhost:8888'

curl -is -X POST http://localhost:8001/services/example_service/plugins \ --data 'name=my-plugin'

curl -i -X POST http://localhost:8001/services/example_service/routes \ --data 'paths[]=/mock' \ --data name=example_route

================================= Verify "If-Match" header request with Kong

(kong-dev) haix@kong:~/workspace/371$ curl --header 'If-Match: "66a7566e-267"' -i http://localhost:8000/mock HTTP/1.1 200 OK Content-Type: text/html; charset=UTF-8 Connection: keep-alive Server: nginx/1.27.0 Date: Mon, 29 Jul 2024 09:38:04 GMT Last-Modified: Mon, 29 Jul 2024 08:44:30 GMT ETag: "66a7566e-267" Accept-Ranges: bytes Content-Length: 615 X-MyPlugin: response X-Kong-Upstream-Latency: 0 X-Kong-Proxy-Latency: 1 Via: kong/3.7.1 X-Kong-Request-Id: bc9e504b7785af61a9bf456b8c7cfd5e

<!DOCTYPE html>
<html>
<head>
<title>Welcome to nginx!</title>
<style>
html { color-scheme: light dark; }
body { width: 35em; margin: 0 auto;
font-family: Tahoma, Verdana, Arial, sans-serif; }
</style>
</head>
<body>
<h1>Welcome to nginx!</h1>
<p>If you see this page, the nginx web server is successfully installed and
working. Further configuration is required.</p>

<p>For online documentation and support please refer to
<a href="http://nginx.org/">nginx.org</a>.<br/>
Commercial support is available at
<a href="http://nginx.com/">nginx.com</a>.</p>

<p><em>Thank you for using nginx.</em></p>
</body>
</html>

(kong-dev) haix@kong:~/workspace/371$ curl --header 'If-None-Match: "66a7566e-267"' -i http://localhost:8000/mock HTTP/1.1 304 Not Modified Connection: keep-alive Server: nginx/1.27.0 Date: Mon, 29 Jul 2024 09:38:14 GMT ETag: "66a7566e-267" Last-Modified: Mon, 29 Jul 2024 08:44:30 GMT X-MyPlugin: response X-Kong-Upstream-Latency: 1 X-Kong-Proxy-Latency: 2 Via: kong/3.7.1 X-Kong-Request-Id: 15b7476e7978010c2f210616bb1fc42c

(kong-dev) haix@kong:~/workspace/371$

TGeb-EV commented 1 month ago

This case ist the case if you only use etag for simple caching. There is another use-case to use this also for optimistic locking. You want to update a resource by a put with is given etag (if-macth content), and if the etag matches you will get the updated resource with a new etag. so your test service should return a different etag as the put request., That does not work in my setup (similar to yours, but with a express service to exclude nginx caching behavior interferences). With the new Etag, the updated resource could be requested via GET (if it changed using the if-match header)

TGeb-EV commented 1 month ago

Update - The Plugin configuration have to be activated, but the bug ist with activated plugin contantly there.

xianghai2 commented 1 month ago

Hi @TGeb-EV , Thanks for more information.

And still I could not reproduce it with PUT to update the resource, what I did as following:

1> Update the resource with a new file via PUT method

(kong-dev) haix@kong:~/workspace/371$ curl -X PUT -T "/home/haix/workspace/371/index.html" --header 'If-Match: "66a7566e-267"' -i http://localhost:8000/mock/index.html HTTP/1.1 204 No Content Connection: keep-alive Server: nginx/1.27.0 Date: Tue, 30 Jul 2024 02:12:33 GMT X-MyPlugin: response X-Kong-Upstream-Latency: 2 X-Kong-Proxy-Latency: 2 Via: kong/3.7.1 X-Kong-Request-Id: 742322180106e05138184b3ca242e64b

2> Check the ETag value for the updated resource

(kong-dev) haix@kong:~/workspace/371$ curl -i http://localhost:8000/mock HTTP/1.1 200 OK Content-Type: text/html; charset=UTF-8 Connection: keep-alive Date: Tue, 30 Jul 2024 02:13:27 GMT Last-Modified: Tue, 30 Jul 2024 02:12:33 GMT ETag: "66a84c11-101" Accept-Ranges: bytes Server: nginx/1.27.0 Content-Length: 257 X-MyPlugin: response X-Kong-Upstream-Latency: 1 X-Kong-Proxy-Latency: 0 Via: kong/3.7.1 X-Kong-Request-Id: bdbd244a5133b45e52959e2488d38643

<!DOCTYPE html>
<html>
<head>
<title>Welcome to nginx!</title>
<style>
html { color-scheme: light dark; }
body { width: 35em; margin: 0 auto;
font-family: Tahoma, Verdana, Arial, sans-serif; }
</style>
</head>
<body>
<h1>Hello World!</h1>

</body>
</html>

(kong-dev) haix@kong:~/workspace/371$

3> Get the updated resource with header "If-Match"

(kong-dev) haix@kong:~/workspace/371$ curl --header 'If-Match: "66a84c11-101"' -i http://localhost:8000/mock HTTP/1.1 200 OK Content-Type: text/html; charset=UTF-8 Connection: keep-alive Date: Tue, 30 Jul 2024 02:14:13 GMT Last-Modified: Tue, 30 Jul 2024 02:12:33 GMT ETag: "66a84c11-101" Accept-Ranges: bytes Server: nginx/1.27.0 Content-Length: 257 X-MyPlugin: response X-Kong-Upstream-Latency: 0 X-Kong-Proxy-Latency: 0 Via: kong/3.7.1 X-Kong-Request-Id: 48e4185fcbc56a30b88f9b352bc57876

<!DOCTYPE html>
<html>
<head>
<title>Welcome to nginx!</title>
<style>
html { color-scheme: light dark; }
body { width: 35em; margin: 0 auto;
font-family: Tahoma, Verdana, Arial, sans-serif; }
</style>
</head>
<body>
<h1>Hello World!</h1>

</body>
</html>

(kong-dev) haix@kong:~/workspace/371$

4> Get the updated resource with header "If-None-Match"

(kong-dev) haix@kong:~/workspace/371$ curl --header 'If-None-Match: "66a84c11-101"' -i http://localhost:8000/mock HTTP/1.1 304 Not Modified Connection: keep-alive Date: Tue, 30 Jul 2024 02:14:51 GMT Last-Modified: Tue, 30 Jul 2024 02:12:33 GMT ETag: "66a84c11-101" Server: nginx/1.27.0 X-MyPlugin: response X-Kong-Upstream-Latency: 1 X-Kong-Proxy-Latency: 1 Via: kong/3.7.1 X-Kong-Request-Id: 8a1ba373b1265a0bf912cb7c177fd348

(kong-dev) haix@kong:~/workspace/371$

TGeb-EV commented 1 month ago

Below you see the broken behavior. Without the custom plugin configured (plugin in response phase) everything works fine. This implementation maybe unusual but should HTTP Standard conform. It (if it works) prevents redundant GET calls. And a hint for the nginx service in your setup. You may also have to disable caching, on openresty you would have to disable proxy caching to get another repsonse than 412 from the service. mermaid-diagram-2024-07-30-112818

xianghai2 commented 1 month ago

Per my local setup, the behavior of Kong and upstream service do really conform to HTTP standards.

According to following text from rfc9110, it looks like that either "2xx" or "412" is permitted by the stardard.

"An origin server that evaluates an If-Match condition MUST NOT perform the requested method if the condition evaluates to false. Instead, the origin server MAY indicate that the conditional request failed by responding with a 412 (Precondition Failed) status code. Alternatively, if the request is a state-changing operation that appears to have already been applied to the selected representation, the origin server MAY respond with a 2xx (Successful) status code (i.e., the change requested by the user agent has already succeeded, but the user agent might not be aware of it, perhaps because the prior response was lost or an equivalent change was made by some other user agent).

Allowing an origin server to send a success response when a change request appears to have already been applied is more efficient for many authoring use cases, but comes with some risk if multiple user agents are making change requests that are very similar but not cooperative. For example, multiple user agents writing to a common resource as a semaphore (e.g., a nonatomic increment) are likely to collide and potentially lose important state transitions. For those kinds of resources, an origin server is better off being stringent in sending 412 for every failed precondition on an unsafe method. In other cases, excluding the ETag field from a success response might encourage the user agent to perform a GET as its next request to eliminate confusion about the resource's current state."

https://www.rfc-editor.org/rfc/rfc9110.html#name-if-match https://www.rfc-editor.org/rfc/rfc9110.html#evaluation https://www.rfc-editor.org/rfc/rfc9111.html#name-sending-a-validation-reques

TGeb-EV commented 1 month ago

Did you tried my request flow? The behavior is different with or without plugin. Both flows (yours and mine) are kind of standard conform, but this is not consistent. The precondition do not fail, the if-match is matching, but the response is with another etag, so kong says this is not matching, but kong should do nothing. For me this is a bug and the kong response phase is not usable. This unexpected behavior is a risk to us and maybe other usecases are also affected or has side effects. The additional GET request in the rfc is a recommendation not a MUST. In our usecase the additional GET request makes really no sense (I understand the concerns). I programmed our needed plugin now in another phase and it works.

chobits commented 1 month ago

@xianghai2 would you take a look at it again?

xianghai2 commented 1 month ago

back to this.

Hi @TGeb-EV, which phase/function was your plugin implemented for? for me, it looks like that my plugin of the access phase or reponse funtion has no such issue. thanks.

TGeb-EV commented 4 weeks ago

Kong Docker Image Build files including plugin source code: kong-docker-with-plugin.tar.gz

Custom Service for testing, called testservice in compose: testservice-dockerfile.tar.gz

Docker compose with configured custom Docker Images and Custom Plugin compose.tar.gz

Service Configuration: Screenshot_20240814_140357

Route Configuration: Screenshot_20240814_140341

Plugin configuration: Screenshot_20240814_140309

Curl to show, that Kong is responding with 412 instead of 200:
curl -vvvv -H "If-Match: V113" "http://localhost:8000/hello"

Curl output you should see:
*   Trying 127.0.0.1:8000...
* Connected to localhost (127.0.0.1) port 8000 (#0)
> GET /hello HTTP/1.1
> Host: localhost:8000
> User-Agent: curl/7.81.0
> Accept: */*
> If-Match: V113
> 
* Mark bundle as not supporting multiuse
< HTTP/1.1 412 Precondition Failed
< Date: Wed, 14 Aug 2024 12:04:12 GMT
< Content-Type: application/json; charset=utf-8
< Connection: keep-alive
< Content-Length: 88
< X-Kong-Upstream-Latency: 1
< X-Kong-Proxy-Latency: 1
< Via: kong/3.7.1
< X-Kong-Request-Id: a4c2917083168d438ed04d17120b6864
< 
{
  "message":"Precondition failed",
  "request_id":"a4c2917083168d438ed04d17120b6864"
* Connection #0 to host localhost left intact
}%               

If you delete the Plugin configuration, the first request after deletion also fails with 412. A further call seems to be a 200 then.

Now you have everything to work with. You have mabe only to adjust the image names for Kong and the testservice in the docker-compose file

Log of compose for a few requests where you see, that the service answers with 200 and Kong with 412, you also see the creation and deletion requests of the plugin configuration: image

chobits commented 3 weeks ago

THe 412 error code is returned by nginx/openresty, and kong redirect its page content, so Its the default bebaviour of nginx/openresty. And we could not disable this default behaviour. Check the source code of nginx:

  1. https://github.com/nginx/nginx/blob/master/src/http/modules/ngx_http_not_modified_filter_module.c#L71
  2. https://github.com/nginx/nginx/blob/master/src/http/modules/ngx_http_not_modified_filter_module.c#L172
TGeb-EV commented 3 weeks ago

@chobits , why is it working with plugins in other phases or without such a plugin? This behavior should be the case everytime if you are right!

Also I tried to disable Proxy Caching for nginx. In a nginx only setup, the nginx is working without this behavior if I disabled the caching or / if-modified-since.

And why is it only when a Plugin in response phase is used?

xianghai2 commented 3 weeks ago

Hi @TGeb-EV , that's probably due to the ETag conformance behavior of response phase plugin or cache.

And your upstream test service(index.js) may want to be something like following to conform to expected ETag behavior:

app.get('/hello', (req, res) => {
  if (req.headers['if-match'] && req.headers['if-match']  != 'V1')
    return res.sendStatus(412);
  res.set('etag','V1')
  res.send('Hello World!')
})
TGeb-EV commented 3 weeks ago

Hi @xianghai2 , I explained my request flow 2 times including a graphic, explained, that the recommended flow in the RFC ist not a MUST just an hint, I just used in the last example a get request, you could replace it with POTST or PUT. And you want to make me believe that activating a an "EMPTY" plugin getting a complete another behavior is correctly. I also provided a detailed examplet for my flow. The last time I will explain the flow...

PUT a changed Entity (same ID) to the service with a if-Match header (value "v1") -> the service checks if the etag ist correct like the following

app.get('/hello', (req, res) => {
  if (req.headers['if-match'] && req.headers['if-match']  != 'V1')
    return res.sendStatus(412); //you are right, it should be send by the service, "not by Kong"! But for showing the unexpected behavior this was not necessary

// some logic to create the new object in the database and count up the version, that matches since now, this is not recommended by RFC but allowed and in some cases the best way

  res.set('etag','V2')  // send the "new" etag value, so that the client not to have to do an additionally GET request
  res.send('The new entity as body or no body')
}))

The service answers with 200 and the new etag value. The Kong then responds with 412 and an error message.

If you now say it is OK, and that the behavior should change only by adding an empty plugin, I will not agree, but I am done with this discussion.

xianghai2 commented 1 week ago

Let's close this as the dissussoin was done. thanks.

TGeb-EV commented 1 week ago

If you don't want to fix it, it is OK to me.