Skip to content
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

[KMETA-1541] Update configure to set up SSL for kraft controllers #264

Closed
wants to merge 4 commits into from
Closed

[KMETA-1541] Update configure to set up SSL for kraft controllers #264

Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
b656337
[KMETA-1541] Update configure to set up SSL for kraft controllers
ahuang98 Mar 21, 2024
864aa64
Merge branch 'master' into configure_ssl_kraft
ahuang98 May 10, 2024
839b47f
address comments
ahuang98 May 10, 2024
8754711
adding back null checks
ahuang98 May 14, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 6 additions & 2 deletions server/include/etc/confluent/docker/configure
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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-}" ]]
Copy link
Contributor

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

Copy link
Contributor

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?

Copy link
Member Author

@ahuang98 ahuang98 May 8, 2024 •
edited
Loading

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.

Copy link
Contributor

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?

Copy link
Member Author

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

Copy link
Member Author

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

Expand Down Expand Up @@ -84,8 +85,11 @@ then
exit 1
fi

# Set if ADVERTISED_LISTENERS has SSL:// or SASL_SSL:// endpoints.
if [[ -n "${KAFKA_ADVERTISED_LISTENERS-}" ]] && [[ $KAFKA_ADVERTISED_LISTENERS == *"SSL://"* ]]
# Set various SSL env vars.
# We do this in KRaft mode if KAFKA_LISTENER_SECURITY_PROTOCOL_MAP has CONTROLLER:SSL or CONTROLLER:SASL_SSL
# We do this in ZK mode if ADVERTISED_LISTENERS has SSL:// or SASL_SSL:// endpoints
if ( [[ -n "${KAFKA_PROCESS_ROLES-}" ]] && [[ $KAFKA_LISTENER_SECURITY_PROTOCOL_MAP =~ CONTROLLER:(SSL|SASL_SSL) ]] ) || \
( [[ -n "${KAFKA_ADVERTISED_LISTENERS-}" ]] && [[ $KAFKA_ADVERTISED_LISTENERS == *"SSL://"* ]] )
then
echo "SSL is enabled."

Expand Down