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

Add option to skip validation when dealing with schema registry #97

Merged
merged 7 commits into from
Jul 13, 2021

Conversation

robbavey
Copy link
Contributor

@robbavey robbavey commented Jul 9, 2021

This PR adds a validate_schema_registry option to the plugin, allowing
users to bypass the schema registry validation while using a plugin. This is
a common issue when using authenticated schema registry, as the validation occurs
outside of the 'KafkaConsumer' code, and will require considerable effort to maintain
parity with that method.

This configuration option has 2 settings auto and skip. The default auto setting
will skip validation when schema registry auth is used that is known to fail, currently
when Kerberos authentication is used, but will try and validate in other cases. skip
will skip regardless of authentication method.

Thanks for contributing to Logstash! If you haven't already signed our CLA, here's a handy link: https://www.elastic.co/contributor-agreement/

This PR adds a `validate_schema_registry` option to the plugin, allowing
users to bypass the schema registry validation while using a plugin. This is
a common issue when using authenticated schema registry, as the validation occurs
outside of the 'KafkaConsumer' code, and will require considerable effort to maintain
parity with that method.

This configuration option has 2 settings `auto` and `skip`. The default `auto` setting
will skip validation when schema registry auth is used that is known to fail, currently
when Kerberos authentication is used, but will try and validate in other cases. `skip`
will skip regardless of authentication method.
@elasticsearch-bot elasticsearch-bot self-assigned this Jul 12, 2021
@yaauie yaauie assigned yaauie and unassigned elasticsearch-bot Jul 12, 2021
Copy link
Contributor

@yaauie yaauie left a comment

Choose a reason for hiding this comment

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

Functionality looks good. I've left a couple nitpicks.

[id="plugins-{type}s-{plugin}-validate_schema_registry"]
===== `schema_registry_validation`

* Value can be either of: `auto`, `skip`
Copy link
Contributor

Choose a reason for hiding this comment

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

👍🏼 I like the auto/skip here. It conveys the complexity a lot better than a true/false would, and similarly doesn't convey promises that we will validate it (if it isn't set, or we can't validate) or that we won't ever do so (e.g., when we start using it and it doesn't provide what we need).

lib/logstash/plugin_mixins/common.rb Outdated Show resolved Hide resolved
spec/unit/inputs/kafka_spec.rb Outdated Show resolved Hide resolved
spec/unit/inputs/kafka_spec.rb Show resolved Hide resolved
@robbavey robbavey requested a review from karenzone July 12, 2021 18:03
Copy link
Contributor

@yaauie yaauie left a comment

Choose a reason for hiding this comment

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

Code LGTM 👍🏼

Copy link
Contributor

@karenzone karenzone left a comment

Choose a reason for hiding this comment

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

Anchors don't match between the table and description, and would crash the docs build. Pick your favorite.

I left suggested wording inline for your consideration. We do some of our best work when we iterate, so your suggestions to my suggestions are welcome (as always). :-)

docs/input-kafka.asciidoc Outdated Show resolved Hide resolved
docs/input-kafka.asciidoc Outdated Show resolved Hide resolved
Co-authored-by: Karen Metts <35154725+karenzone@users.noreply.github.com>
@karenzone karenzone changed the title Add option to skip vaildation when dealing with schema registry Add option to skip validation when dealing with schema registry Jul 12, 2021
@robbavey
Copy link
Contributor Author

@karenzone I've made the changes, ready for another round

Copy link
Contributor

@karenzone karenzone left a comment

Choose a reason for hiding this comment

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

LGTM

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.

None yet

5 participants