-
Notifications
You must be signed in to change notification settings - Fork 10
Secure Kafka docs #71
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
Conversation
tomncooper
left a comment
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 is a very comprehensive tutorial, thanks for putting in the hard work to verify all these options.
No major issues. I think each section probably needs an intro paragraph explaining some of the finer details of each option. Also a bit more detail early on, on things like loading certificates would make things flow better. There is more detail on what I mean in my comments.
Great work.
|
@Frawless @kornys Do we have tests for secure Kafka communication in streams-e2e? If not, could the work in this tutorial form a base for that? |
Just scram sha user passing secret testing is in streams-e2e. |
|
@tomncooper but answer for your question is -> yes it it can be based on this work if we want to have test for mtls etc.. |
tomncooper
left a comment
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.
LGTM, just a few nits. You seem to have a lot of hard wrapping of lines going on. Generally we try to use 1 line per sentence.
katheris
left a comment
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.
From a Strimzi perspective this looks correct. I just added a couple of minor suggestions and then a question around the use of PKCS12 files over PEM files
docs/secure-kafka/_index.md
Outdated
| secret: | ||
| secretName: my-cluster-cluster-ca-cert | ||
| items: | ||
| - key: ca.crt # We only need the cluster CA certificate |
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 is fine for this simple example, but we should consider how we would recommend users do this in a way that handles a new CA key being created. For example by loading all .crt files in the Secret.
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.
Hmm, that might be a bit advanced for this tutorial?
@tomncooper Do you think we should include that? We already have a simple example of using a self-signed CA in the "custom" section.
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.
Is there a strimzi doc link we can point to for proper production setup?
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 couldn't find one (@katheris maybe you know of one?)
Considering there may be one or more certificate(s) during the CA renewal period, I'm not sure how we'd even handle that anyway (maybe add glob support to the secret interpolater to combine the contents of multiple files?).
I added a comment for now, maybe that's enough for TP? d9403ab
Frawless
left a comment
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 just did quick walkthrough ... isn't data-gen-setup.sh a little bit strange name for script that basically setup everything?
f6eab10 to
6a0cf8d
Compare
tomncooper
left a comment
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.
Just a couple of nits but otherwise LGTM.
docs/secure-kafka/_index.md
Outdated
| secret: | ||
| secretName: my-cluster-cluster-ca-cert | ||
| items: | ||
| - key: ca.crt # We only need the cluster CA certificate |
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.
Is there a strimzi doc link we can point to for proper production setup?
5e955e6 to
4888bf4
Compare
Co-authored-by: Thomas Cooper <code@tomcooper.dev>
Co-authored-by: Kate Stanley <11195226+katheris@users.noreply.github.com>
Co-authored-by: Thomas Cooper <code@tomcooper.dev>
4888bf4 to
6718564
Compare
|
@tomncooper Ready for merge? 👀 |
Adds example(s) and docs for securely connecting / authenticating between Flink SQL (from our distribution) and Kafka (Strimzi). Refactors
data-gen-setup.shscript for tutorial reader ease-of-use.PR for ENTMQSTFL-251.
Depends on streamshub/flink-sql#101
Likely needs review from a Strimzi maintainer(s) to verify best practices.