-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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(pulsar sink): support tls options #22148
base: master
Are you sure you want to change the base?
feat(pulsar sink): support tls options #22148
Conversation
#[configurable_component] | ||
#[configurable(description = "TLS options configuration for the Pulsar client.")] | ||
#[derive(Clone, Debug)] | ||
pub struct PulsarTlsOptions { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it be possible to reuse the existing config struct we use for TLS options:
pub struct TlsConfig { |
Or does the pulsar client no support all of the same options?
At the least, we should match the option names (e.g. certificate_chain_file
should be ca_file
to match).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for your feedback.
rust client for pulsar only supports some options https://github.com/streamnative/pulsar-rs/blob/master/src/connection_manager.rs#L73
Changed to use the same field names as TlsConfig 66821d8.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice work on this @pomacanthidae ! I appreciate you updating the integration tests including generating the test certificates. I left a question about the new config option.
description: "TLS options configuration for the Pulsar client." | ||
required: false | ||
type: object: options: { | ||
allow_insecure_connection: { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this should be called verify_certificate
in order to match the existing options.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, fixed in ed01093
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good to me. Not merging just yet, I would like to give @jszwedko a chance to take another look.
Summary
This PR adds TLS options to enable Pulsar sinks and sources to use a custom certificate chain regarding #10888.
Integration tests for pulsar with TLS are added and test data in
tests/data/ca/intermediate_server
including pem files are generated by the following command.Change Type
Is this a breaking change?
How did you test this PR?
run integration test
Does this PR include user facing changes?
Checklist
Cargo.lock
), pleaserun
dd-rust-license-tool write
to regenerate the license inventory and commit the changes (if any). More details here.References