-
Notifications
You must be signed in to change notification settings - Fork 249
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
providers: support for vendor-data in proxmoxve #2014
base: main
Are you sure you want to change the base?
Conversation
@@ -40,7 +41,8 @@ import ( | |||
) | |||
|
|||
const ( | |||
cidataPath = "/user-data" | |||
ciuserdataPath = "/user-data" | |||
civendordataPath = "/vendor-data" |
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.
Let's link to that Proxmox bugzilla issue here for more context.
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.
Done.
} | ||
|
||
// Try to validate if it's JSON | ||
if json.Valid(contents) { |
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.
Why check for JSON here? I prefer the current behaviour of "if it's not cloud-config data, assume that it's an Ignition config and otherwise fail". Though I guess we need to at least check that it's a non-zero file.
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.
It was a simple way to check that the file isn't empty and that it contains at least some valid JSON. I think it is fine to leave as is.
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.
Hmm, again would rather we be more strict here. We should either expect cloud-init configs, Ignition configs, or an empty file, nothing else.
As it is for example, a typo in the user's Ignition config will be ignored rather than hard fail the system. But also I think given the messiness of this platform, I'd rather we always know exactly what we're dealing with as a way to keep track of any changes in expectations.
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.
In the current implementation we don't check if it is valid JSON or valid Ignition config. It only checks the first line and if it contains #cloud-config
and if so it ignores the file. This means it won't ignore the file if they have invalid Ignition and hard fail the system. Checking for JSON at least ensures that there isn't an empty file but I can just revert it back to the current implementation if you want.
There is only so much that can be done to protect against invalid Ignition configuration and I felt at least checking for valid JSON data is better than nothing.
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.
I just pushed an update that implements checking to ensure the file contains data before returning and removes the check for valid JSON data.
This PR adds support for reading Ignition data out of the vendor-data file so we can leave the cloud-config in the user-data file. It will always use the user-data file if it has an ignition config over using the vendor-data file.
0d7040f
to
f60d7f9
Compare
…of it instead of verifying that it contains valid JSON data.
This PR adds support for reading Ignition data out of the vendor-data file so we can leave the cloud-config in the user-data file. It will always use the user-data file if it has an ignition config over using the vendor-data file.
This works as expected: