customization: add firstboot

[lzap]

customization: add firstboot

Adds a new customization called firstboot. There are three types of
firstboot entries:

• custom
• satellite registration
• aap registration

Required field is "type" which decides the type, optionally name can be
used when generating filenames and failure of one firstboot script will
stop chain of execution, unless a flag is set.

The contents for the custom firstboot can be any executable script, such
as shell script, Python, or any other script that can be executed by the
system. If the firstboot string does not contain shebang, it is executed
depending on the target OS configuration, typically as a shell script.

Satellite requires the full registration command, as generated by
Satellite UI or CLI. AAP requires hook URL and a key in order to
generate the registration command. Both entries optionally take one or
more CACerts with CA for the registration command. If no CA cert is
provided for the AAP registration, then HTTP address will be used.

---

I am starting my work on proper firstboot support. This is the first step.

Edit: Since blueprint is now fully extracted from images I need to get this one merged first.

[mvo5]

Thanks! Just a quick question to see if I understand how this will work:

Will every string in the slice create a separate unit? So it looks like:

bp.Firstboot = []string{
  "#!/bin/sh\nset -e\necho do stuff",
  `#/usr/bin/python\n  if __name__ == "__main__":\n    print("do other stuff")`,
}

and that generates a "osbuild-firstboot1.service" and "osbuild-firstboot2.service"?

[lzap]

Good question, initially I thought creating separate units and that is one way of doing that. But then we would need to create dependencies among them to ensure the order which is I think very important (first you want to register, then you want to do something else). So I am currently thinking about one unit + multiple scripts. Something like:

[Unit]
ConditionFileIsExecutable=/usr/local/sbin/osbuild-custom-first-boot-1
ConditionFileIsExecutable=/usr/local/sbin/osbuild-custom-first-boot-2
ConditionFileIsExecutable=/usr/local/sbin/osbuild-custom-first-boot-3
ConditionPathExists=!/var/local/.osbuild-custom-first-boot-done
Wants=network-online.target
After=network-online.target
After=osbuild-first-boot.service

[Service]
Type=oneshot
ExecStart=-/usr/local/sbin/osbuild-custom-first-boot-1
ExecStart=-/usr/local/sbin/osbuild-custom-first-boot-2
ExecStart=-/usr/local/sbin/osbuild-custom-first-boot-3
ExecStartPost=/usr/bin/touch /var/local/.osbuild-custom-first-boot-done
RemainAfterExit=yes

[Install]
WantedBy=basic.target

This (untested) unit should execute script 1, 2 and 3 in this order ignoring their return codes (meaning failure in script 1 will not break the execution). The question is if we want to actually have that behavior or we want to error out early. In that case removing the - character will do the job. Now that I think about it this looks cleaner (e.g. when your registration fails you do not want to continue with other scripts). Well it depends, maybe we want to make firstboot substructure and make this configurable? Pseudo UBP:

firstboot:
  - name: "optional-name" # must be alphanum without spaces (used in filenames)
    fail_early: true
    contents: "#!/usr/bin/python\nprint 'Hello'\n"

The advantage of this is that we can easily extend, string is a string and always a breaking change.

What you all think? CC @achilleas-k

[lzap]

I rebased, now it is a new type instead of string, this is much more extensible in the future:

package blueprint

type FirstbootCustomization struct {
	// 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"`

	// 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"`
}

[achilleas-k]

Having multiple units and adding Before=, After=, and Requires= to order them isn't that much work, but we can experiment with the implementation and see what works best. I think a single unit with multiple Exec= lines would be simpler to manage and create less leftover build-time clutter on the system.

[lzap]

So @achilleas-k made a good call that I should probably think about AAP/Satellite registration scripts too, since we have decided today we want to have them as full customizations.

I amended an idea based on templates, this would mean we do not need to create a full customization for every new registration script.

[lzap]

Converting to draft, let me file an images PR first, there we can discuss the BP struct and I can then refactor this one.

[lzap]

Converting to draft, let me file an images PR first

I am afraid I must take this idea back as blueprint has been fully removed from images. So rebased, let's continue the review here.

I did what @thozza suggested I hope. Also @kingsleyzissou I am suggesting not to add explicit field for skip_tls for AAP as I think this could be done by simply not providing any TLS CA but I can add it if you prefer that.

[lzap]

@achilleas-k how about this?

[achilleas-k]

So this follows the model we used for blueprint.PartitionCustomization, where a single supertype embeds substructs for the subtypes and validation depends on verifying which fields are used based on the value of the type field.
@thozza commented on the old PR about not being a fan of this approach. I'll admit, Go makes these kinds of union types awkward to work with in some places. I would be interested to see if we have alternative ideas here.

In the partitioning case, we landed on that design because it's a good way to describe the reality of the way the partition table is laid out, i.e. partitions are ordered and not homogeneous. So, first of all, a couple of questions for how we imagine the implementation:

1. Are we expecting users to be using more than one firstboot command in combination? I assume so, otherwise we would not be making an array of FirstbootCustomization. But will they be combining different types?
2. Is the order significant? In other words, will adding a custom first followed by a satellite next create a result than if they were flipped?

EDIT/UPDATE: I know this flip-flopping and endless discussion can be frustrating and might be delaying other work, but we should really be giving a lot of thought to this because we'll be stuck with it for a while. I expect that however we end up structuring the new customization here, we'll be doing the same for the new blueprint schema as well.

[achilleas-k]

This is under the blueprint namespace, so the name is misleading (blueprint.CommonCustomization). It should include the term firstboot in there somewhere.

[achilleas-k]

Top-level arrays here have a tendency to get us into trouble when we realise we need to add some kind of global option for the customization group. For example, Disk could have originally been added as an array of PartitionCustomization (which is what we did back when we created Filesystem as an array of FilesystemCustomization), but by adding it as a DiskCustomization that includes an array of partitions, we were then able to add the global disk MinSize and later Type (for dos vs gpt), which we didn't have initially.

I can't think of a global option off the top of my head that we would want to add, but in my experience that's not a good indicator that we're not going to need it.

[lzap]

I would be interested to see if we have alternative ideas here.

I'd say let's not overthink this since blueprint is going away soon. What is more relevant I think is how we solved thisin UBP and things are very different there since we decided the Go code is fully generated the options were limited. OpenAPI/JSON Schema does support union types (and more) so the approach is to use raw JSON string and a detection code that utilizes JSON parser to auto-detect the structure.

type NetworkService struct {
	union json.RawMessage
}

// FirewallFromTo defines model for firewall_from_to.
type FirewallFromTo struct {
	// Protocol The network protocol used for the connection. This can be either TCP, UDP, ICMP or ANY protocol.
	//
	// Empty value is equivalent to `any` for port definitions. Protocol is ignored for service definitions.
	Protocol NetworkProtocol `json:"protocol,omitempty"`

	// Enabled Whether the firewall rule is enabled or not. Defaults to true.
	Enabled *FirewallEnabled `json:"enabled,omitempty"`

	// From The from port number, must be between 1 and 65535.
	From int `json:"from,omitempty"`

	// To The to port number, must be between 1 and 65535.
	To int `json:"to,omitempty"`
}

// FirewallPort defines model for firewall_port.
type FirewallPort struct {
	// Protocol The network protocol used for the connection. This can be either TCP, UDP, ICMP or ANY protocol.
	//
	// Empty value is equivalent to `any` for port definitions. Protocol is ignored for service definitions.
	Protocol NetworkProtocol `json:"protocol,omitempty"`

	// Enabled Whether the firewall rule is enabled or not. Defaults to true.
	Enabled *FirewallEnabled `json:"enabled,omitempty"`

	// Port The port number, must be between 1 and 65535.
	Port int `json:"port,omitempty"`
}

// FirewallService defines model for firewall_service.
type FirewallService struct {
	// Protocol The network protocol used for the connection. This can be either TCP, UDP, ICMP or ANY protocol.
	//
	// Empty value is equivalent to `any` for port definitions. Protocol is ignored for service definitions.
	Protocol NetworkProtocol `json:"protocol,omitempty"`

	// Enabled Whether the firewall rule is enabled or not. Defaults to true.
	Enabled *FirewallEnabled `json:"enabled,omitempty"`

	// Service The name of the IANA service name. This is the name of the service as defined in
	// the /etc/services file.
	Service string `json:"service,omitempty"`
}

func (t NetworkService) SelectUnion() (FirewallService, FirewallPort, FirewallFromTo, error) {
	var fs FirewallService
	err := json.Unmarshal(t.union, &fs)
	if err != nil {
		return FirewallService{}, FirewallPort{}, FirewallFromTo{}, err
	}

	var fp FirewallPort
	err = json.Unmarshal(t.union, &fp)
	if err != nil {
		return FirewallService{}, FirewallPort{}, FirewallFromTo{}, err
	}

	var fft FirewallFromTo
	err = json.Unmarshal(t.union, &fft)
	if err != nil {
		return FirewallService{}, FirewallPort{}, FirewallFromTo{}, err
	}

	return fs, fp, fft, nil
}

This is used for structs which do not have any flag that defines the type, there are also cases where there is such field present, for example:

// FSNode defines model for fsnode.
type FSNode struct {
	// Contents Contents is the file system node contents. When not present,
	// an empty file is created.
	Contents *FSNodeContents `json:"contents,omitempty"`

	// EnsureParents EnsureParents is a boolean that determines if the parent directories
	// should be created if they do not exist.
	EnsureParents bool `json:"ensure_parents,omitempty"`

	// Group Group is the file system node group. Can be also a decimal GID. Defaults to root.
	Group string `json:"group,omitempty"`

	// Mode Mode is the file system node permissions. Defaults to 0644 for
	// files and 0755 for directories. Must be a valid octal number as a string.
	Mode FSNodeMode `json:"mode,omitempty"`

	// Path Path is the absolute path to the file or directory.
	Path string `json:"path"`

	// State State is the state of the file system node, one of: present, absent. Defaults to present.
	State FSNodeState `json:"state,omitempty"`

	// Type Type is the type of the file system node, one of: file, dir.
	Type FSNodeType `json:"type,omitempty"`

	// User User is the file system node owner. Can be also a decimal UID. Defaults to root.
	User string `json:"user,omitempty"`
}

In this case, everything is straightforward and there is no embedding. This is again limitation of the generator as it does not generate any embedded structures, at least user-defined ones. The way we share code is at the JSON Schema level by creating subschemas. These can either be anonymous or named, so either copies or Go types are generated.

1. Are we expecting users to be using more than one firstboot command in combination? I assume so, otherwise we would not be making an array of FirstbootCustomization. But will they be combining different types?

Yes and yes. Let me elaborate below.

Is the order significant?

Yes. Ability to merge all types into one (ordered) list is important because I think there are use cases when you want to register Satellite first and only then execute something (let me do an ugly example of dnf install vim from Satellite repos), or execute some "workaround" before registering to AAP/Satellite. Therefore a common way of entering those in order is important.

I understand that people often think that Satellite users only use Satellite for all the management, but that is almost never the case. I have seen Satellite provisioning that registers a host to a different Satellite, various extra customizations that detect an environment or often workarounds for the process itself.

[lzap]

Rebased @achilleas-k concerns about root-level namespace.

If you do not like the idea of having ordered list of various types, then we can just limit custom firstboot scripts to be called after AAP/Sat and see how users like it and possibly add some PreScripts array if they need to also execute stuff before.

[avitova]

Thanks! I can see a potential issue. As far as I know, with this, it would be possible to create a FirstbootScriptCustomization, that has a Type = "satellite", but can as well have fields of AAP or CustomFirstBoot? Or both at once?
I think that we can also do something like the following, but then I do understand the need to put them under the same type, which is FirstbootScriptCustomization.

type SatelliteFirstbootCustomization struct {
	FirstbootCommonCustomization
	SatelliteCACerts []string `json:"satellite_cacerts,omitempty" toml:"satellite_cacerts,omitempty"`
	SatelliteCommand string `json:"satellite_command,omitempty" toml:"satellite_command,omitempty"`
}

I am just a bit scared that we might eventually end up creating hybrids. Do we ever use interfaces? One other of my thoughts after reminding myself how Go works after such a long time was, that we can have an interface "FirstbooScriptCustomization" with a function .Type(). Something like:

type FirstbootScript interface {
	Type() string
}

type FirstbootCustomization struct {
	Scripts []FirstbootScript `json:"scripts,omitempty" toml:"scripts,omitempty"`
}

type FirstbootCommonCustomization struct {
	Name          string `json:"name,omitempty" toml:"name,omitempty"`
	IgnoreFailure bool   `json:"ignore_failure,omitempty" toml:"ignore_failure,omitempty"`
}

type CustomFirstboot struct {
	FirstbootCommonCustomization
	CustomContents string `json:"custom_contents,omitempty" toml:"custom_contents,omitempty"`
}
func (c CustomFirstboot) Type() string { return "custom" }

But that kinda does not feel like the golang we are using, maybe just a food for thoughts.

[lzap]

Good point @avitova this brings an important topic now that blueprint has been extracted into this repo. I was planning to perform validation but the question is: where the initial data validation should happen? Is it here, in this repo, during JSON/TOML Unmarshal? Is it in images?

For UBP we agreed that basic validation should be done there, this is a typical example of such validation that does not depend on any other input like image type or architecture.

But now that you pointed out on the necessary validation that can be tricky, particularly with legacy blueprint package (because of TOML/JSON), maybe it is worth exploring the simple approach because I feel a lot of pushback. How about going for what I suggested above - a completely hardcoded order approach which we can customize if needed by adding some "priority" field if an only if necessary:

package blueprint

// FirstbootCustomization contains fields specific to firstboot customizations.
// The execution order is:
// - satellite registration
// - aap registration
// - custom scripts (keeping the order in which they are entered)
type FirstbootCustomization struct {
	// Registration command as generated by the Satellite server.
	SatelliteCommand string `json:"satellite_command,omitempty" toml:"satellite_command,omitempty"`

	// Job template URL as generated by the AAP server.
	// 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.
	AAPJobTemplateURL string `json:"aap_job_template_url,omitempty" toml:"aap_job_template_url,omitempty"`

	// The host config key. Required if AAPJobTemplateURL is set.
	AAPHostConfigKey string `json:"aap_host_config_key,omitempty" toml:"aap_host_config_key,omitempty"`

	// Optional CA certificate(s) to enroll into the system during image build phase.
	// The certificate will be available during firstboot for utilities like
	// curl or wget for TLS verification.
	CACerts []string `json:"cacerts,omitempty" toml:"cacerts,omitempty"`

	// Custom firstboot scripts to be executed during the firstboot process. They are executed
	// after registration.
	Scripts []FirstbootScriptCustomization `json:"scripts,omitempty" toml:"scripts,omitempty"`
}

type FirstbootScriptCustomization struct {
	// 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"`

	// Content 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".
	Content string `json:"content,omitempty" toml:"content,omitempty"`
}

That is a ton cleaner, is it?

[achilleas-k]

Thanks for all the work. LGTM.
A few notes below.

The strict unmarshalling I think should be done, perhaps in a follow-up. I also think if we have disagreements on the implementation (struct separation and embedding, unmarshalling methods, etc), we can revisit this later with a bit of experience in how easy or hard this structure is to work with. I think in terms of the user-facing format we all agree this is the best one though (it leaves the whole structure open to expansion at the top level and it orders multiple scripts of different types), so we can lock it in. The implementation can change without affecting the format.

I'd also like to expand the validation a bit more. For example, error cases like invalid or duplicate names aren't caught anywhere. Again though, let's leave this for a follow-up.

[achilleas-k]

What does All fields must be prefixed with "Custom". mean? Same for the other docstrings. Is it left over from a previous version?

[lzap]

Oh yes, a leftover.

[achilleas-k]

One thing this doesn't catch is cross-type fields being set. For example, if type == "custom" and job_template_url != "", that should be invalid. We can catch this automatically with strict unmarshalling (DisallowUnknownFields) and I think that would go a long way to avoiding silently ignoring invalid configs which can lead to confusion.

[lzap]

This is where JSON/OpenAPI Schema is good at, UBP will do this for us. But sure, good point, adding DisallowUnknownFields.

[achilleas-k]

By unmarshaling them into what?

[achilleas-k]

A couple of error test cases would be good here too.

[lzap]

The strict unmarshalling I think should be done, perhaps in a follow-up.

Sorry what strict unmarshalling?

Edit: Oh I see it in the review comments.

[lzap]

All remarks fixed, one additional change: I noticed that some fields can be missing, so I added errors for that plus tests.

[avitova]

Two comments, but I am not requesting changes because one is just a food for thought and the other is just enhancement of error messages.

[avitova]

I think I am good here now.

[ksiekl]

This looks good to me, thank you!

[lzap]

@achilleas-k I know this PR is a long read but in terms of code it is pretty trivial, thanks!