Add support to use PEM string for SSL #3868
Conversation
Make it possible to use CA certificate string (PEM format) for verifying the broker's key, Client's private key string (PEM format) used for authentication and Client's public key string (PEM format) used for authentication.
|
Hi @stephen37. Thanks for your PR. I'm waiting for a SeldonIO or todo member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the jenkins-x/lighthouse repository. |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
| ClientCertFile: GetEnv("KAFKA_SSL_CLIENT_CERT_FILE", ""), | ||
| ClientKeyFile: GetEnv("KAFKA_SSL_CLIENT_KEY_FILE", ""), | ||
| CACertFile: GetEnv("KAFKA_SSL_CA_CERT_FILE", ""), | ||
| ClientKeyPass: GetEnv("KAFKA_SSL_CLIENT_KEY_PASS", ""), | ||
| // Optional password | ||
| ClientKeyPass: GetEnv("KAFKA_SSL_CLIENT_KEY_PASS", ""), |
There was a problem hiding this comment.
I assume if password is set to empty string as no env setting its ok later when its set in Kafka options? Will it be ignored?
There was a problem hiding this comment.
Yes, it will be ignored 😃 It has been tested in the worker_test.go file
|
@stephen37 failing some tests. |
|
@cliveseldon Yes tests won't work because the version of Kafka is too old, I can't fix the test until we use v1.8.2. The error is |
OK so will you add a PR for that so we can get in and rebase? |
|
Sure, here is the PR #3870, I just assumed it would be more complicated than that to upgrade. Also the tests work locally 🙌 |
|
Ah I guess I need you yo do something so that we can check if tests pass now @cliveseldon. |
|
Yaaay tests are passing now 🎉 Only the documentation tests are failing but it seems like they are not working at the moment. |
|
/test integration |
|
/test notebooks |
|
@stephen37 Looks good will run longer integration test. There are 1 or 2 flaky ones so lets see how they go. And yes docs tests seems to have an issue at present. |
|
The integration test failed but I don't have access to it, how can I check what failed? @cliveseldon |
|
/test integration |
|
@stephen37 it failed during test setup. We are looking to see if we can open up the logs without auth but for now will rerun tests. There are a couple of flaky tests so if its just those we can proceed as notebook tests succeed. |
|
@stephen37: The following test failed, say
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the jenkins-x/lighthouse repository. I understand the commands that are listed here. |
|
As just 2 tests failing in integration but we know these have issues can merge. Thanks @stephen37 |
What this PR does / why we need it:
Make it possible to use CA certificate string (PEM format) for verifying the broker's key,
Client's private key string (PEM format) used for authentication and Client's public key string (PEM format) used for authentication.
Without that, it is only possible to use SSL with path to files for PEM.
Which issue(s) this PR fixes:
Fixes #3867
Special notes for your reviewer:
I'm gonna add some documentation later :)