Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(schema-validator) does not convert the null in the declarative configuration to nil #8483

Merged
merged 9 commits into from
Mar 10, 2022

Conversation

ADD-SP
Copy link
Contributor

@ADD-SP ADD-SP commented Mar 2, 2022

Summary

If config.limits.{limit_name}.* is null, this plugin will raise a HTTP 500 error.

Full changelog

  • If config.limits.{limit_name}.* is null, skip it.
  • Add a related test in spec/03-plugins/24-response-rate-limiting/01-schema_spec.lua.

Issue reference

Fix #8314

@CLAassistant
Copy link

CLAassistant commented Mar 2, 2022

CLA assistant check
All committers have signed the CLA.

@ADD-SP ADD-SP changed the title fix(plugin-response-ratelimiting) properly handle the ngx.null for limits.{limit_name}.* fix(plugin-response-ratelimiting) properly handle the ngx.null for limits.{limit_name}.* Mar 2, 2022

if not usage[k] then
usage[k] = {}
if lv ~= null then
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't fully understand why the lv can be null, but can it be nil too, thus?

Suggested change
if lv ~= null then
if lv ~= nil and lv ~= null then

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ADD-SP
Copy link
Contributor Author

ADD-SP commented Mar 3, 2022

@bungle
For example, this declarative configuration:

plugins:
    - name: response-ratelimiting
      config: 
        limits:
          test:
            second: null
            hour: 1000
        block_on_first_violation: false
        policy: cluster
        fault_tolerant: true
        hide_client_headers: false

config.limits.test.second is optional, but once it is set to null by the user, the value in this plugin is ngx.null, related function.

However, this case does not occur if we use the Admin API to configure this plugin, this configuration will be recognized as invalid.

curl -i -X POST http://{admin_host}/consumers/{consumer_id}/plugins \
    --data "name=response-ratelimiting"  \
    --data "config.limits.test.hour=1000" \
    --data "config.limits.test.minute=null" \
    --data "config.block_on_first_violation=false" \
    --data "config.policy=cluster" \
    --data "config.fault_tolerant=true" \
    --data "config.hide_client_headers=false" \
    --data "config.header_name=X-Kong-Limit"

The response is:

{
    "fields": {
        "config": {
            "limits": {
                "minute": "expected a number"
            }
        }
    },
    "code": 2,
    "name": "schema violation",
    "message": "schema violation (config.limits: {\n  minute = \"expected a number\"\n})"
}

The same goes for PATCH requests.

curl -i -X PATCH  http://{admin_host}/consumers/{consumer_Id}/plugins/{plugin_id} \
    --data "config.limits.test.minute=null"

The response is:

{
    "message": "schema violation (config.limits: {\n  minute = \"expected a number\"\n})",
    "name": "schema violation",
    "fields": {
        "config": {
            "limits": {
                "minute": "expected a number"
            }
        }
    },
    "code": 2
}

For POST requests, it looks the same if the user sets an item to null or does not specify it. But for PATCH requests, if the user sets an item to null, then they should expect it to be removed (set it to nil), and if they don't specify it, then they expect no changes to be made to it.

I think this might be a bug. For POST requests, if the user sets an item to null or does not specify it, should we consider the two cases to be the same? For PATCH requests, I think an item should be removed if the user sets it to null.

kong/db/schema/init.lua Outdated Show resolved Hide resolved
Co-authored-by: Aapo Talvensaari <aapo.talvensaari@gmail.com>
@ADD-SP ADD-SP requested a review from bungle March 9, 2022 12:30
@ADD-SP ADD-SP changed the title fix(plugin-response-ratelimiting) properly handle the ngx.null for limits.{limit_name}.* fix(schema-validator) does not convert the null in the declarative configuration to nil Mar 9, 2022
@ADD-SP ADD-SP merged commit 28bc985 into master Mar 10, 2022
@ADD-SP ADD-SP deleted the fix/plugin-response-ratelimiting branch March 10, 2022 03:05
ADD-SP added a commit that referenced this pull request Mar 17, 2022
ADD-SP added a commit that referenced this pull request Mar 17, 2022
Co-authored-by: Mayo <i@shoujo.io>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

An unexpected error occurred for response ratelimiting plugin
4 participants