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

[grok45]

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.

[grok45]

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

[grok45]

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.