Kafka
  1. Kafka
  2. KAFKA-1123

Broker IPv6 addresses parsed incorrectly

    Details

    • Type: Bug Bug
    • Status: Resolved
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 0.8.2.0
    • Fix Version/s: 0.8.2.0
    • Component/s: producer
    • Labels:

      Description

      It seems that broker addresses are parsed incorrectly when IPv6 addresses are supplied. IPv6 addresses have colons in them, and Kafka seems to be interpreting the first : as the address:port separator.

      I have only tried this with the console-producer --broker-list option, so I don't know if this affects anything deeper than the CLI.

      1. KAFKA-1123_v2.patch
        23 kB
        Krzysztof Szafrański
      2. KAFKA-1123_v3.patch
        25 kB
        Krzysztof Szafrański
      3. KAFKA-1123_v4.patch
        24 kB
        Krzysztof Szafrański
      4. KAFKA-1123.patch
        16 kB
        Krzysztof Szafrański

        Activity

        Hide
        Jun Rao added a comment -

        The new producer needs to use bootstrap.servers, instead of metadata.broker.list.

        Show
        Jun Rao added a comment - The new producer needs to use bootstrap.servers, instead of metadata.broker.list.
        Hide
        Mingtao Zhang added a comment -

        Curious anyone tested in Java, i.e. new Producer<>(config). I am on 0.8.2.1, it's doesn't work with metadata.broker.list=[FEDC:BA98:7654:3210:FEDC:BA98:7654:3210]:9092

        Show
        Mingtao Zhang added a comment - Curious anyone tested in Java, i.e. new Producer<>(config). I am on 0.8.2.1, it's doesn't work with metadata.broker.list= [FEDC:BA98:7654:3210:FEDC:BA98:7654:3210] :9092
        Hide
        Jun Rao added a comment -

        Thanks for patch v4. +1 and committed to trunk.

        Show
        Jun Rao added a comment - Thanks for patch v4. +1 and committed to trunk.
        Hide
        Krzysztof Szafrański added a comment -

        I have now removed the string interpolation syntax.

        Show
        Krzysztof Szafrański added a comment - I have now removed the string interpolation syntax.
        Hide
        Jun Rao added a comment -

        Thanks for the latest patch. In Broker, you used the simple string interpolator like the following. So far, we haven't adopted this syntax. Perhaps we can leave out those changes for now for consistency?

        s"Broker id $id does not exist"

        Show
        Jun Rao added a comment - Thanks for the latest patch. In Broker, you used the simple string interpolator like the following. So far, we haven't adopted this syntax. Perhaps we can leave out those changes for now for consistency? s"Broker id $id does not exist"
        Hide
        Krzysztof Szafrański added a comment -

        I have updated the patch according to your remarks.

        Show
        Krzysztof Szafrański added a comment - I have updated the patch according to your remarks.
        Hide
        Jun Rao added a comment -

        Thanks for the patch. Looks good overall. Some minor comments below.

        1. ClientUitls: There is still one place that references "bootstrap url". We can change it to use the BOOTSTRAP constant.

        2. Broker: redundant import of kafka.util.Utils.

        3. DataGenerator, TopicMetadata: unused import kafka.util.Utils

        4. UtilsTest.testParseHostPort seems can be testParsePort?

        5. ClientUtilsTest: Do we need to test "[2001:db8:85a3:8d3:1319:8a2e:370:7348]:1234" twice?

        6. Could you rebase?

        Show
        Jun Rao added a comment - Thanks for the patch. Looks good overall. Some minor comments below. 1. ClientUitls: There is still one place that references "bootstrap url". We can change it to use the BOOTSTRAP constant. 2. Broker: redundant import of kafka.util.Utils. 3. DataGenerator, TopicMetadata: unused import kafka.util.Utils 4. UtilsTest.testParseHostPort seems can be testParsePort? 5. ClientUtilsTest: Do we need to test " [2001:db8:85a3:8d3:1319:8a2e:370:7348] :1234" twice? 6. Could you rebase?
        Hide
        Krzysztof Szafrański added a comment -

        Sorry, I've been busy lately. I'll move the parsing util to ClientUtils this week, and post an updated patch.

        Show
        Krzysztof Szafrański added a comment - Sorry, I've been busy lately. I'll move the parsing util to ClientUtils this week, and post an updated patch.
        Hide
        Neha Narkhede added a comment -

        Bump Krzysztof Szafrański. Are you going to get a chance to update your patch?

        Show
        Neha Narkhede added a comment - Bump Krzysztof Szafrański . Are you going to get a chance to update your patch?
        Hide
        Jun Rao added a comment -

        1. kafka-client doesn't depend on core. However, core already depends on kafka-client. So, we can put the parsing code in org.apache.kafka.common.utils.ClientUtils. Then, both kafka-client and core can use the common code.

        2. Yes, that makes sense.

        Show
        Jun Rao added a comment - 1. kafka-client doesn't depend on core. However, core already depends on kafka-client. So, we can put the parsing code in org.apache.kafka.common.utils.ClientUtils. Then, both kafka-client and core can use the common code. 2. Yes, that makes sense.
        Hide
        Krzysztof Szafrański added a comment - - edited

        1. I agree that parseAndValidateAddress() could reuse ClientUtils (though it implies dealing with Tuple2 in Java). ("core" module is not visible from "clients")
        2. InetSocketAddress can take the address without the square braces, but my intention with addressString() was to propagate this notation throughout the system. And as for the standard it would be RFC 2732 (http://www.ietf.org/rfc/rfc2732.txt).

        Show
        Krzysztof Szafrański added a comment - - edited 1. I agree that parseAndValidateAddress() could reuse ClientUtils (though it implies dealing with Tuple2 in Java). ("core" module is not visible from "clients") 2. InetSocketAddress can take the address without the square braces, but my intention with addressString() was to propagate this notation throughout the system. And as for the standard it would be RFC 2732 ( http://www.ietf.org/rfc/rfc2732.txt ).
        Hide
        Jun Rao added a comment -

        Thanks for the patch. Some comments.

        1. Utils.parseHostPort(): Should we just move the logic into org.apache.kafka.common.utils.ClientUtils and reuse it in parseAndValidateAddresses()? That way, we have only one way of parsing the hostAndPort.

        2. Do we need to wrap the ipv6 address with "[]"? It seems that InetSocketAddress can take that format. However, I am not sure if this is the standard representation. When specifying ipv6-based broker list, do we expect the user to wrap the address part with "[]"?

        Show
        Jun Rao added a comment - Thanks for the patch. Some comments. 1. Utils.parseHostPort(): Should we just move the logic into org.apache.kafka.common.utils.ClientUtils and reuse it in parseAndValidateAddresses()? That way, we have only one way of parsing the hostAndPort. 2. Do we need to wrap the ipv6 address with "[]"? It seems that InetSocketAddress can take that format. However, I am not sure if this is the standard representation. When specifying ipv6-based broker list, do we expect the user to wrap the address part with "[]"?
        Hide
        Krzysztof Szafrański added a comment -

        I have added methods to split the host and port, and to put them back together (surrounding IPv6 addresses with '[', ']').

        Show
        Krzysztof Szafrański added a comment - I have added methods to split the host and port, and to put them back together (surrounding IPv6 addresses with ' [', '] ').
        Hide
        Jun Rao added a comment -

        We probably should add the IPV6 support asap. It seems that for a given string of "host:port", we can just look up for the last : to separate the host and port. This should cover both IPV4 and IPV6. We will need to do that for both the old and the new producer.

        Show
        Jun Rao added a comment - We probably should add the IPV6 support asap. It seems that for a given string of "host:port", we can just look up for the last : to separate the host and port. This should cover both IPV4 and IPV6. We will need to do that for both the old and the new producer.

          People

          • Assignee:
            Krzysztof Szafrański
            Reporter:
            Andrew Otto
            Reviewer:
            Jun Rao
          • Votes:
            0 Vote for this issue
            Watchers:
            5 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development