8000 Remove Kafka scaler requirement for CA/cert/key by iterion · Pull Request #1288 · kedacore/keda · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Remove Kafka scaler requirement for CA/cert/key #1288

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Oct 23, 2020
Merged

Remove Kafka scaler requirement for CA/cert/key #1288

merged 1 commit into from
Oct 23, 2020

Conversation

iterion
Copy link
Contributor
@iterion iterion commented Oct 23, 2020 •
edited by zroubalik
Loading

This PR removes some validations from the TriggerAuthentication derived config for kafka. This allows you to use TLS without needing to specify a CA/cert/key. It was tested with an AWS MSK Kafka cluster. I haven't yet completed the checklist, but happy to do so.

Later validations in the shared TLS config allow us to just pass through the empty strings here when not specified.

Checklist

@zroubalik
Copy link
Member

@iterion do you think it could solve this issue? #1241

@zroubalik
Copy link
Member
zroubalik commented Oct 23, 2020 •
edited
Loading

@iterion could you please update the relevant section in the docs? https://keda.sh/docs/2.0/scalers/apache-kafka/#authentication-parameters (remove the Required from the parameters and add a note about TLS without CA/cert/key)

@zroubalik
Copy link
Member

Fix the tests please

iterion added a commit to iterion/keda-docs that referenced this pull request Oct 23, 2020
@iterion
Remove Kafka scaler CA/Cert/Key requirements
b8a5b14 
Related to kedacore/keda#1288.
@iterion iterion mentioned this pull request Oct 23, 2020
Merged
@iterion
Copy link
Contributor Author
iterion commented Oct 23, 2020

I don't have enough details on the specific problem in #1241 to say with certainty. But, I believe Confluent does allow you to enable TLS only for encryption and not for authentication and uses publicly trusted CAs. If all of those are true, it would exactly mirror the issue we faced and this PR would resolve it.

PR for docs (I'll fix the DCO shortly):
kedacore/keda-docs#287

And, I'll fix the tests soon.

@zroubalik
Copy link
Member

@iterion great. Once this PR is ok, I'll merge it and will immediately trigger a release of KEDA 2.0 RC2.

So your PR arrived just in time :)

@iterion iterion force-pushed the remove-kafka-tls-validations branch from 805dc9e to e9b1e18 Compare October 23, 2020 15:00
iterion added a commit to iterion/keda-docs that referenced this pull request Oct 23, 2020
@iterion
Remove Kafka scaler CA/Cert/Key requirements
82a1077 
Related to kedacore/keda#1288.

Signed-off-by: iterion <adam.sunderland@zapier.com>
@iterion
Remove Kafka scaler requirement for CA/cert/key
8f18e30 
Signed-off-by: iterion <adam.sunderland@zapier.com>
@iterion iterion force-pushed the remove-kafka-tls-validations branch from e9b1e18 to 8f18e30 Compare October 23, 2020 15:11
@iterion
Copy link
Contributor Author
iterion commented Oct 23, 2020

I realized through the tests that it's still worth checking for cert missing when keys are specified and the opposite. So, I added a bit of handling for that and updated the test variants. I removed some as they seemed like duplicates, but happy to add them back.

I also updated the docs PR to better note that specifying cert requires key and I signed my commit. Let me know if there is anything else to address.

@zroubalik
Copy link
Member

I realized through the tests that it's still worth checking for cert missing when keys are specified and the opposite. So, I added a bit of handling for that and updated the test variants. I removed some as they seemed like duplicates, but happy to add them back.

Makes sense!

I also updated the docs PR to better note that specifying cert requires key

I don't see this note in the PR, have you push it? 😄

iterion added a commit to iterion/keda-docs that referenced this pull request Oct 23, 2020
@iterion
Remove Kafka scaler CA/Cert/Key requirements
9599288 
Related to kedacore/keda#1288.

Signed-off-by: iterion <adam.sunderland@zapier.com>
@iterion
Copy link
Contributor Author
iterion commented Oct 23, 2020

Sorry, yes, just did. I pushed the DCO but forgot to add the changes locally. 😅

iterion added a commit to iterion/keda-docs that referenced this pull request Oct 23, 2020
@iterion
Remove Kafka scaler CA/Cert/Key requirements
5d596c8 
Related to kedacore/keda#1288.

Signed-off-by: iterion <adam.sunderland@zapier.com>
@zroubalik zroubalik merged commit 7553e9a into kedacore:v2 Oct 23, 2020
@zroubalik zroubalik mentioned this pull request Oct 23, 2020
Closed
@iterion iterion deleted the remove-kafka-tls-validations branch October 23, 2020 15:42
@silasfrigo silasfrigo mentioned this pull request Sep 22, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ahmelsayed ahmelsayed Awaiting requested review from ahmelsayed

@zroubalik zroubalik Awaiting requested review from zroubalik

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@iterion @zroubalik
0