Extensions Management Commands #4409
Conversation
|
Thanks for sharing the work here, @wbreza! Just sharing some high-level UX observations and wonders for now while I find time to parse through more details. Sharing some observations:
Sharing some wonders I have:
Overall, I think this is a really great start that will help us guide us towards productive conversations as we chat more about this today, so thanks once again for sharing. |
|
Do you envision different types of extensions, such as root-level command extensions, command groups with multiple commands, or extensions added to existing commands (e.g., adding an extension to 'azd up')? Additionally, can I add an extension to extend infrastructure providers? |
Yes, ultimately I envision different types of extensions. The first set of extensions would allow extension authors to create new commands that can be executed through Over time I can also see extensions providing additional functionality to customize experience within Regarding |
wbreza
force-pushed
the
extensions-poc
branch
from
8fa9a3f
to
b252a17
Compare
wbreza
force-pushed
the
extensions-poc
branch
from
3a6ba44
to
161ee79
Compare
wbreza
requested review from
ellismg,
vhvb1989,
hemarina and
weikanglim
as code owners
Azure Dev CLI Install InstructionsInstall scriptsMacOS/Linux
bash: pwsh: WindowsPowerShell install MSI install Standalone Binary
MSI
Documentationlearn.microsoft.com documentationtitle: Azure Developer CLI reference
|
There was a problem hiding this comment.
Partial review - the public key stuff scares me the most - I think if we want to do this we need to engage with the security team and talk with them about how we do this - I think they may want changes to how we transport the signature so we don't, for example, have to change the data we download in order to validate the signature.
Would love to better understand the treat model for that stuff (I'm onboard with doing the SHA validation of downloads, as we do for other tools).
Like how clean and small this is - great first step on the new path forward.
|
|
||
| func validateRegistry(registry *Registry) error { | ||
| // Extract the signature | ||
| signature := registry.Signature |
There was a problem hiding this comment.
Why do we enforce that the registry content is signed with a given key? I'm concerned about the operational tax of this on our end. Presumably we need some level of key management here and a strategy for rotation and compromise and I'm not sure what we attack we are trying to protect against. For example, if this was just to protect against local tampering of the cache'd data, I'd almost say that I'd be willing to just never cache this in favor of not having to bring in a bunch of crypto stuff.
| } | ||
|
|
||
| info, err := os.Stat(c.filePath) | ||
| if os.IsNotExist(err) { |
There was a problem hiding this comment.
What about the other error cases? Should we just fail for them and return false as well? I am guessing that info is likely to be the zero value in that case and maybe this ends up just returning false because the cache file looks "old" - but it might be nice to explicitly codify that in the code.
| return nil, fmt.Errorf("failed to resolve data: %w", err) | ||
| } | ||
|
|
||
| if err := c.Set(value); err != nil { |
There was a problem hiding this comment.
We'll call this (and write the data to the cache) even when AZD_NO_CACHE is set. Is that expected?
| @@ -429,6 +448,11 @@ func getCmdRootHelpCommands(cmd *cobra.Command) (result string) { | |||
|
|
|||
| var paragraph []string | |||
| for _, title := range groups { | |||
| groupCommands := commandGroups[string(title)] | |||
There was a problem hiding this comment.
NIT: I'd personally either rewrite such that we don't introduce the groupCommands local here (by just inlining this into the len call) OR update L458 to now use groupCommands.
| @@ -349,6 +351,23 @@ func NewRootCmd( | |||
| ioc.RegisterNamedInstance(rootContainer, "root-cmd", rootCmd) | |||
| registerCommonDependencies(rootContainer) | |||
|
|
|||
| // Conditionally register the 'extension' commands if the feature is enabled | |||
| var alphaFeatureManager *alpha.FeatureManager | |||
| if err := rootContainer.Resolve(&alphaFeatureManager); err == nil { | |||
There was a problem hiding this comment.
My guess is that if we fail to resolve the *alpha.FeatureManager here, stuff is in a bad way and we should just panic instead of just not running this code, but maybe I'm wrong? We are okay in panicing elsewhere in this method for cases where we can't resolve things we expect, is there a reason this is different?
|
|
||
| // Verify verifies the given data and its Base64-encoded signature | ||
| func verifySignature(data []byte, signature string) error { | ||
| publicKey, err := loadPublicKey(resources.PublicKey) |
There was a problem hiding this comment.
If we end up keeping this public key stuff around to do signature verification - I think we should arrange things such that we do as much of this loading and praising at package init time and just panic on errors, then so that verifySignature can never fail due to an error reading the key we're going to use for verification. In theory if the call to loadPublicKey(resources.PublicKey) fails, it means there's a problem with our embedded key and I'd prefer if that failure was loud and prompt.
| // CacheResolver is a function that resolves the cache value. | ||
| type CacheResolver[T any] func(ctx context.Context) (*T, error) | ||
|
|
||
| // FileCache is a cache that stores the value in a file otherwise resolves it. |
There was a problem hiding this comment.
Nice abstraction - I'll try to move the stuff that caches the latest version for our update check over to it if possible.
You might note that it's the JSON encoding of *T that is stored in the cache here, since that matters a bit for consumers.
| @@ -0,0 +1,18 @@ | |||
|
|
|||
There was a problem hiding this comment.
Is this still intended to be part of this PR?
|
|
||
| forceColor := !color.NoColor | ||
| if forceColor { | ||
| allEnv = append(allEnv, "FORCE_COLOR=1") |
There was a problem hiding this comment.
Is this some new convention we're building?
|
|
||
| extensionPath := filepath.Join(homeDir, extension.Path) | ||
|
|
||
| _, err = os.Stat(extensionPath) |
There was a problem hiding this comment.
Would it be possible to move this logic in to GetInstalled? Could the extension returned have a full path (including the home directory) and do the Stat to ensure the binary is there?
| } | ||
|
|
||
| func (a *extensionShowAction) Run(ctx context.Context) (*actions.ActionResult, error) { | ||
| extensionName := a.args[0] |
There was a problem hiding this comment.
Should we have another length check here and print a more descriptive error, like how we do it for some of the other commands?
This PR adds the
extensioncommand group intoazdand enables the execution of installed extensions.Important
The extensions feature is behind a feature flag and must be enabled by running:
azd config set alpha.extensions onCommands
azd extension list- List available extensions in the registryazd extension show <name>- Show details of a specified extensionazd extension install <name>- Install an extensionazd extension uninstall <name>- Uninstall an extensionazd extension upgrade <name>- Upgrades an extensionRegistry
The registry is loaded from an aka URL @ https://aka.ms/azd/extensions/registry
Examples
azd extension listazd extension show aiazd extension install aiazd -hExtensions show up in standard azd help commands.
azd test -hExtension help shells into extension executable
azd test --unitInvoke extensions through azd command interface
azd extension uninstall ai