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
> > >
> > >
> >
>

Reply via email to