Allow user-defined host containers #386
Conversation
We don't need a custom struct for this. A HashMap is represented the same way on disk and by the ser/de code, and opens the way for custom host containers in the map. (That will require building a single host-containers service to manage them all.)
tjkirch
force-pushed
the
host-containers
branch
from
ee5440d to
b99584e
Compare
|
Sorry for the quick force-push, I forgot to do it before publishing; it's some fixes I discovered while testing. |
|
Note that servicedog is no longer used with this change; host-containers takes its place for existing use cases. We want to keep servicedog for other uses that may be desired soon, per Ben, so I didn't stop it from building. |
There was a problem hiding this comment.
I think my favorite thing about this pull request is how well it demonstrates apiserver's flexibility; this adds functionality without changing the interface and with a simple change to the settings model. (I had to look and check that the defaults were still getting applied because you didn't have to change those.) 👏
packages/api/[email protected]
Outdated
| EnvironmentFile=/etc/host-containers/%i.env | ||
| ExecStart=/usr/bin/host-ctr -ctr-id=%i -source=${CTR_SOURCE} -superpowered=${CTR_SUPERPOWERED} |
There was a problem hiding this comment.
Need to sanitize %i value if we're using it in ways that could introduce a directory traveral. E.g. "../foo" as container name.
There was a problem hiding this comment.
Not sure I understand fully. I should probably quote these, but I'm not sure how or why to sanitize them. "../foo" seems like a silly, but valid, name for something, and nothing should interpret it as a path. Same for the superpowered boolean; if it's not "true" or "false" it should fail to parse or be accepted. With source, any URI the host-ctr process has access to could theoretically be a valid container source, and if we don't want to allow local paths relative to the process, that's a security policy I think host-ctr or containerd should enforce, not the argument handling of a service unit.
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, something went wrong.
There was a problem hiding this comment.
I assume Ben is more worried about the variables coming from the EnvironmentFile which come from settings the user can specify through the API.
| @@ -33,6 +33,7 @@ skip = [ | |||
| { name = "bork", licenses = [] }, | |||
| { name = "data_store_version", licenses = [] }, | |||
There was a problem hiding this comment.
unrelated nit: should this be data-store-version?
There was a problem hiding this comment.
It matches the naming of the directory and crate, which were named with underscores because that's how you import it into other Rust code... not sure if that's the best solution...
tjkirch
force-pushed
the
host-containers
branch
from
b99584e to
0d15bbb
Compare
|
This push addresses some of @bcressey 's comments:
I repeated the testing and updated the results. |
tjkirch
force-pushed
the
host-containers
branch
from
0d15bbb to
dbbd51f
Compare
|
This push adds the comment @bcressey requested, and adds a modeled type to verify that settings like container source, which are headed to configuration files, can't use multiple lines to inject environment settings. Testing in description repeated and passed. Here you can see multi-line strings failing. First, the client won't even accept control characters, which I just learned: Second, if I use escapes, we can see the new error: |
This adds a host-containers service that asks the API for the defined host-containers and tells systemd to start/stop them to match their 'enabled' setting. The settings are written to an EnvironmentFile that's read by the new host-container@ service, a systemd templated service used for all host containers by way of a suffix like "host-container@admin". The existing container-specific thar-be-settings templated systemd unit files were removed in favor of this more general approach. Existing metadata for these was also removed, replaced with a single metadata entry that invokes the new tool. The existing host-containers package, which just contains the Go host-ctr source, was renamed to host-ctr.
tjkirch
force-pushed
the
host-containers
branch
from
dbbd51f to
b40d939
Compare
|
This push uses SingleLineString for the host container names, too. |
| })?; | ||
|
|
||
| info!( | ||
| "Handling host container '{}' which is enabled: {}", |
There was a problem hiding this comment.
This wording is a little weird which is understandable given the limitations of the variables. This might be just as bad but here's my suggestion:
"Handling host container '{}' with 'enabled' status: {}"
🤷♂
Fixes #314
First commit, moving to HashMap in the model:
Testing done:
Before, if you try to send in a custom container, you'll see it fail because it can't deserialize into the existing struct.
After, you can send it:
And it shows up correctly; the output is otherwise identical, and it looks right on the filesystem:
Second commit, the service that uses the HashMap to actually start services:
Testing done:
Built an AMI, SSM was fine (meaning control instance was fine) and started admin and logged in via SSH (meaning admin instance was fine).
Overall, systemctl is happy:
I started the admin container after boot, and we can see both service instances happy:
Which means the host-containers configs (used as EnvironmentFiles) were written correctly:
I also pushed a custom container entry, just another copy of the control container:
We can see the service for it started correctly:
We can also stop it: