fix(dao) check_upsert func return value on invalid options #8831

windmgc · 2022-05-20T09:11:48Z

Summary

Hi, this PR aims to fix the bug that will cause an unexpected 404 error on creating/updating configs with invalid options.

A simple way to reproduce:

Store the example config and sync the config using decK, as you can see the TTL of the consumer key is an invalid value which will not pass the validate_options_value func

_format_version: "1.1"
consumers:
- keyauth_credentials:
  - key: test
    ttl: 100000001
  username: [email protected]
services:
- connect_timeout: 60000
  host: httpbin.org
  name: test1
  path: /anything
  plugins:
  - config:
      anonymous: null
      hide_credentials: false
      key_in_body: false
      key_in_header: true
      key_in_query: true
      key_names:
      - apikey
      run_on_preflight: true
    enabled: true
    name: key-auth
    protocols:
    - grpc
    - grpcs
    - http
    - https
  port: 80
  protocol: http
  read_timeout: 60000
  retries: 5
  routes:
  - https_redirect_status_code: 426
    name: test1
    path_handling: v0
    paths:
    - /auth
    preserve_host: true
    protocols:
    - https
    regex_priority: 100
    request_buffering: true
    response_buffering: true
    strip_path: false
  write_timeout: 60000

# Syncing using decK
$ deck sync --kong-addr http://localhost:8001 -s kong.yaml
creating consumer [email protected]
creating service test1
creating key-auth test for consumer [email protected]
Summary:
  Created: 2
  Updated: 0
  Deleted: 0
Error: 1 errors occurred:
        while processing event: {Create} key-auth test for consumer [email protected] failed: HTTP status 404 (message: "Not found")

As it is shown in the error log, the message is very confusing.

The reason for this weird behaviour is that the check_upsert function is returning a wrong list of values, it should be 4 values(starting with 2 nils), but now it returns 3 values

kong/kong/db/dao/init.lua

Line 571 in fcb7275

return nil, tostring(err_t), err_t

And then it makes err_t a nil value

kong/kong/db/dao/init.lua

Lines 1201 to 1207 in fcb7275

    
           local entity_to_upsert, rbw_entity, err, err_t = check_upsert(self, 
        
                                                                         primary_key, 
        
                                                                         entity, 
        
                                                                         options) 
        
           if not entity_to_upsert then 
        
             return nil, err, err_t 
        
           end

Which leads to the wrong 404 logic(because err_t is nil)

kong/kong/api/endpoints.lua

Lines 488 to 495 in fcb7275

    
           local entity, _, err_t = upsert_entity(self, db, schema, method) 
        
           if err_t then 
        
             return handle_error(err_t) 
        
           end 
        
           if not entity then 
        
             return not_found() 
        
           end

After the fix, the error becomes meaningful

updating key-auth test for consumer [email protected]  {
   "consumer": {
-    "id": "9310d28f-547a-4f25-9ea5-2eda79f1cda8",
+    "id": "92381706-1c6e-405d-91f1-57379722dedf",
-    "username": "Tom"
+    "username": "[email protected]"
   },
   "id": "e711b13b-0f2c-4804-b68e-79be8a20c114",
   "key": "test",
-  "ttl": 993849
+  "ttl": 1.00000001e+08
 }

Summary:
  Created: 0
  Updated: 0
  Deleted: 0
Error: 1 errors occurred:
        while processing event: {Update} key-auth test for consumer [email protected] failed: HTTP status 400 (message: "invalid option (ttl: must be an integer between 0 and 100000000)")

Full changelog

fix check upsert func return value on invalid options

Issue reference

Fix FTI-4021

windmgc · 2022-05-20T10:55:41Z

The weird part is that I can only reproduce this bug via decK, I cannot reproduce it via manual API requests like using cURL, still looking for reasons, but it seems that this bug is not related to decK

mayocream

LGTM

ms2008 · 2022-05-27T03:13:46Z

@windmgc This seems to affect only the PUT method, which is used by decK.

$ curl -i -X POST http://kong:8001/consumers \
>     --data "username=key-ttl-test"
HTTP/1.1 201 Created
Date: Fri, 27 May 2022 02:51:24 GMT
Content-Type: application/json; charset=utf-8
Connection: keep-alive
Access-Control-Allow-Origin: *
X-Kong-Admin-Request-ID: cJBrOP2FxOC79zN4pjPengUCmunWSaep
Content-Length: 165
X-Kong-Admin-Latency: 5
Server: kong/2.8.0.0-dev-enterprise-edition

{"custom_id":null,"username_lower":"key-ttl-test","tags":null,"id":"be3b06b4-d6ba-416f-bc31-57163c0a2bd0","username":"key-ttl-test","type":0,"created_at":1653619884}
$
$ curl -i -X POST http://kong:8001/consumers/key-ttl-test/key-auth \
>     --data "key=test" \
>     --data "ttl=100000001"
HTTP/1.1 400 Bad Request
Date: Fri, 27 May 2022 02:43:51 GMT
Content-Type: application/json; charset=utf-8
Connection: keep-alive
Access-Control-Allow-Origin: *
X-Kong-Admin-Request-ID: ncwR07nzaX3bU6WCMDRUW357uvV4vMeQ
Content-Length: 176
X-Kong-Admin-Latency: 1
Server: kong/2.8.0.0-dev-enterprise-edition

{"options":{"ttl":"must be an integer between 0 and 100000000"},"message":"invalid option (ttl: must be an integer between 0 and 100000000)","code":11,"name":"invalid options"}
$
$ curl -i -X PUT http://kong:8001/consumers/key-ttl-test/key-auth/$(uuidgen) \
>     --data "key=test" \
>     --data "ttl=100000001"
HTTP/1.1 404 Not Found
Date: Fri, 27 May 2022 02:51:32 GMT
Content-Type: application/json; charset=utf-8
Connection: keep-alive
Access-Control-Allow-Origin: *
X-Kong-Admin-Request-ID: Bv1gw037ga0D5ED8E3dmrXrGDEehd678
Content-Length: 23
X-Kong-Admin-Latency: 1
Server: kong/2.8.0.0-dev-enterprise-edition

{"message":"Not found"}
$

windmgc · 2022-05-31T08:50:47Z

@windmgc This seems to affect only the PUT method, which is used by decK.

$ curl -i -X POST http://kong:8001/consumers \
>     --data "username=key-ttl-test"
HTTP/1.1 201 Created
Date: Fri, 27 May 2022 02:51:24 GMT
Content-Type: application/json; charset=utf-8
Connection: keep-alive
Access-Control-Allow-Origin: *
X-Kong-Admin-Request-ID: cJBrOP2FxOC79zN4pjPengUCmunWSaep
Content-Length: 165
X-Kong-Admin-Latency: 5
Server: kong/2.8.0.0-dev-enterprise-edition

{"custom_id":null,"username_lower":"key-ttl-test","tags":null,"id":"be3b06b4-d6ba-416f-bc31-57163c0a2bd0","username":"key-ttl-test","type":0,"created_at":1653619884}
$
$ curl -i -X POST http://kong:8001/consumers/key-ttl-test/key-auth \
>     --data "key=test" \
>     --data "ttl=100000001"
HTTP/1.1 400 Bad Request
Date: Fri, 27 May 2022 02:43:51 GMT
Content-Type: application/json; charset=utf-8
Connection: keep-alive
Access-Control-Allow-Origin: *
X-Kong-Admin-Request-ID: ncwR07nzaX3bU6WCMDRUW357uvV4vMeQ
Content-Length: 176
X-Kong-Admin-Latency: 1
Server: kong/2.8.0.0-dev-enterprise-edition

{"options":{"ttl":"must be an integer between 0 and 100000000"},"message":"invalid option (ttl: must be an integer between 0 and 100000000)","code":11,"name":"invalid options"}
$
$ curl -i -X PUT http://kong:8001/consumers/key-ttl-test/key-auth/$(uuidgen) \
>     --data "key=test" \
>     --data "ttl=100000001"
HTTP/1.1 404 Not Found
Date: Fri, 27 May 2022 02:51:32 GMT
Content-Type: application/json; charset=utf-8
Connection: keep-alive
Access-Control-Allow-Origin: *
X-Kong-Admin-Request-ID: Bv1gw037ga0D5ED8E3dmrXrGDEehd678
Content-Length: 23
X-Kong-Admin-Latency: 1
Server: kong/2.8.0.0-dev-enterprise-edition

{"message":"Not found"}
$

@ms2008 Yes you're right, after reading decK and Kong admin API's code, I confirm that this bug will affect PUT related requests.

Before the fix:

$ curl -X PUT -Lv http://$(gojira port 8001)/consumers/5b2ca676-2302-4a26-ac25-327f8d7ebf98/key-auth/332d97c3-020a-49d9-95b8-e5242ae320d7 -H 'Content-Type: application/json' --data '{"consumer":{"id":"5b2ca676-2302-4a26-ac25-327f8d7ebf98","username":"[email protected]
"},"id":"332d97c3-020a-49d9-95b8-e5242ae320d7","key":"test","ttl":100000001}'
*   Trying 0.0.0.0:58189...
* Connected to 0.0.0.0 (127.0.0.1) port 58189 (#0)                                                                                                                                                                                                                  > PUT /consumers/5b2ca676-2302-4a26-ac25-327f8d7ebf98/key-auth/332d97c3-020a-49d9-95b8-e5242ae320d7 HTTP/1.1
> Host: 0.0.0.0:58189
> User-Agent: curl/7.79.1
> Accept: */*
> Content-Type: application/json
> Content-Length: 166
>
* Mark bundle as not supporting multiuse
< HTTP/1.1 404 Not Found
< Date: Tue, 31 May 2022 08:46:09 GMT
< Content-Type: application/json; charset=utf-8
< Connection: keep-alive
< Access-Control-Allow-Origin: *
< Content-Length: 23
< X-Kong-Admin-Latency: 2
< Server: kong/2.8.0
<
* Connection #0 to host 0.0.0.0 left intact
{"message":"Not found"}%

After the fix:

$ curl -X PUT -Lv http://$(gojira port 8001)/consumers/5b2ca676-2302-4a26-ac25-327f8d7ebf98/key-auth/332d97c3-020a-49d9-95b8-e5242ae320d7 -H 'Content-Type: application/json' --data '{"consumer":{"id":"5b2ca676-2302-4a26-ac25-327f8d7ebf98","username":"[email protected]"},"id":"332d97c3-020a-49d9-95b8-e5242ae320d7","key":"test","ttl":100000001}'
*   Trying 0.0.0.0:58189...
* Connected to 0.0.0.0 (127.0.0.1) port 58189 (#0)
> PUT /consumers/5b2ca676-2302-4a26-ac25-327f8d7ebf98/key-auth/332d97c3-020a-49d9-95b8-e5242ae320d7 HTTP/1.1
> Host: 0.0.0.0:58189
> User-Agent: curl/7.79.1
> Accept: */*
> Content-Type: application/json
> Content-Length: 166
>
* Mark bundle as not supporting multiuse
< HTTP/1.1 400 Bad Request
< Date: Tue, 31 May 2022 08:47:50 GMT
< Content-Type: application/json; charset=utf-8
< Connection: keep-alive
< Access-Control-Allow-Origin: *
< Content-Length: 176
< X-Kong-Admin-Latency: 1310
< Server: kong/2.8.0
<
* Connection #0 to host 0.0.0.0 left intact
{"name":"invalid options","message":"invalid option (ttl: must be an integer between 0 and 100000000)","options":{"ttl":"must be an integer between 0 and 100000000"},"code":11}%

locao

This is not the proper fix. You are changing the default return pattern for errors, which is nil, error-string, with an addition error table in the cases for the DAO. In this case it works by chance, but you have to check what function is calling check_upsert() to understand why it behaves as you expect when returning nil, nil.

windmgc · 2022-06-01T02:52:25Z

This is not the proper fix. You are changing the default return pattern for errors, which is nil, error-string, with an addition error table in the cases for the DAO. In this case it works by chance, but you have to check what function is calling check_upsert() to understand why it behaves as you expect when returning nil, nil.

Hi @locao, I don't think I'm changing the default return pattern of three values for the DAO functions, because check_upsert is only a local function used inside this module(not the DAO: series functions), and it seems that every return(excluding the only one that got changed in this PR) in the check_upsert function has four return values, and there are two lines referencing the check_upsert function, one is the generate_foreign_key_methods local function,

kong/kong/db/dao/init.lua

Line 819 in 830bbe8

local entity_to_upsert, rbw_entity, err, err_t = check_upsert(self,

, and the other is DAO:upsert

kong/kong/db/dao/init.lua

Line 1192 in 830bbe8

local entity_to_upsert, rbw_entity, err, err_t = check_upsert(self,

, both of them are expecting four return values.

So actually I don't quite get your point, could you explain more about it? Thanks!

@windmgc

overriding this, see @windmgc's response

windmgc requested a review from a team as a code owner May 20, 2022 09:11

pull-request-size bot added the size/XS label May 20, 2022

github-actions bot added the core/db label May 20, 2022

mayocream approved these changes May 23, 2022

View reviewed changes

mayocream changed the title ~~fix(dao): fix check upsert func return value on invalid options~~ fix(dao) check_upsert func return value on invalid options May 23, 2022

fix(dao): fix check upsert func return value on invalid options

2468f62

windmgc force-pushed the FTI-4021 branch from 894cfd8 to 2468f62 Compare May 23, 2022 08:27

windmgc mentioned this pull request May 30, 2022

[FTI-4021]add explanation for valid value range of key's ttl in key-auth plugin Kong/docs.konghq.com#3985

Merged

locao previously requested changes May 31, 2022

View reviewed changes

kikito approved these changes Jun 1, 2022

View reviewed changes

kikito merged commit f7c1cb8 into Kong:master Jun 1, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(dao) check_upsert func return value on invalid options #8831

fix(dao) check_upsert func return value on invalid options #8831

windmgc commented May 20, 2022

windmgc commented May 20, 2022 •

edited

Loading

mayocream left a comment

ms2008 commented May 27, 2022

windmgc commented May 31, 2022 •

edited

Loading

locao left a comment

windmgc commented Jun 1, 2022

	local entity_to_upsert, rbw_entity, err, err_t = check_upsert(self,
	primary_key,
	entity,
	options)
	if not entity_to_upsert then
	return nil, err, err_t
	end

	local entity, _, err_t = upsert_entity(self, db, schema, method)
	if err_t then
	return handle_error(err_t)
	end

	if not entity then
	return not_found()
	end

fix(dao) check_upsert func return value on invalid options #8831

fix(dao) check_upsert func return value on invalid options #8831

Conversation

windmgc commented May 20, 2022

Summary

Full changelog

Issue reference

windmgc commented May 20, 2022 • edited Loading

mayocream left a comment

Choose a reason for hiding this comment

ms2008 commented May 27, 2022

windmgc commented May 31, 2022 • edited Loading

locao left a comment

Choose a reason for hiding this comment

windmgc commented Jun 1, 2022

windmgc commented May 20, 2022 •

edited

Loading

windmgc commented May 31, 2022 •

edited

Loading