manifest: add firstboot support (HMS-9187)#1913
Conversation
supakeen
left a comment
supakeen
left a comment
There was a problem hiding this comment.
Did a quick read through, there might be other or more things but this is what jumped out with my morning coffee.
Two things:
- You forgot to add
FirstboottoOSCustomizations. - The panic thing commit can be a separate PR since it's the first thing I saw when opening this PR and I thought 'wait this isnt firstboot' ;)
|
Resolved all problems, thanks.
Not sure what you mean, there is no new type everything uses existing stages:
I see correct data in generated manifests but I need to test booting an image. Here is a snippet from Fedora: https://gist.github.com/lzap/ad28760b96ddf56e64ef4852f33650c5 |
|
So I boot-tested an image and all files are in place, but it does not work since I made a wrong assumption that multiple Several solutions:
What do you prefer? |
avitova
left a comment
avitova
left a comment
There was a problem hiding this comment.
I tried very hard to find something, but this actually looks very nice. No objections.
Want/After feels cleaner, but there might be details there that might not make it work as expected (like the second script starting after the first script has started, but before the first script has finished). |
I did a quick test and it works the way we want it with [achilleas@osbuild ~]$ cat /home/achilleas/.local/share/systemd/user/test.service
[Unit]
Description=test
[Service]
Type=oneshot
ExecStart=echo "ONE"
ExecStart=-ls /doesnotexist
ExecStart=echo "FIN"
[Install]
WantedBy=graphical-session.target
[achilleas@osbuild ~]$ systemctl --user start test.service
[achilleas@osbuild ~]$ systemctl --user status test.service
○ test.service - test
Loaded: loaded (/home/achilleas/.local/share/systemd/user/test.service; disabled; preset: disabled)
Drop-In: /usr/lib/systemd/user/service.d
└─10-timeout-abort.conf
Active: inactive (dead)
Oct 13 21:30:10 osbuild.devel systemd[1041]: Starting test.service - test...
Oct 13 21:30:10 osbuild.devel echo[1347]: ONE
Oct 13 21:30:10 osbuild.devel ls[1348]: ls: cannot access '/doesnotexist': No such file or directory
Oct 13 21:30:10 osbuild.devel echo[1350]: FIN
Oct 13 21:30:10 osbuild.devel systemd[1041]: Finished test.service - test.
Oct 13 21:31:30 osbuild.devel systemd[1041]: Starting test.service - test...
Oct 13 21:31:30 osbuild.devel echo[1360]: ONE
Oct 13 21:31:30 osbuild.devel ls[1362]: ls: cannot access '/doesnotexist': No such file or directory
Oct 13 21:31:30 osbuild.devel echo[1364]: FIN
Oct 13 21:31:30 osbuild.devel systemd[1041]: Finished test.service - test. |
EDIT: Oh and that's actually how you did it. Curious why it didn't work. 🤔 |
In addition, specifying the ordering of the custom first-boot script on an arbitrary other service is something that should be considered from the beginning. Installing a 3rd party service, enabling it and then having a custom first-boot script that does something after it is started does not sound like a far-fetched use case to me. |
Ha, I know why. The AAP command I was looking for had 
From the man page: If more than one command is configured, the commands are invoked sequentially in the order they appear in the unit file. If one of the commands fails (and is not prefixed with "-"), other lines are not executed, and the unit is considered failed. So I am going to just remove the |
|
Rebased, reversed the marker file logic. |
There was a problem hiding this comment.
I think the duplication of the Blueprint structures in pkg/customizations/firstboot/ is a bit unnecessary. The pkg/customizations/ types are meant to be convenient internal representations of options that serve as intermediate types between blueprint customizations and osbuild stages. Sometimes, it's convenient to have something that very closely resembles the blueprint itself (like with users and groups), because that makes the most sense. In other cases, it might resemble the stage options closer, or be completely independent.
I think in this case, the Blueprint structures aren't convenient internally. They're designed to be convenient for user input, but the union types make it awkward to handle. So copying them from one set of union types to another isn't really giving us any benefit.
Looking at the implementations of the each functions, it looks like for every firstboot customization we basically generate an executable file and an exec line for the systemd unit (with the optional - prefix). Also certs for AAP and Satellite. It seems to me that these things would be perfect as an intermediate representation of the firstboot customizations. So what I'm imagining is the following:
pkg/customizations/firstboot/defines a type,Scriptthat's basically
type Script struct {
Filename string
Contents string
IgnoreFailure bool
Certs []string
}-
The
FirstBootOptionsFromBP(), instead of copying the BP structs to identical ones inside images, implements analogues to theeachfunctions (the ones that are currently inpkg/manifest/) that takeblueprint.FirstBootCustomizationand return[]firstboot.Script. -
In
pkg/manifest/, a function takes[]firstboot.Scriptand produces a set of[]fsnode.File, CA certs, and systemd unit create stage options (equivalent to whatparse()does now, but I imagine with a simpler implementation).
This way, it would be a bit easier to (for example) define a firstboot script internally for something we want to define statically in an image type. Consider:
regFirstboot := firstboot.SatelliteFirstbootOptions{
FirstbootCommonOptions: firstboot.FirstbootCommonOptions{
Name: "satellite",
IgnoreFailure: true,
},
CACerts: []string{"cert1", "cert2"},
Command: "#!/usr/bin/bash\ncurl https://sat.example.com/register",
}
certs, files, unit, err := parse(&firstboot.FirstbootOptions{
Scripts: []firstboot.FirstbootOption{
regFirstBoot,
}
})vs
regFirstboot := firstboot.Script{
Filename: "satellite",
Command: "#!/usr/bin/bash\ncurl https://sat.example.com/register",
IgnoreFailure: true
CACerts: []string{"cert1", "cert2"},
}
files, certs, unit, err := genFirstbootComponents([]firstboot.Script{regFirstBoot})(minor difference, but more readable IMO).
|
I was struggling to understand the difference between the two structures and this really helped to sort out my understanding. Yes, this makes sense and after I refactored the code it looks so much nicer. Rebased, and fixed tests as well. |
|
Rebased, resolved all your comments thanks. diff --git a/pkg/customizations/firstboot/firstboot.go b/pkg/customizations/firstboot/firstboot.go
index 6a7b04bf5..0f05f9b5a 100644
--- a/pkg/customizations/firstboot/firstboot.go
+++ b/pkg/customizations/firstboot/firstboot.go
@@ -4,8 +4,6 @@ import (
"errors"
"fmt"
"path/filepath"
- "strings"
- "text/template"
"github.com/osbuild/blueprint/pkg/blueprint"
"github.com/osbuild/images/pkg/shutil"
@@ -90,25 +88,6 @@ func (AAPFirstbootOptions) isFirstbootOption() {}
var ErrFirstbootAlreadySet = errors.New("firstboot customization already set")
-var tmplFirstbootAAP = `#!/usr/bin/bash
-curl -i --data {{ .HostConfigKey }} {{ .URL }}
-`
-
-func renderFirstboot(tmplStr string, data any) (string, error) {
- tmpl, err := template.New("firstboot-unit").Parse(tmplStr)
- if err != nil {
- return "", fmt.Errorf("error parsing firstboot unit template: %w", err)
- }
-
- var result strings.Builder
- err = tmpl.Execute(&result, data)
- if err != nil {
- return "", fmt.Errorf("error rendering firstboot unit: %w", err)
- }
-
- return result.String(), nil
-}
-
// FirstbootOptionsFromBP converts a blueprint FirstbootCustomization to
// FirstbootOptions. Validation is done in the blueprint package, so this function
// assumes the input is valid, however, JSON unmarshalling errors are possible.
@@ -162,17 +141,10 @@ func FirstbootOptionsFromBP(bpFirstboot blueprint.FirstbootCustomization) (*Firs
}
aapDone = true
- data := struct {
- URL string
- HostConfigKey string
- }{
- URL: shutil.Quote(aap.JobTemplateURL),
- HostConfigKey: shutil.Quote("host_config_key=" + aap.HostConfigKey),
- }
- contents, err := renderFirstboot(tmplFirstbootAAP, data)
- if err != nil {
- return nil, err
- }
+ contents := fmt.Sprintf("#!/usr/bin/bash\ncurl -i --data %s %s\n",
+ shutil.Quote("host_config_key="+aap.HostConfigKey),
+ shutil.Quote(aap.JobTemplateURL),
+ )
fo.Scripts = append(fo.Scripts, Script{
Filename: nameFunc(aap.Name, "aap"),
diff --git a/pkg/manifest/firstboot.go b/pkg/manifest/firstboot.go
index bd9a0dfdd..35f8cacdb 100644
--- a/pkg/manifest/firstboot.go
+++ b/pkg/manifest/firstboot.go
@@ -10,11 +10,10 @@ import (
"github.com/osbuild/images/pkg/osbuild"
)
-// parse processes the firstboot options and returns a list of CA certificates to
+// handleFirstbootOptions processes the firstboot options and returns a list of CA certificates to
// include in the image, a list of file nodes to create the firstboot scripts, and
// a systemd unit to run the scripts on first boot.
-// TODO RENAME THIS
-func parse(fbo *firstboot.FirstbootOptions) ([]string, []*fsnode.File, *osbuild.SystemdUnitCreateStageOptions, error) {
+func handleFirstbootOptions(fbo *firstboot.FirstbootOptions) ([]string, []*fsnode.File, *osbuild.SystemdUnitCreateStageOptions, error) {
if fbo == nil {
return nil, nil, nil, nil
}
diff --git a/pkg/manifest/firstboot_test.go b/pkg/manifest/firstboot_test.go
index e9017d4d9..32e5291a8 100644
--- a/pkg/manifest/firstboot_test.go
+++ b/pkg/manifest/firstboot_test.go
@@ -134,7 +134,7 @@ echo 'unnamed'`
}
}`
- certs, files, unit, err := parse(fbo)
+ certs, files, unit, err := handleFirstbootOptions(fbo)
assert.NoError(t, err)
assert.Equal(t, []string{"cert1", "cert2", "cert3", "cert4"}, certs)
diff --git a/pkg/manifest/os.go b/pkg/manifest/os.go
index 9ebc9781a..b9090de19 100644
--- a/pkg/manifest/os.go
+++ b/pkg/manifest/os.go
@@ -661,9 +661,9 @@ func (p *OS) serialize() (osbuild.Pipeline, error) {
pipeline = prependStage(pipeline, osbuild.NewDracutConfStage(dracutConfConfig))
}
- fbCerts, fbFiles, fbUnit, err := parse(p.OSCustomizations.Firstboot)
+ fbCerts, fbFiles, fbUnit, err := handleFirstbootOptions(p.OSCustomizations.Firstboot)
if err != nil {
- panic(err)
+ return osbuild.Pipeline{}, err
}
if len(fbFiles) > 0 { |
|
Ah missed one comment about the commit ordering, reordered and generating checksums now. |
|
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. |
avitova
left a comment
avitova
left a comment
There was a problem hiding this comment.
I am only unsure about the logic to create names for firstboot files, otherwise LGTM.
| if len(fbCerts) > 0 { | ||
| p.OSCustomizations.CACerts = append(p.OSCustomizations.CACerts, fbCerts...) | ||
| } | ||
|
|
||
| if fbUnit != nil { | ||
| p.OSCustomizations.EnabledServices = append(p.OSCustomizations.EnabledServices, fbUnit.Filename) | ||
| p.OSCustomizations.SystemdUnit = append(p.OSCustomizations.SystemdUnit, fbUnit) | ||
| } |
There was a problem hiding this comment.
I'm not the biggest fan of writing to OSCustomizations from within pipeline generators. Since we already do it twice in this file I won't block on that but it's something to revisit in the future.
There was a problem hiding this comment.
I am not entirely sure if I get what you mean, but I added one extra follow-up commit as a refactoring of all three places. I can drop this if this is not what you meant.
|
Sorry, almost forgot about this. Could you resolve conflicts? And we should try to get this in. |
|
Rebased, cheers. |
|
Pulled the wrong command from my shell history, once again. |
|
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. |
|
Rebased. |
In order to create firstboot customizations, we need to define a new type in the manifest package. This commit introduces the FirstbootOptions type along with its associated methods and tests. The type uses a common Go pattern for handling unions, allowing for different customization options such as CustomFirstbootOptions, SatelliteFirstbootOptions, and AAPFirstbootOptions. Function FirstbootOptionsFromBP converts a Blueprint firstboot customization to a manifest FirstbootOptions which is a slice of scripts.
Adds some firstboot into all customization.
Also, in case of ostree cacert was not present and it is a dependency of firstboot, add it too.
Writing directly to OSCustomizations is not a good idea. Use local variables instead. This patch refactors Subscription, WSL and Firstboot customizations all at once since this is the pattern that is common to all of them.
|
Resolved conclicts so many reviews but why everyone dropped it @avitova @achilleas-k @thozza thanks |
thozza
dismissed
their stale review
I won't block, but also won't approve. Cheers.
brlane-rht
left a comment
brlane-rht
left a comment
There was a problem hiding this comment.
It would be a bit cleaner if you split out the new code into separate commits first, one for the new firstboot customization, one for the new osbuild stage, then hook everything up to the rest of the code. I also agree with the change to OSCustomizations, it should be squashed into the other commit -- the expectation is that OSCustomization has already been setup when the manifest serialize gets called.
This should also have a reference to a Jira ticket if there is one.
| var ci int | ||
| var alreadyUsed []string | ||
|
|
||
| nameFunc := func(inputName, prefix string) string { |
There was a problem hiding this comment.
I don't like the use of anonymous functions, it feels too much like javascript and makes it impossible to test the function to make sure it behaves as expected.
You're using it because you want ci and alreadyUsed to be available, but I think it would be cleaner and easier to test if you put those common values into a struct with a name generator function method.
|
Note for myself (I am busy atm):
|
lzap
changed the title
6 participants
Replaces: #1705
Fixes: https://redhat.atlassian.net/browse/HMS-9187