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.

Sign up for GitHub

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

Jump to bottom

config/fcos: Add extensions param and generate treefile with packages #304

Merged
merged 1 commit into from
Jan 25, 2022
Merged

config/fcos: Add extensions param and generate treefile with packages #304

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions config/fcos/v1_5_exp/schema.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ import (
type Config struct {
base.Config `yaml:",inline"`
BootDevice BootDevice `yaml:"boot_device"`
Extensions Extensions `yaml:"extensions"`
}

type BootDevice struct {
Expand All @@ -38,3 +39,5 @@ type BootDeviceLuks struct {
type BootDeviceMirror struct {
Devices []string `yaml:"devices"`
}

type Extensions []string
Copy link
Contributor

Choose a reason for hiding this comment

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

Are we sure we won't have additional options related to extensions in the future? This schema doesn't allow for any.

The usual way we do this is:

extensions:
  - name: foo
  - name: bar

Copy link
Member Author

Choose a reason for hiding this comment

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

We are basically adding something like the RHCOS extensions to butane: https://github.com/openshift/machine-config-operator/blob/master/docs/MachineConfiguration.md#rhcos-extensions . But I am not sure how this will evolve. This is still being discussed but for the first pass AFAIK we just care about a list of packages.

Copy link
Contributor

Choose a reason for hiding this comment

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

Once this spec version has been stabilized, we lose the ability to add metadata in this way without making a new top-level field. We could merge this as-is and revisit before stabilization, but if we do, we should make sure to leave clear signposts in the issue tracker so we don't forget. Or we could go for the extensible approach now. It might be worth raising with the team.

Copy link
Member Author

Choose a reason for hiding this comment

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

@cgwalters @jlebon any preference on how to handle this?

Copy link
Member

Choose a reason for hiding this comment

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

Agree with the concern, but not sure what to recommend since it's hard to predict what we'll need to generalize (container-based extensions? more treefile support? repo-pinned extensions?).

So... I'd say let's roll with this for now and revisit before stabilization. In the worst case, we should be able to change the spec in future versions, right?

Copy link
Contributor

Choose a reason for hiding this comment

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

Discussed this more with @jlebon OOB. If we wanted to add extensibility in future versions, and didn't want to have a major spec bump (i.e. one that requires config changes when bumping the version number), we'd need to deprecate the extensions field and add e.g. extensions2. We've done that before but it's a bit awkward.

I think the maximum possible extensibility is:

extensions:
  packages:
    - name: foo
    - name: bar

which lets us add system-wide extension options, per-package ones, and new extension types:

extensions:
  global-option: z
  packages:
    - name: foo
      package-option: z
    - name: bar
  containers:
    - name: baz

Global options don't play well with config merging, since a child config that adds a package would be affected by global parameters it isn't aware of. New extension types could be implemented as a package field:

extensions:
  - name: foo
    package-option: z
  - name: bar
  - name: baz
    type: container

which is consistent with the design elsewhere in the schema.

I think that's a reasonable middle ground. Let's not block the current PR on this, but as a followup I think we should switch from an array of strings to an array of structs.

Copy link
Member

Choose a reason for hiding this comment

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

This is very related to what happened to packages in
https://github.com/coreos/rpm-ostree/blob/main/docs/treefile.md

We ended up adding repo-packages which allows to pin to packages from a specific repo.

--

The idea of type: container is a really interesting idea. Definitely touches on https://www.freedesktop.org/software/systemd/man/systemd-sysext.html etc.

Copy link
Member

Choose a reason for hiding this comment

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

In rpm-ostree's treefiles today to be nice we allow multiple packages in single YAML list entry. See for example https://github.com/coreos/fedora-coreos-config/blob/aa50439847f34282b41bf2eb60d478a45d2a27d7/manifests/system-configuration.yaml#L7

Might make sense to do that too to avoid the overhead in the simple cases?

Copy link
Contributor

Choose a reason for hiding this comment

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

We generally don't do that in Ignition configs because it wouldn't work with the config merging logic, which (with one unfortunate exception) doesn't know the semantics of what it's merging. That argument doesn't directly apply here, since this is sugar, but it still doesn't feel very Butane-like. We've favored explicitness over ergonomics when they're in conflict.

Copy link
Contributor

Choose a reason for hiding this comment

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

Followup in #316.

55 changes: 55 additions & 0 deletions config/fcos/v1_5_exp/translate.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,8 @@
package v1_5_exp

import (
"crypto/sha256"
"encoding/hex"
"fmt"

baseutil "github.com/coreos/butane/base/util"
Expand All @@ -26,6 +28,7 @@ import (
"github.com/coreos/ignition/v2/config/v3_4_experimental/types"
"github.com/coreos/vcontext/path"
"github.com/coreos/vcontext/report"
"gopkg.in/yaml.v3"
)

const (
Expand Down Expand Up @@ -78,6 +81,11 @@ func (c Config) ToIgn3_4Unvalidated(options common.TranslateOptions) (types.Conf
}
}
}

retp, tsp, rp := c.processPackages(options)
retConfig, ts := baseutil.MergeTranslatedConfigs(retp, tsp, ret, ts)
ret = retConfig.(types.Config)
r.Merge(rp)
bgilbert marked this conversation as resolved.
Show resolved Hide resolved
return ret, ts, r
}

Expand Down Expand Up @@ -292,3 +300,50 @@ func translateBootDeviceLuks(from BootDeviceLuks, options common.TranslateOption
tm.AddTranslation(path.New("yaml"), path.New("json"))
return
}

func (c Config) processPackages(options common.TranslateOptions) (types.Config, translate.TranslationSet, report.Report) {
yamlPath := path.New("yaml", "extensions")
ret := types.Config{}
ts := translate.NewTranslationSet("yaml", "json")
var r report.Report
if len(c.Extensions) == 0 {
return ret, ts, r
}

treeFileContents, err := yaml.Marshal(&struct {
Packages []string `yaml:"packages"`
}{
Packages: c.Extensions,
})
if err != nil {
r.AddOnError(yamlPath, err)
return ret, ts, r
}
fullYamlContents := append([]byte("# Generated by Butane\n\n"), treeFileContents...)
src, gzipped, err := baseutil.MakeDataURL(fullYamlContents, nil, !options.NoResourceAutoCompression)
if err != nil {
r.AddOnError(yamlPath, err)
return ret, ts, r
}
hash := sha256.New()
hash.Write([]byte(src))
sha := hex.EncodeToString(hash.Sum(nil))[0:7]
file := types.File{
Node: types.Node{
Path: "/etc/rpm-ostree/origin.d/extensions-" + sha + ".yaml",
},
FileEmbedded1: types.FileEmbedded1{
Contents: types.Resource{
Source: util.StrToPtr(src),
},
Mode: util.IntToPtr(0644),
},
}
if gzipped {
file.Contents.Compression = util.StrToPtr("gzip")
}

ret.Storage.Files = append(ret.Storage.Files, file)
ts.AddFromCommonSource(yamlPath, path.New("json", "storage"), ret.Storage)
return ret, ts, r
}
58 changes: 58 additions & 0 deletions config/fcos/v1_5_exp/translate_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1420,3 +1420,61 @@ func TestTranslateBootDevice(t *testing.T) {
})
}
}

// TestTranslateExtensions tests translating the Butane config extensions section.
func TestTranslateExtensions(t *testing.T) {
tests := []struct {
in Config
out types.Config
exceptions []translate.Translation
report report.Report
}{
// config with two extensions/packages
{
Config{
Extensions: []string{"strace", "zsh"},
},
types.Config{
Ignition: types.Ignition{
Version: "3.4.0-experimental",
},
Storage: types.Storage{
Files: []types.File{
{
Node: types.Node{
Path: "/etc/rpm-ostree/origin.d/extensions-e2ecf66.yaml",
},
FileEmbedded1: types.FileEmbedded1{
Contents: types.Resource{
Source: util.StrToPtr("data:;base64,IyBHZW5lcmF0ZWQgYnkgQnV0YW5lCgpwYWNrYWdlczoKICAgIC0gc3RyYWNlCiAgICAtIHpzaAo="),
},
Mode: util.IntToPtr(420),
},
},
},
},
},
[]translate.Translation{
{path.New("yaml", "version"), path.New("json", "ignition", "version")},
{path.New("yaml", "extensions"), path.New("json", "storage")},
{path.New("yaml", "extensions"), path.New("json", "storage", "files")},
{path.New("yaml", "extensions"), path.New("json", "storage", "files", 0)},
{path.New("yaml", "extensions"), path.New("json", "storage", "files", 0, "path")},
{path.New("yaml", "extensions"), path.New("json", "storage", "files", 0, "mode")},
{path.New("yaml", "extensions"), path.New("json", "storage", "files", 0, "contents")},
{path.New("yaml", "extensions"), path.New("json", "storage", "files", 0, "contents", "source")},
},
report.Report{},
},
}

for i, test := range tests {
jmarrero marked this conversation as resolved.
Show resolved Hide resolved
t.Run(fmt.Sprintf("translate %d", i), func(t *testing.T) {
actual, translations, r := test.in.ToIgn3_4Unvalidated(common.TranslateOptions{})
assert.Equal(t, test.out, actual, "translation mismatch")
assert.Equal(t, test.report, r, "report mismatch")
baseutil.VerifyTranslations(t, translations, test.exceptions)
assert.NoError(t, translations.DebugVerifyCoverage(actual), "incomplete TranslationSet coverage")
})
}
}
1 change: 1 addition & 0 deletions docs/config-fcos-v1_5-exp.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -206,6 +206,7 @@ The Fedora CoreOS configuration is a YAML document conforming to the following s
* **_threshold_** (int): sets the minimum number of pieces required to decrypt the device. Default is 1.
* **_mirror_** (object): describes mirroring of the boot disk for fault tolerance.
* **_devices_** (list of strings): the list of whole-disk devices (not partitions) to include in the disk array, referenced by their absolute path. At least two devices must be specified.
* **_extensions_** (list of strings): a list of packages to layer on top of the OS.

[part-types]: http://en.wikipedia.org/wiki/GUID_Partition_Table#Partition_type_GUIDs
[rfc2397]: https://tools.ietf.org/html/rfc2397
Expand Down
1 change: 1 addition & 0 deletions docs/config-openshift-v4_11-exp.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -156,6 +156,7 @@ The OpenShift configuration is a YAML document conforming to the following speci
* **_threshold_** (int): sets the minimum number of pieces required to decrypt the device. Default is 1.
* **_mirror_** (object): describes mirroring of the boot disk for fault tolerance.
* **_devices_** (list of strings): the list of whole-disk devices (not partitions) to include in the disk array, referenced by their absolute path. At least two devices must be specified.
* **_extensions_** (list of strings): a list of packages to layer on top of the OS.
* **_openshift_** (object): describes miscellaneous OpenShift configuration. Respected when rendering to a MachineConfig, ignored when rendering directly to an Ignition config.
* **_kernel_type_** (string): which kernel to use on the node. Must be `default` or `realtime`.
* **_kernel_arguments_** (list of strings): arguments to be added to the kernel command line.
Expand Down