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.

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

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

Merged
merged 4 commits into from
Mar 25, 2021

Conversation

tjkirch
Copy link
Contributor

@tjkirch tjkirch commented Mar 12, 2021

Description of changes:

apiclient: rename update-related types with clearer prefix

Having types named "check", "apply", and "cancel" that are related to updates,
but don't have an "update" prefix, prevents us from having other types or
subcommands called "check", "apply", or "cancel".  This adds an "update" prefix
to each of them to make it clear what they're checking, applying, or canceling.
apiclient: add apply mode for applying settings from URIs
apiclient: also accept JSON files for 'apply'
apiclient: allow 'apply' input on stdin

There are a few potential benefits here:

  • Easier to apply settings that may be large, like host container user data or keys, if gzip is insufficient or inconvenient.
  • Easier to share sets of settings, whether by making settings files public or storing them privately in your network and using them across instances.
  • file://settings.toml URIs open the way to potentially store settings in Bottlerocket images, rather than compiled into storewolf, for cases where having them available at runtime would be handy. (Potentially as default backfill for deleted settings, or explicit requests to "return to default". Potentially as alternate sets of defaults..?)

apiclient apply could be called from a host container or bootstrap container. We could have a default bootstrap container that applies a conveniently listed set of URLs, or something - just a thought. (The downside there is that you're taking on some level of risk from potential network failure and inability to apply settings.) Or settings files could be baked into custom host/bootstrap containers.

One potential use of JSON/stdin mode is https://github.com/mozilla/sops, which could help with encrypting inputs like keys; it doesn't support TOML, hence JSON, and you wouldn't want to store the file on disk after decryption, hence stdin rather than requiring creation of a file. But they're generally useful, too.

Testing done:

Happy tests:

  • I set up two settings files in S3, did apiclient apply 'https://bla/settings-1.toml' 'https://bla/settings-2.toml', and saw settings from both apply to the system.
  • I did the same with a single file.
  • I did the same with a file:// URL and a file I created in /tmp.
  • I retested apiclient's normal get/set functionality.
  • I tested normal BR system functionality; pods healthy, system services healthy.
Failure tests, to ensure errors are clear (CLICK ME)
  • Tested a file with no [settings] block:
apiclient apply 'good-url-1' 'good-url-2' 'https://bla/settings-no-settings.toml'`
Failed to apply settings: Settings from 'https://bla/settings-no-settings.toml' did not contain a 'settings' key at top level
  • Tested a file with bad TOML:
$ apiclient apply 'good-url-1' 'good-url-2' 'https://bla/settings-not-object.toml'
Failed to apply settings: Settings from 'https://bla/settings-not-object.toml' are not a TOML table / JSON object
  • Tested a bad URL:
$ apiclient apply 'good-url-1' 'good-url-2' 'https://nope'
Failed to apply settings: Failed GET request to 'https://nope': error sending request for url (https://nope/): error trying to connect: dns error: failed to lookup address information: Name does not resolve
  • Tested unknown settings in the file:
$ apiclient apply 'good-url-1' 'good-url-2' 'https://bla/settings-invalid.toml'
Failed to apply settings: Failed to deserialize settings from 'https://bla/settings-invalid.toml' into this variant's model: unknown field `xxx`, expected one of `motd`, `kubernetes`, `updates`, `host-containers`, `ntp`, `network`, `kernel`, `aws`, `metrics`
  • Tested an HTTP error:
$ apiclient apply 'good-url-1' 'good-url-2' 'https://www.example.com/lksdjfaoiselekjr'
Failed to apply settings: Failed GET request to 'https://www.example.com/lksdjfaoiselekjr': HTTP status client error (404 Not Found) for url (https://www.example.com/lksdjfaoiselekjr)

In each failure scenario, as expected, the settings from 'good' URLs were not applied. We want all-or-nothing behavior in a single apiclient call.

stdin/JSON tests (CLICK ME)

Input on stdin, ending with ctrl-d:

$ apiclient apply
[settings]
motd = "from stdin, neat"
bash-5.0# cat /etc/motd
from stdin, neat

Input piped in:

bash-5.0# echo -e '[settings]\nmotd = "from pipe stdin"' | apiclient apply
bash-5.0# cat /etc/motd
from pipe stdin

Input typed in, in between some URIs, where good-url-1 sets a motd and good-url-2 sets lockdown:

bash-5.0# apiclient apply 'good-url-1' - 'good-url-2'
[settings]
motd = 'in the middle'
bash-5.0# cat /etc/motd
in the middle
bash-5.0# cat /sys/kernel/security/lockdown
none [integrity] confidentiality

As before, no settings are applied if any settings fail:

bash-5.0# apiclient apply 'good-url-1' - 'https://nope'
[settings]
motd = 'more middle'
Failed to apply settings: Failed GET request to 'https://nope': error sending request for url (https://nope/): error trying to connect: dns error: failed to lookup address information: Name does not resolve
bash-5.0# cat /etc/motd
in the middle

The last input given takes effect, even if the earlier one takes (much) longer to retrieve, because I typed it by hand:

bash-5.0# apiclient apply - 'good-url-1'
[settings]
motd = 'dash'
bash-5.0# cat /etc/motd
settings 1

The same thing works with JSON:

bash-5.0# echo -e '{"settings": {"motd": "json pipe"}}' | apiclient apply 'good-url-1' -
bash-5.0# cat /etc/motd
json pipe
bash-5.0# echo -e '{"settings": {"motd": "json pipe"}}' | apiclient apply - 'good-url-1'  
bash-5.0# cat /etc/motd
settings 1

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.

Having types named "check", "apply", and "cancel" that are related to updates,
but don't have an "update" prefix, prevents us from having other types or
subcommands called "check", "apply", or "cancel".  This adds an "update" prefix
to each of them to make it clear what they're checking, applying, or canceling.
Copy link
Contributor

@zmrow zmrow left a comment

Choose a reason for hiding this comment

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

Nice work. Just a quick question.

sources/api/apiclient/src/apply.rs Outdated Show resolved Hide resolved
sources/api/apiclient/src/apply.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@zmrow zmrow left a comment

Choose a reason for hiding this comment

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

🪀

@tjkirch
Copy link
Contributor Author

tjkirch commented Mar 17, 2021

^ This push addresses @webern's good catch about error_for_status. (It wouldn't have applied before (unless an HTTP server gave a TOML settings representation in their error response) but the error would have been confusing.)

Before:

$ apiclient apply 'https://www.example.com/lksdjfaoiselekjr'
Failed to apply settings: Failed to parse settings from 'https://www.example.com/lksdjfaoiselekjr' as TOML: unexpected character found: `<` at line 1 column 1

After:

$ apiclient apply 'https://www.example.com/lksdjfaoiselekjr'
Failed to apply settings: Failed GET request to 'https://www.example.com/lksdjfaoiselekjr': HTTP status client error (404 Not Found) for url (https://www.example.com/lksdjfaoiselekjr)

Repeated all prior testing as well.

Comment on lines +146 to +153
/// Generates a random ID, affectionately known as a 'rando'.
pub(crate) fn rando() -> String {
thread_rng()
.sample_iter(&Alphanumeric)
.take(16)
.map(char::from)
.collect()
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we split this into its own crate? We've got a copy in a few places:

  • models build script
  • storewolf
  • migrator

@bcressey
Copy link
Contributor

apiclient apply --json would be useful also. Among other things, it'd let us support using sops to decrypt secrets in the settings.

@tjkirch
Copy link
Contributor Author

tjkirch commented Mar 23, 2021

^ This push makes three changes:

  • Fixes the bug @zmrow pointed out that I didn't have good enough testing to catch earlier. Switches buffer_unordered to buffered so we actually apply the changes in the requested order, regardless of how long URIs take to download. Sorry I missed this earlier.
  • Per @bcressey, adds JSON support. Rather than a flag, I just made the code detect the format.
  • Added the ability to pass data on stdin, either by giving no URIs or by explicitly passing "-", which would also let you order it.

One expected use of JSON/stdin is for things like https://github.com/mozilla/sops, which could help with encrypting some data; it doesn't support TOML, hence JSON, and you wouldn't want to store the file on disk after decryption, hence stdin rather than requiring creation of a file. But they're generally useful, too.

I repeated all testing in the description above, and I'm adding some more test logs there now.

@tjkirch tjkirch requested review from zmrow and bcressey March 23, 2021 22:24
Copy link
Contributor

@zmrow zmrow left a comment

Choose a reason for hiding this comment

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

Nice work with the futures!

🏈

@tjkirch tjkirch merged commit 26afc25 into bottlerocket-os:develop Mar 25, 2021
@tjkirch tjkirch deleted the apiclient-apply branch March 25, 2021 18:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants