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

Improve detection of Windows EC2 nodes by using UUID information #1052

Merged
merged 6 commits into from
Sep 7, 2017

Conversation

tas50
Copy link
Contributor

@tas50 tas50 commented Sep 6, 2017

We're currently relying on "amazon" Organization data being present in the Windows installation. This doesn't work if the VM was migrated from VMware and it doesn't work if the org builds their own AMIs that include their organization. Amazon recommends checking UUID data and we're already doing that on Linux. Lets do that on Windows.

Signed-off-by: Tim Smith <tsmith@chef.io>
Signed-off-by: Tim Smith <tsmith@chef.io>
This is actually how Amazon recommends doing it. It works no matter how the VM was built.

Signed-off-by: Tim Smith <tsmith@chef.io>
Signed-off-by: Tim Smith <tsmith@chef.io>
@tas50 tas50 requested a review from a team September 6, 2017 23:18
Signed-off-by: Tim Smith <tsmith@chef.io>
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.

Just a question about whether we need to be defensive about wmi.query possibly returning an empty result set. Otherwise LGTM!

if RUBY_PLATFORM =~ /mswin|mingw32|windows/
require "wmi-lite/wmi"
wmi = WmiLite::Wmi.new
if wmi.query("select uuid from Win32_ComputerSystemProduct")[0]["identifyingnumber"] =~ /^ec2/
Copy link
Contributor

Choose a reason for hiding this comment

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

Isn't there a chance that wmi.query(...)[0] will return nil (in the event of an empty result set) and then you'd be trying to call :[] on NilClass?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was thinking about that after I pushed it up. If we get an empty result set we didn't have a hint and we have no way to know we're on EC2. The plugin will fail and get skipped and the node won't be recognized as EC2, which is actually what we want. Ohai rescues the world (for better or worse) so we do sorta awful things like that throughout.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also I'm not sure how we'd not have a UUID, although I suppose anything is possible

Copy link
Contributor

Choose a reason for hiding this comment

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

What does Get-WmiObject Win32_ComputerSystemProduct | Select-Object IdentifyingNumber, UUID return on a Windows system in EC2?

You're searching for UUID but actually returning 'IdentifyingNumber' which is typically a serial number. That makes more sense than UUID if we're looking for a string with /^ec2/ in it.

In which case I think you just want this:

if wmi.first_of("Win32_ComputerSystemProduct")["identifyingnumber"] =~ /^ec2/

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's a much better query. Thanks

@tas50
Copy link
Contributor Author

tas50 commented Sep 7, 2017

Appveyor failure is valid. Right now the code is trying to run on windows in Appveyor and failing. Needs some mocking love.

Copy link
Contributor

@btm btm left a comment

Choose a reason for hiding this comment

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

fix appveyor and consider my nit about the more correct WmiLite::Wmi method, and 👍

Signed-off-by: Tim Smith <tsmith@chef.io>
@tas50 tas50 merged commit 2266373 into master Sep 7, 2017
@tas50 tas50 deleted the ec2_windows branch September 7, 2017 20:23
@chef chef locked and limited conversation to collaborators Feb 14, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants