feat: enable users to disable validation only on specific topics #759

eliax1996 · 2023-11-09T11:12:16Z

This PR enables users to avoid validation only on specific topics. It is necessary because, prior to this commit it was possible to create messages with incorrect or missing subjects. By fixing the validation of subject flows that were previously functioning (but shouldn't have been) now no longer work due to the added validation.

With this PR, users can migrate their flows one by one while adding new validated flows, rather than opting out of validation completely if they wish to retain the functionality of old flows without refactoring them.

GSokol · 2023-11-09T16:15:36Z

Thank you so much, @eliax1996! We are now struggling from disability of publishing new schemes for existing topics.

eliax1996 · 2023-11-09T18:57:46Z

@GSokol glad to know that this would help. I will create a new optional release in Aiven as soon as it is merged.

jjaakola-aiven

Some comments, I need to do another round of reading and thinking.

README.rst

jjaakola-aiven · 2023-11-14T13:45:36Z

karapace/schema_registry.py

+        topic_name: TopicName,
+        skip_validation: bool,
+    ) -> None:
+        key = {"topic": topic_name, "keytype": str(MessageType.schema_validation), "magic": 0}


The formatting of the key needs to be aligned in the key_format.py. This record needs the canonical format, topic is an unknown key in data currently.
How does the Confluent Schema Registry react to custom message like this?

still a valid point, taking a look tomorrow

discussed offline, probably solved. Waiting an update on that

I just realized that I need to add the topic to the key, otherwise the compaction could delete the topic information, this doesn't change the behaviour of confluent schema registry that is simply to skip the messages he cannot understand (a behaviour that we also do while receiving messages that aren't parsable by us)

karapace/schema_registry_apis.py

GSokol · 2023-11-15T09:47:16Z

Also it's good to have a config setting, to disable schema validation by default. But it could be done in a separate PR to make this thing in faster.

eliax1996 · 2023-11-15T10:00:36Z

@GSokol this is already implemented (and merged and available in Aiven), if you look there is a config called name_strategy_validation that if set to false disable the validation. This PR is just to enable the users to explicitly disable the validation only on a specific subset of topics since using the validation brings more order to the topics (otherwise it's basically like Kafka vanilla where you can write anything in a topic)

In that way, users who didn't follow a validation strategy in the past can now start using it without losing or having to rewrite the existing flows but they can enforce the validation on the new ones

GSokol · 2023-11-16T08:40:43Z

@eliax1996, name_strategy_validation is only used in the REST API logic.

…ypes of formats

tvainika · 2023-11-23T07:51:15Z

Hi @GSokol, you wrote that

name_strategy_validation is only used in the REST API logic.

and it made me curious about your expectations regarding this feature. This PR adds ability to disable validation per topic, but also this configuration affects only REST API logic. I hope this still matches your expectations.

tvainika

Access control for new endpoint needs to be defined and documented.

Also new API endpoints need some documentation of semantics. For example if configuration name_strategy_validation is disabled, is it sill possible to enable validation for specific topics? With this code it seems that is not possible. Would it be useful, or not? Maybe per topic validation endpoints should reject setting if name_strategy_validation is disabled?

tvainika · 2023-11-23T07:53:14Z

karapace/schema_registry_apis.py

+            schema_request=True,
+            with_request=False,
+            json_body=False,
+            auth=None,


auth=None means anyone can change this setting. I think this should be better defined what permissions are needed to change validation configuration per topic, and also documented in README.

tvainika · 2023-11-23T08:10:14Z

karapace/schema_registry.py

+    ) -> None:
+        key = {"keytype": str(MessageType.schema_validation), "magic": 0}
+        value = {"skip_validation": skip_validation, "topic": topic_name}
+        self.producer.send_message(key=key, value=value)


Depending on skip_validation this could also send tombstone record instead of toggling between true/false.

I don't think so, otherwise how can you enable again the validation for a certain topic (after you disable it)?

eliax1996 force-pushed the eliax1996/enable-users-to-disable-validation-only-on-specific-topics branch 6 times, most recently from 474140a to 0932649 Compare November 9, 2023 14:12

eliax1996 changed the title ~~Eliax1996/enable users to disable validation only on specific topics~~ feat: enable users to disable validation only on specific topics Nov 9, 2023

eliax1996 force-pushed the eliax1996/enable-users-to-disable-validation-only-on-specific-topics branch 3 times, most recently from 65bf208 to 1fd6c57 Compare November 9, 2023 16:08

eliax1996 force-pushed the eliax1996/enable-users-to-disable-validation-only-on-specific-topics branch from 1fd6c57 to bf1f114 Compare November 9, 2023 18:56

eliax1996 marked this pull request as ready for review November 9, 2023 18:57

eliax1996 requested review from a team as code owners November 9, 2023 18:57

eliax1996 force-pushed the eliax1996/enable-users-to-disable-validation-only-on-specific-topics branch 3 times, most recently from a0e13c6 to d6c1221 Compare November 10, 2023 10:23

eliax1996 requested review from tvainika and giuseppelillo November 10, 2023 10:25

eliax1996 force-pushed the eliax1996/enable-users-to-disable-validation-only-on-specific-topics branch 3 times, most recently from 1f024a8 to 7603add Compare November 10, 2023 17:13

jjaakola-aiven reviewed Nov 14, 2023

View reviewed changes

eliax1996 force-pushed the eliax1996/enable-users-to-disable-validation-only-on-specific-topics branch 2 times, most recently from b1564e8 to 7cebf32 Compare November 21, 2023 17:19

feat: enable users to disable validation only on specific topics

ffc5550

eliax1996 force-pushed the eliax1996/enable-users-to-disable-validation-only-on-specific-topics branch 2 times, most recently from ee21be0 to c4cffdd Compare November 21, 2023 17:58

wip: adding integration tests for the name strategy and the various t…

20b7256

…ypes of formats

eliax1996 force-pushed the eliax1996/enable-users-to-disable-validation-only-on-specific-topics branch 4 times, most recently from 2cec245 to 3305ba6 Compare November 22, 2023 14:22

tvainika requested changes Nov 23, 2023

View reviewed changes

eliax1996 force-pushed the eliax1996/enable-users-to-disable-validation-only-on-specific-topics branch 2 times, most recently from 9b1eb58 to c8f35f3 Compare November 24, 2023 16:05

implemented feedback from the review

c5cfd46

eliax1996 force-pushed the eliax1996/enable-users-to-disable-validation-only-on-specific-topics branch from c8f35f3 to c5cfd46 Compare November 24, 2023 16:12

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: enable users to disable validation only on specific topics #759

feat: enable users to disable validation only on specific topics #759

eliax1996 commented Nov 9, 2023

GSokol commented Nov 9, 2023

eliax1996 commented Nov 9, 2023

jjaakola-aiven left a comment

jjaakola-aiven Nov 14, 2023

eliax1996 Nov 21, 2023

eliax1996 Nov 22, 2023

eliax1996 Nov 24, 2023

GSokol commented Nov 15, 2023

eliax1996 commented Nov 15, 2023 •

edited

GSokol commented Nov 16, 2023

tvainika commented Nov 23, 2023

tvainika left a comment

tvainika Nov 23, 2023

tvainika Nov 23, 2023

eliax1996 Nov 24, 2023 •

edited

feat: enable users to disable validation only on specific topics #759

Are you sure you want to change the base?

feat: enable users to disable validation only on specific topics #759

Conversation

eliax1996 commented Nov 9, 2023

GSokol commented Nov 9, 2023

eliax1996 commented Nov 9, 2023

jjaakola-aiven left a comment

Choose a reason for hiding this comment

jjaakola-aiven Nov 14, 2023

Choose a reason for hiding this comment

eliax1996 Nov 21, 2023

Choose a reason for hiding this comment

eliax1996 Nov 22, 2023

Choose a reason for hiding this comment

eliax1996 Nov 24, 2023

Choose a reason for hiding this comment

GSokol commented Nov 15, 2023

eliax1996 commented Nov 15, 2023 • edited

GSokol commented Nov 16, 2023

tvainika commented Nov 23, 2023

tvainika left a comment

Choose a reason for hiding this comment

tvainika Nov 23, 2023

Choose a reason for hiding this comment

tvainika Nov 23, 2023

Choose a reason for hiding this comment

eliax1996 Nov 24, 2023 • edited

Choose a reason for hiding this comment

eliax1996 commented Nov 15, 2023 •

edited

eliax1996 Nov 24, 2023 •

edited