-
Notifications
You must be signed in to change notification settings - Fork 484
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
fix(snap): Apply proxy's runtime config options after startup #3856
Conversation
@farshidtz , please amend you commit with a 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.
LGTM. Should present the feature at the next security WG meeting.
This is required to apply config options that depend on security-proxy setup Signed-off-by: Farshid Tavakolizadeh <[email protected]>
2ab3b34
to
e2748b7
Compare
Thanks for the reviews. I've amended the commit to add the signature. Please note that the PR is a draft. I will continue testing and update the description before making it ready. |
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.
Nicely done, code changes look good to me. That said, I'd like a quick review from @anonymouse64 to ensure that there are no issues with re-running the configure hook from a script like this...
As mentioned in our meeting, there are no issues from snapd's perspective with running the configure hook script/program, since you're really just running some code here. Snapd doesn't care that the code being executed was also executed for the configure hook at all. That being said, I didn't test this PR so I can't say whether it works, but the intention of the feature is very cool and I'm happy to see it come to fruition 😄 |
# add bin directory to have e.g. secret-config in PATH | ||
export PATH="$SNAP/bin:$PATH" | ||
|
||
$SNAP/snap/hooks/configure |
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.
As discussed on mattermost, I think this is problematic. If this is triggered during installation, it's going to be racing with "seed" change and may cause conflict error ("seed" change in progress...).
The conflict arises because your configure hook code tries to start services with snapctl and since it is triggered "externally" (in this case by systemd) snapd will not consider this to be part of the seeding (install) change.
This change defers the application of the following config options to after the setup of security-proxy: * env.security-proxy.user * env.security-proxy.public-key * env.security-proxy.tls-certificate * env.security-proxy.tls-private-key * env.security-proxy.tls-sni This is change is coupled with edgexfoundry/edgex-go#3856
This change defers the application of the following config options to after the setup of security-proxy: * env.security-proxy.user * env.security-proxy.public-key * env.security-proxy.tls-certificate * env.security-proxy.tls-private-key * env.security-proxy.tls-sni This is change is coupled with edgexfoundry/edgex-go#3856
Signed-off-by: Farshid Tavakolizadeh <[email protected]>
Thanks a lot for all the reviews, in particular to @anonymouse64 and @stolowski for rigorously testing the changes. I've changed the logic to apply specific service-specific configurations after proxy setup, instead of calling the configure executable to perform a full config (as expected from the configure hook). We'll have to do some refactoring in future to better separate the "configure hook" from "configuring a service" without replicating code. The PR description has been updated with details. This PR remains in draft until canonical/edgex-snap-hooks#25 is merged and included. |
This change disables the application of the following config options in defer-startup install mode to be applied separately after the setup of security-proxy: * env.security-proxy.user * env.security-proxy.public-key * env.security-proxy.tls-certificate * env.security-proxy.tls-private-key * env.security-proxy.tls-sni This is change is coupled with edgexfoundry/edgex-go#3856
This is required to skip application of runtime config options in defer-startup install mode Signed-off-by: Farshid Tavakolizadeh <[email protected]>
Kudos, SonarCloud Quality Gate passed! 0 Bugs No Coverage information |
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've reviewed the code with no comments - it looks fine to me. I'm proceeding to run checkbox/taf tests and then create the gadget and image.
-
TAF functional tests: 5 failures (482 pass). The failures are not related to this PR and latest/edge results are identical.
-
TAF integration tests: All pass
-
Created a Ubuntu Core image as per instructions in Google Doc, with gadget-yaml specifying the public key for a new user. Then created a JWT token by copying the private key to the image with
scp -P8022 private.pem [username]@localhost:
, then ssh-ing onto the box and doing -
TOKEN=edgexfoundry.secrets-config proxy jwt --algorithm ES256 --private_key /var/snap/edgexfoundry/current/private.pem --id 1 --expiration=1h
(with "1") being the user ID used in the gadget.yaml -
edgexfoundry.curl -k -X GET https://localhost:8443/core-data/api/v2/ping? -H "Authorization: Bearer $TOKEN"
(Note that I could obviously have run edgexfoundry.secrets-config proxy jwt on my desktop instead to generate the token)
and this worked fine.
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.
LGTM - Approved - with tests passing as per my comment.
Thanks a lot @siggiskulason for the review and testing.
Or you could generate the token on your machine using openssl as instructed here (3. alternative approach): https://github.com/edgexfoundry/edgex-go/tree/main/snap#jwt-tokens |
…oundry#3856) * fix: add options subcommand to apply partial configuration * build: bump github.com/canonical/edgex-snap-hooks/v2 to skip runtime config during defer-startup install mode * feat: apply security-proxy runtime configurations after startup Signed-off-by: Farshid Tavakolizadeh <[email protected]> (cherry picked from commit 3825f82)
…oundry#3856) * fix: add options subcommand to apply partial configuration * build: bump github.com/canonical/edgex-snap-hooks/v2 to skip runtime config during defer-startup install mode * feat: apply security-proxy runtime configurations after startup Signed-off-by: Farshid Tavakolizadeh <[email protected]> (cherry picked from commit 3825f82)
…oundry#3856) * fix: add options subcommand to apply partial configuration * build: bump github.com/canonical/edgex-snap-hooks/v2 to skip runtime config during defer-startup install mode * feat: apply security-proxy runtime configurations after startup Signed-off-by: Farshid Tavakolizadeh <[email protected]> (cherry picked from commit 3825f82)
…#4046) * fix: add options subcommand to apply partial configuration * build: bump github.com/canonical/edgex-snap-hooks/v2 to skip runtime config during defer-startup install mode * feat: apply security-proxy runtime configurations after startup Signed-off-by: Farshid Tavakolizadeh <[email protected]> (cherry picked from commit 3825f82)
Fixes #3863, see the issue for details.
This PR changes the configure hook to skip configuration options of security-proxy in defer-startup mode. To apply those options, the configure hook is called again after security-proxy-setup completes.This PR changes the configure hook to skip runtime configuration options of security-proxy in defer-startup mode. Those config options should be applied after security-proxy-setup completes.
To achieve that:
a) The config option handling function is changed to ignore runtime config options of security-proxy in defer-startup install mode: canonical/edgex-snap-hooks#25
b) The
configure
executable is extended with anoptions
subcommand which takes a service name as follows:The above command is called in a
post-stop
script after the security-proxy setup.Prior to this change, the
configure
executable was called only as the snap configure hook. Future work may focus on file re-structuring of various executables undersnap
directory to add better visiblity into various helper executables.PR Checklist
Please check if your PR fulfills the following requirements:
BREAKING CHANGE:
describing the break)Testing Instructions
The checkbox tests have coverage for the affected config options. Those tests were passing even before this fix because the config sets in those tests are executed after service startup.
To test the seeding, we should pass defaults from a Gadget snap:
The OS must boot without error and the config options must be applied.
To seed this snap into an Ubuntu Core image to test the above, use the build from this branch under channel:
latest/edge/fix-snap-proxy-config
New Dependency Instructions (If applicable)