From e4b64db113e6991ca40f9bd1da4e2fa735f41271 Mon Sep 17 00:00:00 2001 From: Ben Pope Date: Mon, 29 Jul 2024 18:35:02 +0100 Subject: [PATCH 1/2] schema_registry: Translate base64_decoder_exception This is a regression introduced in https://github.com/redpanda-data/redpanda/commit/e7fab4a27489a8862e2bce36589e5770ec089882 Signed-off-by: Ben Pope --- src/v/pandaproxy/schema_registry/protobuf.cc | 12 ++-- .../test/compatibility_protobuf.cc | 35 ++++++++++ .../test/compatibility_protobuf.h | 67 +++++++++++++++++++ 3 files changed, 110 insertions(+), 4 deletions(-) diff --git a/src/v/pandaproxy/schema_registry/protobuf.cc b/src/v/pandaproxy/schema_registry/protobuf.cc index c0a55b8ed27ba..02bc5f22edf28 100644 --- a/src/v/pandaproxy/schema_registry/protobuf.cc +++ b/src/v/pandaproxy/schema_registry/protobuf.cc @@ -231,10 +231,14 @@ class parser { // Attempt parse a .proto file if (!_parser.Parse(&t, &_fdp)) { - // base64 decode the schema - iobuf_istream is{base64_to_iobuf(schema.def().raw()())}; - // Attempt parse as an encoded FileDescriptorProto.pb - if (!_fdp.ParseFromIstream(&is.istream())) { + try { + // base64 decode the schema + iobuf_istream is{base64_to_iobuf(schema.def().raw()())}; + // Attempt parse as an encoded FileDescriptorProto.pb + if (!_fdp.ParseFromIstream(&is.istream())) { + throw as_exception(error_collector.error()); + } + } catch (const base64_decoder_exception&) { throw as_exception(error_collector.error()); } } diff --git a/src/v/pandaproxy/schema_registry/test/compatibility_protobuf.cc b/src/v/pandaproxy/schema_registry/test/compatibility_protobuf.cc index 5ca0449d2a61d..813b9d9fad45b 100644 --- a/src/v/pandaproxy/schema_registry/test/compatibility_protobuf.cc +++ b/src/v/pandaproxy/schema_registry/test/compatibility_protobuf.cc @@ -189,6 +189,41 @@ SEASTAR_THREAD_TEST_CASE(test_protobuf_recursive_reference) { .get(); } +SEASTAR_THREAD_TEST_CASE(test_binary_protobuf) { + simple_sharded_store store; + + BOOST_REQUIRE_NO_THROW(store.store + .make_valid_schema(pps::canonical_schema{ + pps::subject{"com.redpanda.Payload.proto"}, + pps::canonical_schema_definition{ + base64_raw_proto, pps::schema_type::protobuf}}) + .get()); +} + +SEASTAR_THREAD_TEST_CASE(test_invalid_binary_protobuf) { + simple_sharded_store store; + + auto broken_base64_raw_proto = base64_raw_proto.substr(1); + + auto schema = pps::canonical_schema{ + pps::subject{"com.redpanda.Payload.proto"}, + pps::canonical_schema_definition{ + broken_base64_raw_proto, pps::schema_type::protobuf}}; + + BOOST_REQUIRE_EXCEPTION( + store.store + .make_valid_schema(pps::canonical_schema{ + pps::subject{"com.redpanda.Payload.proto"}, + pps::canonical_schema_definition{ + broken_base64_raw_proto, pps::schema_type::protobuf}}) + .get(), + pps::exception, + [](const pps::exception& e) { + std::cout << e.what(); + return e.code() == pps::error_code::schema_invalid; + }); +} + SEASTAR_THREAD_TEST_CASE(test_protobuf_well_known) { simple_sharded_store store; diff --git a/src/v/pandaproxy/schema_registry/test/compatibility_protobuf.h b/src/v/pandaproxy/schema_registry/test/compatibility_protobuf.h index 8dc490e622bfd..881e97c1b635d 100644 --- a/src/v/pandaproxy/schema_registry/test/compatibility_protobuf.h +++ b/src/v/pandaproxy/schema_registry/test/compatibility_protobuf.h @@ -88,3 +88,70 @@ message A1 { } })", pps::schema_type::protobuf}; + +// Binary encoded protobuf descriptor: +// +// syntax = "proto3"; +// +// package com.redpanda; +// +// option go_package = "./;main"; +// option java_multiple_files = true; +// +// import "google/protobuf/timestamp.proto"; +// +// message Payload { +// int32 val = 1; +// google.protobuf.Timestamp timestamp = 2; +// } +// +// message A { +// message B { +// message C { +// message D { +// message M00 {} +// message M01 {} +// message M02 {} +// message M03 {} +// message M04 {} +// message M05 {} +// message M06 {} +// message M07 {} +// message M08 {} +// message M09 {} +// message M10 {} +// message M11 {} +// message M12 {} +// message M13 {} +// message M14 {} +// message M15 {} +// message M16 {} +// message M17 {} +// message NestedPayload { +// int32 val = 1; +// google.protobuf.Timestamp timestamp = 2; +// } +// } +// } +// } +// } +// +// message CompressiblePayload { +// int32 val = 1; +// google.protobuf.Timestamp timestamp = 2; +// string message = 3; +// } +constexpr std::string_view base64_raw_proto{ + "Cg1wYXlsb2FkLnByb3RvEgxjb20ucmVkcGFuZGEaH2dvb2dsZS9wcm90b2J1Zi90aW1lc3RhbXAu" + "cHJvdG8iRQoHUGF5bG9hZBILCgN2YWwYASABKAUSLQoJdGltZXN0YW1wGAIgASgLMhouZ29vZ2xl" + "LnByb3RvYnVmLlRpbWVzdGFtcCLgAQoBQRraAQoBQhrUAQoBQxrOAQoBRBoFCgNNMDAaBQoDTTAx" + "GgUKA00wMhoFCgNNMDMaBQoDTTA0GgUKA00wNRoFCgNNMDYaBQoDTTA3GgUKA00wOBoFCgNNMDka" + "BQoDTTEwGgUKA00xMRoFCgNNMTIaBQoDTTEzGgUKA00xNBoFCgNNMTUaBQoDTTE2GgUKA00xNxpL" + "Cg1OZXN0ZWRQYXlsb2FkEgsKA3ZhbBgBIAEoBRItCgl0aW1lc3RhbXAYAiABKAsyGi5nb29nbGUu" + "cHJvdG9idWYuVGltZXN0YW1wImIKE0NvbXByZXNzaWJsZVBheWxvYWQSCwoDdmFsGAEgASgFEi0K" + "CXRpbWVzdGFtcBgCIAEoCzIaLmdvb2dsZS5wcm90b2J1Zi5UaW1lc3RhbXASDwoHbWVzc2FnZRgD" + "IAEoCUILUAFaBy4vO21haW5iBnByb3RvMw=="}; + +const pps::canonical_schema_definition base64_proto{ + pps::canonical_schema_definition{ + base64_raw_proto, pps::schema_type::protobuf}}; From 3aacc02536a970997c8e707e20db1d7a28ed9a73 Mon Sep 17 00:00:00 2001 From: Ben Pope Date: Mon, 29 Jul 2024 18:35:21 +0100 Subject: [PATCH 2/2] schema_registry: Disable protobuf logging Fixes CORE-2144 Signed-off-by: Ben Pope --- src/v/redpanda/application.cc | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/src/v/redpanda/application.cc b/src/v/redpanda/application.cc index 165724cc22a5e..b0b4c177a5816 100644 --- a/src/v/redpanda/application.cc +++ b/src/v/redpanda/application.cc @@ -147,6 +147,7 @@ #include #include +#include #include #include @@ -525,6 +526,12 @@ void application::initialize( .get(); _cpu_profiler.invoke_on_all(&resources::cpu_profiler::start).get(); + /* + * Disable the logger for protobuf; some interfaces don't allow a pluggable + * error collector. + */ + google::protobuf::SetLogHandler(nullptr); + /* * allocate per-core zstd decompression workspace and per-core * async_stream_zstd workspaces. it can be several megabytes in size, so do