ZOOKEEPER-4259 - Allow AdminServer to force https

[nkalmar]

Initial commit to allow adminServer to only serve HTTPS requests
Questions:
-Is this fine that I stayed with portunification, or should we have a seperate secure port?

[symat]

LGTM
Although I haven't tested it locally yet, I assume you did it. I'll try to do it tomorrow if noone else does it.

Is this fine that I stayed with portunification, or should we have a seperate secure port?

I don't mind to stick to the same port. The user can decide now to make that single port to be http-only, https-only or http-and-https-unified. I think this is flexible enough. Usually in production people will stick to either http-only or https-only, and maybe using http-and-https-unified during upgrade / migration.

Do you thing we can add any unit test? Do we have unit tests for admin port unification? Maybe those could be reused / extended.

[nkalmar]

Thanks for the review @symat , yes, I was planning to add unit tests, this was just an initial commit to see what folks think about the implementation (mainly to sticking to one port only). I will do the unit tests shortly.

[eolivelli]

LGTM

[symat]

great work, also thanks for the unit tests!

I tested the patch locally:

• portUnification is still working (if portUnification=true and forceHttps=false)
• https enforced if forceHttps=true (independently of the value of portUnification)

I left a minor comment for the test code, PTAL.

[nkalmar]

Also, looking at the code "forceHTTPS" seems funky, I will change it to "forceHttps". Taking a quick look, all caps is not used for HTTPS in configs.

[symat]

great, thanks! LGTM

[ztzg]

LGTM (but not tested). This is now in master and branch-3.7. It is not in branch-3.7.0, as we currently have an open RC, nor in branch-3.6, as it did not cherry-pick cleanly. (Can you open another PR if you want it there?)

Cheers, -D

[nkalmar]

Thank you @ztzg , about 3.6... I did the cherry-pick and accidentally committed to apache 3.6 branch instead of mine, so, consider it done I guess. Sorry about that... I will see that the commit did not break anything.

Edit: commit seems good, spotbugs fails but from a different commit (quorumpeertest has a whitespace after a function start "(". It is not caused by my commit.

[ztzg]

Thank you @ztzg , about 3.6... I did the cherry-pick and accidentally committed to apache 3.6 branch instead of mine, so, consider it done I guess. Sorry about that... I will see that the commit did not break anything.

:)

Edit: commit seems good, spotbugs fails but from a different commit (quorumpeertest has a whitespace after a function start "(". It is not caused by my commit.

Right; that has been fixed by @arshadmohammad while preparing 3.6.3. Thanks!