-
Notifications
You must be signed in to change notification settings - Fork 466
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
Pulsar PubSub: allow TLS configuration for hostname and insecure connections #3270
Pulsar PubSub: allow TLS configuration for hostname and insecure connections #3270
Conversation
Signed-off-by: Thomas O'Neill <toneill@new-innov.com>
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 the contribution.
This PR needs an integration test (certification test) at:
https://github.com/dapr/components-contrib/tree/main/tests/certification/pubsub/pulsar
You can take a look at some other certification tests that specifically test the TLS modes for other components. The hardest part will be creating a docker compose file / setup which spins up Pulsar with the TLS configuration that you need to test. Then you need a Dapr component configuration which actually sets the appropriate new TLS settings. For the actual operations you can just reuse the code that exists in the basic test today.
Note that the existing test run should not be updated to use TLS, this needs to be a separate test run.
Signed-off-by: Thomas O'Neill <toneill@new-innov.com>
@berndverst Integration tests have been added for pulsar |
@toneill818 the Pulsar certification test is failing - see the build check |
Signed-off-by: Thomas O'Neill <toneill@new-innov.com>
@toneill818 the pulsar certification test is still failing |
@yaron2 I'll keep looking into this. It isn't timing out while I run the tests locally. |
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.
Normally, test failures in cert tests like these, which happen only on the CI, are due to a race condition somewhere in the code.
I am not sure where specifically. My advice would be to try running one of the tests at a time, multiple times, and see if you can at least identify where this is happening :(
Thanks for looking into this!
Signed-off-by: Thomas O'Neill <toneill@new-innov.com>
Signed-off-by: Thomas O'Neill <toneill@new-innov.com>
Signed-off-by: Patrick Assuied <patrick.assuied@elationhealth.com> Co-authored-by: Alessandro (Ale) Segala <43508+ItalyPaleAle@users.noreply.github.com> Signed-off-by: Thomas O'Neill <toneill@new-innov.com>
Signed-off-by: Eunice Compra <eunicecompra@gmail.com> Signed-off-by: ItalyPaleAle <43508+ItalyPaleAle@users.noreply.github.com> Signed-off-by: Fabian Martinez <46371672+famarting@users.noreply.github.com> Signed-off-by: Alessandro (Ale) Segala <43508+ItalyPaleAle@users.noreply.github.com> Co-authored-by: Eunice Compra <eunice.compra@gmail.com> Co-authored-by: Alessandro (Ale) Segala <43508+ItalyPaleAle@users.noreply.github.com> Co-authored-by: Bernd Verst <github@bernd.dev> Co-authored-by: Fabian Martinez <46371672+famarting@users.noreply.github.com> Co-authored-by: Yaron Schneider <schneider.yaron@live.com> Signed-off-by: Thomas O'Neill <toneill@new-innov.com>
Signed-off-by: Patrick Assuied <patrick.assuied@elationhealth.com> Signed-off-by: Thomas O'Neill <toneill@new-innov.com>
Signed-off-by: Eunice Compra <eunicecompra@gmail.com> Co-authored-by: Eunice Compra <eunice.compra@gmail.com> Signed-off-by: Thomas O'Neill <toneill@new-innov.com>
Signed-off-by: Pravin Pushkar <ppushkar@microsoft.com> Signed-off-by: Thomas O'Neill <toneill@new-innov.com>
Signed-off-by: Ryan Lettieri <ryanLettieri@microsoft.com> Signed-off-by: Thomas O'Neill <toneill@new-innov.com>
Signed-off-by: ItalyPaleAle <43508+ItalyPaleAle@users.noreply.github.com> Signed-off-by: Thomas O'Neill <toneill@new-innov.com>
This pull request has been automatically marked as stale because it has not had activity in the last 30 days. It will be closed in 7 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions! |
This pull request has been automatically closed because it has not had activity in the last 37 days. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions! |
This pull request has been automatically marked as stale because it has not had activity in the last 30 days. It will be closed in 7 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions! |
Signed-off-by: Bernd Verst <github@bernd.dev>
@toneill818 v the certification tests are all failing, please take a look |
a08257a
to
bb5df50
Compare
Signed-off-by: Thomas O'Neill <toneill818@gmail.com> Signed-off-by: Thomas O'Neill <toneill@new-innov.com>
Signed-off-by: Thomas O'Neill <toneill@new-innov.com>
9928378
to
9ebd09f
Compare
This pull request has been automatically marked as stale because it has not had activity in the last 30 days. It will be closed in 7 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions! |
This pull request has been automatically closed because it has not had activity in the last 37 days. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions! |
Description
When connecting to pulsar with TLS enabled set the prefix to be
pulsar+ssl://
if a prefix was not set when defining the host. Allow setting theTLSAllowInsecureConnection
andTLSValidateHostname
options when connecting to pulsar. They default to the pulsar client's default.Issue reference
Please reference the issue this PR will close: #1860
Please reference the issue this PR will close: #3269
Checklist
Please make sure you've completed the relevant tasks for this PR, out of the following list: