Skip to content

Conversation

@rkunwar-28
Copy link
Member

Description

Fix ZooKeeper chroot creation when ACLs are enabled

Problem
The "Create Zookeeper chroot" task fails with "Insufficient permission" error when ZooKeeper digest authentication is configured (zookeeper_client_authentication_type: digest). This happens because the task doesn't include authentication context when zookeeper.set.acl=true is automatically set.

Solution
Add the same authentication conditional logic used by SCRAM user creation tasks:

{% if kafka_broker_final_properties['zookeeper.set.acl']|default('false')|lower == 'true' %}KAFKA_OPTS='-Djava.security.auth.login.config={{kafka_broker.jaas_file}}'{% endif %}

Impact
✅ Fixes chroot creation for digest/kerberos authenticated ZooKeeper setups
✅ Maintains backward compatibility with non-authenticated configurations
✅ Eliminates need for manual KAFKA_OPTS workarounds
✅ Makes chroot task consistent with other ZooKeeper operations

Fixes # (issue)

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

How Has This Been Tested?

Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration

Checklist:

  • Any variable/code changes have been validated to be backwards compatible (doesn't break upgrade)
  • I have added tests that prove my fix is effective or that my feature works
  • If required, I have ensured the changes can be discovered by cp-ansible discovery codebase
  • My code follows the style guidelines of this project
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • Any dependent changes have been merged and published in downstream modules

@rkunwar-28 rkunwar-28 requested a review from a team as a code owner September 21, 2025 13:20
Copilot AI review requested due to automatic review settings September 21, 2025 13:20
@rkunwar-28 rkunwar-28 changed the base branch from master to 7.9.x September 21, 2025 13:20
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR fixes a ZooKeeper chroot creation issue that occurs when ACLs are enabled with digest authentication. The main problem was that the chroot creation task lacked authentication context, causing "Insufficient permission" errors.

  • Adds conditional KAFKA_OPTS authentication configuration to the chroot creation command
  • Includes verification test to ensure chroot creation works with digest authentication
  • Updates molecule configuration to simulate production environment with sudo privileges

Reviewed Changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.

File Description
roles/kafka_broker/tasks/main.yml Adds KAFKA_OPTS authentication context to ZooKeeper chroot creation task
molecule/zookeeper-digest-rhel/verify.yml Adds verification test for chroot creation with digest authentication
molecule/zookeeper-digest-rhel/molecule.yml Enables ansible_become to simulate production environment

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

Comment on lines 49 to 57
- name: Verify chroot exists in ZooKeeper with authentication
shell: >
{% if kafka_broker_final_properties['zookeeper.set.acl']|default('false')|lower == 'true' %}KAFKA_OPTS='-Djava.security.auth.login.config={{kafka_broker.jaas_file}}'{% endif %} \
{{ binary_base_path }}/bin/zookeeper-shell {{ hostvars[groups['zookeeper'][0]] | confluent.platform.resolve_hostname }}:{{zookeeper_client_port}} \
ls /
register: zk_root_listing
run_once: true
changed_when: false
failed_when: false
Copy link

Copilot AI Sep 21, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This duplicates the same complex conditional logic from the main task. Consider extracting this authentication logic into a variable or using the environment parameter to avoid code duplication.

Suggested change
- name: Verify chroot exists in ZooKeeper with authentication
shell: >
{% if kafka_broker_final_properties['zookeeper.set.acl']|default('false')|lower == 'true' %}KAFKA_OPTS='-Djava.security.auth.login.config={{kafka_broker.jaas_file}}'{% endif %} \
{{ binary_base_path }}/bin/zookeeper-shell {{ hostvars[groups['zookeeper'][0]] | confluent.platform.resolve_hostname }}:{{zookeeper_client_port}} \
ls /
register: zk_root_listing
run_once: true
changed_when: false
failed_when: false
- name: Set KAFKA_OPTS for ZooKeeper shell if needed
set_fact:
zk_kafka_opts: >-
{% if kafka_broker_final_properties['zookeeper.set.acl']|default('false')|lower == 'true' %}
-Djava.security.auth.login.config={{ kafka_broker.jaas_file }}
{% else %}
""
{% endif %}
- name: Verify chroot exists in ZooKeeper with authentication
shell: >
{{ binary_base_path }}/bin/zookeeper-shell {{ hostvars[groups['zookeeper'][0]] | confluent.platform.resolve_hostname }}:{{zookeeper_client_port}} \
ls /
register: zk_root_listing
run_once: true
changed_when: false
failed_when: false
environment:
KAFKA_OPTS: "{{ zk_kafka_opts }}"

Copilot uses AI. Check for mistakes.
Copy link
Member Author

@rkunwar-28 rkunwar-28 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Test Details:
Reproduced the issue:
image

Also confirmed by running the command to add znode directly through the terminal (without the Kafka environment overrides):
image

Adding security config via kafka environment overrides solves the issue:
Direct command run:
image

Playbook task also passed:
image

@confluentinc confluentinc deleted a comment from Copilot AI Nov 4, 2025
@rkunwar-28 rkunwar-28 changed the base branch from 7.9.x to 7.4.x November 4, 2025 10:40
@rkunwar-28 rkunwar-28 changed the base branch from 7.4.x to 7.9.x November 4, 2025 10:40
# Only runs with zookeeper
- name: Create Zookeeper chroot
shell: >
{% if kafka_broker_final_properties['zookeeper.set.acl']|default('false')|lower == 'true' %}KAFKA_OPTS='-Djava.security.auth.login.config={{kafka_broker.jaas_file}}'{% endif %} \
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why is this not caught in molecule tests ?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is this issue also present in case of zookeeper client authentication being kerberos as there too we are defining the zookeeper.set.acl property in kafka ?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can is zookeeper-shell command also fail in these tasks Get Kafka Cluster ID from Zookeeper
or Get Controller Broker ID as there also we have not provided the jaas file as argument ?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why is this not caught in molecule tests ?

In the related incident, the customer's znode was nested:
image

According to the above, the getACL command is telling only zkuser has privilege to w on that particular znode. In ansible playbook, since in most cases znode is only at root this command to create znode doesn't fail in molecule tests, but in case of customer's its nested znode. For this kind of scenario, we need the auth configs to be passed.

is this issue also present in case of zookeeper client authentication being kerberos as there too we are defining the zookeeper.set.acl property in kafka ?

Yes, if a nested znode with similar permissions are used, then we will see this issue since zookeeper.set.acl = true

can is zookeeper-shell command also fail in these tasks Get Kafka Cluster ID from Zookeeper
or Get Controller Broker ID as there also we have not provided the jaas file as argument ?

No, these tasks will not fail

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lets add a nested znode in 1 or few of our test cases as well

@rrbadiani
Copy link
Member

is this bug fix going for all patch branches ?
if so we can keep molecule changes and playbook changes in seperate prs so playbooks can be pint merged and molecule changes we can probably keep only for 7.9.x branch

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants