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

deck always update rate-limiter plugin config even no changes #1251

Closed
jtorkkel opened this issue Mar 20, 2024 · 1 comment · Fixed by Kong/go-kong#458
Closed

deck always update rate-limiter plugin config even no changes #1251

jtorkkel opened this issue Mar 20, 2024 · 1 comment · Fixed by Kong/go-kong#458
Assignees

Comments

@jtorkkel
Copy link

Kong gateway diff/sync report need to update every rate-limiter plugin config every sync/diff applied

There seems to be undocumented redis_* keys which cause the issue. Either those keys should not be checked or those should be added to render output.
Also sync with merged fails when unused config keys are not set.

Repro steps

Kong 3.6.1.1, Deck 1.25 Linux

Create simple service with route and rate-limiter plugin

services:
- host: localhost
  name: debug_node_log-level
  port: 8001
  protocol: http
  routes:
  - expression: (net.protocol == "http") && (http.method == "GET") && (http.path ==
      "/api/debug/node/log-level")
    name: debug_node_log-level
    priority: 100
plugins:
- config:
    replace:
      uri: /debug/node/log-level
  name: request-transformer
  route: debug_node_log-level
- name: rate-limiting
  service: routes
  config:
    second: 25
    hour: 20000
    policy: local

render config and sync with Kong

Then run validate or sync again

Every time deck indicate needs to update the rate-limit config

updating plugin rate-limiting for service debug_node_log-level  {
   "config": {
     "day": null,
     "error_code": 429,
     "error_message": "API rate limit exceeded",
     "fault_tolerant": true,
     "header_name": null,
     "hide_client_headers": false,
     "hour": 10000,
     "limit_by": "ip",
     "minute": null,
     "month": null,
     "path": null,
     "policy": "local",
     "redis": {
       "database": 0,
       "host": null,
       "password": null,
       "port": 6379,
       "server_name": null,
       "ssl": false,
       "ssl_verify": false,
       "timeout": 2000,
       "username": null
     },
-    "redis_database": 0,
-    "redis_host": null,
-    "redis_password": null,
-    "redis_port": 6379,
-    "redis_server_name": null,
-    "redis_ssl": false,
-    "redis_ssl_verify": false,
-    "redis_timeout": 2000,
-    "redis_username": null,
     "second": 10,
     "sync_rate": -1,
     "year": null
   },
   "enabled": true,
   "id": "89fa57cd-e8eb-4c61-bf06-ec10c1dcde9d",
   "name": "rate-limiting",
   "protocols": [
     "grpc",
     "grpcs",
     "http",
     "https"
   ],
   "service": {
     "id": "f9bef1cb-c742-49f7-8300-3b90b9d54658",
     "name": "debug_node_log-level"
   }
 }
Summary:
  Created: 0
  Updated: 1
  Deleted: 0

Workaround is possible by adding dummy redis_* keys into into declarative rate-limiter plugin config

- name: rate-limiting
  service: routes
  config:
    second: 25
    hour: 20000
    policy: local
    redis_database: 0
    redis_host: null
    redis_password: null
    redis_port: 6379
    redis_server_name: null
    redis_ssl: false
    redis_ssl_verify: false
    redis_timeout: 2000
    redis_username: null
@szesch
Copy link

szesch commented Apr 9, 2024

I've also run into this with Kong 3.6 and deck 1.26.0.

@Prashansa-K Prashansa-K self-assigned this Jul 23, 2024
Prashansa-K added a commit to Kong/go-kong that referenced this issue Aug 5, 2024
As of now, deprecated fields are renamed to shorthand_fields in all
schemas, as per the conventions. Till now, we do not fill any
values for these fields while attempting to fill config records for
plugins. This creates a visualisation problem in decK. Everytime, a
deck diff or deck sync command is issued, it shows that the deprecated
field values are changed and need to be updated, thus running an
unnecessary update process each time and also confusing end users.
Check #1251: Kong/deck#1251 for clarity.

This fix attempts to fill defaults for the shorthand_fields, retaining
their values, if passed. Also, since shorthand_fields take priority
over normal/nested fields in the gateway, if any changes are detected in
shorthand_fields, it is backfilled to nested fields as well.
@Prashansa-K Prashansa-K added this to the next milestone Aug 5, 2024
pmalek pushed a commit to Kong/go-kong that referenced this issue Aug 5, 2024
As of now, deprecated fields are renamed to shorthand_fields in all
schemas, as per the conventions. Till now, we do not fill any
values for these fields while attempting to fill config records for
plugins. This creates a visualisation problem in decK. Everytime, a
deck diff or deck sync command is issued, it shows that the deprecated
field values are changed and need to be updated, thus running an
unnecessary update process each time and also confusing end users.
Check #1251: Kong/deck#1251 for clarity.

This fix attempts to fill defaults for the shorthand_fields, retaining
their values, if passed. Also, since shorthand_fields take priority
over normal/nested fields in the gateway, if any changes are detected in
shorthand_fields, it is backfilled to nested fields as well.
Prashansa-K added a commit to Kong/go-kong that referenced this issue Aug 5, 2024
* fix: fill config values and defaults in shorthand fields

As of now, deprecated fields are renamed to shorthand_fields in all
schemas, as per the conventions. Till now, we do not fill any
values for these fields while attempting to fill config records for
plugins. This creates a visualisation problem in decK. Everytime, a
deck diff or deck sync command is issued, it shows that the deprecated
field values are changed and need to be updated, thus running an
unnecessary update process each time and also confusing end users.
Check #1251: Kong/deck#1251 for clarity.

This fix attempts to fill defaults for the shorthand_fields, retaining
their values, if passed. Also, since shorthand_fields take priority
over normal/nested fields in the gateway, if any changes are detected in
shorthand_fields, it is backfilled to nested fields as well.

* test: unit test added for shorthand_fields filling

* chore: godoc for new functions added, comments added for clarification

* chore: typo correction and preallocation of slice for backward path translation
@Prashansa-K Prashansa-K removed this from the 1.39.6 milestone Sep 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants