chore: added replaced_with path in otel plugin schema

[Prashansa-K]

Summary

Opentelemetery plugin schema has a deprecated field endpoint, that is now replaced with "traces_endpoint". The schema is missing information which can be used by other tools, like deck to understand the new field, that should be used in place of the deprecated one. This change adds the path as per the new convention of using replaced_with path.

With this change, otel plugin schema would show the necessary replacement paths for shorthand_fields that are deprecated.

Checklist

• [NA] The Pull Request has tests
• A changelog file has been created under changelog/unreleased/kong or skip-changelog label added on PR if changelog is unnecessary. README.md
• [NA] There is a user-facing docs PR against https://github.com/Kong/docs.konghq.com - PUT DOCS PR HERE - NA

Issue reference

Fix deck #1404
Corresponding PR: Kong/go-kong#472

[CLAassistant]

All committers have signed the CLA.

[CLAassistant]

Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
_{You have signed the CLA already but the status is still pending? Let us recheck it.}

[samugi]

can I get more background on this change? From the changelog: "to show the new config path to use instead of the deprecated fields in the opentelemetry plugin schema" I'm not sure I understand where this needs to be visualized and what issue it is currently creating, if any.

My understanding is that we're copying the value assigned to the (deprecated) endpoint back into the endpoint field in the schema, in addition to the traces_endpoint one. I think this can be a breaking change if vault references are used, as explained in my comment below, let me know if I'm missing something.

[samugi]

Suggested change

	Added "replaced_with" in deprecation fields to show the new config path to use instead
	of the deprecated fields in the opentelemetry plugin schema.
	**OpenTelemetry**: Added "replaced_with" in deprecation fields to show the new config path to use instead of the deprecated fields in the plugin schema.

[samugi]

does this highlight a breaking change? I tried to run the test with endpoint and it seems it now fails indeed with: schema violation (endpoint: both deprecated and new field are used but their values mismatch: endpoint = {vault://env/test_otel_endpoint} vs traces_endpoint = http://127.0.0.1:21306).

This appears to be because the dereferencing is only done when traces_endpoint is set, but the compared value (the vault reference) is detected as a difference. This is an issue we already had, but before this change, it was only a problem when the two fields (deprecated and new) were both set, and a vault reference was used as a value.

Now, with this change, the endpoint field is dereferenced and copied into the new traces_endpoint field, but apparently not being dereferenced before being copied into endpoint? Thus resulting in a breaking change for whoever is using a vault reference for the endpoint field and upgrades to this version.

cc @nowNick wdyt?

[Prashansa-K]

Now, with this change, the endpoint field is dereferenced and copied into the new traces_endpoint field,

Where does this dereferencing happen? Could you please help me by pointing to the code maybe, that does the dereferencing part? Since, I just added a new field in the deprecation block, which already existed, I didn't understand how the issue was caused.

[nowNick]

That's a good point about references @samugi ! I think adding replaced_with to the field itself might not be "bad" - it's supposed to be there so that the endpoint returns responses in backwards compatible manner. However I don't think this approach is going to fix the underlying issue that's been raised in Kong/deck#1404 The problem is that not all deprecations are simple renaming. An example here is cluster_nodes in RateLimitingAdvanced. It is defined as a list of records:
[ {ip1, port2}, {ip2, port2}, ... ]
but it used to be an list of strings:
[ 'ip1:port1', 'ip2:port2', ... ].

So to translate between those two values (new and old) is not as simple as rewriting this data back. I think we need a different approach when it comes to new & deprecated fields on decK diff.

[samugi]

to clarify, @Prashansa-K , I don't think your change in particular is problematic, it just highlighted a problem (during schema validation?) that already existed, i.e. if a field is referenceable, adding a replaced_with to the corresponding shorthand can create breaking change scenarios for users who had the field configured with a vault reference as a value. In which case, apparently, the plugin's configuration will start throwing schema validation failures.

@nowNick about:

I think adding replaced_with to the field itself might not be "bad"

I think we should avoid adding replaced_with to referenceable fields until the issue above is resolved, wdyt?

[nowNick]

Yeah. You're probably right @samugi . Could you provide a small decK file that highlights this issue? (probably the one you used for testing?)

[samugi]

I didn't actually test this on decK, the test that reproduces this is the one that this thread is referencing.

[Prashansa-K]

Closing this PR for now. We need to deal with referencing issues first before we can make the schemas uniform. Meanwhile, we need to find a better way in deck to deal with deprecations.