Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Conversation

eliax1996
Copy link
Contributor

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.

@eliax1996 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 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 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
@GSokol
Copy link

GSokol commented Nov 9, 2023

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

@eliax1996 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
Copy link
Contributor Author

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

@eliax1996 eliax1996 marked this pull request as ready for review November 9, 2023 18:57
@eliax1996 eliax1996 requested review from a team as code owners November 9, 2023 18:57
@eliax1996 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 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
Copy link
Contributor

@jjaakola-aiven jjaakola-aiven left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

README.rst Show resolved Hide resolved
topic_name: TopicName,
skip_validation: bool,
) -> None:
key = {"topic": topic_name, "keytype": str(MessageType.schema_validation), "magic": 0}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

still a valid point, taking a look tomorrow

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

discussed offline, probably solved. Waiting an update on that

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 Show resolved Hide resolved
@GSokol
Copy link

GSokol commented Nov 15, 2023

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
Copy link
Contributor Author

eliax1996 commented Nov 15, 2023

@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
Copy link

GSokol commented Nov 16, 2023

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

@eliax1996 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
@eliax1996 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
@eliax1996 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
Copy link
Contributor

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.

Copy link
Contributor

@tvainika tvainika left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

schema_request=True,
with_request=False,
json_body=False,
auth=None,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

) -> 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)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@eliax1996 eliax1996 Nov 24, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@eliax1996 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
@eliax1996 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
@eliax1996 eliax1996 closed this Oct 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants