[jira] [Commented] (MESOS-1201) Store IP addresses in host order

["Marco Massenzio (JIRA)" <ji...@apache.org>]

    [ https://issues.apache.org/jira/browse/MESOS-1201?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=14555293#comment-14555293 ] 

Marco Massenzio commented on MESOS-1201:
----------------------------------------

Ran into this while working on MESOS-2340 (related to MESOS-2298).

{{net::IP}} is internally consistent: you can create from a {{string}} and it stores it as an {{uint32}} (actually an {{in_addr}}) , then stream it back to a string and you’ll get back what you started with; for example, this works just fine:
{code}
TEST(IpTest, conversions)
{
  string ipStr = "192.169.10.11";
  std::ostringstream actual;

  net::IP ip = net::IP::parse(ipStr, AF_INET).get();
  actual << ip;
  ASSERT_EQ(ipStr, actual.str());
}
{code}

The problem is that the Protobuf “gets in the way:"  it stores the bytes in Network order - so, if you give them back to get a {{net::IP}} object it will cause weird stuff as the constructor expects them in “host order."

Here’s a (trivial) solution, which however is not a very robust fix:

{code}
  // you get the minfo from … magic!
  MasterInfo minfo = getMyMasterInfo(...);

  // then convert the bytes into host-order:
  net::IP ip{ ntohl(minfo.ip()) };
  // you can then generate back the original string ("192.168.1.10")
  std::ostringstream out;
  out << ip;
  return out.str();
{code}

Tthe long-term fix, however, would be to add a bool flag {{networkOrder}} to the {{net::IP(uint32 ip)}} constructor, I think (and maybe elaborate in the comment too); this is the current code:
{code}
  // Creates an IP from a 32 bit unsigned integer. Note that the
  // integer stores the IP address in host order.
  explicit IP(uint32_t _storage)
    : family_(AF_INET)
{code}
my suggestion would be to do something like:
{code}
  // Creates an IP from a 32 bit unsigned integer. Note that 'ip' is considered
  // to represent an IP address with the bytes in host order.
  //
  // They will be stored in network order internally; if they are already in
  // network order, pass in the 'networkOrder' flag as 'true'.
  explicit IP(uint32_t _storage, bool networkOrder = false)
    : family_(AF_INET)
{code}

BTW - right now, we can’t support INETv6; passing anything other than {{AF_INET}} will cause an error in this:
{code}
  static Try<IP> parse(const std::string& value, int family);
{code}

> Store IP addresses in host order
> --------------------------------
>
>                 Key: MESOS-1201
>                 URL: https://issues.apache.org/jira/browse/MESOS-1201
>             Project: Mesos
>          Issue Type: Bug
>          Components: technical debt
>            Reporter: Jie Yu
>
> Currently, in our code base, we store ip addresses in network order. For instance, in UPID. Ironically, we store ports in host order.
> This can cause some subtle bugs which will be very hard to debug. For example, we store ip in MasterInfo. Say the IP address is: 01.02.03.04. Since we don't convert it into host order in our code, on x86 (little endian), it's integer value will be 0x04030201. Now, we store it as an uint32 field in MasterInfo protobuf. Protobuf will convert all integers into little endian format, since x86 is little endian machine, no conversion will take place. As a result, the value stored in probobuf will be 0x04030201. Now, if a big endian machine reads this protobuf, it will do the conversion. If it later interprets the ip from this integer, it will interpret it to be 04.03.02.01.
> So I plan to store all IP addresses in our code base to be in host order (which is the common practice).
> We may have some compatibility issues as we store MasterInfo in ZooKeeper for master detection and redirection. For example, what if the new code reads an old MasterInfo? What would happen?



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)