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.

Sign up for GitHub

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

Merged
merged 7 commits into from
Apr 26, 2022
Merged

feat(snap): Add support for environment variable injection #3986

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
c2f6aef
feat(snap): use updated environment variable injection
Mar 24, 2022
9d02839
feat(snap): use updated environment variable injection
Mar 30, 2022
3744ca2
feat(snap): use updated environment variable injection
Apr 1, 2022
4a7468f
feat(snap): use updated environment variable injection
Apr 1, 2022
327cf1d
feat(snap): add proxy option handling, bump mod
farshidtz Apr 22, 2022
1642766
feat: upgrade edgex-snap-hooks to remove support for complex config keys
farshidtz Apr 25, 2022
1d3cb66
build: bump edgex-snap-hooks for env value bugfix
farshidtz Apr 25, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion snap/local/hooks/cmd/configure/main.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,9 @@ var cli *hooks.CtlCli = hooks.NewSnapCtl()
func main() {
// no subcommand, as called by snapd
if len(os.Args) == 1 {
// configure everything
// process the EdgeX >=2.2 snap options
processAppOptions()
// configure everything else, incl. the legacy snap options
configure()
return
}
Expand Down
52 changes: 52 additions & 0 deletions snap/local/hooks/cmd/configure/options.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ import (
"os"

hooks "github.com/canonical/edgex-snap-hooks/v2"
app_options "github.com/canonical/edgex-snap-hooks/v2/options"
)

func applyConfigOptions(service string) error {
Expand Down Expand Up @@ -60,8 +61,59 @@ func options() {

hooks.Info("edgexfoundry:configure-options: handling config options for a single service: " + *service)

// process the EdgeX >=2.2 snap options
err = app_options.ProcessAppCustomOptions(*service)
if err != nil {
hooks.Error(fmt.Sprintf("edgexfoundry:configure-options: could not process custom options: %v", err))
os.Exit(1)
}

// process the legacy snap options
if err := applyConfigOptions(*service); err != nil {
hooks.Error(fmt.Sprintf("edgexfoundry:configure-options: error handling config options for %s: %v", *service, err))
os.Exit(1)
}
}

func processAppOptions() {
err := app_options.ProcessAppConfig(
"core-data",
"core-metadata",
"core-command",
"support-notifications",
"support-scheduler",
"app-service-configurable",
"device-virtual",
"security-secret-store",
"security-secretstore-setup",
"security-proxy-setup",
"security-bootstrapper",
"sys-mgmgt-agent",
Copy link
Member

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.

Copy link
Member Author

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.

Copy link
Member

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.

Copy link
Member Author

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.

)
if err != nil {
hooks.Error(fmt.Sprintf("edgexfoundry:configure could not process config options: %v", err))
os.Exit(1)
}

// After installation, the configure hook initiates the deferred startup of services,
// processes snap options and exits. The actual services startup happens only
// after the configure hook exits.
//
// The following options should not be processed within the configure hook during
// the initial installation (install-mode=defer-startup). They should be processed
// only on follow-up calls to the configure hook (i.e. when snap set/unset is called)
installMode, err := hooks.NewSnapCtl().Config("install-mode") // this set in the install hook
if err != nil {
hooks.Error(fmt.Sprintf("edgexfoundry:configure failed to read 'install-mode': %s", err))
os.Exit(1)
}
if installMode != "defer-startup" {
err = app_options.ProcessAppCustomOptions(
"secrets-config", // also processed in security-proxy-post-setup.sh
)
if err != nil {
hooks.Error(fmt.Sprintf("edgexfoundry:configure: could not process custom options: %v", err))
os.Exit(1)
}
}
}
5 changes: 4 additions & 1 deletion snap/local/hooks/go.mod
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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
Copy link
Member

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?

Copy link
Member Author

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.


// replace github.com/canonical/edgex-snap-hooks/v2 => ./edgex-snap-hooks
// replace github.com/canonical/edgex-snap-hooks/v2 => github.com/farshidtz/edgex-snap-hooks/v2 d43ccc771100d663099c8ca8e3974d78076b2058

go 1.17
4 changes: 2 additions & 2 deletions snap/local/hooks/go.sum
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
github.com/canonical/edgex-snap-hooks/v2 v2.1.3 h1:mcV/atn6k6sN6Uik+lSQGEZi4Q6r96epgBW+u6AGZ3Y=
github.com/canonical/edgex-snap-hooks/v2 v2.1.3/go.mod h1:rOxrwdYL7hJDhxFH3uV+nVgLPjWOhJWgM5PRD5YG1jI=
github.com/canonical/edgex-snap-hooks/v2 v2.2.0-beta.5 h1:EDFjmHy8CG4T8uFPqD+Per8Hgk250PvRsMgEDCXjYtE=
github.com/canonical/edgex-snap-hooks/v2 v2.2.0-beta.5/go.mod h1:rOxrwdYL7hJDhxFH3uV+nVgLPjWOhJWgM5PRD5YG1jI=
github.com/davecgh/go-spew v1.1.0 h1:ZDRjVQ15GmhC3fiQ8ni8+OwkZQO4DARzQgrnXU1Liz8=
github.com/davecgh/go-spew v1.1.0/go.mod h1:J7Y8YcW2NihsgmVo/mv3lAwl/skON4iLHjSsI+c5H38=
github.com/pmezard/go-difflib v1.0.0 h1:4DBwDE0NGyQoBHbLQYPwSUPoCMWR5BEzIk/f1lZbAQM=
Expand Down
3 changes: 3 additions & 0 deletions snap/local/runtime-helpers/bin/security-proxy-post-setup.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Member

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?

Copy link
Member Author

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.

Copy link
Member

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.

Copy link
Member Author

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.