-
Notifications
You must be signed in to change notification settings - Fork 53
Add apiclient network configure subcommand
#714
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
Add apiclient network configure subcommand
#714
Conversation
f5579da to
f9a988d
Compare
|
Force pushed to address comments from @cbgbt and @arnaldo2792 |
f9a988d to
0ad1427
Compare
There was a problem hiding this 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 few suggestions.
| /// | ||
| /// The configuration will be applied at the next boot - a reboot is required for changes to take effect. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: I wouldn't put this here as this is relevant to the user (which won't read this), and rather should be in the website docs or the main apiclient documentation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I personally found it helpful, without this comment I spent time looking around after seeing the apiserver implementation to try to understand if we were missing the code to actually apply the config after saving it.
But I've also been on the unpopular side of this same tension many times, so I'm curious to see how others think about it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't have strong opinion here and I could see the tradeoffs. But this change is made to address comments here - #714 (comment)
So I would like to keep it as is now to avoid extra churns.
cc67f0e to
d5a8454
Compare
|
Force pushed to address comments from @cbgbt and @arnaldo2792 :
|
bf21c47 to
4facba9
Compare
|
^ This is a decent diff URL: Above I added 2 minor changes:
I retested both valid and invalid net.toml files on |
4facba9 to
2b89a4a
Compare
|
^ The above push is just a rebase to get back on/near the tip of |
2b89a4a to
12e3f9c
Compare
|
^ Fix comment typo, renamed 1 error, and created a const. |
12e3f9c to
cfe4cee
Compare
cbgbt
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Apologies, I reviewed this yesterday but never hit submit.
Do you think you could fix the PartialEq comment left earlier?
Implements 'apiclient network configure <input-source>' to enable runtime network configuration for Bottlerocket. stdin is the default input, but also supports file://, base64: input sources with client-side URI processing. Content is sent to the API server for further processing. Signed-off-by: Yutong Sun <yutongsu@amazon.com> Signed-off-by: Kyle Sessions <kssessio@amazon.com>
Adds 'netdog commit' subcommand that validates and writes network configuration from stdin to /.bottlerocket/net.toml. Signed-off-by: Kyle Sessions <kssessio@amazon.com>
Adds POST /actions/network/configure endpoint to write network configuration content directly to /.bottlerocket/net.toml. Includes error handling for UTF-8 validation, directory creation, and file writing operations. The endpoint integrates with netdog commit functionality to apply network configuration changes. Signed-off-by: Yutong Sun <yutongsu@amazon.com> Signed-off-by: Kyle Sessions <kssessio@amazon.com>
cfe4cee to
d6e763e
Compare
Add PartialEq derivation to Subcommand and all related argument structs to enable direct equality comparison in tests. Simplifies network configure tests by replacing match statements with assert_eq comparisons. Signed-off-by: Kyle Sessions <kssessio@amazon.com>
Sorry missed this - fixed in the latest push Along with the other nits (update docstring and remove what comments from netdog commit) |
Issue number:
Closes #713
Description of changes:
Adds
apiclient network configure <URI>command to configure network settings at runtime. The implementation includes:New /network/configurePOST endpoint that validates and writes net.toml content to/.bottlerocket/net.tomlnetwork configuresubcommand supportingfile://andbase64:URI schemesTesting done:
1. Tested the change via
bootstrap-commandsusingbase64schemeStarted
metal-devwith command./tools/start-local-vm --variant metal-dev --arch $(uname -m) --inject-file net.toml --inject-file user-data.tomlMy initial
net.tomlonly has configurations for a single interfaceMy
user-data.tomlusesbootstrap-commandsWhere the base64 encoded net.toml is
Confirmed that the node automatically rebooted and the net.toml was written to the desired path /.bottlerocket/net.toml
And both interfaces were brought up
2. Tested the change via
bootstrap-containerusingfile://schemeUsed similar setup as 1. with
metal-dev. Only difference is theuser-data.tomlWhere the encoded user-data here is:
With this approach a network-config.toml file was created within the container at
/tmp/network-config.tomland we then usedapiclient network configure file:///tmp/network-config.tomlto configure the net.toml.Note that for bootstrap-containers, a marker file is needed to prevent reboot loop. The reboot happens during the execution of the bootstrap script so the subsequent process that turns the
oncemode tooffwill never happen.systemd-inhibitcommand we used for bootstrap-commands is something we can look into which could be a way to avoid the usage of marker file.@KCSesh - Contribution:
1. Re-Rean the
bootstrap-commandsusingbase64scheme2. Re-ran: Tested the change via
bootstrap-containerusingfile://schemeNote: Running this test multiple times required:
--force-extract, to clean PRIVATE partition marker file3. Validated STDIN
4. Tested invalid input
Invalid Version:
Unknown field:
Invalid stdin:
5. Verified new tmp approach, with admin-container and inotify on metal:
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.