Prevent Kafka protobuf schema registry message parsing dead loop recursion by mx123 · Pull Request #15312 · trinodb/trino

mx123 · 2022-12-06T09:40:14Z

Description

In case when Kafka protobuf registry has included self referenced object types in schema it falls with stack overflow exception. To improve user experience the schema parsing is need to break with unsupported type that will be displayed to user in more meaningful manner.

.../trino-kafka/src/test/java/io/trino/plugin/kafka/protobuf/TestingUnsupportedNestedTypes.java

plugin/trino-kafka/pom.xml

core/trino-main/src/main/java/io/trino/cost/ValuesStatsRule.java

...n/trino-kafka/src/main/java/io/trino/plugin/kafka/encoder/protobuf/ProtobufSchemaParser.java

kokosing

Looks good, minor comments

.../io/trino/plugin/kafka/protobuf/TestKafkaProtobufWithSchemaRegistryMinimalFunctionality.java

plugin/trino-kafka/pom.xml

.../io/trino/plugin/kafka/protobuf/TestKafkaProtobufWithSchemaRegistryMinimalFunctionality.java

...n/trino-kafka/src/main/java/io/trino/plugin/kafka/encoder/protobuf/ProtobufSchemaParser.java

plugin/trino-kafka/src/test/resources/protobuf/google/protobuf/timestamp.proto

...n/trino-kafka/src/main/java/io/trino/plugin/kafka/encoder/protobuf/ProtobufSchemaParser.java

Praveen2112 · 2022-12-08T10:24:37Z

plugin/trino-kafka/src/test/resources/protobuf-sources/unsupported_nested.proto

Can we have them in protobuf directory so that all the protobuf files would be in one directory ?

Unfortunately, then generator will create objects for all proto files, but existing protos are intended to be used with DynamicMessage and these objects will be unneded/unused. On attempt to move existing protos to new folder unreachable for generator it will be need to refactor many reference places in few libs since those proto file location depends ProtobufUtils class package (since it's getting them by getResource, see for details). So that's looks like out of scope of the current issue.

This will be solved if we parse the proto files and generate the DynamicMessage then we could have all the proto files in one directory.

but we cannot due to 0710dce#r1043458738

Praveen2112 · 2022-12-08T10:27:43Z

.../io/trino/plugin/kafka/protobuf/TestKafkaProtobufWithSchemaRegistryMinimalFunctionality.java

Is it possible for us to create the DynamicMessage via descriptor and use them to populate it ?

Protobuf static generator is used to workaround of the PR unrelated issue, when the parsing by ProtobufUtils of the unsupported_nested.proto throws:

com.google.protobuf.Descriptors$DescriptorValidationException: io.trino.protobuf.schema.nested_value_one: "NestedValue" is not an enum type. at com.google.protobuf.Descriptors$FieldDescriptor.crossLink(Descriptors.java:1587) at com.google.protobuf.Descriptors$FieldDescriptor.access$1400(Descriptors.java:1057) at com.google.protobuf.Descriptors$Descriptor.crossLink(Descriptors.java:1000) at com.google.protobuf.Descriptors$Descriptor.access$1200(Descriptors.java:648) at com.google.protobuf.Descriptors$FileDescriptor.crossLink(Descriptors.java:600) at com.google.protobuf.Descriptors$FileDescriptor.buildFrom(Descriptors.java:321) at com.google.protobuf.Descriptors$FileDescriptor.buildFrom(Descriptors.java:289) at io.trino.decoder.protobuf.ProtobufUtils.getFileDescriptor(ProtobufUtils.java:128) at io.trino.decoder.protobuf.ProtobufUtils.getFileDescriptor(ProtobufUtils.java:82) at io.trino.decoder.protobuf.ProtobufUtils.getFileDescriptor(ProtobufUtils.java:75) ....

lib/trino-record-decoder/src/main/java/io/trino/decoder/protobuf/ProtobufUtils.java

...n/trino-kafka/src/main/java/io/trino/plugin/kafka/encoder/protobuf/ProtobufSchemaParser.java

Praveen2112 · 2022-12-08T10:31:11Z

.../io/trino/plugin/kafka/protobuf/TestKafkaProtobufWithSchemaRegistryMinimalFunctionality.java

Now we have disabled this parsing so as we don't run into a infinite loop. Is it possible for us to fix this issue - if we have a complicated schema like this ?

Generally the issue of sophisticated schema parsing is need to be address to presence of output format data structure. As i see Trino SQL data types have not nothing similar to describe this object relation format. So, in common case now the issue cannot be resolved. But if we have particular known to us object structure we can describe it with some limitation to nested self referenced objects inclusions. It will be similar with google.protobuf.Timestamp object parsing implementation .

When refresh token is retrieved for UI, currently we were sending HTTP Status 303, assuming that all the request will just repeat the call on the Location header. When this works for GET/PUT verbs, it does not for non-idempotent ones like POST, as every js http client should do a GET on LOCATION after 303 on POST. Due to that I change it to 307, that should force every client to repeat exactly the same request, no matter the verb. Co-authored-by: s2lomon <s2lomon@gmail.com>

Actual work is done in `pageProjectWork.process()` call while `projection.project` only performs setup of projection. So both `expressionProfiler` and `metrics.recordProjectionTime` needed to be around that method.

Removes outdated comments and unnecessary methods in local exchange PartitioningExchanger since the operator is no longer implemented in a way that attempts to be thread-safe.

- Change ColumnHandle to BigQueryColumnHandle in BigQueryTableHandle - Extract buildColumnHandles in BigQueryClient

kokosing

Looks good to me. Just few comments.

.../io/trino/plugin/kafka/protobuf/TestKafkaProtobufWithSchemaRegistryMinimalFunctionality.java

kokosing · 2022-12-14T12:25:58Z

plugin/trino-kafka/pom.xml

Why we are using this library? Why not simple HashSet? Is it about we are not removing items from it when we are closing the stack?

Is it possible to have a test for a case where using this library is needed (closing the recursion)? Is it possible (and reasonable) to not use this dependency?

please see my explanation #15312 (comment)

Kafka protobuf registry schema cannot be translated to plain Trino SQL structure in common cases when included data types use referencing to same objects recursively.

Message sending to Kafka thows NPE if proto file has `import` derective.

Kafka protobuf schema registry parsing falls to dead loop if has included references to the same object. It's need to abort the recursion loop with appropriated parsing error.

kokosing · 2023-01-04T10:41:15Z

@mx123 Can you please rebase?

mx123 · 2023-02-09T11:33:50Z

closed due to PR#15724 opened

cla-bot bot added the cla-signed label Dec 6, 2022

mx123 changed the title ~~Kafka protobuf schema registry nested objects~~ Prevent Kafka protobuf schema registry message parsing dead loop recursion Dec 6, 2022

mx123 requested review from Praveen2112 and kokosing December 6, 2022 09:42

kokosing reviewed Dec 6, 2022

View reviewed changes

hashhar reviewed Dec 6, 2022

View reviewed changes

...n/trino-kafka/src/main/java/io/trino/plugin/kafka/encoder/protobuf/ProtobufSchemaParser.java Outdated Show resolved Hide resolved

mx123 force-pushed the kafka_protobuf_schema_registry_nested_objects branch 4 times, most recently from 880535a to 56ffe83 Compare December 7, 2022 09:19

kokosing reviewed Dec 7, 2022

View reviewed changes

mx123 force-pushed the kafka_protobuf_schema_registry_nested_objects branch 2 times, most recently from 5420cd6 to 8291c7d Compare December 7, 2022 16:59

Praveen2112 reviewed Dec 8, 2022

View reviewed changes

mx123 force-pushed the kafka_protobuf_schema_registry_nested_objects branch 3 times, most recently from 91f48c2 to 6935b25 Compare December 9, 2022 10:48

dain and others added 13 commits December 11, 2022 17:32

Add Redshift tests

87b8e6f

Add Redshift schema, table, and column length checks

0312463

Implement proper type mapping for Redshift

7eab69a

Implement Redshift DELETE

1e8887e

Add Redshift statistics

e65585d

Add Redshift pushdown

c225b8f

Test SET PATH support by clients

be62233

Fix recording of projection metrics

c056bc7

Actual work is done in `pageProjectWork.process()` call while `projection.project` only performs setup of projection. So both `expressionProfiler` and `metrics.recordProjectionTime` needed to be around that method.

Fix dereference operations for union type in Hive Connector

a46a510

Cleanup PartitioningExchanger

0459735

Removes outdated comments and unnecessary methods in local exchange PartitioningExchanger since the operator is no longer implemented in a way that attempts to be thread-safe.

Fix table name in TestDropTableTask

1788829

Document examples for datetime functions

3c1c1fa

jkylling and others added 4 commits December 13, 2022 13:40

Decode path as URI in Delta Lake connector

58bd159

Remove unused method from AstBuilder

35da905

Refactor BigQuery connector

ce7b57f

- Change ColumnHandle to BigQueryColumnHandle in BigQueryTableHandle - Extract buildColumnHandles in BigQueryClient

Fix projection pushdown when unsupported column exists in BigQuery

a660133

mx123 mentioned this pull request Dec 13, 2022

Support protobuf build-in Value within c #15384

Closed

Fokko and others added 3 commits December 13, 2022 16:25

Update Iceberg to 1.1.0

cd8eac6

Document Top-N pushdown

b9d26a7

Remove unnecessary override for getTableProperties method

f5b5fb8

kokosing reviewed Dec 14, 2022

View reviewed changes

mx123 added 4 commits December 14, 2022 15:24

Reproduce kafka protobuf schema parsing stack overflow error

1a79def

Kafka protobuf registry schema cannot be translated to plain Trino SQL structure in common cases when included data types use referencing to same objects recursively.

Reproduce NPE on Kafka sending when proto file has import

e669359

Message sending to Kafka thows NPE if proto file has `import` derective.

Use declared variable for descriptor

e5b3590

Prevent protobuf schema message parsing dead loop recursion

4cad364

Kafka protobuf schema registry parsing falls to dead loop if has included references to the same object. It's need to abort the recursion loop with appropriated parsing error.

mx123 force-pushed the kafka_protobuf_schema_registry_nested_objects branch from 6935b25 to 4cad364 Compare December 14, 2022 13:24

martint force-pushed the master branch from 40dbb4f to 0d73d10 Compare December 19, 2022 20:15

hashhar mentioned this pull request Jan 16, 2023

Improve error message in Kafka connector when parsing recursive protobuf schemas #15724

Merged

mx123 closed this Feb 9, 2023

Conversation

mx123 commented Dec 6, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

kokosing left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mx123 Dec 8, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kokosing left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kokosing commented Jan 4, 2023

Uh oh!

mx123 commented Feb 9, 2023

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

16 participants

mx123 commented Dec 6, 2022 •

edited

Loading

mx123 Dec 8, 2022 •

edited

Loading