Stop hardcoding base and default configs in Ignition binary #475

bgilbert · 2017-11-16T23:54:44Z

Add a new "system config dir" (defaulting to /usr/lib/ignition in the initramfs) which contains base.ign and default.ign. Both files are optional. Read base.ign instead of oem.BaseConfig() and default.ign instead of oem.DefaultUserConfig(); no longer ship those configs in oem.go. This improves the distro-independence of the codebase and allows the OEM configs to be updated independently of an Ignition release.

Also add an optional user.ign in the system config dir, which is lower priority than coreos.config.url but higher priority than the regular config provider. This allows tools like coreos-install to write a user config to disk without also modifying grub.cfg.

The idea is for bootengine to mount the OEM partition, copy the three Ignition config files into /usr/lib/ignition in the initramfs, unmount, and then run Ignition. That is implemented in coreos/bootengine#135.

Addresses coreos/bugs#2004.

arithx · 2017-11-17T00:40:23Z

@bgilbert what should happen in the event that no configuration is found? I added a test locally that calls makePreemptTest("") which still passes.

Is this something where we should bubble out a warning/error to the user saying "Ignition couldn't find any configs"?

bgilbert · 2017-11-17T01:55:25Z

No configs is a valid state: the distro didn't provide any base or default configs (e.g. CL on bare metal) and the user didn't provide a config either. I'll add a test though.

bgilbert · 2017-11-17T02:18:52Z

Updated.

ajeddeloh · 2017-11-17T18:09:28Z

GH mangles commit order. Can you post the actual commit order?

bgilbert · 2017-11-17T22:26:30Z

In this case it should be the same as in GitHub's list:

exec: move baseConfig to top of Run for clarity
internal/*: read base and default configs from initramfs
internal/*: allow putting user config in system dir
internal/oem: drop hardcoded base and default configs
tests: test system configs and preemption
doc/development: document OEMLookasideFiles and SystemDirFiles

Instead of hardcoding them in Ignition, expect the distro to put them in /usr/lib/ignition (by default).

bgilbert · 2017-12-04T05:54:34Z

This is ready for review.

arithx

Tests LGTM, thanks for updating the doc/development blurb about the Test struct.

ajeddeloh · 2017-12-04T21:19:00Z

internal/exec/engine.go

+	var err error
+	for _, fetcher := range fetchers {
+		cfg, r, err = fetcher(f)
+		if err != providers.ErrNoProvider {


It's non-obvious that this is true when err == nil. I'd either explicitly do an if err == nil || err !=n providers.ErrNoProvider or leave a comment.

ajeddeloh · 2017-12-04T21:36:22Z

internal/exec/engine.go

-		cfg, r, err = e.OEMConfig.FetchFunc()(f)
+	fetchers := []providers.FuncFetchConfig{
+		cmdline.FetchConfig,
+		system.FetchConfig,


Should this and e.OEMConfig.FetchFunc() be swapped? Wouldn't this always override use supplied configs when there is a default config in the system dir?

Erm, I think I got confused there.

The old way Ignition worked was to take the base config, append the given provider's base config, then append the user config (one of cmdline, a thing fetched by the provider, or a default config, prioritized in that order).

This now takes the base config, appends the provider's base config, then appends the user config (one of cmdline, user.ign from the initramfs, a provider fetched config, or a default config, prioritized in that order).

Is that correct or am I reading it incorrectly? Is there a reason to prioritize a oem partition user.ign over a provider fetched one?

Looking at this further, it might be possible to refactor the no supplied config case into this.

This logic is looking for the first "online" provider, meaning that it will happily accept an empty file, or a script, or a cloud-config. The no-supplied-config case is the opposite: we successfully read zero or more bytes of data from some provider, but they weren't an Ignition config. It seems cleaner to keep the two code paths separate.

And your latter reading is correct. My thinking was that if a user writes a user.ign, they are specifically asking for it to be used, so it should be a higher priority.

Read user Ignition config from the command line first, otherwise the system dir, otherwise the OEM's provider.

bgilbert · 2017-12-05T02:12:33Z

Added comment and rebased.

ajeddeloh

LGTM, pending bootengine support

bgilbert · 2017-12-06T18:05:06Z

Good to merge.

…b.com/coreos/ignition/v2-2.16.0 build(deps): bump github.com/coreos/ignition/v2 from 2.15.1-0.20230510033844-8b6e7f111ca5 to 2.16.0

bgilbert mentioned this pull request Nov 16, 2017

OEM configs are hardcoded in Ignition coreos/bugs#2004

Closed

lucab mentioned this pull request Nov 21, 2017

providers: add initial sakuracloud support coreos/afterburn#32

Closed

bgilbert added 2 commits December 3, 2017 14:49

exec: move baseConfig to top of Run for clarity

85936d1

internal/*: read base and default configs from initramfs

8d59b1b

Instead of hardcoding them in Ignition, expect the distro to put them in /usr/lib/ignition (by default).

This was referenced Dec 4, 2017

Copy Ignition base/user configs from OEM to Ignition system dir coreos/bootengine#135

Merged

coreos-base/oem-*: move Ignition base/default configs into OEMs coreos/coreos-overlay#2936

Merged

arithx reviewed Dec 4, 2017

View reviewed changes

ajeddeloh reviewed Dec 4, 2017

View reviewed changes

bgilbert added 4 commits December 4, 2017 18:11

internal/*: allow putting user config in system dir

3f4f32f

Read user Ignition config from the command line first, otherwise the system dir, otherwise the OEM's provider.

internal/oem: drop hardcoded base and default configs

5585f55

tests: test system configs and preemption

d190698

doc/development: document OEMLookasideFiles and SystemDirFiles

1e2c248

ajeddeloh approved these changes Dec 5, 2017

View reviewed changes

cgonyeo merged commit 6f7aceb into coreos:master Dec 6, 2017

bgilbert deleted the external-oem-configs branch December 6, 2017 23:17

Stop hardcoding base and default configs in Ignition binary #475

Stop hardcoding base and default configs in Ignition binary #475

Uh oh!

Conversation

bgilbert commented Nov 16, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

arithx commented Nov 17, 2017

Uh oh!

bgilbert commented Nov 17, 2017

Uh oh!

bgilbert commented Nov 17, 2017

Uh oh!

ajeddeloh commented Nov 17, 2017

Uh oh!

bgilbert commented Nov 17, 2017

Uh oh!

bgilbert commented Dec 4, 2017

Uh oh!

arithx left a comment

Choose a reason for hiding this comment

Uh oh!

ajeddeloh Dec 4, 2017

Choose a reason for hiding this comment

Uh oh!

ajeddeloh Dec 4, 2017

Choose a reason for hiding this comment

Uh oh!

ajeddeloh Dec 4, 2017

Choose a reason for hiding this comment

Uh oh!

ajeddeloh Dec 4, 2017

Choose a reason for hiding this comment

Uh oh!

bgilbert Dec 5, 2017

Choose a reason for hiding this comment

Uh oh!

bgilbert Dec 5, 2017

Choose a reason for hiding this comment

Uh oh!

bgilbert commented Dec 5, 2017

Uh oh!

ajeddeloh left a comment

Choose a reason for hiding this comment

Uh oh!

bgilbert commented Dec 6, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

bgilbert commented Nov 16, 2017 •

edited

Loading