-
Notifications
You must be signed in to change notification settings - Fork 2k
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
Don't exec uname for node attribute kernel.version #2380
Conversation
Previously with client fingerprinting, sys/exec's Command function was being used to execute `uname -r` and the return string processed into the kernel.version node attribute. This change uses gopsutil/host KernelVersion function instead. This means we can drop the os/exec, strings and fmt imports... and not execute an external binary. Signed-off-by: Dave Walker (Daviey) <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! I quickly peeked at gopsutil and it looks like it reads the same underlying source as uname on Linux and falls back to uname on OS's without /proc.
Seems like a safe simplification to me. One tiny change and this is ready to go!
client/fingerprint/host.go
Outdated
@@ -38,11 +35,7 @@ func (f *HostFingerprint) Fingerprint(cfg *config.Config, node *structs.Node) (b | |||
node.Attributes["kernel.version"] = "" | |||
|
|||
if runtime.GOOS != "windows" { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
KernelVersion will be an empty string on platforms without support so you can remove this guard and set kernel.version
above directly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, i left it there because I assumed it was not an interesting value on Windows, but happy enable it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I doubt it will ever be interesting on Windows but the fewer runtime.GOOS checks we have the happier I am! :)
Who knows, maybe someday gopsutils will populate a meaningful value for Windows.
Previously, this value was guarded against running on Windows because it called the `uname` command which is unlikely to be there. This change now sets the value from gopsutil, which might well be an empty string. Signed-off-by: Dave Walker (Daviey) <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! I kicked the Travis build to try to get it to go green again. Will merge soon.
I'm going to lock this pull request because it has been closed for 120 days ⏳. This helps our maintainers find and focus on the active contributions. |
Previously with client fingerprinting, sys/exec's Command
function was being used to execute
uname -r
and the returnstring processed into the kernel.version node attribute.
This change uses gopsutil/host KernelVersion function
instead. This means we can drop the os/exec, strings and
fmt imports... and not execute an external binary.
Signed-off-by: Dave Walker (Daviey) [email protected]