feat(schema): add deprecation field attribute #12686

samugi · 2024-03-04T17:12:05Z

Summary

Add a deprecation record field attribute to identify fields that are deprecated. The attribute requires the message and removal_in_version fields to be configured, which are used to generate a warning when the deprecated field is configured (uses kong.deprecation). The optional old_default field can be used to determine whether the warning should be logged (in case the field was configured to a value other than the old default).

This PR adds the deprecation attribute to two kind of fields:

regular plugin fields that have been deprecated and that were not yet removed from the schema, i.e. the old queue parameters in datadog, opentelemetry, http-log, statsd
shorthand fields, for fields that have been removed from the plugin's schema and that only exist as shorthands, i.e. acme, rate-limiting, response-ratelimiting

Example of a deprecated field (in schema/fields):

{ retry_count = {
    description = "Number of times to retry when sending data to the upstream server.",
    type = "integer",
    deprecation = {
      message = "http-log: config.retry_count no longer works, please use config.queue.max_retry_time instead",
      removal_in_version = "4.0",
      old_default = 10 }, }, },

Example of a deprecated field (in schema/shorthand_fields):

{ auth = {
  type = "string",
  len_min = 0,
  translate_backwards = {'password'},
  deprecation = {
    message = "acme: config.storage_config.redis.auth is deprecated, please use config.storage_config.redis.password instead",
    removal_in_version = "4.0", },
  func = function(value)
    return { password = value }
  end
}},

Checklist

The Pull Request has tests (individual plugins have tests to verify the deprecation warnings are logged correctly)
A changelog file has been created under changelog/unreleased/kong or skip-changelog label added on PR if changelog is unnecessary. README.md
(no) There is a user-facing docs PR against https://github.com/Kong/docs.konghq.com - PUT DOCS PR HERE

Issue reference

KAG-3915

samugi · 2024-03-05T14:08:58Z

the second commit applies the deprecation attribute to the shorthand fields (where the actual field was removed from the schema).

hanshuebner · 2024-03-05T14:48:00Z

removed_version is confusing because it is in past tense, when it really is about the future. Maybe removal_in_version?

changelog/unreleased/kong/plugin-schema-deprecation-record.yml

jschmid1

love it. Can we have a very simple test case to verify the presence of the field in the /schemas/.. output?

samugi · 2024-03-06T14:40:09Z

love it. Can we have a very simple test case to verify the presence of the field in the /schemas/.. output?

@jschmid1 I added a test, since the field is optional we have to test against something that we can make sure will always have a deprecated field, so I added one to the dummy custom plugin (let me know if you were thinking of a better way to test this though)

jschmid1

lgtm

zhongweiy · 2024-03-08T19:40:17Z

kong/db/schema/init.lua

@@ -882,6 +884,16 @@ function Schema:validate_field(field, value)
    return nil, validation_errors.SUBSCHEMA_ABSTRACT_FIELD
  end

+  if field.deprecation then
+    local old_default = field.deprecation.old_default
+    local should_warn = old_default == nil


This line seems conflict with the doc in metaschema.lua "If old_default is set, logging is conditional to the field's value being different from the value of old_default.". Should it be old_default ~= nil or update the document like:
"If old_default is not set, logging is conditional to the field's value being different from the value of old_default."
?

I realize the comment (in the code) wasn't very clear, I reworded it, could you check again? Thanks!

Thanks! My previous comment also does not correctly catch the meaning of the code. But your last version makes it more clear. And sorry for the confusion comment.

zhongweiy · 2024-03-12T22:35:36Z

kong/db/schema/init.lua

@@ -882,6 +884,16 @@ function Schema:validate_field(field, value)
    return nil, validation_errors.SUBSCHEMA_ABSTRACT_FIELD
  end

+  if field.deprecation then
+    local old_default = field.deprecation.old_default
+    local should_warn = old_default == nil


Thanks! My previous comment also does not correctly catch the meaning of the code. But your last version makes it more clear. And sorry for the confusion comment.

Add a deprecation record field attribute to identify fields that are deprecated. The attribute requires the `message` and `removed_version` fields to be configured, which are used to generate a warning when the deprecated field is configured (uses `kong.deprecation`). Updated schemas of the following plugins: * OpenTelemetry * DataDog * StatsD * HTTP Log

convert shorthand fields usages of the deprecation module to the new attribute-based deprecation.

team-gateway-bot · 2024-03-13T08:39:52Z

Successfully created cherry-pick PR for master:

https://github.com/kong/kong-ee/pull/8484

### Summary The shorthand fields e.g. as used in (the url is a shorthand) take precedence: ``` http PUT :8001/services/test url=http://test.org/ port=2345 ``` That means that `port=2345` will be overwritten by http default port of `80` that is implicit in shorthand field `url`. This is how it has been for a long long time. In PR #12686 we added `deprecation` field to shorthand fields definition. Some of our automatic migrations without database migrations magic use this functionality, but the precedence there does not good: ``` http -f PUT :8001/plugins/x name=x config.new=test config.old=should_be_ignored ``` In above the `config.old` is a shorthand field with a deprecation property. Thus it should not overwrite the `config.new`, but in current code base it does. This PR changes it so that it doesn't do it anymore. KAG-5134 and https://kongstrong.slack.com/archives/C07AQH7SAF8/p1722589141558609 Signed-off-by: Aapo Talvensaari <aapo.talvensaari@gmail.com>

The shorthand fields e.g. as used in (the url is a shorthand) take precedence: ``` http PUT :8001/services/test url=http://test.org/ port=2345 ``` That means that `port=2345` will be overwritten by http default port of `80` that is implicit in shorthand field `url`. This is how it has been for a long long time. In PR #12686 we added `deprecation` field to shorthand fields definition. Some of our automatic migrations without database migrations magic use this functionality, but the precedence there does not good: ``` http -f PUT :8001/plugins/x name=x config.new=test config.old=should_be_ignored ``` In above the `config.old` is a shorthand field with a deprecation property. Thus it should not overwrite the `config.new`, but in current code base it does. This PR changes it so that it doesn't do it anymore. KAG-5134 and https://kongstrong.slack.com/archives/C07AQH7SAF8/p1722589141558609 Signed-off-by: Aapo Talvensaari <aapo.talvensaari@gmail.com>

pull-request-size bot added the size/M label Mar 4, 2024

samugi marked this pull request as draft March 4, 2024 17:12

github-actions bot added core/db plugins/opentelemetry schema-change-noteworthy labels Mar 4, 2024

github-actions bot assigned samugi Mar 4, 2024

samugi force-pushed the feat/plugin-schema-deprecation-record branch from 55709aa to b1bc028 Compare March 5, 2024 08:36

pull-request-size bot added size/L and removed size/M labels Mar 5, 2024

github-actions bot added plugins/datadog plugins/statsd plugins/http-log labels Mar 5, 2024

samugi force-pushed the feat/plugin-schema-deprecation-record branch 2 times, most recently from 4ebdb9b to 74206e5 Compare March 5, 2024 08:52

samugi marked this pull request as ready for review March 5, 2024 08:54

samugi marked this pull request as draft March 5, 2024 10:23

samugi force-pushed the feat/plugin-schema-deprecation-record branch from 74206e5 to 34149ea Compare March 5, 2024 11:01

samugi marked this pull request as ready for review March 5, 2024 11:02

github-actions bot added plugins/rate-limiting plugins/acme plugins/response-ratelimiting labels Mar 5, 2024

locao requested a review from zhongweiy March 5, 2024 18:15

samugi force-pushed the feat/plugin-schema-deprecation-record branch from 6b992e7 to 00bb3e8 Compare March 6, 2024 09:23

samugi requested a review from hanshuebner March 6, 2024 09:24

jschmid1 reviewed Mar 6, 2024

View reviewed changes

changelog/unreleased/kong/plugin-schema-deprecation-record.yml Outdated Show resolved Hide resolved

jschmid1 reviewed Mar 6, 2024

View reviewed changes

samugi force-pushed the feat/plugin-schema-deprecation-record branch from 00bb3e8 to dd61fa2 Compare March 6, 2024 14:37

samugi requested a review from jschmid1 March 6, 2024 14:40

jschmid1 approved these changes Mar 6, 2024

View reviewed changes

samugi added the cherry-pick kong-ee schedule this PR for cherry-picking to kong/kong-ee label Mar 8, 2024

zhongweiy reviewed Mar 8, 2024

View reviewed changes

samugi force-pushed the feat/plugin-schema-deprecation-record branch from dd61fa2 to 50ea8be Compare March 12, 2024 19:16

samugi requested a review from zhongweiy March 12, 2024 19:18

zhongweiy approved these changes Mar 12, 2024

View reviewed changes

samugi added 2 commits March 13, 2024 09:24

chore(schema): convert shorthand fields deprecations

8bb6e38

convert shorthand fields usages of the deprecation module to the new attribute-based deprecation.

samugi force-pushed the feat/plugin-schema-deprecation-record branch from 50ea8be to 8bb6e38 Compare March 13, 2024 08:24

samugi merged commit 649060a into master Mar 13, 2024
26 checks passed

samugi deleted the feat/plugin-schema-deprecation-record branch March 13, 2024 08:39

bungle mentioned this pull request Aug 9, 2024

fix(schema): deprecated shorthand fields precedence #13486

Merged

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(schema): add deprecation field attribute #12686

feat(schema): add deprecation field attribute #12686

samugi commented Mar 4, 2024 •

edited

Loading

samugi commented Mar 5, 2024

hanshuebner commented Mar 5, 2024

jschmid1 left a comment

samugi commented Mar 6, 2024

jschmid1 left a comment

zhongweiy Mar 8, 2024

samugi Mar 12, 2024 •

edited

Loading

zhongweiy Mar 12, 2024

zhongweiy Mar 12, 2024

team-gateway-bot commented Mar 13, 2024

feat(schema): add deprecation field attribute #12686

feat(schema): add deprecation field attribute #12686

Conversation

samugi commented Mar 4, 2024 • edited Loading

Summary

Checklist

Issue reference

samugi commented Mar 5, 2024

hanshuebner commented Mar 5, 2024

jschmid1 left a comment

Choose a reason for hiding this comment

samugi commented Mar 6, 2024

jschmid1 left a comment

Choose a reason for hiding this comment

zhongweiy Mar 8, 2024

Choose a reason for hiding this comment

samugi Mar 12, 2024 • edited Loading

Choose a reason for hiding this comment

zhongweiy Mar 12, 2024

Choose a reason for hiding this comment

zhongweiy Mar 12, 2024

Choose a reason for hiding this comment

team-gateway-bot commented Mar 13, 2024

samugi commented Mar 4, 2024 •

edited

Loading

samugi Mar 12, 2024 •

edited

Loading