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.

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

check for plugins file schema version and type #2532

Merged
merged 4 commits into from
Jan 25, 2023
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
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
19 changes: 10 additions & 9 deletions docs/content/reference/file-formats.md
Original file line number Diff line number Diff line change
Expand Up @@ -162,21 +162,22 @@ You can use this [json schema][plugins-schema] to validate a plugins config file
```yaml
schemaType: Plugins
schemaVersion: 1.0.0
azure:
version: v1.0.0
feedURL: https://cdn.porter.sh/plugins/atom.xml
url: https://example.com
mirror: https://example.com
plugins:
carolynvs marked this conversation as resolved.
Show resolved Hide resolved
azure:
version: v1.0.0
feedURL: https://cdn.porter.sh/plugins/atom.xml
url: https://example.com
mirror: https://example.com
```

| Field | Required | Description |
|----------------------|----------|------------------------------------------------------------------------------------------------------------------------------------------------|
| schemaType | false | The type of document. This isn't used by Porter but is included when Porter outputs the file, so that editors can determine the resource type. |
| schemaVersion | true | The version of the Plugins schema used in this file. |
| <pluginName>.version | false | The version of the plugin. |
| <pluginName>.feedURL | false | The url of an atom feed where the plugin can be downloaded.
| <pluginName>.url | false | The url from where the plugin can be downloaded. |
| <pluginName>.mirror | false | The mirror of official Porter assets. |
| plugins.<pluginName>.version | false | The version of the plugin. |
| plugins.<pluginName>.feedURL | false | The url of an atom feed where the plugin can be downloaded.
| plugins.<pluginName>.url | false | The url from where the plugin can be downloaded. |
| plugins.<pluginName>.mirror | false | The mirror of official Porter assets. |

[cs-schema]: /schema/v1/credential-set.schema.json
[ps-schema]: /schema/v1/parameter-set.schema.json
Expand Down
47 changes: 35 additions & 12 deletions pkg/plugins/install.go
Original file line number Diff line number Diff line change
Expand Up @@ -6,8 +6,13 @@ import (

"get.porter.sh/porter/pkg/pkgmgmt"
"get.porter.sh/porter/pkg/portercontext"
"github.com/cnabio/cnab-go/schema"
)

// InstallPluginsSchemaVersion represents the version associated with the schema
// plugins configuration documents.
var InstallPluginsSchemaVersion = schema.Version("1.0.0")

type InstallOptions struct {
pkgmgmt.InstallOptions

Expand All @@ -25,8 +30,9 @@ func (o *InstallOptions) Validate(args []string, cxt *portercontext.Context) err
return fmt.Errorf("plugin URL should not be specified when --file is provided")
}

if o.Version != "" {
return fmt.Errorf("plugin version should not be specified when --file is provided")
// version should not be set to anything other than the default value
if o.Version != "" && o.Version != "latest" {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When --version is empty, it defaults to latest so I'm not sure I get what this is checking for or guarding against? The error message just says that they can't specify --version at all so can't we just stick with the original check that version was specified?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When no version is specified by users, the o.Version is default to latest. The original check would fail bc the o.Version is not empty.
The check here is to guard that users didn't try to set a version value through the flag. o.Version should not have value other than "latest" or empty

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! I didn't realize that we were defaulting to "latest" so early, I'm assuming it's the flag default. Makes sense! 👍

return fmt.Errorf("plugin version %s should not be specified when --file is provided", o.Version)
}

if _, err := cxt.FileSystem.Stat(o.File); err != nil {
Expand All @@ -39,20 +45,37 @@ func (o *InstallOptions) Validate(args []string, cxt *portercontext.Context) err
return o.InstallOptions.Validate(args)
}

// InstallFileOption is the go representation of plugin installation file format.
type InstallFileOption map[string]pkgmgmt.InstallOptions
// InstallPluginsSpec represents the user-defined configuration for plugins installation.
type InstallPluginsSpec struct {
SchemaType string `yaml:"schemaType"`
SchemaVersion string `yaml:"schemaVersion"`
Plugins InstallPluginsConfig `yaml:"plugins"`
}

func (spec InstallPluginsSpec) Validate() error {
if InstallPluginsSchemaVersion != schema.Version(spec.SchemaVersion) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just remembered that we are missing schemaType validation on our other resources. I had created a branch to fix that and then got pulled off onto other things and forgot to submit a PR.

Let's do a check here for the schemaType as well, so that if they enter in a value other than what's expected, we don't try to process the file further. For example, they accidentally pass a bundle or installation file to the -f parameter.

https://github.com/carolynvs/porter/blob/ea694aa668a448eb2293775da5f5cfda3567214f/pkg/manifest/manifest.go#L185-L187

I'll submit my PR afterwards that adds the check to the other resources.

if spec.SchemaVersion == "" {
spec.SchemaVersion = "(none)"
}
return fmt.Errorf("invalid schemaVersion provided: %s. This version of Porter is compatible with %s.", spec.SchemaVersion, InstallPluginsSchemaVersion)
}
return nil
}

// InstallPluginsConfig is the go representation of plugin installation file format.
type InstallPluginsConfig map[string]pkgmgmt.InstallOptions

// InstallPluginsConfig is a sorted list of InstallationFileOption in alphabetical order.
type InstallPluginsConfig struct {
data InstallFileOption
// InstallPluginsConfigList is a sorted list of InstallationFileOption in alphabetical order.
type InstallPluginsConfigList struct {
data InstallPluginsConfig
keys []string
}

// NewInstallPluginConfigs returns a new instance of InstallPluginConfigs with plugins sorted in alphabetical order
// using their names.
func NewInstallPluginConfigs(opt InstallFileOption) InstallPluginsConfig {
func NewInstallPluginConfigs(opt InstallPluginsConfig) InstallPluginsConfigList {
keys := make([]string, 0, len(opt))
data := make(InstallFileOption, len(opt))
data := make(InstallPluginsConfig, len(opt))
for k, v := range opt {
keys = append(keys, k)

Expand All @@ -65,14 +88,14 @@ func NewInstallPluginConfigs(opt InstallFileOption) InstallPluginsConfig {
return keys[i] < keys[j]
})

return InstallPluginsConfig{
return InstallPluginsConfigList{
data: data,
keys: keys,
}
}

// Configs returns InstallOptions list in alphabetical order.
func (pc InstallPluginsConfig) Configs() []pkgmgmt.InstallOptions {
// Values returns InstallOptions list in alphabetical order.
func (pc InstallPluginsConfigList) Values() []pkgmgmt.InstallOptions {
value := make([]pkgmgmt.InstallOptions, 0, len(pc.keys))
for _, k := range pc.keys {
value = append(value, pc.data[k])
Expand Down
4 changes: 2 additions & 2 deletions pkg/plugins/install_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -19,9 +19,9 @@ func TestInstallOptions_Validate(t *testing.T) {
}

func TestInstallPluginsConfig(t *testing.T) {
input := InstallFileOption{"kubernetes": pkgmgmt.InstallOptions{URL: "test-kubernetes.com"}, "azure": pkgmgmt.InstallOptions{URL: "test-azure.com"}}
input := InstallPluginsConfig{"kubernetes": pkgmgmt.InstallOptions{URL: "test-kubernetes.com"}, "azure": pkgmgmt.InstallOptions{URL: "test-azure.com"}}
expected := []pkgmgmt.InstallOptions{{Name: "azure", PackageType: "plugin", URL: "test-azure.com"}, {Name: "kubernetes", PackageType: "plugin", URL: "test-kubernetes.com"}}

cfg := NewInstallPluginConfigs(input)
require.Equal(t, expected, cfg.Configs())
require.Equal(t, expected, cfg.Values())
}
23 changes: 14 additions & 9 deletions pkg/porter/plugins.go
Original file line number Diff line number Diff line change
Expand Up @@ -158,23 +158,23 @@ func (p *Porter) InstallPlugin(ctx context.Context, opts plugins.InstallOptions)
ctx, log := tracing.StartSpan(ctx)
defer log.EndSpan()

installConfigs, err := p.getPluginInstallConfigs(ctx, opts)
installOpts, err := p.getPluginInstallOptions(ctx, opts)
if err != nil {
return err
}
for _, cfg := range installConfigs {
err := p.Plugins.Install(ctx, cfg)
for _, opt := range installOpts {
err := p.Plugins.Install(ctx, opt)
if err != nil {
return err
}

plugin, err := p.Plugins.GetMetadata(ctx, cfg.Name)
plugin, err := p.Plugins.GetMetadata(ctx, opt.Name)
if err != nil {
return fmt.Errorf("failed to get plugin metadata: %w", err)
}

v := plugin.GetVersionInfo()
fmt.Fprintf(p.Out, "installed %s plugin %s (%s)\n", cfg.Name, v.Version, v.Commit)
fmt.Fprintf(p.Out, "installed %s plugin %s (%s)\n", opt.Name, v.Version, v.Commit)
}

return nil
Expand All @@ -191,13 +191,13 @@ func (p *Porter) UninstallPlugin(ctx context.Context, opts pkgmgmt.UninstallOpti
return nil
}

func (p *Porter) getPluginInstallConfigs(ctx context.Context, opts plugins.InstallOptions) ([]pkgmgmt.InstallOptions, error) {
func (p *Porter) getPluginInstallOptions(ctx context.Context, opts plugins.InstallOptions) ([]pkgmgmt.InstallOptions, error) {
_, log := tracing.StartSpan(ctx)
defer log.EndSpan()

var installConfigs []pkgmgmt.InstallOptions
if opts.File != "" {
var data plugins.InstallFileOption
var data plugins.InstallPluginsSpec
if log.ShouldLog(zapcore.DebugLevel) {
// ignoring any error here, printing debug info isn't critical
contents, _ := p.FileSystem.ReadFile(opts.File)
Expand All @@ -207,9 +207,14 @@ func (p *Porter) getPluginInstallConfigs(ctx context.Context, opts plugins.Insta
if err := encoding.UnmarshalFile(p.FileSystem, opts.File, &data); err != nil {
return nil, fmt.Errorf("unable to parse %s as an installation document: %w", opts.File, err)
}
sortedCfgs := plugins.NewInstallPluginConfigs(data)

for _, config := range sortedCfgs.Configs() {
if err := data.Validate(); err != nil {
return nil, err
}

sortedCfgs := plugins.NewInstallPluginConfigs(data.Plugins)

for _, config := range sortedCfgs.Values() {
// if user specified a feed url or mirror using the flags, it will become
// the default value and apply to empty values parsed from the provided file
if config.FeedURL == "" {
Expand Down
14 changes: 9 additions & 5 deletions pkg/porter/testdata/plugins.json
Original file line number Diff line number Diff line change
@@ -1,8 +1,12 @@
{
"plugin1": {
"version": "v1.0"
},
"plugin2": {
"version": "v1.0"
"schemaType": "Plugins",
"schemaVersion": "1.0.0",
"plugins": {
"plugin1": {
"version": "v1.0"
},
"plugin2": {
"version": "v1.0"
}
}
}
11 changes: 7 additions & 4 deletions pkg/porter/testdata/plugins.yaml
Original file line number Diff line number Diff line change
@@ -1,4 +1,7 @@
plugin1:
version: v1.0
plugin2:
version: v1.0
schemaType: Plugins
schemaVersion: 1.0.0
plugins:
plugin1:
version: v1.0
plugin2:
version: v1.0
65 changes: 35 additions & 30 deletions pkg/schema/plugins.schema.json
Original file line number Diff line number Diff line change
Expand Up @@ -11,41 +11,46 @@
"description": "Version of the plugins schema to which this document adheres",
"type": "string",
"default": "1.0.0"
}
},
"required": [
"schemaVersion"
],
"title": "Plugins json schema",
"type": "object",
"additionalProperties": {
"type": "object",
"properties": {
"key": {
"type": "string"
},
"value": {
},
"plugins": {
"description": "The definition of plugins to install",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would help to give people a hint that the field names under plugins should be the name of the plugin

Suggested change
"description": "The definition of plugins to install",
"description": "A map of plugins to install, keyed by the plugin name",

"type": "object",
"default": "1.0.0",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This default shouldn't be defined here. I assume you meant to define it directly on schemaVersion?

"additionalProperties": {
"type": "object",
"properties": {
"version": {
"description": "The version for the plugins.",
"type": "string"
},
"feedURL": {
"description": "The URL of an atom feed where the plugin can be downloaded.",
"key": {
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 think this is how the properties json schema field works for objects. Have you tested this against the json schema validator (https://www.jsonschemavalidator.net/) with both matching and incorrect schema to make sure it's correct?

Here is how I have defined an object with unknown keys in porter elsewhere (for custom action metadata):

https://github.com/getporter/porter/blob/2aa80a6670bd0aac7284cb8f5ad375b6b44b3365/pkg/schema/manifest.schema.json#L417C6-L421

"type": "string"
},
"url": {
"description": "The URL from where the plugin can be downloaded",
"type": "string"
},
"mirror": {
"description": "Mirror of official Porter assets.",
"type": "string"
},
"additionalProperties": false
"value": {
"type": "object",
"properties": {
"version": {
"description": "The version for the plugin.",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like to give the user hints that will show up in the hover text / documentation that will help them understand how to fill out the file better:

Suggested change
"description": "The version for the plugin.",
"description": "The version for the plugin. Defaults to latest when unspecified",

"type": "string"
},
"feedURL": {
"description": "The URL of an atom feed where the plugin can be downloaded.",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
"description": "The URL of an atom feed where the plugin can be downloaded.",
"description": "The URL of an atom feed where the plugin can be downloaded. Defaults to the official Porter plugin feed.",

"type": "string"
},
"url": {
"description": "The URL from where the plugin can be downloaded",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This field is hard to guess, so I always like to include an example of how to download from github releases:

Suggested change
"description": "The URL from where the plugin can be downloaded",
"description": "The URL from where the plugin can be downloaded. For example, https://github.com/MChorfa/porter-helm3/releases/download",

"type": "string"
},
"mirror": {
"description": "Mirror of official Porter assets.",
"type": "string"
},
"additionalProperties": false
}
}
}
}
}
}
},
"required": [
"schemaVersion"
],
"title": "Plugins json schema",
"type": "object"
}
7 changes: 6 additions & 1 deletion tests/smoke/hello_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -22,10 +22,15 @@ func TestHelloBundle(t *testing.T) {

test.PrepareTestBundle()
require.NoError(t, shx.Copy("testdata/buncfg.json", test.TestDir))
require.NoError(t, shx.Copy("testdata/plugins.yaml", test.TestDir))
test.Chdir(test.TestDir)

// Verify plugins installation
_, output := test.RequirePorter("plugins", "install", "-f", "plugins.yaml")
require.Contains(t, output, "installed azure plugin", "expected to see plugin successfully installed")

// Run a stateless action before we install and make sure nothing is persisted
_, output := test.RequirePorter("invoke", testdata.MyBuns, "--action=dry-run", "--reference", testdata.MyBunsRef, "-c=mybuns")
_, output = test.RequirePorter("invoke", testdata.MyBuns, "--action=dry-run", "--reference", testdata.MyBunsRef, "-c=mybuns")
t.Log(output)
test.RequireInstallationNotFound(test.CurrentNamespace(), testdata.MyBuns)

Expand Down
4 changes: 4 additions & 0 deletions tests/smoke/testdata/plugins.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
schemaType: Plugins
schemaVersion: 1.0.0
plugins:
azure: {}