Skip to content

providers/virtualbox: read config from /Ignition/Config guest property#1269

Merged
bgilbert merged 1 commit intocoreos:mainfrom
bgilbert:virtualbox
Nov 1, 2021
Merged

providers/virtualbox: read config from /Ignition/Config guest property#1269
bgilbert merged 1 commit intocoreos:mainfrom
bgilbert:virtualbox

Conversation

@bgilbert
Copy link
Copy Markdown
Contributor

@bgilbert bgilbert commented Oct 9, 2021

VirtualBox supports "guest properties", which are persistent UTF-8 key-value pairs accessible from both the host and guest. Property values were originally limited to 128 bytes, and the VirtualBox codebase still contains vestiges of that limit, such as fixed-size buffers in some client code. The limit is no longer enforced by the VirtualBox core; it seems to have been removed in r21629 in 2009.

Read Ignition configs from an /Ignition/Config guest property by talking directly to the upstream vboxguest kernel module via /dev/vboxguest. Ignition configs can be attached to a VM with something like this command:

VBoxManage guestproperty set vm-name /Ignition/Config "$(cat config.ign)"

When using this approach (rather than the VirtualBox XPCOM API), configs are subject to these limits:

OS Limit Cause
Linux 128 KiB MAX_ARG_STRLEN
macOS 256 KiB ARG_MAX
Windows CreateProcess 32 KiB UNICODE_STRING.MaximumLength
Windows shells 8 KiB

(The limits are actually slightly lower for various reasons.)

For forward compatibility, check that the /Ignition/Config/Encoding property is missing or empty. In the future we might want to support alternate encodings (e.g. gzip+base64), similar to the VMware provider.

Drop support for the old magic partition GUID. It was implemented for the old Container Linux Vagrant provider, which was never widely deployed. It isn't practical for hand-crafting a config drive, since it requires creating a disk image with a partition table and then inserting the config into a partition, and I'm not aware of any active users in the wild.

Fixes #629.

msg->parm_count = 4;

// init arguments
char ch;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This seems not really used by us and mostly innocent, but I think it won't hurt zero-initializing it anyway.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I hesitate to initialize it, only because that might imply that the value is used. We don't similarly zero-fill the real buffer after allocating it.

@lucab
Copy link
Copy Markdown
Contributor

lucab commented Oct 11, 2021

Nice!
Do we maybe want to stick closer to the vmware provider, using a pair of /Ignition/Config/Data plus /Ignition/Config/Data/Encoding keys?

@bgilbert
Copy link
Copy Markdown
Contributor Author

Do we maybe want to stick closer to the vmware provider, using a pair of /Ignition/Config/Data plus /Ignition/Config/Data/Encoding keys?

I'm interested in thoughts on that topic. It looks like the pattern was originally introduced in #191 for compatibility with coreos-cloudinit, and I'm not sure if we should propagate it.

Both the FCOS and OCP docs recommend base64 encoding on VMware, which makes some sense when configs are embedded in VMX files. It seems less necessary for VirtualBox, which provides a proper API and correctly escapes guest properties in config files.

As to compression support, there was a previous discussion in #1100. I still tend to think users should handle userdata size limits by moving data into a merged config rather than relying on compression, but I wouldn't say there's a universal consensus there. I'm not aware of existing users of gzip encoding with VMware.

@bgilbert
Copy link
Copy Markdown
Contributor Author

Rebased onto #1271.

@lucab
Copy link
Copy Markdown
Contributor

lucab commented Oct 13, 2021

Fair points.

To be clear, I'm not arguing for multi-format auto-detection like in #1100, but for (optional) explicit encoding declaration.

For vmware we have some compression usecases that users directly documented for us, see https://docs.fedoraproject.org/en-US/fedora-coreos/provisioning-vmware/#_encoding_ignition_configuration. I'm unsure if we could be affected by similar limits here.

Overall I'm fine with this without encoding support upfront (or maybe ever), as long as we have some room to add it later if needed.
On that topic, does vbox support having a KV-pair at /Ignition/Config and also nesting another leaf at /Ignition/Config/Foo (i.e. is it internally a flat namespace of an actual tree)?

@travier
Copy link
Copy Markdown
Member

travier commented Oct 13, 2021

Linking coreos/fedora-coreos-tracker#144. This is interesting and could potentially enable Vagrant support!

@bgilbert
Copy link
Copy Markdown
Contributor Author

To be clear, I'm not arguing for multi-format auto-detection like in #1100, but for (optional) explicit encoding declaration.

Yup, understood.

For vmware we have some compression usecases that users directly documented for us, see https://docs.fedoraproject.org/en-US/fedora-coreos/provisioning-vmware/#_encoding_ignition_configuration. I'm unsure if we could be affected by similar limits here.

When injecting configs using VBoxManage, we're affected by the same 131071-byte limit for single arguments. (Which isn't ARG_MAX, contrary to that page. I'll PR a fix.)

Overall I'm fine with this without encoding support upfront (or maybe ever), as long as we have some room to add it later if needed. On that topic, does vbox support having a KV-pair at /Ignition/Config and also nesting another leaf at /Ignition/Config/Foo (i.e. is it internally a flat namespace of an actual tree)?

Yes, that's supported. The namespace is just key-value pairs.

Pursuant to that, I've added code to fail if /Ignition/Config/Encoding is present and non-empty. That way we can add alternate encodings later without those encodings causing parse errors on old Ignition versions.

VirtualBox supports "guest properties", which are persistent UTF-8
key-value pairs accessible from both the host and guest.  Property values
were originally limited to 128 bytes, and the VirtualBox codebase still
contains vestiges of that limit, such as fixed-size buffers in some client
code.  The limit is no longer enforced by the VirtualBox core; it seems to
have been removed in r21629 in 2009.

Read Ignition configs from an /Ignition/Config guest property by talking
directly to the upstream vboxguest kernel module via /dev/vboxguest.
Ignition configs can be attached to a VM with something like this command:

    VBoxManage guestproperty set vm-name /Ignition/Config "$(cat config.ign)"

When using this approach (rather than the VirtualBox XPCOM API), configs
are subject to these limits:

    Linux: 128 KiB (MAX_ARG_STRLEN)
    macOS: 256 KiB (ARG_MAX)
    Windows bash: 32 KiB (UNICODE_STRING.MaximumLength)
    Windows cmd.exe or PowerShell: 8 KiB

(The limits are actually slightly lower for various reasons.)

For forward compatibility, check that the /Ignition/Config/Encoding
property is missing or empty.  In the future we might want to support
alternate encodings (e.g. gzip+base64), similar to the VMware provider.

Drop support for the old magic partition GUID.  It was implemented for
the old Container Linux Vagrant provider, which was never widely deployed.
It isn't practical for hand-crafting a config drive, since it requires
creating a disk image with a partition table and then inserting the config
into a partition, and I'm not aware of any active users in the wild.

Fixes #629.
@saqibali-2k
Copy link
Copy Markdown

After playing around on windows, I found that the maximum size limit for a command is 8KiB for bash. cmd.exe and PowerShell also had the same limit but this was expected.

I tested this on WSL and git bash with VBoxManage guestproperty set vm-name /Ignition/Config "$(cat config.ign)" where config.ign contained a 33,000 character string. However, when running sudo ./ignition -platform virtualbox -stage fetch-offline -log-to-stdout on the guest command line, the sha512sum matched the hash of only the first 8098 characters.

@bgilbert bgilbert requested a review from lucab October 19, 2021 16:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

providers/virtualbox: investigate using GuestProperties

4 participants