Conversation
achilleas-k
left a comment
There was a problem hiding this comment.
This is a very ugly draft of the firstboot support,
I'll skip any comments on code style then.
there are no BP changes yet only the internal BP struct because I think this will change as the review goes on.
I think in this case, the blueprint change is the most important thing we need to figure out. I think if we all had to separately figure out how this would work internally we'd all have roughly the same idea. We already have a few firstboot-type things (the subscription handling, the frontend feature, etc), so in terms of designing what these things will look like in the manifest, we have that pretty much solved.
pkg/manifest/firstboot.go
Outdated
| } | ||
|
|
||
| // create the executable | ||
| exec := fmt.Sprintf("/usr/local/sbin/%s", name) |
There was a problem hiding this comment.
Ah is a symlink now, old habit I guess, fixing.
There was a problem hiding this comment.
As followup to that, should this really life in /usr/local/bin/ ? it will be available via PATH for the entire life of the system, shouldn't it go to a different place outside PATH? I mean, its not meant to be run by the user and not meant to be run again
There was a problem hiding this comment.
/usr/libexec might make more sense for one-off scripts not expected to be ran by users.
There was a problem hiding this comment.
/usr/libexecmight make more sense for one-off scripts not expected to be ran by users.
We shouldn't ideally put there random scripts that are not owned by any package on the system. But in general, I agree. So /usr/local/libexec/ would probably be the ideal place.
| // Optional Satellite firstboot customization which is executed before all | ||
| // custom firstboot scripts. | ||
| Satellite *SatelliteFirstbootCustomization `json:"satellite,omitempty" toml:"satellite,omitempty"` | ||
|
|
||
| // Optional Ansible Automation Platform (AAP) firstboot customization which | ||
| // is executed before all custom firstboot scripts. | ||
| AAP *AAPFirstbootCustomization `json:"aap,omitempty" toml:"aap,omitempty"` |
There was a problem hiding this comment.
Is there any case where one would want multiple Satellite or AAP things to run? Should those also be arrays? Also, can these all be used in combination?
Or maybe this could be done in the same way as we did partitioning, where we would have an array of FirstBootCustomization at the top level and each customization has a type = custom|satellite|aap? That does get tricky to work with and validate though, so maybe we can avoid it if we can.
There was a problem hiding this comment.
Multiple Satellites no. Multiple AAP, I do believe that is very unlikely case but can't say for sure. Run both: yes, although Satellite does provide some Ansible capabilities the AAP is very often used in combination with Satellite IIRC.
That does get tricky to work with and validate though, so maybe we can avoid it if we can.
I thought about it, yeah. Well, what needs to be done must be done. Unless there are objections, I am gonna do that.
There was a problem hiding this comment.
One idea tho, this pattern will allow to repeat AAP/Sat. I guess in that case it is an error.
The big advantage of this approach is the ordering is always guaranteed.
| // Required firstboot script contents. Strings without shebang will be | ||
| // interpreted as shell scripts, otherwise the script will be executed | ||
| // using the shebang interpreter. | ||
| Contents string `json:"contents,omitempty" toml:"contents,omitempty"` |
There was a problem hiding this comment.
I suggest providing just a path to a script in the image, or as an alternative to providing the script content. We already have convenient ways to create custom files (scripts) in the image by specifying them inline or referencing them using a URI from the host filesystem.
There was a problem hiding this comment.
The same mechanism also allows you to create a systemd unit file itself. I can take your idea to the extreme by saying the feature I am working on can be replaced just with files customization. I want to keep this simple and convenient to the user, which was the motivation for this change.
There was a problem hiding this comment.
I'm sorry, but I disagree with your statement because you are trying to make it something that wasn't the core of my comment.
My main point is that file customizations already provide a much more convenient way to incorporate any custom script into the image and will continue to evolve in the future. With your approach, you'd always have to catch up to provide the same level of convenience with custom first boot customization.
EDIT: i.e, for on-prem, the ability to reference an actual local script by its path, instead of adding it inline to the BP, would be IMHO a very convenient thing.
Lastly, I suggest making this an option at least. That way, you can keep your current approach as well.
| ConditionPathExists=!/var/local/.osbuild-custom-first-boot-done | ||
| Wants=network-online.target | ||
| After=network-online.target | ||
| After=osbuild-first-boot.service | ||
|
|
||
| [Service] | ||
| Type=oneshot | ||
| {{ range .Executables }} | ||
| ExecStart={{ . -}} | ||
| {{ end }} | ||
| ExecStartPost=/usr/bin/touch /var/local/.osbuild-custom-first-boot-done | ||
| RemainAfterExit=yes | ||
|
|
||
| [Install] | ||
| WantedBy=basic.target |
There was a problem hiding this comment.
The customization allows one to define multiple first-boot custom scripts, which is a very good approach.
However, AFAICT, all of them would conditionally (not) run based on the existence of the same file, so in practice, only one would be run.
We could also order them based on the position in the list and allow specifying additional ordering for other unit files.
There was a problem hiding this comment.
I will be implementing @achilleas-k suggestion to have one array with multiple "types" of firstboot scripts similarly as in disk customization. Therefore, order will always be guaranteed.
There was a problem hiding this comment.
I meant order when systemd runs the unit. That needs to be expressed in the unit file, which was not handled at all in the original PR. Lastly, the ConditionPathExists needs to be addressed in any case.
|
In the meantime, I was able to test the PR and it fails the build :-D Not sure I understand. Could it be that |
Correct, however it's not about the buildroot but the |
This means the path doesn't exist in the OS tree itself. Which is weird because |
Right, but as a symlink to |
|
Just for the record, I created an empty build config with just firstboot customizations. It is a Fedora 42 server qcow2 config. |
| "github.com/osbuild/images/pkg/customizations/fsnode" | ||
| ) | ||
|
|
||
| // checkName prevents path traversal |
There was a problem hiding this comment.
I think the comment is a bit misleading, the checkName function is much stricter, as a side effect it prevents path traversal but if that was the goal the name could be relaxed.
| return fmt.Errorf("name cannot be empty") | ||
| } | ||
|
|
||
| for _, r := range str { |
There was a problem hiding this comment.
Could we just use: ^[a-zA-Z0-9_-]+$ here?
pkg/manifest/firstboot.go
Outdated
| } | ||
|
|
||
| // create the executable | ||
| exec := fmt.Sprintf("/usr/local/sbin/%s", name) |
There was a problem hiding this comment.
As followup to that, should this really life in /usr/local/bin/ ? it will be available via PATH for the entire life of the system, shouldn't it go to a different place outside PATH? I mean, its not meant to be run by the user and not meant to be run again
|
Can we agree on the customization before I commit to the unmarshaling implementation? package blueprint
type FirstbootCustomization struct {
// Type of the firstboot customization. Supported values are:
// "custom", "satellite", and "aap".
Type string `json:"type,omitempty" toml:"type,omitempty"`
// Optional firstboot name. Must be unique within the blueprint and only
// alphanumeric characters with dashes and underscores are allowed.
Name string `json:"name,omitempty" toml:"name,omitempty"`
// Ignore errors when executing the firstboot script and continue with
// execution of the following firstboot scripts, if any. By default,
// firstboot scripts are executed in order and if one of them fails, the
// execution stops immediately.
IgnoreFailure bool `json:"ignore_failure,omitempty" toml:"ignore_failure,omitempty"`
// Optional CA certificate to enroll into the system before executing the
// firstboot script.
CACerts []string `json:"cacerts,omitempty" toml:"cacerts,omitempty"`
CustomFirstbootCustomization
SatelliteFirstbootCustomization
AAPFirstbootCustomization
}
type CustomFirstbootCustomization struct {
// Strings without shebang will be interpreted as shell scripts, otherwise
// the script will be executed using the shebang interpreter. Required if
// type is set to "custom".
Contents string `json:"contents,omitempty" toml:"contents,omitempty"`
}
type SatelliteFirstbootCustomization struct {
// Registration command as generated by the Satellite server. Required, if
// type is set to "satellite".
Command string `json:"command,omitempty" toml:"command,omitempty"`
}
type AAPFirstbootCustomization struct {
// Job template URL as generated by the AAP server. Required if type is set
// to "aap". Example URLs are
// https://aap.example.com/api/controller/v2/job_templates/9/callback/ or
// https://aap.example.com/api/v2/job_templates/9/callback/ depending on the
// AAP version.
JobTemplateURL string `json:"job_template_url,omitempty" toml:"job_template_url,omitempty"`
// The host config key. Required if type is set to "aap".
HostConfigKey string `json:"host_config_key,omitempty" toml:"host_config_key,omitempty"`
}I am thinking prefixing all AAP fields with AAP and Satellite as well because possible future clash when we add a new type. |
|
Please don't just downvote, explain what is wrong with this. |
Sorry. Basically, the comment that @mvo5 had on Slack. I'd suggest doing it the other way around. Instead of |
It is weird from my PoV for the Hope this helps. |
| type AAPFirstbootOptions struct { | ||
| JobTemplateURL string | ||
| HostConfigKey string | ||
| CACerts []string |
There was a problem hiding this comment.
I think CRC also accepts a skip tls verification option too:
There was a problem hiding this comment.
I was thinking not having any certs would skip but being explicit is probably safer.
There was a problem hiding this comment.
Ya that's valid, just making a note of it :)
|
Since blueprints were extracted now, I am moving the conversation about the customization struct back to osbuild/blueprint#23 then I will continue this PR once that one is merged. |
|
Note for myself: Once
Good point,
|
|
This PR is stale because it had no activity for the past 30 days. Remove the "Stale" label or add a comment, otherwise this PR will be closed in 7 days. |
|
This PR was closed because it has been stalled for 30+7 days with no activity. |
This is a very ugly draft of the firstboot support, there are no BP changes yet only the internal BP struct because I think this will change as the review goes on. Also, I stuffed everything into one commit, will break it up later. I just want to get some feedback because I am not sure about this yet.
There are three ways how to submit firstboot:
Scripts are executed in-order and they do not fail the whole systemd unit, unless flag is set. Optional name can be provided.
For AAP, host config key and URL must be provided, the fistboot script is rendered to a shell script which looks like:
Satellite is actually a shell one-liner generated by Satellite UI/CLI, looks like:
In both cases, additional CA certificates (one or more) can be provided which are then added to the list of CA certs enrolled into the system bundle. This is done by a separate code (and stage) which I contributed last year.