[CORE-1507] SR: Add support for `?verbose` compatibility checks #18332

oleiman · 2024-05-09T20:16:37Z

Some side effects of this:

In general, this PR keeps existing checks in place but adds reference-compatible error codes and messages to provide human-readable detail.
On the protobuf side, this PR introduces a handful of checks that are functionally new (i.e. schemas which were previously considered compatible are no longer so):
- ONEOF_FIELD_REMOVED
- MULTIPLE_FIELDS_MOVED_TO_ONEOF
- REQUIRED_FIELD_{ADDED,REMOVED}
- FIELD_NAMED_TYPE_CHANGED
- MESSAGE_REMOVED
  - We already detect some of these (file level) but this PR adds additional checks for nested message definitions
Previously our compatibility checks would short circuit, returning a 👍 / 👎 as soon as a determination is made. Now we want to build up a collection of errors so that users can make all the required changes and avoid repeated checks. To do this, we need to rejigger the structure of the recursion a bit and introduce some new types for collecting errors.
We also want to report "path" information about errors (i.e. where in the schema the error occurred), so we need to carry that information through the recursion. I used std::filesystem::path for this as a first pass, but in the end it looks fit for purpose. We could implement something similar if the cost is a concern, but I suspect most of the cost is in storing the path elements themselves, so it may be a wash. It might be nice to just not do this if verbose error tracking is not enabled.

Fixes CORE-1507

Backports Required

Release Notes

Improvements

SchemaRegistry: Adds support for the ?verbose parameter on POST /compatibility/subjects/{subject}/versions/{version}

oleiman · 2024-05-09T20:29:37Z

force push move some compatibility code from header

oleiman · 2024-05-09T20:29:59Z

/dt

vbotbuildovich · 2024-05-09T22:58:09Z

new failures in https://buildkite.com/redpanda/redpanda/builds/48905#018f5f60-2f1f-49b5-9525-d2010401037a:

"rptest.tests.schema_registry_test.SchemaRegistryBasicAuthTest.test_post_compatibility_subject_version"

new failures in https://buildkite.com/redpanda/redpanda/builds/48905#018f5f51-1de4-47ce-b35c-e447df7d2995:

"rptest.tests.schema_registry_test.SchemaRegistryBasicAuthTest.test_post_compatibility_subject_version"

vbotbuildovich · 2024-05-09T23:00:40Z

ducktape was retried in https://buildkite.com/redpanda/redpanda/builds/48905#018f5f60-2f1f-49b5-9525-d2010401037a

ducktape was retried in https://buildkite.com/redpanda/redpanda/builds/48905#018f5f60-2f27-47c1-8a6a-c53f8393bb98

ducktape was retried in https://buildkite.com/redpanda/redpanda/builds/48912#018f6055-325b-4a7f-883b-d9f53a360d48

ducktape was retried in https://buildkite.com/redpanda/redpanda/builds/48962#018f65f4-a4d7-4fb0-94e8-9e3b0c3d643e

ducktape was retried in https://buildkite.com/redpanda/redpanda/builds/48962#018f65f4-a4d2-4971-aa11-9112ae7a151e

ducktape was retried in https://buildkite.com/redpanda/redpanda/builds/49069#018f757f-a9d4-405c-923d-0c9527e33c52

oleiman · 2024-05-09T23:06:59Z

force push typo

oleiman · 2024-05-10T00:54:57Z

/ci-repeat 1

Deflaimun · 2024-05-13T15:00:18Z

tests/rptest/tests/schema_registry_test.py

        assert result_raw.status_code == requests.codes.ok
        assert result_raw.json()["is_compatible"] == False

+        self.logger.debug(
+            "Check compatibility backward, no default, not verbose")


I know this is just a logger but I don't understand what it's supposed to mean...Writing can possibly be improved

FYI - these are integration tests (i.e. not broker logs), but I can try to make it more clear on the next push.

oleiman · 2024-05-14T03:36:28Z

force push improve commits, tests, remove some junk

oleiman · 2024-05-14T04:02:01Z

force push remove some cruft

src/go/rpk/pkg/cli/registry/schema/check_compatibility.go

BenPope

Partial review up until avro - but plenty of work, hopefully mostly just a rebase of our libavro fork.

src/v/pandaproxy/schema_registry/compatibility.h

src/v/pandaproxy/schema_registry/avro.cc

BenPope · 2024-05-16T11:07:49Z

src/v/pandaproxy/schema_registry/avro.cc

+        if (reader.type() == avro::Type::AVRO_ARRAY) {
+            compat_result.merge(check_compatible(
+              *reader.leafAt(0), *writer.leafAt(0), p / "items"));
+        } else if (reader.hasName() && reader.name() != writer.name()) {


This check is too restrictive; it needs to handle aliases. That appears to be a recent addition to libavro, so you're in luck, to some extent.

Does this set of alternatives need to be broken into a set of checks (i.e., drop the else) to record all incompatible checks?

Does this set of alternatives need to be broken into a set of checks

Yeah probably. At least this one (checking the name). The strict type checks are mutually exclusive. Maybe you're right actually - we don't lose anything by breaking it all up.

BenPope · 2024-05-16T11:25:59Z

src/v/pandaproxy/schema_registry/avro.cc

+            // ?? - emplace an UNKNOWN error with the current path
+            compat_result.emplace<avro_incompatibility>(
+              std::move(p), avro_incompatibility::Type::UNKNOWN);
+            return compat_result;


More thought required from me, I feel like this is too restrictive.

Too restrictive in the sense that

a. we might discover more context later (i.e. short-circuit too soon) OR
b. the schemas might still be compatible.

To (a), that's why I added the extra checks above, so it's possible that there are more!

I do think we're guaranteed to be incompatible here though; as I understand it, Node::resolve is meant to detect "obvious" incompatibilities like scalar type mismatch, etc. Everything below (records, enums, unions) is not detectable by Node::resolve

src/v/pandaproxy/schema_registry/avro.cc