Add support for bootstrap containers via API settings #1387

arnaldo2792 · 2021-03-12T17:37:15Z

Issue number:
#1392

Description of changes:

5153d08c host-containers: change permissions in `/etc/host-containers`

This commit updates the permissions in /etc/host-containers from 755 to 750

d59628c7 models: change ContainerImage to HostContainer

This commit updates the model name used to configure a host container from ContainerImage to HostContainer

8ce2cae3 Add support for bootstrap-containers via settings

This commit adds support to create bootstrap containers through the API. Bootstrap containers are host containers that can be used to setup the host during the execution of the configured target.

These containers are created with the prefix boot to prevent collisions with normal host containers. They can be setup to fail the boot process if the underlying container task exists with a non-zero status code.

f5e29ce9 host-ctr: support for bootstrap containers

This commit adds support for bootstrap containers in host-ctr. The container-type flag was added to specify the type of container to be setup, which defaults to host. This allows to add support for other type of containers in the future without adding flags for each of them.

The root filesystem mounts were refactored out from withSuperpowered to withRootFilesystemMounts, since bootstrap containers require access to the underlying root filesystem without being given the superpowered treatment.

Failed container tasks will now return an error if the task exited with a non-zero status.

Testing done:
In k8s 1.18, ecs and dev aarch64:

Run systemctl status to check that no units were failing
Create nginx pods/containers, access them and curl http://localhost successfully
Verifiy that both the control and admin host containers are running, with the proper configurations
Execute systemctl isolate multi-user to verify that no services were killed/restarted
Migrate from 1.0.7 to 1.0.8, back and forth to verify the new configurations were deleted/created
Create a bootstrap container with the following configurations:

# user-data
[settings.bootstrap-containers.admin]
mode = "once"
source = "docker.io/arnaldo2792/sleeper:latest"
user-data = "ypXCt82h4bSlwrfKlA==" # base64 of ʕ·͡ᴥ·ʔ

# Image definition
FROM alpine
ADD bootstrap-script /
RUN chmod +x /bootstrap-script
ENTRYPOINT ["sh", "bootstrap-script"]

# bootstrap-script
set -e
CURRENT_DIR=/.bottlerocket/bootstrap-containers/current
BEAR_DIR=/.bottlerocket/bootstrap-containers/admin
VAR_DIR=/.bottlerocket/rootfs/var
MY_DIR=$VAR_DIR/lib/my_directory
cat $BEAR_DIR/user-data
touch $CURRENT_DIR/access
mkdir $MY_DIR
chmod -R o+r $MY_DIR
chown -R 1000:1000 $MY_DIR
sleep 120

Verified that:
- ʕ·͡ᴥ·ʔ appears in the journal logs
- The directory /var/lib/my_directory was created, with the expected user/group ids, and permissions
- The file /local/bootstrap-containers/access created through the current mount
- configured.target was "blocked" for two minutes
- mark-successful-boot.service executed successfully
- After a reboot, the bootstrap container wasn't executed
Update the bootstrap container's configuration with apiclient set bootstrap-containers.admin.mode=once, reboot and verified its execution failed because /var/lib/my_dir already exists.

Starting bootstrap container admin...
[8.350595] host-ctr[456]: mkdir: can't create directory '/.bottlerocket/rootfs/var/lib/my_directory': File exists
[ 8.415213] host-ctr[456]: time="2021-04-02T00:54:43Z" level=fatal msg="Container exited with non-zero status"
[FAILED] Failed to start bootstrap container admin.

Update the bootstrap container's configuration with apiclient set bootstrap-containers.admin.essential=true, reboot and verified the boot failed:

[FAILED] Failed to start bootstrap container admin.
See 'systemctl status bootstrap-containers@admin.service' for details.
[DEPEND] Dependency failed for Bottlerocket final configuration complete.
[DEPEND] Dependency failed for Isolates multi-user.target.
[FAILED] Failed to start Isolates configured.target.

Terms of contribution:

By submitting this pull request, I agree that this contribution is dual-licensed under the terms of both the Apache License, version 2.0, and the MIT license.

bcressey

Nice!

I've reviewed the changes to the unit dependencies; haven't looked at the actual bootstrap containers implementation yet.

packages/os/host-containers@.service

packages/host-ctr/host-containerd.service

packages/os/bootstrap-containers@.service

packages/release/activate-configured.service

packages/release/configured.target

packages/os/settings-applier.service

packages/os/early-boot-config.service

packages/systemd/systemd.spec

zmrow · 2021-03-16T21:32:22Z

sources/host-ctr/cmd/host-ctr/main.go

@@ -154,7 +161,7 @@ func App() *cli.App {
 	return app
 }

-func runCtr(containerdSocket string, namespace string, containerID string, source string, superpowered bool) error {
+func runCtr(containerdSocket string, namespace string, containerID string, source string, superpowered bool, bootstrap bool) error {


Since this function's definition is getting fairly large, would it make sense to use a single struct containing the arguments (essentially the "config" of the container to run)? (The Go folks might have more to say about how idiomatic this would be).

Since this is a private function that really only gets called in one place, I don't know if we need to do that yet. One change we could make is to separate this function a bit more. The first two arguments here are only used up until line 181 for constructing the client; if we extract the client object from this function and instead pass it in (either directly or as a data field on a receiver object) that might be a bit cleaner. I think that's the route I would go.

zmrow · 2021-03-16T21:34:17Z

sources/host-ctr/cmd/host-ctr/main.go

+		var persistentPath string
+
+		if bootstrap {
+			persistentPath = "bootstrap-containers/" + containerID


Do these make sense as constants?

I used a different approach now, let me know if you still think the new approach should use constants

README.md

sources/api/README.md

sources/api/bootstrap-containers/Cargo.toml

sources/api/bootstrap-containers/src/main.rs

README.md

sources/api/bootstrap-containers/README.md

README.md

sources/api/README.md

sources/api/bootstrap-containers/src/main.rs

sources/api/migration/migrations/v1.0.8/add-bootstrap-containers/src/main.rs

sources/host-ctr/cmd/host-ctr/main.go

sources/models/src/modeled_types/mod.rs

samuelkarp

I haven't gotten through all the rust code in the third commit yet, but figured I'd stop here to get the questions I already have answered first. Two things I'd like to get worked out:

If bootstrap containers can't be superpowered, we should enforce this assertion in the code of host-ctr
If host containers and bootstrap containers are allowed to share the same name, we need to work out non-collision for the container IDs in containerd

sources/host-ctr/cmd/host-ctr/main.go

README.md

packages/os/bootstrap-containers@.service

packages/host-ctr/host-containerd.service

packages/os/bootstrap-containers@.service

packages/os/host-containers@.service

packages/release/activate-configured.service

packages/release/configured.target

packages/release/activate-multi-user.service

packages/release/activate-configured.service

packages/release/preconfigured.target

packages/systemd/systemd.spec

arnaldo2792 · 2021-04-01T22:15:02Z

IMPORTANT UPDATE

I changed the base branch to have this and #1423 reviewed in parallel

Changes in forced push:

Replaced "fatals" with Result<> in bootstrap containers
Use the containerd's namespace "bootstrap" for bootstrap containers to avoid collisions
Removed the withPrivilegedAccess specs function, and replace it with withRootFilesystemMounts
Added the essential flag on bootstrap containers, used to fail boots when set to true

arnaldo2792 · 2021-04-01T23:54:43Z

Changes in forced push:

Update the commits messages
Update bootstrap-containers documentation
Use the BOOTSTRAP_CONTAINERS_NS constant in clean-up commands

arnaldo2792 · 2021-04-02T01:04:34Z

Rebase base branch

arnaldo2792 · 2021-04-02T01:25:32Z

@samuelkarp

If bootstrap containers can't be superpowered, we should enforce this assertion in the code of host-ctr
If host containers and bootstrap containers are allowed to share the same name, we need to work out non-collision for the container IDs in containerd

I updated to code to enforce that bootstrap containers can't be superpowered
Bootstrap containers are now created in the bootstrap namespace to prevent collisions

@bcressey

I marked all the systemd changes as resolved since I separated those changes in #1423

arnaldo2792 · 2021-04-07T03:09:01Z

Fixed typo in documentation
Removed repeated documentation in crate
Error out when no command is passed to bootstrap-containers
Suggested essential=true in documentation
Added container id in non-zero status error message in host-ctr
Removed status code from bootstrap-containers command function
Removed Valid from BootstrapContainerMode

arnaldo2792 · 2021-04-07T18:57:24Z

Use off as default for mode
Removed a note from the documentation
Removed --now from disable command and added a note explaining why

bcressey@ confirmed this needs regular re-review, but not a big red "changes requested" :)

sources/api/bootstrap-containers/src/main.rs

arnaldo2792 · 2021-04-07T21:13:29Z

Rebase upstream
Fixed typo

arnaldo2792 · 2021-04-07T21:38:43Z

Updated the commit message that mentioned using namespaces instead of the prefix in host-ctr

arnaldo2792 · 2021-04-07T21:42:43Z

Fixed documentation in bootstrap containers

jahkeup

There's a couple small changes I'd like to see in host-ctr before merge.

jahkeup · 2021-04-07T21:44:55Z

sources/host-ctr/cmd/host-ctr/main.go

+// PersistentDir returns the persistent base directory for the container type
+func (ct ContainerType) PersistentDir() (string, bool) {
+	switch ct {
+	case Host:
+		return "host-containers", true
+	case Bootstrap:
+		return "bootstrap-containers", true
+	}
+
+	return "", false
+}


The call sites for these don't seem to use the third state, and is not idiomatic in this case (return the zero value). Can we drop this from the signature please?

Suggested change

// PersistentDir returns the persistent base directory for the container type

func (ct ContainerType) PersistentDir() (string, bool) {

switch ct {

case Host:

return "host-containers", true

case Bootstrap:

return "bootstrap-containers", true

}

return "", false

}

// PersistentDir returns the persistent base directory for the container type

func (ct ContainerType) PersistentDir() string {

switch ct {

case Host:

return "host-containers"

case Bootstrap:

return "bootstrap-containers"

}

return ""

}

jahkeup · 2021-04-07T21:45:29Z

sources/host-ctr/cmd/host-ctr/main.go

+// Prefix returns the prefix for the container type
+func (ct ContainerType) Prefix() (string, bool) {
+	switch ct {
+	case Bootstrap:
+		return "boot.", true
+	case Host:
+		return "", true
+	}
+
+	return "", false
+}


The call sites for these don't seem to use the third state, and is not idiomatic in this case (return the zero value). Can we drop this from the signature please?

Suggested change

// Prefix returns the prefix for the container type

func (ct ContainerType) Prefix() (string, bool) {

switch ct {

case Bootstrap:

return "boot.", true

case Host:

return "", true

}

return "", false

}

// Prefix returns the prefix for the container type

func (ct ContainerType) Prefix() string {

switch ct {

case Bootstrap:

return "boot."

case Host:

return ""

}

return ""

}

arnaldo2792 · 2021-04-07T21:49:24Z

Rebase upstream

jahkeup · 2021-04-07T21:54:40Z

sources/host-ctr/cmd/host-ctr/main.go

+// Used to define valid container types
+type ContainerType string
+
+const (
+	Host      ContainerType = "host"
+	Bootstrap ContainerType = "bootstrap"
+)


GitHub lost my comment.

Please update this to keep the types private.

Suggested change

// Used to define valid container types

type ContainerType string

const (

Host ContainerType = "host"

Bootstrap ContainerType = "bootstrap"

)

// Used to define valid container types

type containerType string

const (

containerTypeHost containerType = "host"

containerTypeBootstrap containerType = "bootstrap"

)

arnaldo2792 · 2021-04-07T22:35:15Z

Refactored ContainerType, Host and Bootstrap to containerType, host and bootstrap. I had to use cType as variables' names to prevent collisions

This commit adds support for bootstrap containers in `host-ctr`. The `container-type` flag was added to specify the type of container to be setup, which defaults to `host`. This allows to add support for other type of containers in the future without adding flags for each of them. The root filesystem mounts were refactored out from `withSuperpowered` to `withRootFilesystemMounts`, since bootstrap containers require access to the underlying root filesystem without being given the superpowered treatment. Failed container tasks will now return an error if the task exited with a non-zero status.

This commit adds support to create bootstrap containers through the API. Bootstrap containers are host containers that can be used to setup the host during the execution of the `configured` target. These containers are created with the prefix `boot` to prevent collisions with normal host containers. They can be setup to fail the boot process if the underlying container task exists with a non-zero status code.

This commit updates the model name used to configure a host container from `ContainerImage` to `HostContainer`

This commit updates the permissions in `/etc/host-containers` from `755` to `750`

arnaldo2792 · 2021-04-07T23:10:49Z

I rebased upstream, and had to update some rust dependencies as part of the rebase
Removed user-data base64 string from error message (shouldn't be part of the rebased change, but I forge to not merge it)

arnaldo2792 requested review from tjkirch, bcressey and samuelkarp March 12, 2021 17:39

arnaldo2792 force-pushed the bootstrap-container branch from 1b6608f to 154f3c4 Compare March 12, 2021 18:09

tjkirch mentioned this pull request Mar 12, 2021

apiclient: add 'apply' mode for applying settings from URIs #1391

Merged

bcressey reviewed Mar 13, 2021

View reviewed changes

bcressey mentioned this pull request Mar 15, 2021

add a mechanism for containers to configure the host on first boot #1392

Closed

zmrow reviewed Mar 16, 2021

View reviewed changes

webern self-requested a review March 17, 2021 21:54

webern reviewed Mar 17, 2021

View reviewed changes

sources/api/bootstrap-containers/Cargo.toml Outdated Show resolved Hide resolved

sources/api/bootstrap-containers/src/main.rs Outdated Show resolved Hide resolved

sources/api/bootstrap-containers/src/main.rs Outdated Show resolved Hide resolved

diranged reviewed Mar 18, 2021

View reviewed changes

README.md Outdated Show resolved Hide resolved

diranged reviewed Mar 18, 2021

View reviewed changes

README.md Outdated Show resolved Hide resolved

diranged reviewed Mar 18, 2021

View reviewed changes

sources/api/bootstrap-containers/README.md Show resolved Hide resolved

diranged reviewed Mar 18, 2021

View reviewed changes

sources/api/bootstrap-containers/README.md Show resolved Hide resolved

arnaldo2792 force-pushed the bootstrap-container branch from 154f3c4 to a4db0cd Compare March 20, 2021 01:32

webern reviewed Mar 22, 2021

View reviewed changes

samuelkarp reviewed Mar 22, 2021

View reviewed changes

bcressey reviewed Mar 23, 2021

View reviewed changes

packages/host-ctr/host-containerd.service Outdated Show resolved Hide resolved

packages/os/bootstrap-containers@.service Outdated Show resolved Hide resolved

bcressey previously requested changes Mar 23, 2021

View reviewed changes

arnaldo2792 force-pushed the bootstrap-container branch from a4db0cd to 933955e Compare April 1, 2021 21:59

arnaldo2792 changed the base branch from develop to new-boot-sequence April 1, 2021 21:59

arnaldo2792 force-pushed the bootstrap-container branch from 933955e to 86a662d Compare April 1, 2021 23:53

arnaldo2792 force-pushed the new-boot-sequence branch from 95c752a to cad9380 Compare April 2, 2021 00:00

arnaldo2792 force-pushed the bootstrap-container branch from 86a662d to db60990 Compare April 2, 2021 01:03

arnaldo2792 force-pushed the new-boot-sequence branch from cad9380 to 3874edb Compare April 2, 2021 16:22

arnaldo2792 force-pushed the bootstrap-container branch from db60990 to 01a31c2 Compare April 2, 2021 17:45

arnaldo2792 force-pushed the bootstrap-container branch from 196dc2e to 74cbcdb Compare April 7, 2021 18:56

arnaldo2792 requested a review from bcressey April 7, 2021 18:59

tjkirch added this to the 1.0.8 milestone Apr 7, 2021

bcressey approved these changes Apr 7, 2021

View reviewed changes

sources/api/bootstrap-containers/src/main.rs Outdated Show resolved Hide resolved

arnaldo2792 force-pushed the bootstrap-container branch from 74cbcdb to 5153d08 Compare April 7, 2021 21:13

arnaldo2792 force-pushed the bootstrap-container branch from 5153d08 to cceca9b Compare April 7, 2021 21:37

arnaldo2792 force-pushed the bootstrap-container branch from cceca9b to 3b111ff Compare April 7, 2021 21:42

arnaldo2792 force-pushed the bootstrap-container branch from 3b111ff to ce55093 Compare April 7, 2021 21:48

jahkeup suggested changes Apr 7, 2021

View reviewed changes

jahkeup reviewed Apr 7, 2021

View reviewed changes

arnaldo2792 force-pushed the bootstrap-container branch from ce55093 to 7bc729c Compare April 7, 2021 22:34

arnaldo2792 added 4 commits April 7, 2021 22:35

models: change ContainerImage to HostContainer

a53244a

This commit updates the model name used to configure a host container from `ContainerImage` to `HostContainer`

host-containers: change permissions in /etc/host-containers

5894d58

This commit updates the permissions in `/etc/host-containers` from `755` to `750`

arnaldo2792 force-pushed the bootstrap-container branch from 7bc729c to 5894d58 Compare April 7, 2021 23:08

jahkeup approved these changes Apr 7, 2021

View reviewed changes

arnaldo2792 merged commit 11c367f into bottlerocket-os:develop Apr 7, 2021

arnaldo2792 linked an issue Apr 7, 2021 that may be closed by this pull request

inconsistent handling of invalid user data #1346

Closed

arnaldo2792 mentioned this pull request Apr 8, 2021

inconsistent handling of invalid user data #1346

Closed

arnaldo2792 deleted the bootstrap-container branch April 15, 2021 17:16

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for bootstrap containers via API settings #1387

Add support for bootstrap containers via API settings #1387

arnaldo2792 commented Mar 12, 2021 •

edited

Loading

bcressey left a comment

zmrow Mar 16, 2021

samuelkarp Mar 22, 2021

zmrow Mar 16, 2021

arnaldo2792 Apr 2, 2021

samuelkarp left a comment

arnaldo2792 commented Apr 1, 2021

arnaldo2792 commented Apr 1, 2021

arnaldo2792 commented Apr 2, 2021

arnaldo2792 commented Apr 2, 2021 •

edited

Loading

arnaldo2792 commented Apr 7, 2021 •

edited

Loading

arnaldo2792 commented Apr 7, 2021 •

edited

Loading

arnaldo2792 commented Apr 7, 2021

arnaldo2792 commented Apr 7, 2021

arnaldo2792 commented Apr 7, 2021

jahkeup left a comment

jahkeup Apr 7, 2021

jahkeup Apr 7, 2021

arnaldo2792 commented Apr 7, 2021

jahkeup Apr 7, 2021

arnaldo2792 commented Apr 7, 2021

arnaldo2792 commented Apr 7, 2021

Add support for bootstrap containers via API settings #1387

Add support for bootstrap containers via API settings #1387

Conversation

arnaldo2792 commented Mar 12, 2021 • edited Loading

bcressey left a comment

Choose a reason for hiding this comment

zmrow Mar 16, 2021

Choose a reason for hiding this comment

samuelkarp Mar 22, 2021

Choose a reason for hiding this comment

zmrow Mar 16, 2021

Choose a reason for hiding this comment

arnaldo2792 Apr 2, 2021

Choose a reason for hiding this comment

samuelkarp left a comment

Choose a reason for hiding this comment

arnaldo2792 commented Apr 1, 2021

IMPORTANT UPDATE

arnaldo2792 commented Apr 1, 2021

arnaldo2792 commented Apr 2, 2021

arnaldo2792 commented Apr 2, 2021 • edited Loading

arnaldo2792 commented Apr 7, 2021 • edited Loading

arnaldo2792 commented Apr 7, 2021 • edited Loading

arnaldo2792 commented Apr 7, 2021

arnaldo2792 commented Apr 7, 2021

arnaldo2792 commented Apr 7, 2021

jahkeup left a comment

Choose a reason for hiding this comment

jahkeup Apr 7, 2021

Choose a reason for hiding this comment

jahkeup Apr 7, 2021

Choose a reason for hiding this comment

arnaldo2792 commented Apr 7, 2021

jahkeup Apr 7, 2021

Choose a reason for hiding this comment

arnaldo2792 commented Apr 7, 2021

arnaldo2792 commented Apr 7, 2021

arnaldo2792 commented Mar 12, 2021 •

edited

Loading

arnaldo2792 commented Apr 2, 2021 •

edited

Loading

arnaldo2792 commented Apr 7, 2021 •

edited

Loading

arnaldo2792 commented Apr 7, 2021 •

edited

Loading