-
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
feat(snap): Add support for environment variable injection #3986
feat(snap): Add support for environment variable injection #3986
Conversation
Signed-off-by: Siggi Skulason <[email protected]>
Signed-off-by: Siggi Skulason <[email protected]>
Use ProcessAppConfig instead of ProcessOptions Signed-off-by: siggi <[email protected]>
Signed-off-by: siggi <[email protected]>
Signed-off-by: Farshid Tavakolizadeh <[email protected]>
1feff7d
to
305fe6d
Compare
canonical/edgex-snap-hooks#45 Signed-off-by: Farshid Tavakolizadeh <[email protected]>
canonical/edgex-snap-hooks#46 Signed-off-by: Farshid Tavakolizadeh <[email protected]>
6d22163
to
1d3cb66
Compare
Kudos, SonarCloud Quality Gate passed! 0 Bugs No Coverage information |
Validate error handling:
Test: $ snap set edgexfoundry config.service-startupmsg="testing env injection"
$ snap restart edgexfoundry
Started.
$ snap logs -n=all edgexfoundry | grep "testing"
2022-04-26T13:30:27+02:00 edgexfoundry.core-command[10034]: level=INFO ts=2022-04-26T11:30:27.496559088Z app=core-command source=variables.go:352 msg="Variables override of 'Service.StartupMsg' by environment variable: SERVICE_STARTUPMSG=testing env injection"
2022-04-26T13:30:28+02:00 edgexfoundry.core-data[10084]: level=INFO ts=2022-04-26T11:30:28.751877152Z app=core-data source=variables.go:352 msg="Variables override of 'Service.StartupMsg' by environment variable: SERVICE_STARTUPMSG=testing env injection"
2022-04-26T13:30:30+02:00 edgexfoundry.core-metadata[10136]: level=INFO ts=2022-04-26T11:30:30.24381414Z app=core-metadata source=variables.go:352 msg="Variables override of 'Service.StartupMsg' by environment variable: SERVICE_STARTUPMSG=testing env injection"
$ snap set edgexfoundry apps.core-data.config.service-port=11111
$ snap restart edgexfoundry.core-data
Restarted.
$ snap logs -n=all edgexfoundry | grep "11111"
2022-04-26T13:49:49+02:00 edgexfoundry.core-data[18760]: level=INFO ts=2022-04-26T11:49:49.030920692Z app=core-data source=variables.go:352 msg="Variables override of 'Service.Port' by environment variable: SERVICE_PORT=11111"
2022-04-26T13:49:49+02:00 edgexfoundry.core-data[18760]: level=INFO ts=2022-04-26T11:49:49.060236271Z app=core-data source=httpserver.go:123 msg="Web server starting (localhost:11111)"
$ cat /var/snap/edgexfoundry/current/config/core-data/res/core-data.env
export SERVICE_STARTUPMSG="testing env injection"
export SERVICE_PORT="11111"
$ cat /var/snap/edgexfoundry/current/config/device-virtual/res/device-virtual.env
export SERVICE_STARTUPMSG="testing env injection" |
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.
@farshidtz Thank you. It works as expected. It also passed the checkbox test security-services-proxy-certs locally with new secrets-config proxy options.
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.
Overall this looks good, testing works as expected. That said, there are a couple of comments in-line that should be reviewed before approval.
Also curious why updates to snap/README weren't included (has the doc moved elsewhere)?
"security-secretstore-setup", | ||
"security-proxy-setup", | ||
"security-bootstrapper", | ||
"sys-mgmgt-agent", |
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'd suggest dropping this as the SMA is considered deprecated and will be removed completely in EdgeX 3.0.
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.
Combining the new options with the old ones is disallowed. As a result, it will not be possible for someone to configure SMA using the old options and everything else using the new ones. The only way to respect the deprecation policy is to keep this service functional (and configurable) until EdgeX 3.0 release, at which we can drop that too.
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 wasn't suggesting combining the options, I was just suggesting that you don't support the SMA at all with the new scheme. And afaik there's nothing in the deprecation policy which states that new features must support deprecated services or applications, that would seem to defeat the purpose of deprecation IMHOP.
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 explained, this is necessary to allow using the new scheme with applications while still allowing the configuration of SMA. This isn't a new feature for SMA, but a new feature for the snap as a whole and that should not make the SMA incompatible with everything else when using the new scheme.
@@ -1,5 +1,8 @@ | |||
module github.com/canonical/edgex-go/hooks | |||
|
|||
require github.com/canonical/edgex-snap-hooks/v2 v2.1.3 | |||
require github.com/canonical/edgex-snap-hooks/v2 v2.2.0-beta.5 |
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.
Shouldn't this require
be referencing a released version?
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.
We'd be doing more beta testing and add a stable version before the release. I believe other 1st party EdgeX dependencies will also get passed the code freeze with dev versions and switch to stable after a week or so. At least that's what happened in the previous release.
@@ -11,3 +11,6 @@ export PATH="$SNAP/bin:$PATH" | |||
# Several config options depend on resources that only exist after proxy | |||
# setup. This re-applies the config options logic after deferred startup: | |||
$SNAP/snap/hooks/configure options --service=security-proxy | |||
|
|||
# Process the EdgeX >=2.2 snap options | |||
$SNAP/snap/hooks/configure options --service=secrets-config |
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 love the fact we use the service name secrets-config
here, as its not really a service, it's an application. Is there a reason you changed this from security-proxy
?
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.
The new options are grouped per app and here, we are calling the processing of options for the secrets-config app. It should have been --app=secrets-config
for accuracy but that'd involve changes to the parts that are being deprecated. I will add an issue to take care of this as part of future refactoring. But for now I would leave it as is because this is an internal CLI typically not seen or used by any user.
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.
That doesn't really make sense as you're not configuring the secrets-config application, you're configuring the proxy, secrets-config is a helper application.
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.
If at some point the proxy supports provisioning of users and TLS certificates via environment variables, then we can use the apps.secrets-proxy.config
namespace to set those. In the meantime and in the scope of the new scheme, a user has to rely on the secrets-config app to perform the operations during runtime by setting e.g. snap set edgexfoundry apps.secrets-config.proxy.admin.public-key=xxx
. Note that there is no config
in the key because this isn't a configuration override.
The app in the suggested --app=<app>
CLI argument is equivalent to apps.<app>
namespace for snap options. In other words, as said, we are calling the processing of options for the secrets-config app. For now, only proxy is supported so we don't need to specify that in the CLI.
Thanks a lot @tonyespy for the much needed code review. I've replied to your comments.
We will be updating the snap/README and docs in the next two weeks, before the release. I'll update the PR description to make that clear. |
Supersceding #3924
This PR updates the snap to support environment variable injection as part of introducing the following scheme:
where:
<app>
is the name of the app (service, executable)<type>
is the type of option with respect to the app<key>
is key for the option. It could contain a path to set a value inside an object, e.g.x.y=z
sets{"x": {"y": "z"}}
.This change effectively makes options prefixed with
env.
deprecated. Such options will continue to work but be removed as of EdgeX 3.The reasons for the schema change is as follows:
apps.<app>.config.
andconfig.
prefixes exclusively.Environment Variables
The environment variable injection key uses the the following format:
apps.<app>.config.<env-var>
: setting app-specific env var ENV_VAR (e.g.apps.core-data.config.service-port=1000
).config.<env-var>
: setting global env vars by omitting theapps.<app>
prefix (e.g.config.service-host=localhost
,config.writable-loglevel=DEBUG
)Other
<env-var>
mapping examples:edgex-startup-duration
:EDGEX_STARTUP_DURATION
add-secretstore-tokens
:ADD_SECRETSTORE_TOKENS
.Secrets Proxy Options
New secrets-config proxy options:
apps.secrets-config.proxy.tls.cert
equivalent toenv.security-proxy.tls-certificate
apps.secrets-config.proxy.tls.key
equivalent toenv.security-proxy.tls-private-key
apps.secrets-config.proxy.tls.snis
equivalent toenv.security-proxy.tls-sni
apps.secrets-config.proxy.admin.public-key
equivalent toenv.security-proxy.public-key
andenv.security-proxy.user=admin,1,ES256
For usability purposes, the username, userid, algorithm of the single user have been hardcoded to admin, 1, ES256. This should be sufficient since we don't allow setting multiple users using snap options. The algorithm is set to ES256 rather than RS256, because it is more secure. If needed, it can be made configurable in future using keys
apps.secrets-config.proxy.admin.{user|id|algorithm}
.Usage
The new scheme is allowed only after opting-in by setting
config-enabled=true
app-options=true
. It will be enabled by default as of EdgeX 3.Setting
config-enabled=true
app-options=true
will:env.
options and ignore future sets.env.security-secret-store.add-secretstore-tokens
->apps.security-secretstore-setup.config.add-secretstore-tokens
env.security-secret-store.add-known-secrets
->apps.security-secretstore-setup.config.add-known-secrets
env.security-bootstrapper.add-registry-acl-roles
->apps.security-bootstrapper.config.add-registry-acl-roles
If your build fails due to your commit message not passing the build checks, please review the guidelines here: https://github.com/edgexfoundry/edgex-go/blob/main/.github/Contributing.md
PR Checklist
Please check if your PR fulfills the following requirements:
BREAKING CHANGE:
describing the break)Testing Instructions
Install
Run
It should be rejected because
config-enabled
is not set.Run
New Dependency Instructions (If applicable)