Add VsphereGuestInfo settings source for vSphere stemcells #392
Conversation
cf-foundation-community-automation
bot
moved this to Inbox
in Foundational Infrastructure Working Group
- add cloudinit 'userData' settings as a possible source for agent settings information. This is intended as an alternative to CDRom settings for vSphere stemcells, and should fall back to the CDRom method in case the CPI is older and doesn't set the userData.
julian-hj
force-pushed
the
cloud-init-settings-source
branch
from
cdecf8e to
334db54
Compare
github-project-automation
bot
moved this from Inbox
to Waiting for Changes | Open for Contribution
in Foundational Infrastructure Working Group
There was a problem hiding this comment.
Pull request overview
This PR adds CloudInit 'userData' as a settings source for vSphere stemcells, providing an alternative to the CDROM-based settings retrieval method. The implementation reads settings from VMware guest tools (via vmware-rpctool or vmtoolsd as fallback), supporting both base64-encoded and gzip-compressed formats, and clears the data after reading for security purposes.
- Adds a new
CloudInitSettingsSourcethat retrieves agent settings from VMware guest info userdata - Integrates the new source type into the settings source factory with JSON unmarshalling support
- Includes comprehensive tests covering normal operation, gzip compression, tool fallback, error handling, and data clearing
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| infrastructure/settings_source_factory.go | Adds CloudInit source type registration and factory instantiation logic |
| infrastructure/cloudinit_settings_source.go | Implements new CloudInit settings source with VMware tools integration |
| infrastructure/cloudinit_settings_source_test.go | Provides comprehensive test coverage for CloudInit settings source functionality |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot <[email protected]>
Co-authored-by: Copilot <[email protected]>
julian-hj
force-pushed
the
cloud-init-settings-source
branch
from
b6ae98f to
8b4f3a4
Compare
julian-hj
changed the title
| _, _, _, err = s.cmdRunner.RunCommand("vmware-rpctool", "info-set guestinfo.userdata ---") | ||
| if err != nil { | ||
| //nolint:errcheck | ||
| s.cmdRunner.RunCommand("vmtoolsd", "--cmd", "info-set guestinfo.userdata ---") | ||
| } | ||
| _, _, _, err = s.cmdRunner.RunCommand("vmware-rpctool", "info-set guestinfo.userdata.encoding ") | ||
| if err != nil { | ||
| //nolint:errcheck | ||
| s.cmdRunner.RunCommand("vmtoolsd", "--cmd", "info-set guestinfo.userdata.encoding ") | ||
| } |
There was a problem hiding this comment.
What's happening here? Is this zeroing out the data that was just read?
If we always ignore the errors from vmtoolsd is it possible for the intention of these to stanzas to silently fail? e.g. in the event where lie 80 fails, and then line 83 also fails?
There was a problem hiding this comment.
Yeah, this is making a best-effort attempt to zero out the guest info (kind of akin to how we eject the CDROM)
If this operation fails then the creds we put in the guestInfo will hang around, but since those are generally not useful for very long, I don't think it is critical.
so basically we are trying it with vmware-rpctool, which is faster to run on some older versions, and then if that doesn't work we are using vmtoolsd, and if that also doesn't work, we're quietly giving up.
We might be able to log the failure, but I don't think we want to treat this failure as a failure to load settings--if we got this far, then we have the settings and ought to use them.
There was a problem hiding this comment.
@aramprice I made some tweaks to extract out a vmWareRPC method that handles the vmware-rpctool -> vmtoolsd fallback, and added logging when clearing the guestinfo.userdata fails. Please have another look.
There was a problem hiding this comment.
Maybe we still need to address this block in multi_settings_source.go though? Currently if there are multiple settings sources, and the first one fails, we will drop any errors on the floor without logging them, and just move on to the next settings source.
There was a problem hiding this comment.
Yeah - that block is generally not great. Using err for flow control 😬
Your idea of, at least, logging the fall-through is probably a good thing. It would be nice if vSphere didn't silently fall through to CDROM...
to VsphereGuestInfoSettingsSource, per PR feedback. Also add unit for no-op PublicSSHKeyForUsername function And DRY out the code a little bit and add logging when guestinfo.userdata cannot be cleared.
julian-hj
force-pushed
the
cloud-init-settings-source
branch
from
8b4f3a4 to
66f4a37
Compare
3 participants
Uh oh!
There was an error while loading. Please reload this page.