KAFKA-6195: Resolve DNS aliases in bootstrap.server by lepolac · Pull Request #4485 · apache/kafka

lepolac · 2018-01-29T17:19:03Z

Change described in KIP-235
https://cwiki.apache.org/confluence/display/KAFKA/KIP-235%3A+Add+DNS+alias+support+for+secured+connection

I license the work to the Apache Kafka project under the project's open source license.

Reverse DNS lookup performed regardless of the SecurityProtocol used if bootstrap.reverse.dns.lookup is set to true.

cmccabe · 2018-06-05T14:54:10Z

-import org.apache.kafka.clients.KafkaClient;
-import org.apache.kafka.clients.Metadata;
-import org.apache.kafka.clients.NetworkClient;
+import org.apache.kafka.clients.*;


I don't think we want star imports.

cmccabe · 2018-06-05T14:55:00Z

        this.metadata = metadata;
-        List<InetSocketAddress> addresses = ClientUtils.parseAndValidateAddresses(
-            config.getList(AdminClientConfig.BOOTSTRAP_SERVERS_CONFIG));
+        List<InetSocketAddress> addresses = ClientUtils.parseAndValidateAddresses(config.getList(AdminClientConfig.BOOTSTRAP_SERVERS_CONFIG),config.getString(CommonClientConfigs.CLIENT_DNS_LOOKUP));


This line is too long. We need the linebreaks back here

cmccabe · 2018-06-05T15:00:38Z

+        checkWithoutLookup("127.0.0.1:8000");
+        checkWithoutLookup("mydomain.com:8080");
+        checkWithoutLookup("[::1]:8000");
+        checkWithoutLookup("[2001:db8:85a3:8d3:1319:8a2e:370:7348]:1234", "mydomain.com:10000");


Some day, mydomain.com will be changed and all of this will break. Additionally, this prevents people from running the unit tests without network connectivity. Our unit tests shouldn't have this dependency on an external network resource.

We should be able to figure out what the IP address is for localhost and go from there.

If we want to validate a non-localhost address as well, we can use the standard "example.com" (See https://en.wikipedia.org/wiki/Example.com )

Good point. I was merely following what was already in place.
Would we want to address this now or could the feature be merged as is and I can take a stab at having something better for this test next week?

updated the tests.

KAFKA-6195: Add new parameter to toggle reverse dns lookup KAFKA-6195: Remove specific handling for SSL. Reverse DNS lookup performed regardless of the SecurityProtocol used if bootstrap.reverse.dns.lookup is set to true. KAFKA-6195 introduce enum to drive dns lookup behaviour Remove star import adding apache license header

…FKA-6195

harshach · 2018-07-09T17:57:01Z

                                                       + "servers (you may want more than one, though, in case a server is down).";

+    public static final String CLIENT_DNS_LOOKUP = "client.dns.lookup";
+    public static final String CLIENT_DNS_LOOKUP_DOC = "Drives whether the client performs DNS lookups on entries of bootstrap.servers";


I would phrase this as "To enable client DNS lookups on bootstrap.servers"

harshach · 2018-07-09T18:02:58Z

@lepolac overall looks good to me. Left a minor nit and there are few un-addressed comments from previous reviewers. Can you address these and rebase your commits into single one.

lepolac · 2018-08-14T22:27:51Z

@harshach I'm a bit struggling to rebase the commits, started from a fresh directory, cloned my fork, checked out my KAFKA-6195 branch. Then added official repo as a remote, fetched it. A git merge-base KAFKA-6195 kafka/trunk gives me hash b5da5f8, but doing a rebase -i on that hash doesn't show me anything to pick.
Basically followed https://github.com/edx/edx-platform/wiki/How-to-Rebase-a-Pull-Request

Any ideas ?

harshach · 2018-08-24T16:48:07Z

@lepolac sorry didn't see your comment. The approach I usually go for is to update the trunk branch in my fork and merge with my JIRA branch.

git pull apache trunk
git checkout KAFKA-6195
git merge trunk

lepolac · 2018-08-24T23:25:08Z

@harshach Ok, managed to push, hopefully this looks ok ?

omkreddy · 2018-08-28T09:53:38Z

retest this please

omkreddy · 2018-09-20T07:01:05Z

@lepolac couple of compilation errors are coming. You can run ./gradlew clean :clients:test
to compile and test the changes.

/Users/mkumar/workspace/kafka/clients/src/main/java/org/apache/kafka/clients/producer/KafkaProducer.java:401: error: constructor RecordAccumulator in class RecordAccumulator cannot be applied to given types;
            this.accumulator = new RecordAccumulator(logContext,
                               ^
  required: LogContext,int,CompressionType,long,long,long,Metrics,String,Time,ApiVersions,TransactionManager,BufferPool
  found: LogContext,Integer,long,CompressionType,Integer,long,int,Metrics,String,Time,ApiVersions,TransactionManager,BufferPool
  reason: actual and formal argument lists differ in length

/Users/mkumar/workspace/kafka/clients/src/main/java/org/apache/kafka/clients/producer/KafkaProducer.java:414: error: method parseAndValidateAddresses in class ClientUtils cannot be applied to given types;
            List<InetSocketAddress> addresses = ClientUtils.parseAndValidateAddresses(config.getList(ProducerConfig.BOOTSTRAP_SERVERS_CONFIG));
                                                           ^
  required: List<String>,String
  found: List<String>
  reason: actual and formal argument lists differ in length

rajinisivaram · 2018-10-11T11:05:23Z

@lepolac @edoardocomar @mimaison I have reviewed this as well as PR #4987. If we can the config naming consistent today, address review comments and get clean builds today, then we can merge both to 2.1.0 and trunk.

rajinisivaram · 2018-10-11T13:56:50Z

@lepolac I will merge PR #4987 once its PR builds complete. Can you rebase this on top of that, once the build is ready? Thank you!

…ry-catch, adding config in WorkerConfig

rajinisivaram · 2018-10-11T17:50:04Z

@lepolac I have merged the other PR. Can you rebase this one please?

lepolac · 2018-10-11T22:13:54Z

Ok, looks like ClientDnsLookup from the other KIP was put in another module and uses other value lookup methods, will merge this.

…FKA-6195

rajinisivaram · 2018-10-12T08:21:50Z

@lepolac I think ClientDnsLookup should be in org.apache.kafka.clients as you have in this PR. I will check if @edoardocomar or @mimaison can fix that.

lepolac · 2018-10-12T09:32:28Z

Probably faster for us to change on this PR I think as other one is already merged

rajinisivaram · 2018-10-12T10:14:33Z

@lepolac @adammilnesmith There are PR build failures, can you take a look?

…nt failure

rajinisivaram

@lepolac Thanks for the updates, looks good. Left some minor comments. I think I can merge this once they are addressed.

rajinisivaram · 2018-10-12T13:27:28Z

+                            String resolvedCanonicalName = inetAddress.getCanonicalHostName();
+                            InetSocketAddress address = new InetSocketAddress(resolvedCanonicalName, port);
+                            if (address.isUnresolved()) {
+                                log.warn("Couldn't resolve server {} from {} as DNS resolution of the canonical hostname failed for {}", url, CommonClientConfigs.BOOTSTRAP_SERVERS_CONFIG, host);


Can we include resolvedCanonicalName in the warning?

rajinisivaram · 2018-10-12T13:29:18Z

@@ -162,7 +168,9 @@ public class AdminClientConfig extends AbstractConfig {
                                .define(CommonClientConfigs.CLIENT_DNS_LOOKUP_CONFIG,


Change from CommonClientConfigs.CLIENT_DNS_LOOKUP_CONFIG to CLIENT_DNS_LOOKUP_CONFIG?

rajinisivaram · 2018-10-12T13:29:41Z

+                                           ClientDnsLookup.USE_ALL_DNS_IPS.toString(),
+                                           ClientDnsLookup.RESOLVE_CANONICAL_BOOTSTRAP_SERVERS_ONLY.toString()),
                                        Importance.MEDIUM,
                                        CommonClientConfigs.CLIENT_DNS_LOOKUP_DOC)


Change from CommonClientConfigs.CLIENT_DNS_LOOKUP_DOC to CLIENT_DNS_LOOKUP_DOC?

rajinisivaram · 2018-10-12T13:31:00Z

                                        new ConfigDef.NonNullValidator(),
                                        Importance.HIGH,
                                        CommonClientConfigs.BOOTSTRAP_SERVERS_DOC)
+                                .define(CommonClientConfigs.CLIENT_DNS_LOOKUP_CONFIG,


Change CommonClientConfigs.CLIENT_DNS_LOOKUP_CONFIG to CLIENT_DNS_LOOKUP_CONFIG?

rajinisivaram · 2018-10-12T13:38:30Z

 import scala.collection.mutable.HashMap
 import scala.collection.{Set, mutable}
-import org.apache.kafka.common.config.ClientDnsLookup
+import org.apache.kafka.clients.ClientDnsLookup


There is a wildcard import in this class for org.apache.kafka.clients, so this import is not required.

rajinisivaram · 2018-10-12T13:39:43Z

 import collection.JavaConverters._
 import scala.collection.{concurrent, immutable}
-import org.apache.kafka.common.config.ClientDnsLookup
+import org.apache.kafka.clients.ClientDnsLookup


There is a wildcard import in this class for org.apache.kafka.clients, so this import is not required.

rajinisivaram · 2018-10-12T13:40:19Z


 import scala.collection.JavaConverters._
-import org.apache.kafka.common.config.ClientDnsLookup
+import org.apache.kafka.clients.ClientDnsLookup


There is a wildcard import in this class for org.apache.kafka.clients, so this import is not required.

rajinisivaram · 2018-10-12T13:40:43Z


 import scala.collection.JavaConverters._
-import org.apache.kafka.common.config.ClientDnsLookup
+import org.apache.kafka.clients.ClientDnsLookup


There is a wildcard import in this class for org.apache.kafka.clients, so this import is not required.

rajinisivaram · 2018-10-12T13:41:10Z

 import org.apache.kafka.common.Cluster;
 import org.apache.kafka.common.Node;
-import org.apache.kafka.common.config.ClientDnsLookup;
+import org.apache.kafka.clients.ClientDnsLookup;


Can we just move this above alongside client imports?

lepolac · 2018-10-13T08:22:53Z

Looks like one of the build fails with unrelated tests ?
The other one passes

rajinisivaram

@lepolac Thanks for the updates, LGTM. I will make some minor updates to reorganize imports where package changed and merge the PR.

Adds `client.dns.lookup=resolve_canonical_bootstrap_servers_only` option to perform full dns resolution of bootstrap addresses Reviewers: Colin Patrick McCabe <[email protected]>, Sriharsha Chintalapani <[email protected]>, Edoardo Comar <[email protected]>, Mickael Maison <[email protected]>, Manikumar Reddy <[email protected]>, Rajini Sivaram <[email protected]>

rajinisivaram · 2018-10-13T20:57:41Z

Merged to trunk and 2.1.

lepolac · 2018-10-13T21:19:56Z

Thanks !!

…#4485) Adds `client.dns.lookup=resolve_canonical_bootstrap_servers_only` option to perform full dns resolution of bootstrap addresses Reviewers: Colin Patrick McCabe <[email protected]>, Sriharsha Chintalapani <[email protected]>, Edoardo Comar <[email protected]>, Mickael Maison <[email protected]>, Manikumar Reddy <[email protected]>, Rajini Sivaram <[email protected]>

sowhynot added 4 commits January 29, 2018 17:12

KAFKA-6195: Resolve DNS aliases in bootstrap.server

9273e39

KAFKA-6195: Add new parameter to toggle reverse dns lookup

24757eb

KAFKA-6195: Remove specific handling for SSL.

7452717

Reverse DNS lookup performed regardless of the SecurityProtocol used if bootstrap.reverse.dns.lookup is set to true.

KAFKA-6195 introduce enum to drive dns lookup behaviour

7b034fd

cmccabe reviewed Jun 5, 2018

View reviewed changes

Comment thread clients/src/test/java/org/apache/kafka/clients/ClientUtilsTest.java

Remove star import

2d94952

omkreddy reviewed Jun 7, 2018

View reviewed changes

Comment thread clients/src/main/java/org/apache/kafka/clients/ClientDnsLookup.java

jonathanskrzypek and others added 6 commits June 7, 2018 13:22

adding apache license header

eb4f48c

Merge branch 'KAFKA-6195' of https://github.com/lepolac/kafka into KA…

c6fdd6a

…FKA-6195

KAFKA-6195 : checkStyle issues + imports

2d6837c

KAFKA-6195 : fix incorrect import

cbb280b

KAFKA-6195: Change test to avoid relying on a specific domain

4518932

harshach reviewed Jun 27, 2018

View reviewed changes

Comment thread clients/src/main/java/org/apache/kafka/clients/ClientUtils.java

harshach reviewed Jul 9, 2018

View reviewed changes

KAFKA-6195 change setting documentation

014a0a7

Merge branch 'trunk' into KAFKA-6195

34b0eb7

harshach reviewed Aug 28, 2018

View reviewed changes

Comment thread clients/src/main/java/org/apache/kafka/clients/admin/KafkaAdminClient.java Outdated

jonathanskrzypek added 3 commits August 29, 2018 23:40

Update KafkaAdminClient.java

fd4e889

Fix merge leftovers

fa1fef1

Merge branch 'trunk' into KAFKA-6195

b09b9ba

KAFKA-6195 Changing parameter naming convention, remove unnecessary t…

60a633c

…ry-catch, adding config in WorkerConfig

Merge branch 'trunk' into KAFKA-6195

7734845

sowhynot added 2 commits October 11, 2018 23:52

fixing admin client configs

0734bdc

Merge branch 'KAFKA-6195' of https://github.com/lepolac/kafka into KA…

73ead8c

…FKA-6195

adammilnesmith added 2 commits October 12, 2018 10:32

KAFKA-6195 Fixing after merge with changes for KIP-302

5ec8c0a

KAFKA-6195 Moving ClientDnsLookup to org.apache.kafka.client

23622e6

KAFKA-6195 Correcting IDE package move auto-refactor's import stateme…

30fea4d

…nt failure

rajinisivaram reviewed Oct 12, 2018

View reviewed changes

jonathanskrzypek and others added 6 commits October 12, 2018 15:24

Update ControllerChannelManager.scala

d0e8598

Update TransactionMarkerChannelManager.scala

d90fa15

Update ReplicaFetcherBlockingSend.scala

11429b4

Update ReplicaVerificationTool.scala

45dc01c

Update ConnectionStressWorker.java

c0a5d96

configs consistency

9bc259f

Organize imports

85cae7d

rajinisivaram approved these changes Oct 13, 2018

View reviewed changes

rajinisivaram merged commit a947fe8 into apache:trunk Oct 13, 2018

anchitj mentioned this pull request May 26, 2023

Add KIP-235 implementation confluentinc/librdkafka#4292

Merged

		@@ -162,7 +168,9 @@ public class AdminClientConfig extends AbstractConfig {
		.define(CommonClientConfigs.CLIENT_DNS_LOOKUP_CONFIG,

Conversation

lepolac commented Jan 29, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

harshach commented Jul 9, 2018

Uh oh!

lepolac commented Aug 14, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

harshach commented Aug 24, 2018

Uh oh!

lepolac commented Aug 24, 2018

Uh oh!

omkreddy commented Aug 28, 2018

Uh oh!

Uh oh!

omkreddy commented Sep 20, 2018

Uh oh!

rajinisivaram commented Oct 11, 2018

Uh oh!

rajinisivaram commented Oct 11, 2018

Uh oh!

rajinisivaram commented Oct 11, 2018

Uh oh!

lepolac commented Oct 11, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rajinisivaram commented Oct 12, 2018

Uh oh!

lepolac commented Oct 12, 2018

Uh oh!

rajinisivaram commented Oct 12, 2018

Uh oh!

rajinisivaram left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lepolac commented Oct 13, 2018

Uh oh!

rajinisivaram left a comment

Choose a reason for hiding this comment

Uh oh!

lepolac commented Aug 14, 2018 •

edited

Loading

lepolac commented Oct 11, 2018 •

edited

Loading