MESOS-9499 extended URI syntax to support any Zookeeper authentication schemes

[dlazarus]

Zookeeper URL now optionally can have syntax:
zk://zk_auth_scheme!zk_auth_data@host:port/path

If there is no "!" in URL it works as before with digest auth scheme.

For example, I use it with our Zk auth plugin like this:
zk://simple!login:[email protected]:2181/mesos
Here "simple" is an authentication scheme name.

[kaysoky]

We will want to add some regression tests too, before we commit any changes.

You can start looking/reading/familiarizing yourself with these:
https://github.com/apache/mesos/blob/master/src/tests/zookeeper.hpp
https://github.com/apache/mesos/blob/master/src/tests/zookeeper_tests.cpp

Since you are adding additional authentication schemes, we can use the other built-in schemes which ZK provides, like host or ip for testing.
https://zookeeper.apache.org/doc/r3.1.2/zookeeperProgrammers.html#sc_BuiltinACLSchemes

[kaysoky]

Adding the authentication scheme into this location gives us a small-ish patch, but has downsides:

• The addition of an exclamation mark delimiter invalidates the URI schema, according to https://tools.ietf.org/html/rfc1034#section-3.5 . Only letters, digits, and dashes are allowed in a "host" field. If we (Mesos) ever change our URI parsing logic, we would need to rip out this code.
• Zookeeper's pluggable system does not seem to limit the characters in an authentication scheme (as far as I know). If a plugin's scheme has an @ in the name for whatever reason, the URI parsing logic here would fail spectacularly 😈 .

My recommendation is to add an optional master/agent flag which specifies the ZK authentication scheme.

[dlazarus]

It is not in the hostname. Look:
example: zk://simple!login:password@host:port/path
hostname starts after '@'.
Everything after zk:// and before '@' has nothing to do with DNS and hostnames.

Check https://tools.ietf.org/html/rfc3986
Page 49 describes allowed delimiters in this part, explicitly including '!'.
Section 3.2.1 describes this part as "User Information".

[kaysoky]

Ah, I missed that. However, if we use a ! as a delimiter, that means the user info cannot have exclamation marks. So something like:

zk://user:pass!word@host:port/path

Would give us a "authentication scheme" of user:pass, which would be incorrect.

[dlazarus]

Yes. But it is an obvious side effect of design decision to pass authentication information as URL. Which was already in place.

It is also not a big problem. We can allow escape it somehow. Like "\!".

[dlazarus]

Also we can choose another delimiter symbol, RFS allows to use several symbols.

[dlazarus]

We will want to add some regression tests too, before we commit any changes.

You can start looking/reading/familiarizing yourself with these:
https://github.com/apache/mesos/blob/master/src/tests/zookeeper.hpp
https://github.com/apache/mesos/blob/master/src/tests/zookeeper_tests.cpp

Since you are adding additional authentication schemes, we can use the other built-in schemes which ZK provides, like host or ip for testing.
https://zookeeper.apache.org/doc/r3.1.2/zookeeperProgrammers.html#sc_BuiltinACLSchemes

We will want to add some regression tests too, before we commit any changes.

You can start looking/reading/familiarizing yourself with these:
https://github.com/apache/mesos/blob/master/src/tests/zookeeper.hpp
https://github.com/apache/mesos/blob/master/src/tests/zookeeper_tests.cpp

Since you are adding additional authentication schemes, we can use the other built-in schemes which ZK provides, like host or ip for testing.
https://zookeeper.apache.org/doc/r3.1.2/zookeeperProgrammers.html#sc_BuiltinACLSchemes
It looks like those tests use some kind of mock implementation of Zookeeper. Because only "digest scheme" works.

On my setup patched Mesos works with any schemes.

Really all Mesos has to do with Zookeeper auth is to pass schema and auth data to Zookeeper. Two strings. It does not matter what inside and which particular scheme it uses.

So maybe better to make unit test for URL class. Just to test parsing extracts auth data correctly from the string.

Or someone needs to explain to me where is Zookeeper setup for those tests and how to change it. But IMHO it is overkill.

[kaysoky]

Yes, a unit test for this URL parsing would be good, if that is the approach we end up with.

In terms of how the tests are set up:

• The gtest class ZooKeeperTest instantiates a ZooKeeperTestServer in its constructor: https://github.com/apache/mesos/blob/master/src/tests/zookeeper.hpp#L119
• This object is defined here: https://github.com/apache/mesos/blob/master/src/tests/zookeeper_test_server.hpp#L36
• This wrapper class instantiates a ZK server using the C API provided by ZK: https://github.com/apache/mesos/blob/master/src/tests/zookeeper_test_server.cpp#L57-L59

[dlazarus]

OK. I propose I am going to add a unit test for the new parsing logic and we'll merge this pull request.

If you want to develop a better approach to configure authentication data (which is definitely a good idea) it is better to do in a separate ticket. Because it is a separate issue.

Auth data in URL will stay anyway at least for backward compatibility.

Right now, they only way to use Mesos securely is to isolate the whole cluster in a private network. Because digest authentication is not good enough. On my work, without this patch, we cannot use Mesos in production. So it is a critical issue.