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

ipautil: check for open ports on all resolved IPs #284

Closed
wants to merge 1 commit into from

Conversation

tkrizek
Copy link
Contributor

@tkrizek tkrizek commented Nov 29, 2016

When a hostname is provided to host_port_open, it should check if
ports are open for ALL IPs that are resolved from the hostname, instead
of checking whether the port is reachable on at least one of the IPs.

https://fedorahosted.org/freeipa/ticket/6522

Copy link
Contributor

@pspacek pspacek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please fix the socket closure in ipautil. I would not bother with it in ipa-replica-conncheck but the function can be used from other modules as well and who knows what could break.

if socket_type == socket.SOCK_DGRAM:
root_logger.debug(msg)
else:
root_logger.error(msg)
finally:
if s:
s.close()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The socket is not closed properly, I would say. s.close() is outside of the cycle but the cycle can open many sockets...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The finally block is inside the cycle, so all opened sockets should be closed properly.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are right, I'm blind.

# Do not log udp failures as errors (to be consistent with
# the rest of the code that checks for open ports)
if socket_type == socket.SOCK_DGRAM:
root_logger.debug(msg)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would like to see this on WARNING level so it is consistent with messages printed by conncheck (and actually visible to user).

Copy link
Contributor

@pspacek pspacek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK

# Do not log udp failures as errors (to be consistent with
# the rest of the code that checks for open ports)
if socket_type == socket.SOCK_DGRAM:
root_logger.warning(msg)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just noticed that FreeIPA logging is so botched that it does not show WARNING messages by default. Please leave the code as it is and I will ack it.

@pspacek pspacek added the ack Pull Request approved, can be merged label Nov 30, 2016
@MartinBasti
Copy link
Contributor

needs rebase

When a hostname is provided to host_port_open, it should check if
ports are open for ALL IPs that are resolved from the hostname, instead
of checking whether the port is reachable on at least one of the IPs.

https://fedorahosted.org/freeipa/ticket/6522
@MartinBasti
Copy link
Contributor

@MartinBasti MartinBasti added the pushed Pull Request has already been pushed label Dec 7, 2016
@MartinBasti MartinBasti closed this Dec 7, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ack Pull Request approved, can be merged pushed Pull Request has already been pushed
Projects
None yet
3 participants