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.

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

Add KIP-235 implementation #4292

Merged
merged 14 commits into from
Jul 7, 2023
Merged

Add KIP-235 implementation #4292

merged 14 commits into from
Jul 7, 2023

Conversation

anchitj
Copy link
Member

@anchitj anchitj commented May 25, 2023

Changes done

Add functionality to support KIP-235. Kept the implementation similar to the Java client in apache/kafka#4485

  • Added a new config boolean property resolve.canonical.bootstrap.servers.only. User should enable this if they are specifying DNS aliases in the bootstrap servers.
  • If the above property is enabled, an address -> A records -> PTR lookup is performed before the brokers are added for each address in the broker list.

Testing

Manually. Adding automated test will probably need some sort of integration with a mock DNS server within librdkafka.

Steps:

  1. Setup Kerberos environment from kafka-docker-playground.
  2. Add another container c2 in the docker-compose with contents and Dockerfile.
  3. Start the containers using ./../../scripts/cli/playground run -f start.sh --disable-ksqldb --disable-control-center.
  4. SSH into the c2 container, and in the /etc/hosts file add two new lines, replacing 172.18.0.5 with the actual address of the broker container.
172.18.0.5 broker.kerberos-demo.local
172.18.0.5 randomExample.com
  1. Use client code client.c which is using randomExample.com:9092 and client.dns.lookup set to resolve_canonical_bootstrap_servers_only to connect with the Kerberos protected Kafka broker.
  2. If client.dns.lookup is not set, the client is unable to connect with the broker.

@anchitj anchitj marked this pull request as ready for review May 26, 2023 08:40
@anchitj anchitj changed the title Add KIP-235 initial implementation Add KIP-235 implementation May 26, 2023
tests/0141-resolve_cname_bootstrap_servers.c Outdated Show resolved Hide resolved
src/rdkafka_conf.c Outdated Show resolved Hide resolved
src/rdkafka_broker.c Outdated Show resolved Hide resolved
src/rdkafka_broker.c Outdated Show resolved Hide resolved
src/rdkafka_broker.c Outdated Show resolved Hide resolved
src/rdkafka_broker.c Outdated Show resolved Hide resolved
src/rdkafka_broker.c Show resolved Hide resolved
src/rdkafka_broker.c Outdated Show resolved Hide resolved
src/rdkafka_broker.c Outdated Show resolved Hide resolved
src/rdkafka_broker.c Show resolved Hide resolved
src/rdkafka_broker.c Outdated Show resolved Hide resolved
@anchitj
Copy link
Member Author

anchitj commented Jun 8, 2023

Semaphore PR corresponding - #4307

CONFIGURATION.md Outdated Show resolved Hide resolved
src/rdkafka_broker.h Outdated Show resolved Hide resolved
@pranavrth
Copy link
Member

Update supported kips section as well here

src/rdkafka_broker.c Show resolved Hide resolved
Copy link
Member Author

@anchitj anchitj left a comment

Choose a reason for hiding this comment

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

Updated the supported KIPs section as well

src/rdkafka_broker.c Show resolved Hide resolved
src/rdkafka_broker.h Outdated Show resolved Hide resolved
@anchitj
Copy link
Member Author

anchitj commented Jun 13, 2023

Verified from that all the tests except 0017_compression work when running individually from make run_seq

| 0017_compression                         |     FAILED | 888.679s | test_dr_msg_cb():1967: Message delivery (to rdkafkatest_rnd55cf09950b7e4136_snappy [0]) failed: expected Success, got Unknown broker error

This above error is occurring even when running with the latest master, so is unrelated to the PR.

Copy link
Member

@pranavrth pranavrth left a comment

Choose a reason for hiding this comment

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

LGTM! Nice Work.

Just a couple of points we need to confirm with @emasab.

@anchitj anchitj requested a review from emasab June 14, 2023 07:02
@pranavrth
Copy link
Member

Correct the description of the PR.

Copy link
Contributor

@emasab emasab left a comment

Choose a reason for hiding this comment

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

LGTM too, thanks Anchit!

@emasab emasab merged commit 961946e into master Jul 7, 2023
@emasab emasab deleted the kip235 branch July 7, 2023 10:57
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