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

Host resource ping method should return stdout #1927

Merged
merged 4 commits into from
Jun 15, 2017

Conversation

justincmoy
Copy link
Contributor

Prior to this PR, the ping method would return the following for nc (stderr output):

> echo | nc -v -w 1 127.0.0.1 1900 1>/dev/null
Ncat: Version 6.40 ( http://nmap.org/ncat )
Ncat: Connected to 127.0.0.1:1900.
Ncat: Connection reset by peer.

After this PR, the ping method would return the following for nc (stdout output):

> echo | nc -v -w 1 127.0.0.1 1900 2>/dev/null
DOWN

Signed-off-by: Justin Moy <justin.moy@sendgrid.com>
@justincmoy justincmoy force-pushed the host_resource_ping_method_stdout branch from db437da to 07d6f60 Compare June 14, 2017 22:16
@adamleff
Copy link
Contributor

@justincmoy I'm 👎 on this as-written because the STDERR from netcat is actually quite useful in troubleshooting, and the STDOUT isn't nearly as useful, especially since many far-end targets will not respond with any data when you send an empty message.

Can you explain the reason for this proposed change?

@justincmoy
Copy link
Contributor Author

justincmoy commented Jun 15, 2017

@adamleff Sure thing. We have an haproxy agent-check that connects to an xinetd daemon, similar to this blog post: https://icicimov.github.io/blog/server/HAProxy-dynamically-adjust-server-weight-using-external-agent/

We would like to write an inspec test that verifies connecting to our specified xinetd port returns expected output.

Edit:
Would it make sense to have these methods return an additional key corresponding to the stdout? And then a public method that can access it?

@adamleff
Copy link
Contributor

Thanks for sharing your use case. It makes the case for exposing both sets of content.

I'd like to avoid using stdout and stderr as the method names since we may change the implementation in the future and an end-user shouldn't need to know how we gathered the data. And if we want to provide both sets of output to the user, output isn't specific enough anymore.

What if we call the stderr output connection_output and the stdout output socket_output and have method names that match that?

@justincmoy
Copy link
Contributor Author

👍 I'll update this PR accordingly

Signed-off-by: Justin Moy <justin.moy@sendgrid.com>
@justincmoy justincmoy force-pushed the host_resource_ping_method_stdout branch from 29951f8 to f13a04c Compare June 15, 2017 16:36
Copy link
Contributor

@adamleff adamleff left a comment

Choose a reason for hiding this comment

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

@justincmoy this looks great, thank you. :)

@@ -156,7 +160,8 @@ def ping(hostname, port, protocol)

{
success: resp.exit_status.to_i.zero?,
output: resp.stderr,
connection_output: resp.stderr,
Copy link
Contributor

Choose a reason for hiding this comment

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

why not call it straight stdout and stderr

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This came up in an earlier comment. It's to hide the implementation details from the end user.

Copy link
Contributor

Choose a reason for hiding this comment

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

Got ya. Thank you, I misread this

Copy link
Contributor

Choose a reason for hiding this comment

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

As stated in #1927 (comment). Netcat's stdout and stderr is what we're using now, but if we iterate on this in the future and find a tool that doesn't use stdout/stderr the same way, then we're training the user on a specific implementation.

There are two bits of data being exposed: output about the connection setup, and the output from the socket itself after the connection is established.

I proposed connection_output and socket_output accordingly, and I think the _output piece is important so someone doesn't believe these can be manipulated -- it's simply the output of the connection attempt. I'm totally fine with removing _output if it helps calm any concerns.

Signed-off-by: Justin Moy <justin.moy@sendgrid.com>
@justincmoy justincmoy force-pushed the host_resource_ping_method_stdout branch from 7d09062 to aa09a93 Compare June 15, 2017 16:51
@adamleff
Copy link
Contributor

@justincmoy after an internal discussion, would you mind changing the method names and the hash keys to remove the _output from them?

connection_output => connection
socket_output => socket

Signed-off-by: Justin Moy <justin.moy@sendgrid.com>
Copy link
Contributor

@chris-rock chris-rock left a comment

Choose a reason for hiding this comment

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

Thank you @justincmoy and @adamleff

@adamleff adamleff merged commit 45f3b81 into inspec:master Jun 15, 2017
@adamleff adamleff added the Type: Enhancement Improves an existing feature label Jun 15, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: Enhancement Improves an existing feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants