Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Kerberos] Find if port is available before using it for Kdc server #36192

Merged
merged 6 commits into from
Dec 5, 2018

Conversation

bizybot
Copy link
Contributor

@bizybot bizybot commented Dec 4, 2018

If the randomly selected port was already in use the Kerberos
tests would fail. This commit adds check to see if the network
port is available and if not continue to find one for KDC server.
If it does not find port after 100 retries it throws an exception.

Closes #34261

If the randomly selected port was already in use the kerberos
tests would fail. This commit adds check to see if the network
port is available and if not continue to find one for kdc server.
If it does not find port after 100 retries it throws an exception.

Closes elastic#34261
@bizybot bizybot added >test Issues or PRs that are addressing/adding tests v7.0.0 :Security/Authentication Logging in, Usernames/passwords, Realms (Native/LDAP/AD/SAML/PKI/etc) v6.6.0 labels Dec 4, 2018
@bizybot bizybot requested a review from tvernum December 4, 2018 01:16
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-security

@tvernum
Copy link
Contributor

tvernum commented Dec 4, 2018

I don't quite follow how this happens and/or how this will fix it.

NetworkUtil.getServerPort() already uses ServerSocket to request an unused port number.
Is the problem that it's binding to the wrong local address, or that it doesn't distinguish between TCP and UDP?

I simply don't follow why this change is going to solve a problem if getServerPort isn't.

@bizybot
Copy link
Contributor Author

bizybot commented Dec 4, 2018

Yes, you are right the NetworkUtil.getServerPort() already uses ServerSocket to request an unused port number. But does not distinguish between TCP and UDP and also I found that in my test it usually fails for the UDP (even when the port was bound getServerPort() used to return that port).

@tvernum
Copy link
Contributor

tvernum commented Dec 4, 2018

It doesn't make a lot of sense to use getServerPort to allocate the port number and then re-test it, and then loop again if it fails.
If the logic in getServerPort is incomplete for our use case, then we shouldn't bother calling it.

avoid use of `NetworkUtili#getServerPort()` function
which does not correctly provide a random available
port.
@bizybot
Copy link
Contributor Author

bizybot commented Dec 4, 2018

Instead of NetworkUtil#getServerPort() logic, the change now selects a random port, tests for availability and then bind to it. Thanks.

@bizybot bizybot merged commit 090d766 into elastic:master Dec 5, 2018
@bizybot bizybot deleted the fix-34261 branch December 5, 2018 05:45
bizybot added a commit to bizybot/elasticsearch that referenced this pull request Dec 5, 2018
…lastic#36192)

If the randomly selected port was already in use the Kerberos
tests would fail. This commit adds check to see if the network
port is available and if not continue to find one for KDC server.
If it does not find port after 100 retries it throws an exception.

Closes elastic#34261
bizybot added a commit that referenced this pull request Dec 5, 2018
…36192)

If the randomly selected port was already in use the Kerberos
tests would fail. This commit adds check to see if the network
port is available and if not continue to find one for KDC server.
If it does not find port after 100 retries it throws an exception.

Closes #34261
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
:Security/Authentication Logging in, Usernames/passwords, Realms (Native/LDAP/AD/SAML/PKI/etc) >test Issues or PRs that are addressing/adding tests v6.6.0 v7.0.0-beta1
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants