Skip to content

Add KIP-235 implementation#4292

Merged
Emanuele Sabellico (emasab) merged 14 commits intomasterfrom
kip235
Jul 7, 2023
Merged

Add KIP-235 implementation#4292
Emanuele Sabellico (emasab) merged 14 commits intomasterfrom
kip235

Conversation

@anchitj
Copy link
Copy Markdown
Contributor

@anchitj Anchit Jain (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 Anchit Jain (anchitj) marked this pull request as ready for review May 26, 2023 08:40
@anchitj Anchit Jain (anchitj) changed the title Add KIP-235 initial implementation Add KIP-235 implementation May 26, 2023
Comment thread tests/0141-resolve_cname_bootstrap_servers.c Outdated
Comment thread src/rdkafka_conf.c Outdated
Comment thread src/rdkafka_broker.c Outdated
Comment thread src/rdkafka_broker.c Outdated
Comment thread src/rdkafka_broker.c Outdated
Comment thread src/rdkafka_broker.c Outdated
Comment thread src/rdkafka_broker.c
Comment thread src/rdkafka_broker.c Outdated
Comment thread src/rdkafka_broker.c Outdated
Comment thread src/rdkafka_broker.c
Comment thread src/rdkafka_broker.c Outdated
@anchitj
Copy link
Copy Markdown
Contributor Author

Semaphore PR corresponding - #4307

Comment thread CONFIGURATION.md Outdated
Comment thread src/rdkafka_conf.c
Comment thread src/rdkafka_broker.h Outdated
@pranavrth
Copy link
Copy Markdown
Member

Update supported kips section as well here

Comment thread src/rdkafka_broker.c
Copy link
Copy Markdown
Contributor Author

@anchitj Anchit Jain (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

Comment thread src/rdkafka_broker.c
Comment thread src/rdkafka_broker.h Outdated
@anchitj
Copy link
Copy Markdown
Contributor Author

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
Copy Markdown
Member

@pranavrth Pranav Rathi (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 Emanuele Sabellico (@emasab).

@pranavrth
Copy link
Copy Markdown
Member

Correct the description of the PR.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM too, thanks Anchit!

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