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 high-level 'set' subcommand for changing settings #1278

Merged
merged 2 commits into from
Jan 19, 2021

Conversation

tjkirch
Copy link
Contributor

@tjkirch tjkirch commented Jan 12, 2021

Issue number:

Fixes #1238.

Description of changes:

commit 41ad62a0b44b1dcc3289117c2cba0aa9ba99bfce
Author: Tom Kirchner <tjk@amazon.com>
Date:   Mon Nov 30 23:23:10 2020 +0000

    apiclient: move 'reboot' to a library submodule
commit 5e80b6189e37debaf85dbf73be9d8474ad1201b9
Author: Tom Kirchner <tjk@amazon.com>
Date:   Sat Dec 5 00:05:56 2020 +0000

    apiclient: add high-level 'set' subcommand for changing settings
    
    This gives a more natural key=value input format suitable for most changes, and
    takes care of committing and applying the API transaction.

Unfortunately, I don't know that it's possible right now to handle all our keys in the simpler key=value form. I tried, but deserialization into the model requires knowledge of the type, and unless we require the user to annotate their inputs (removing a good portion of the benefit of this) then we can't be sure what they intend, and we can't introspect the model for the type we need. I'd really like to fix this, but I think it requires improving/replacing the ser/de code of the data store.

What that actually means - you can't set settings.kernel.sysctl."vm.max_map_count"=262144 (regardless of whether you quote the number) and instead have to do the JSON form, for now.

Testing done:

apiclient reboot still reboots successfully.

key=value form works with and without the settings. prefix:

[ec2-user@ip-192-168-13-246 ~]$ apiclient set settings.motd=hi
[ec2-user@ip-192-168-13-246 ~]$ apiclient -u /settings?prefix=motd
{"motd":"hi"}
[ec2-user@ip-192-168-13-246 ~]$ apiclient set motd=sup
[ec2-user@ip-192-168-13-246 ~]$ apiclient -u /settings?prefix=motd
{"motd":"sup"}

Similarly, JSON form will remove the unnecessary settings layer:

[ec2-user@ip-192-168-13-246 ~]$ apiclient set -j '{"settings": {"updates": {"seed": 42}}}'
[ec2-user@ip-192-168-13-246 ~]$ apiclient -u /settings?keys=settings.updates.seed
{"updates":{"seed":42}}

Plain quoting works for handling spaces:

[ec2-user@ip-192-168-13-246 ~]$ apiclient set motd="hi there"
[ec2-user@ip-192-168-13-246 ~]$ apiclient -u /settings?prefix=motd
{"motd":"hi there"}

Quoting of "fancy keys" with internal dots works:

[ec2-user@ip-192-168-13-246 ~]$ apiclient set 'kubernetes.node-labels."my.label"=hello'
[ec2-user@ip-192-168-13-246 ~]$ apiclient -u /settings?prefix=kubernetes.node-labels
{"kubernetes":{"node-labels":{"my.label":"hello"}}}

Here's an example of the (hopefully clear) message when key=value form can't handle a type:

[ec2-user@ip-192-168-13-246 ~]$ apiclient set motd=42
Unable to match your input to the data model.  We may not have enough type information.  Please try the --json input form.  Cause: Error deserializing scalar value: invalid type: integer `42`, expected a string at line 1 column 2

The error is simpler with JSON, since the JSON input specifies the type through its format:

[ec2-user@ip-192-168-13-246 ~]$ apiclient set -j '{"settings": {"updates": {"seed": "42"}}}'
Unable to deserialize input JSON into model: invalid type: string "42", expected u32

Setting multiple values works:

[ec2-user@ip-192-168-13-246 ~]$ apiclient set kernel.lockdown=integrity motd=howdy
[ec2-user@ip-192-168-13-246 ~]$ apiclient -u /settings?keys=settings.kernel.lockdown,settings.motd
{"motd":"howdy","kernel":{"lockdown":"integrity"}}

JSON form, with multiple values, internal dots, and quotes/types working as expected:

[ec2-user@ip-192-168-13-246 ~]$ apiclient set --json '{"kernel": {"sysctl": {"vm.max_map_count": "888888", "user.max_user_namespaces": "9999"}}}'
[ec2-user@ip-192-168-13-246 ~]$ apiclient -u /settings?prefix=kernel.sysctl
{"kernel":{"sysctl":{"user.max_user_namespaces":"9999","vm.max_map_count":"888888"}}}

Setting an integer, in key=value and JSON form, works:

[ec2-user@ip-192-168-13-246 ~]$ apiclient -u /settings?keys=settings.updates.seed
{"updates":{"seed":42}}
[ec2-user@ip-192-168-13-246 ~]$ apiclient set --json '{"updates": {"seed": 43}}'
[ec2-user@ip-192-168-13-246 ~]$ apiclient -u /settings?keys=settings.updates.seed
{"updates":{"seed":43}}

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.

Copy link
Contributor

@webern webern left a comment

Choose a reason for hiding this comment

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

I continue to be concerned by lack of testing. I think we should open an issue to test the parts of this lib and main that are testable (arg parsing at least).

```

It can also be useful if your desired value is "complex" or looks like a different type.
For example, the "vm.max_map_count" value set above looks like an integer, but the kernel requires a string, so it has to be specified in JSON form and as a string.
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

sources/api/apiclient/src/main.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.

🏐

This gives a more natural key=value input format suitable for most changes, and
takes care of committing and applying the API transaction.
@tjkirch
Copy link
Contributor Author

tjkirch commented Jan 16, 2021

^ This push addresses @bcressey's concern by preventing a second --json argument. Before, the second argument would have replaced the first, leading to only some of your settings being set. (Allowing multiple would be possible in the future, but it's tricky to join the settings correctly.)

@tjkirch tjkirch merged commit dfb130e into bottlerocket-os:develop Jan 19, 2021
@tjkirch tjkirch deleted the apiclient-set branch January 19, 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.

Make it easier to change settings with apiclient
5 participants