-
Notifications
You must be signed in to change notification settings - Fork 73
Add proposal for supporting SSL for KafkaBridge #171
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
base: main
Are you sure you want to change the base?
Conversation
Signed-off-by: Gantigmaa Selenge <[email protected]>
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 some quick thoughts.
The way we setup the certificates seems also important to whether we ever expect to have some better scalability or not I guess. |
Signed-off-by: Gantigmaa Selenge <[email protected]>
With this proposal, the same certificate and key (via referenced secret in the CR) will be volume mounted into each KafkaBridge pod if it is scaled to multiple replicas, which is not different than how TLS authentication is configured for KafkaBridge talking to Kafka. My understanding is that KafkaBridge is not truly scalable in terms of consumers, but I'm not sure what is the expectation at the moment, in terms of improving that and what to consider for the certificate setup. |
I've marked this as ready for review now :) |
Signed-off-by: Gantigmaa Selenge <[email protected]>
Signed-off-by: Gantigmaa Selenge <[email protected]>
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 proposal, it looks good to me, just few small things.
Signed-off-by: Gantigmaa Selenge <[email protected]>
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, thanks a lot Tina :)
…view comments Signed-off-by: Gantigmaa Selenge <[email protected]>
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.
Few more nits. Mostly formatting. The only real point for discussion is about the SSL protocols. Looks good otherwise.
Signed-off-by: Gantigmaa Selenge <[email protected]>
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. LGTM, +1 binding.
Thank you so much @scholzj @im-konge! @ppatierno could you please take another look when you get a chance? Thank you! @PaulRMellor @katheris could you please also take a look if you have time? Thank you! |
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, Tina.
I made some suggestions as I read though.
I wonder if the proposal would benefit from mentioning requirements for documentation and testing?
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.
Overall this makes sense to me, I just added some clarification and wording comments
Signed-off-by: Gantigmaa Selenge <[email protected]>
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 updates, Tina
POC:
https://github.com/strimzi/strimzi-kafka-bridge/compare/main...tinaselenge:strimzi-kafka-bridge:ssl-support?expand=1
https://github.com/strimzi/strimzi-kafka-operator/compare/main...tinaselenge:bridge-ssl?expand=1
Related to strimzi/strimzi-kafka-bridge#939