-
Notifications
You must be signed in to change notification settings - Fork 139
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
[KMETA-1541] Update configure to set up SSL for kraft controllers #264
base: master
Are you sure you want to change the base?
Conversation
@@ -23,6 +23,7 @@ if [[ -n "${KAFKA_PROCESS_ROLES-}" ]] | |||
then | |||
echo "Running in KRaft mode..." | |||
dub ensure CLUSTER_ID | |||
dub ensure KAFKA_LISTENER_SECURITY_PROTOCOL_MAP | |||
if [[ $KAFKA_PROCESS_ROLES == "controller" ]] | |||
then | |||
if [[ -n "${KAFKA_ADVERTISED_LISTENERS-}" ]] |
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.
Doesn't this line and the ones below it have to be updated
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.
Or this is only needed for ZK mode?
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 line/condition doesn't need to be updated because we want to keep the behavior that KAFKA_ADVERTISED_LISTENERS
should not be set in KRaft mode when running as a controller only process. The thing we're changing is, if we're in KRaft mode (and perhaps running as a controller only process), we check KAFKA_LISTENER_SECURITY_PROTOCOL_MAP
instead of KAFKA_ADVERTISED_LISTENERS
to determine if SSL is enabled.
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.
What would happen if this env var KAFKA_LISTENER_SECURITY_PROTOCOL_MAP
is not defined? Does it have to be defined?
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.
hm, you're right that requiring KAFKA_LISTENER_SECURITY_PROTOCOL_MAP
to exist is wrong here - none of our documentation indicates this is required (esp in the non-SSL case).
https://docs.confluent.io/platform/current/kafka/authentication_sasl/authentication_sasl_scram.html#configuration indicates brokers should have listeners
indicate SSL
Tell the Kafka brokers on which ports to listen for client and interbroker SASL connections. You must configure listeners
I'm not sure if this is the same case for controllers
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 think it would work to remove this line (ensuring the listener security protocol map is defined). When determining if SSL is enabled, if the map env var is not set then we'll also know SSL is not enabled. I think this is already achieved by the PR -
# if KRaft mode && KAFKA_LISTENER_SECURITY_PROTOCOL_MAP indicates SSL
# or if ZK mode && KAFKA_ADVERTISED_LISTENERS indicates SSL
if ( [[ -n "${KAFKA_PROCESS_ROLES-}" ]] && [[ $KAFKA_LISTENER_SECURITY_PROTOCOL_MAP =~ CONTROLLER:(SSL|SASL_SSL) ]] ) || \
( [[ -n "${KAFKA_ADVERTISED_LISTENERS-}" ]] && [[ $KAFKA_ADVERTISED_LISTENERS == *"SSL://"* ]] )
As a tangent, the [[ -n "${KAFKA_ADVERTISED_LISTENERS-}" ]]
check is excessive if we're already checking the value to see if it includes SSL, I can remove it
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
https://confluentinc.atlassian.net/browse/KMETA-1541
We only assign various SSL configs if
KAFKA_ADVERTISED_LISTENERS
indicates SSL is enabled, which doesn’t work when in KRaft controller-only mode (env var is NOT set in that case). We could check ifKAFKA_LISTENER_SECURITY_PROTOCOL_MAP
containsCONTROLLER:SSL
orCONTROLLER:SASL_SSL
in the KRaft case.