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

allow to pass user/meta data files for cloud-init config #263

Merged
merged 3 commits into from
Feb 26, 2025

Conversation

lstocchi
Copy link
Contributor

@lstocchi lstocchi commented Feb 24, 2025

this patch allows vfkit to accept raw cloud-init config files to customize the VM.
It uses those files to generate an ISO image and inject it using virtio-blk device (0ae085d)

The supported and expected files are user-data and meta-data.
As user-data can be written in different formats (https://cloudinit.readthedocs.io/en/latest/explanation/format.html)
vfkit relies on the file names to detect them.
If the user do not pass user-data or meta-data, an empty file with that name will be added into the ISO image.

Vfkit deletes the cloud-init ISO file when it exits

Copy link

openshift-ci bot commented Feb 24, 2025

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

return "", err
}

// based on the file name we save its content on the corresponding var
Copy link
Collaborator

Choose a reason for hiding this comment

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

is it possible to use marshalling to determine the type?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

user-data can be written using different formats so I think it is easier to rely on the file name.

Cloud-init expects a user-data and meta-data files (they are mandatory) so asking the user to pass files with those names does not seem an absurdity

return "", errors.Wrap(err, "unable to fetch user cache dir")
}

isoFilePath := filepath.Join(cacheDir, "vfkit-cloudinit.iso")
Copy link
Collaborator

Choose a reason for hiding this comment

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

is this file deleted after vfkit exits? If not, I wonder if that filename should be somehow different per run? I dont know the origin of cachedir, so if the runs are namespaced, then you are probably cool. But I was thinking of a scneario where two vms are run, both with cloud-init, would they overwrite eachother?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, updated. It was fixed on that specific path bc of some specific script I was using for testing 👍
but yes, there is no reason to keep it fixed, updated to a random filename

@baude
Copy link
Collaborator

baude commented Feb 24, 2025

really cool idea ... i like where you are taking this. couple of questions from me bc I had to make some assumptions without a PR/commit message (hint hint)

@lstocchi
Copy link
Contributor Author

really cool idea ... i like where you are taking this. couple of questions from me bc I had to make some assumptions without a PR/commit message (hint hint)

yes, sorry. I just pushed the code to keep track of it but it was non-working code. Now it is fixed. I added a commit message 👍

}

if userDataReader == nil && metaDataReader == nil {
return "", fmt.Errorf("cloud-init needs user-data and meta-data files to work")
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm a bit confused by the error message and the test. If I'm not mistaken, the test implies that we need user-data or meta-data to be provided, but the way I understand the error message, we expect to get both?

Copy link
Contributor Author

@lstocchi lstocchi Feb 25, 2025

Choose a reason for hiding this comment

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

Cloud-init requires both files to work. They can be empty but they must exist otherwise it won't work.

However I think it's reasonable to allow the user to only pass one file. Maybe you just need to push some customization using the user-data (or the meta-data) and leave the other file empty. This way if we find out that you only have the user-data, we create an empty meta-data file (or vice versa).

On the other hand, if we do not find any of those files, there is most probably an error and the user needs to be informed

Copy link
Collaborator

Choose a reason for hiding this comment

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

should the input be a dir that contains files and we check for meta and user data as a minimum and proceed? or is it common to have multiples of those? (when I was a cloud-init expert in the day, it was just mostly those two)

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 am using this stuff in another tool and I thought the same thing.
Would it be acceptable to allow the user to pass a folder or files? Or would it be confusing?
--cloud-init <folder-path>
and also
--cloud-init <folder-path>/userdata,<other-folder>/meta-data

@lstocchi lstocchi force-pushed the i208 branch 2 times, most recently from a62bde9 to 4e8f093 Compare February 25, 2025 15:52
@lstocchi lstocchi requested a review from cfergeau February 25, 2025 15:54
Copy link
Collaborator

@cfergeau cfergeau left a comment

Choose a reason for hiding this comment

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

one minor go.mod comment, otherwise good to go

go.mod Outdated
@@ -53,7 +55,6 @@ require (
github.com/modern-go/concurrent v0.0.0-20180306012644-bacd9c7ef1dd // indirect
github.com/modern-go/reflect2 v1.0.2 // indirect
github.com/pelletier/go-toml/v2 v2.2.2 // indirect
github.com/pkg/errors v0.9.1 // indirect
Copy link
Collaborator

Choose a reason for hiding this comment

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

This change is unexpected now that you no longer use pkg/errors, maybe go mod tidy changes it back?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done 👍

this patch allows vfkit to accept raw cloud-init config files to customize the VM.
It uses those files to generate an ISO image and inject it using virtio-blk device (crc-org@0ae085d)

The supported and expected files are user-data and meta-data.
As user-data can be written in different formats (https://cloudinit.readthedocs.io/en/latest/explanation/format.html)
vfkit relies on the file names to detect them.
If the user do not pass user-data or meta-data, an empty file with that name will be added into the ISO image.

Vfkit deletes the cloud-init ISO file when it exits

Signed-off-by: Luca Stocchi <[email protected]>
Signed-off-by: Luca Stocchi <[email protected]>
@cfergeau
Copy link
Collaborator

/lgtm
/approve

Copy link

openshift-ci bot commented Feb 26, 2025

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: cfergeau

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-merge-bot openshift-merge-bot bot merged commit aa3cf4c into crc-org:main Feb 26, 2025
5 checks passed
lstocchi added a commit to lstocchi/macadam that referenced this pull request Feb 27, 2025
with crc-org/vfkit#263 and cfergeau/podman#3 we can leverage cloud-init to inject ssh key into a rhel vm.
this patch allows to set the cloudinit flag from macadam that would define if the vm should be customized by using cloud-init or the default ignition.

Signed-off-by: Luca Stocchi <[email protected]>
lstocchi added a commit to lstocchi/macadam that referenced this pull request Feb 27, 2025
with crc-org/vfkit#263 and cfergeau/podman#3 we can leverage cloud-init to inject ssh key into a rhel vm.
this patch allows to set the cloudinit flag from macadam that would define if the vm should be customized by using cloud-init or the default ignition.

Signed-off-by: Luca Stocchi <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants