Implement plugin manager #37

qmuntal · 2022-04-26T18:53:59Z

This PR implements the common code of the plugin-extensibility spec so it will be used by notary and others.

The Manager implementation will probably change once notaryproject/notation#167 is discussed.

It also contains unit test and integration tests with ad-hoc plugins. Coverage is around 95%.

@SteveLasker

Signed-off-by: qmuntal qmuntaldiaz@microsoft.com

Signed-off-by: qmuntal <qmuntaldiaz@microsoft.com>

SteveLasker

Great to get the baseline in, as we continue to iterate.

gokarnm · 2022-04-27T18:29:40Z

cc: @rgnote @shizhMSFT for review.

rgnote · 2022-04-28T00:14:12Z

plugin/metadata.go

+	Description               string   `json:"description"`
+	Version                   string   `json:"version"`
+	URL                       string   `json:"url"`
+	SupportedContractVersions []string `json:"supported-contract-versions"`


supported-contract-versions

Should it be supportedContractVersions?

Good catch!

rgnote

Overall, looks solid

rgnote · 2022-04-28T00:39:43Z

plugin/plugin.go

+	// MetadataSubcommandName is the name of the plugin subcommand
+	// which must be supported by every plugin and returns the
+	// plugin metadata.
+	MetadataSubcommandName = "get-plugin-metadata"


Isn't this discover?

We changed it to get-plugin-metadata in notaryproject/specifications#150

rgnote · 2022-04-28T00:44:29Z

plugin/manager.go

+	return cmd.Output()
+}
+
+type rootedFS struct {


nit: doc comment

Signed-off-by: qmuntal <qmuntaldiaz@microsoft.com>

qmuntal · 2022-04-29T15:36:09Z

@jaredpar @SteveLasker as the review process is going slower than expected, I've pushed a new batch of commits since. They don't add new functionality but refine the API after the learnings I got using it in notaryproject/notation#168.

SteveLasker · 2022-04-29T15:59:40Z

@qmuntal, question for how we can test a set of these changes.
Just an idea (might be a bad one), should we create a branch in notation, notation-go and azure/notation-azure-kv to test these changes?

qmuntal · 2022-04-30T16:51:21Z

@qmuntal, question for how we can test a set of these changes. Just an idea (might be a bad one), should we create a branch in notation, notation-go and azure/notation-azure-kv to test these changes?

I like the idea! I'll come with something and share it with you all.

Signed-off-by: qmuntal <qmuntaldiaz@microsoft.com>

gokarnm

Overall structure and organization looks great, added some comments. Will make another pass tomorrow for some areas I skipped. I'm new to Go, some high level questions.

Some of these error messages will be presented to users through the CLI, how do we differentiate them, and case and format them correctly with more context.
We need to support verbose logging through out the library and CLI, is there a standard way to do that in Go? With verbose logging, we can return concise error messages to the user, and they can enable verbose logging for more details/troubleshooting.

plugin/errors.go

gokarnm · 2022-05-02T07:12:59Z

plugin/errors.go

+	"fmt"
+)
+
+var ErrUnknownCommand = errors.New("not a plugin command")


Do we need this exported variable? Plugin implementation should ideally return a custom error message that includes the unsupported command name.

Wops, this variable is an unused leftover, will remove it.

plugin/manager/manager.go

plugin/plugin.go

plugin/manager/manager.go

gokarnm · 2022-05-02T08:23:33Z

plugin/manager/manager.go

+	if fsys, ok := fsys.(rootedFS); ok {
+		return filepath.Join(fsys.root, name, base)
+	}
+	return filepath.Join(name, base)


I didn't understand what this code achieves, specially line 204. I'm new to Go.

if fsys, ok := fsys.(rootedFS); ok { this is an interface type cast, being ok a flag which is true when fsys can be casted to rootedFS.

In production fsys type is always rootedFS, as this is what NewManager instantiates, but for testing purposes I'm also supporting generic fsys types, aka fs.FS, which are mostly instantiated from testing/fstest package.

Got it. Should we add a code comment here to make this clearer, that the alternate path is to support testing?

Signed-off-by: qmuntal <qmuntaldiaz@microsoft.com>

qmuntal · 2022-05-03T06:46:56Z

Overall structure and organization looks great, added some comments. Will make another pass tomorrow for some areas I skipped. I'm new to Go, some high level questions.

Some of these error messages will be presented to users through the CLI, how do we differentiate them, and case and format them correctly with more context.

We need to support verbose logging through out the library and CLI, is there a standard way to do that in Go? With verbose logging, we can return concise error messages to the user, and they can enable verbose logging for more details/troubleshooting.

Thanks for the review @gokarnm. My take is that notation-go should provide fine-grained error messaged which are good for displaying but also can be captured programmatically. If an error is missing some context, then we should add it.

What I wouldn't support is verbose levels in the returned errors. It either returns an error or not, leaving to the caller to decide if it should be logged as a debug, info, warning or whatever other level.

The build-in log package does not have support for using log levels, but there are multiple well-known packages with does support it, e.g. github.com/sirupsen/logrus.

Signed-off-by: qmuntal <qmuntaldiaz@microsoft.com>

gokarnm · 2022-05-04T07:03:37Z

Thanks for the review @gokarnm. My take is that notation-go should provide fine-grained error messaged which are good for displaying but also can be captured programmatically. If an error is missing some context, then we should add it.

What about the error message casing, error messages which are returned to CLI users should be correctly cased. It seems to be a convention to use small case error messages in Go?

What I wouldn't support is verbose levels in the returned errors. It either returns an error or not, leaving to the caller to decide if it should be logged as a debug, info, warning or whatever other level.

Agreed, I wasn't clear enough. I meant verbose level should log to stdout so that it's visible to in CLI or an upstream caller if they enable verbose logging. The verbose logging will need to be supported in notation-go and notation-core-go, as that's where bulk of the decision paths and validations will be. This need not be implemented right away.

gokarnm

LGTM for this iteration. Added a couple of comments to not allow symlinks for plugin dir, that can be addressed in next revision, when we add support for describe-key.

gokarnm · 2022-05-04T07:25:43Z

plugin/manager/manager.go

+	if fi.Mode().Type() != 0 {
+		// Ignore non-regular files.
+		return false
+	}


We should additionally check if the containing dir ~/.notation/plugins/{plugin-name}/notation-{plugin-name} is also not a symlink, something that was missed in the spec.

This is exactly what it is being checked here. Non-regular files include symlinks and directories.

I'm bit confused here. Isn't this code only checking that the plugin executable (notation-{plugin-name}) itself is not a symlink, and not that additionally the enclosing directory is also not a symlink (~/.notation/plugins/{plugin-name} dir)?

gokarnm · 2022-05-04T07:28:15Z

plugin/manager/manager.go

+		if dir == "." || !d.IsDir() {
+			return nil
+		}


We should skip over plugin dirs that are symlinks, something that was missed in the spec.

qmuntal · 2022-05-04T10:04:20Z

What about the error message casing, error messages which are returned to CLI users should be correctly cased. It seems to be a convention to use small case error messages in Go?

There are multiple options to transform and localize text, being https://pkg.go.dev/golang.org/x/text the most common.

Agreed, I wasn't clear enough. I meant verbose level should log to stdout so that it's visible to in CLI or an upstream caller if they enable verbose logging. The verbose logging will need to be supported in notation-go and notation-core-go, as that's where bulk of the decision paths and validations will be. This need not be implemented right away.

If we need to log something from those libraries, which is still not clear to me if we should, we will find the way. For example, http.Server logs error into the optional http.Server.ErrorLog property.

Signed-off-by: qmuntal <qmuntaldiaz@microsoft.com>

gokarnm · 2022-05-05T05:33:32Z

If we need to log something from those libraries, which is still not clear to me if we should, we will find the way. For example, http.Server logs error into the optional http.Server.ErrorLog property.

Yes, that's what I'm looking for, similar to libraries that support emitting verbose logs to aid debugging issues. It would be useful for us to debug user reported errors as the logic in many of these areas, plugin and signature verification logic, is quite complex.

implement plugin manager

d0c0218

Signed-off-by: qmuntal <qmuntaldiaz@microsoft.com>

qmuntal requested a review from a team April 26, 2022 18:54

qmuntal added 3 commits April 26, 2022 20:59

ignore symlinks

2943a57

Signed-off-by: qmuntal <qmuntaldiaz@microsoft.com>

fix Manager.Command

c33c354

Signed-off-by: qmuntal <qmuntaldiaz@microsoft.com>

export constants

5bfff33

Signed-off-by: qmuntal <qmuntaldiaz@microsoft.com>

qmuntal mentioned this pull request Apr 27, 2022

Support managing plugin keys notaryproject/notation#168

Merged

SteveLasker requested review from gokarnm and iamsamirzon April 27, 2022 17:25

SteveLasker approved these changes Apr 27, 2022

View reviewed changes

rgnote reviewed Apr 28, 2022

View reviewed changes

jaredpar approved these changes Apr 28, 2022

View reviewed changes

qmuntal added 4 commits April 28, 2022 09:27

pr feedback

c09b4a1

Signed-off-by: qmuntal <qmuntaldiaz@microsoft.com>

improve error message

e1f2d49

Signed-off-by: qmuntal <qmuntaldiaz@microsoft.com>

move manager to its own package

2cacb9f

Signed-off-by: qmuntal <qmuntaldiaz@microsoft.com>

change metadata error messages

55fc95a

Signed-off-by: qmuntal <qmuntaldiaz@microsoft.com>

SteveLasker requested review from shizhMSFT and removed request for iamsamirzon April 29, 2022 03:56

improve the plugin manager interface

f0636e1

Signed-off-by: qmuntal <qmuntaldiaz@microsoft.com>

improve Manager.Run

3f03a94

Signed-off-by: qmuntal <qmuntaldiaz@microsoft.com>

gokarnm requested changes May 2, 2022

View reviewed changes

pr feedback

e5a4e47

Signed-off-by: qmuntal <qmuntaldiaz@microsoft.com>

qmuntal requested a review from gokarnm May 3, 2022 06:47

qmuntal added 2 commits May 3, 2022 09:15

remove Command.Capability() and Command.NewResponse()

7d75201

Signed-off-by: qmuntal <qmuntaldiaz@microsoft.com>

remove command validation

2a7ef8f

Signed-off-by: qmuntal <qmuntaldiaz@microsoft.com>

gokarnm approved these changes May 4, 2022

View reviewed changes

ignore symlinked plugin directories

0cacbd8

Signed-off-by: qmuntal <qmuntaldiaz@microsoft.com>

SteveLasker merged commit 182873b into notaryproject:main May 4, 2022

yizha1 mentioned this pull request Aug 22, 2022

Notation plug-in design / notation-go-lib integration #23

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement plugin manager #37

Implement plugin manager #37

qmuntal commented Apr 26, 2022 •

edited

Loading

SteveLasker left a comment

gokarnm commented Apr 27, 2022

rgnote Apr 28, 2022

qmuntal Apr 28, 2022

rgnote left a comment

rgnote Apr 28, 2022

qmuntal Apr 28, 2022

rgnote Apr 28, 2022

qmuntal Apr 28, 2022

qmuntal commented Apr 29, 2022

SteveLasker commented Apr 29, 2022

qmuntal commented Apr 30, 2022

gokarnm left a comment

gokarnm May 2, 2022

qmuntal May 3, 2022

gokarnm May 2, 2022

qmuntal May 3, 2022

gokarnm May 4, 2022

qmuntal May 4, 2022

qmuntal commented May 3, 2022

gokarnm commented May 4, 2022

gokarnm left a comment

gokarnm May 4, 2022

qmuntal May 4, 2022

gokarnm May 5, 2022

gokarnm May 4, 2022

qmuntal May 4, 2022

qmuntal commented May 4, 2022

gokarnm commented May 5, 2022

Implement plugin manager #37

Implement plugin manager #37

Conversation

qmuntal commented Apr 26, 2022 • edited Loading

SteveLasker left a comment

Choose a reason for hiding this comment

gokarnm commented Apr 27, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rgnote left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

qmuntal commented Apr 29, 2022

SteveLasker commented Apr 29, 2022

qmuntal commented Apr 30, 2022

gokarnm left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

qmuntal commented May 3, 2022

gokarnm commented May 4, 2022

gokarnm left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

qmuntal commented May 4, 2022

gokarnm commented May 5, 2022

qmuntal commented Apr 26, 2022 •

edited

Loading