Thanks for the review Enrico! Do we need another +1 or are we ready to merge?
Regards, Abhilash Kishore On Tue, 20 Feb 2024 at 13:59, Enrico Olivelli <eolive...@gmail.com> wrote: > Il Mar 20 Feb 2024, 22:43 Abhilash Kishore <abhilash...@gmail.com> ha > scritto: > > > Yes, that's the PR <https://github.com/apache/zookeeper/pull/2117>. > > > > It's ready now. Do you mind taking a look? > > > > > Reviewed. > Thanks > > Enrico > > > > Regards, > > Abhilash Kishore > > > > > > On Tue, 13 Feb 2024 at 02:28, Andor Molnar <an...@apache.org> wrote: > > > > > Hi Abhilash, > > > > > > Is this the patch that you're working on? > > > > > > https://github.com/apache/zookeeper/pull/2117 > > > > > > I see it's still draft, are u going to finish it soon? > > > > > > Andor > > > > > > > > > > > > > > > On Mon, 2024-01-08 at 18:46 -0800, Abhilash Kishore wrote: > > > > Thanks Andor, that makes sense. I agree with you, this is a simpler > > > > and > > > > cleaner solution. > > > > > > > > I'll work on the changes and will try to keep it backwards > > > > compatible. > > > > > > > > Regards, > > > > Abhilash Kishore > > > > > > > > > > > > On Fri, 5 Jan 2024 at 09:00, Andor Molnar <an...@apache.org> wrote: > > > > > > > > > Hi Abhilash, > > > > > > > > > > Thanks for looking into this issue. > > > > > > > > > > I wouldn't complicate things by trying to get reconfig parameters > > > > > aligned and mixed with clientPort/secureClientPort. Since the > > > > > documentation says these options are already deprecated I suggest > > > > > to > > > > > upgrade Reconfig config line to support secure client port as well. > > > > > > > > > > So, the following reconfig line: > > > > > > > > > > "server.1=abhilash-ubuntu:3183:4183:participant;0.0.0.0:2181" > > > > > > > > > > will become: > > > > > > > > > > "server.1=abhilash- > > > > > ubuntu:3183:4183:participant;0.0.0.0:2181;0.0.0.0:21 > > > > > 82". > > > > > > > > > > The 3 scenarios will become: > > > > > > > > > > 1. Non-TLS only: > > > > > > > > > > "server.1=abhilash-ubuntu:3183:4183:participant;0.0.0.0:2181;" > > > > > > > > > > 2. TLS-only: > > > > > > > > > > "server.1=abhilash-ubuntu:3183:4183:participant;;0.0.0.0:2182". > > > > > > > > > > 3. TLS/non-TLS mixed: > > > > > > > > > > "server.1=abhilash- > > > > > ubuntu:3183:4183:participant;0.0.0.0:2181;0.0.0.0:21 > > > > > 82". > > > > > > > > > > In addition to that I would force the user to use either the > > > > > deprecated > > > > > settings (clientPort/secureClientPort) OR reconfig lines, but not > > > > > both. > > > > > Throw an exception and halt the server if both options are > > > > > specified at > > > > > the same time. > > > > > > > > > > Thoughts? > > > > > > > > > > Regards, > > > > > Andor > > > > > > > > > > > > > > > > > > > > On Tue, 2024-01-02 at 11:48 -0800, Abhilash Kishore wrote: > > > > > > Many organizations, large and small, have strict security and > > > > > > compliance > > > > > > requirements to only accept encrypted/TLS connections and not > > > > > > plain > > > > > > text > > > > > > connections. > > > > > > > > > > > > I'd like to discuss an issue which is preventing us from starting > > > > > > our > > > > > > ZK > > > > > > clusters in TLS only mode (for client traffic). > > > > > > > > > > > > As per dynamic reconfig doc > > > > > > <https://zookeeper.apache.org/doc/current/zookeeperReconfig.html > > > > > > > > ;;, > > > > > > > > > > > > > Starting with 3.5.0 the *clientPort* and *clientPortAddress* > > > > > > > configuration > > > > > > > parameters should no longer be used. Instead, this information > > > > > > > is > > > > > > > now part > > > > > > > of the server keyword specification, which becomes as follows: > > > > > > > server.<positive id> = > > > > > > > <address1>:<port1>:<port2>[:role];[<client > > > > > > > port > > > > > > > address>:]<client port> > > > > > > > > > > > > Let's say the dynamic config entry of a server is > > > > > > "server.1=abhilash-ubuntu:3183:4183:participant;0.0.0.0:2181". > > > > > > The > > > > > > server > > > > > > starts up with a (plaintext) clientPort listener on 2181. > > > > > > > > > > > > Now, if we want to make this server TLS-only, what options do we > > > > > > have? We > > > > > > want to stop accepting plaintext traffic on 2181 and make the > > > > > > same > > > > > > port > > > > > > accept TLS connections only (make clientPort as > > > > > > secureClientPort). > > > > > > > > > > > > If we add "secureClientPort=2181" in zoo.cfg, then ZK server > > > > > > first > > > > > > starts a > > > > > > plaintext listener on 2181 because of ";0.0.0.0:2181" in > > > > > > "server.1" > > > > > > dynamic > > > > > > config entry and then attempts to start a TLS client listener on > > > > > > the > > > > > > same > > > > > > port (2181) and fails. The reason for this behavior is already > > > > > > described in > > > > > > ZOOKEEPER-4276 < > > > > > > https://issues.apache.org/jira/browse/ZOOKEEPER-4276' > > > > > > > (highly > > > > > > recommended pre-read). > > > > > > > > > > > > It is not possible to just remove the "<client port>" part from > > > > > > the > > > > > > "server.1" entry as well (I believe it is mandatory from v3.5). I > > > > > > tried: > > > > > > > > > > > > [zk: localhost:2181(CONNECTED) 4] reconfig -remove 1 > > > > > > [zk: localhost:2181(CONNECTED) 5] reconfig -add > > > > > > server.1=abhilash-ubuntu:3183:4183:participant > > > > > > Arguments are not valid : > > > > > > > > > > > > > > > > > > The reconfig command does not allow us to add a server entry > > > > > > without > > > > > > ";[<client > > > > > > port address>:]<client port>". > > > > > > > > > > > > How do we support a "TLS-only" cluster in this case? > > > > > > > > > > > > My recommendation: > > > > > > > > > > > > 1. If both clientPort and secureClientPort are not set in > > > > > > zoo.cfg, > > > > > > then > > > > > > use the client port address from dynamic config. > > > > > > 2. If only clientPort is set in zoo.cfg, then it has to match > > > > > > the > > > > > > port > > > > > > in dynamic config and ZK starts a plaintext listener on this > > > > > > port. > > > > > > 3. If only secureClientPort is set in zoo.cfg, then it has to > > > > > > match the > > > > > > port in dynamic config and ZK starts a TLS listener on this > > > > > > port. > > > > > > 4. If both clientPort and secureClientPort are set in zoo.cfg, > > > > > > then the > > > > > > client port in zoo.cfg should match the port in dynamic > > > > > > config. ZK > > > > > > starts a > > > > > > plaintext listener on clientPort and TLS listener on > > > > > > secureClientPort (dual > > > > > > mode). > > > > > > > > > > > > > > > > > > This would reintroduce the requirement to set "clientPort" in > > > > > > zoo.cfg > > > > > > if > > > > > > someone wants to start the cluster in dual mode. > > > > > > > > > > > > For example, > > > > > > > > > > > > secureClientPort=2182 > > > > > > server.1=abhilash-ubuntu:3183:4183:participant;0.0.0.0:2181 > > > > > > > > > > > > will no longer be a valid config because of rule 3 above. > > > > > > > > > > > > It has to be: > > > > > > > > > > > > clientPort=2181 > > > > > > secureClientPort=2182 > > > > > > server.1=abhilash-ubuntu:3183:4183:participant;0.0.0.0:2181 > > > > > > > > > > > > > > > > > > I can create a PR to make the above changes, but first I'd like > > > > > > to > > > > > > know > > > > > > your thoughts on this and discuss further on whether there's a > > > > > > better > > > > > > way > > > > > > to handle this. > > > > > > > > > > > > Regards, > > > > > > Abhilash Kishore > > > > > > > > >
