Skip to content

validation library #3

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

Merged
merged 20 commits into from
Nov 25, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
20 commits
Select commit Hold shift + click to select a range
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
10 changes: 9 additions & 1 deletion README.md
Original file line number Diff line number Diff line change
@@ -1,2 +1,10 @@
# api
Contains the API definitions used by OLM and Marketplace

Contains the API definitions used by [Operator Lifecycle Manager][olm] (OLM) and [Marketplace Operator][marketplace]

## `pkg/validation`: Operator Manifest Validation

`pkg/validation` exposes a convenient set of interfaces to validate Kubernetes object manifests, primarily for use in an Operator project.
Copy link
Member

Choose a reason for hiding this comment

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

What do you think of adding some examples? This could be in a separate PR/folder

Copy link
Member

Choose a reason for hiding this comment

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

I'm thinking examples of implementing validators / using the apis

Copy link
Member

Choose a reason for hiding this comment

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

This might make a nice follow-up.


[olm]:https://github.com/operator-framework/operator-lifecycle-manager
[marketplace]:https://github.com/operator-framework/operator-marketplace
24 changes: 24 additions & 0 deletions cmd/operator-verify/main.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
package main

import (
"fmt"
"os"

manifests "github.com/operator-framework/api/cmd/operator-verify/manifests"

Copy link
Member

Choose a reason for hiding this comment

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

Is this newline necessary?

Copy link
Member Author

Choose a reason for hiding this comment

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

No, but I prefer to separate intra-repo from extra-repo imports. I can merge them all if you prefer.

Copy link
Member

Choose a reason for hiding this comment

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

In OLM we use goimports which actually does something similar, just with the position of the intra/extra imports flipped from what you have here. Do we want to make it the org wide formatter?

"github.com/spf13/cobra"
)

func main() {
rootCmd := &cobra.Command{
Use: "operator-verify",
Short: "Operator manifest validation tool",
Long: `operator-verify is a CLI tool that calls functions in pkg/validation.`,
}

Copy link
Member

Choose a reason for hiding this comment

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

what do you think of adding an option to output the resulting errors/warnings to a log file - I think this was a use case that Ralph or Scott had with operator courier

Copy link
Member

Choose a reason for hiding this comment

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

I also think this can be a follow-up.

rootCmd.AddCommand(manifests.NewCmd())
if err := rootCmd.Execute(); err != nil {
fmt.Println(err)
os.Exit(1)
}
}
40 changes: 40 additions & 0 deletions cmd/operator-verify/manifests/cmd.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,40 @@
package manifests

import (
"fmt"
"os"

"github.com/operator-framework/api/pkg/manifests"
"github.com/operator-framework/api/pkg/validation/errors"

Copy link
Member

Choose a reason for hiding this comment

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

Is this newline necessary?

log "github.com/sirupsen/logrus"
"github.com/spf13/cobra"
)

func NewCmd() *cobra.Command {
return &cobra.Command{
Use: "manifests",
Short: "Validates all manifests in a directory",
Long: `'operator-verify manifests' validates all manifests in the supplied directory
and prints errors and warnings corresponding to each manifest found to be
invalid. Manifests are only validated if a validator for that manifest
type/kind, ex. CustomResourceDefinition, is implemented in the Operator
validation library.`,
Run: func(cmd *cobra.Command, args []string) {
if len(args) != 1 {
log.Fatalf("command %s requires exactly one argument", cmd.CommandPath())
}
_, _, results := manifests.GetManifestsDir(args[0])
nonEmptyResults := []errors.ManifestResult{}
for _, result := range results {
if result.HasError() || result.HasWarn() {
nonEmptyResults = append(nonEmptyResults, result)
}
}
if len(nonEmptyResults) != 0 {
fmt.Println(nonEmptyResults)
os.Exit(1)
}
},
}
}
38 changes: 38 additions & 0 deletions go.mod
Original file line number Diff line number Diff line change
@@ -0,0 +1,38 @@
module github.com/operator-framework/api

go 1.12

replace (
// Pin openshift version to 4.2 (uses kube 1.14)
github.com/openshift/api => github.com/openshift/api v3.9.1-0.20190717200738-0390d1e77d64+incompatible
github.com/openshift/client-go => github.com/openshift/client-go v0.0.0-20190627172412-c44a8b61b9f4

// Pin kube version to 1.14
k8s.io/api => k8s.io/api v0.0.0-20190704095032-f4ca3d3bdf1d
k8s.io/apiextensions-apiserver => k8s.io/apiextensions-apiserver v0.0.0-20190704104557-6209bbe9f7a9
k8s.io/apimachinery => k8s.io/apimachinery v0.0.0-20190704094733-8f6ac2502e51
k8s.io/apiserver => k8s.io/apiserver v0.0.0-20190704101451-e5f5c6e528cd
k8s.io/client-go => k8s.io/client-go v11.0.1-0.20190521191137-11646d1007e0+incompatible
k8s.io/code-generator => k8s.io/code-generator v0.0.0-20190704094409-6c2a4329ac29
k8s.io/component-base => k8s.io/component-base v0.0.0-20190704100636-f0322db00a10
k8s.io/kube-aggregator => k8s.io/kube-aggregator v0.0.0-20190704101955-e796fd6d55e0
k8s.io/kube-openapi => k8s.io/kube-openapi v0.0.0-20190228160746-b3a7cee44a30
k8s.io/kubernetes => k8s.io/kubernetes v1.14.5-beta.0.0.20190708100021-7936da50c68f
sigs.k8s.io/structured-merge-diff => sigs.k8s.io/structured-merge-diff v0.0.0-20190302045857-e85c7b244fd2
)

require (
github.com/blang/semver v3.5.1+incompatible
github.com/ghodss/yaml v1.0.0
github.com/golang/groupcache v0.0.0-20191027212112-611e8accdfc9 // indirect
github.com/hashicorp/golang-lru v0.5.3 // indirect
github.com/operator-framework/operator-lifecycle-manager v0.0.0-20190926160646-a61144936680
github.com/operator-framework/operator-registry v1.5.3
github.com/pkg/errors v0.8.1
github.com/sirupsen/logrus v1.4.2
github.com/spf13/cobra v0.0.5
github.com/stretchr/testify v1.3.0
k8s.io/apiextensions-apiserver v0.0.0-20190918161926-8f644eb6e783
k8s.io/apimachinery v0.0.0-20190913080033-27d36303b655
k8s.io/client-go v8.0.0+incompatible
)
560 changes: 560 additions & 0 deletions go.sum

Large diffs are not rendered by default.

134 changes: 134 additions & 0 deletions pkg/internal/bundle.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,134 @@
package manifests

import (
"encoding/json"

"github.com/blang/semver"
operatorsv1alpha1 "github.com/operator-framework/operator-lifecycle-manager/pkg/api/apis/operators/v1alpha1"
"github.com/operator-framework/operator-registry/pkg/registry"
"github.com/operator-framework/operator-registry/pkg/sqlite"
"github.com/pkg/errors"
)

// TODO: use internal version of registry.Bundle/registry.PackageManifest so
// operator-registry can use validation library.

// manifestsLoad loads a manifests directory from disk.
type manifestsLoad struct {
dir string
pkg registry.PackageManifest
bundles map[string]*registry.Bundle
}

// Ensure manifestsLoad implements registry.Load.
var _ registry.Load = &manifestsLoad{}

// populate uses operator-registry's sqlite.NewSQLLoaderForDirectory to load
// l.dir's manifests. Note that this method does not call any functions that
// use SQL drivers.
func (l *manifestsLoad) populate() error {
loader := sqlite.NewSQLLoaderForDirectory(l, l.dir)
if err := loader.Populate(); err != nil {
return errors.Wrapf(err, "error getting bundles from manifests dir %q", l.dir)
Copy link
Member

Choose a reason for hiding this comment

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

Can we capitalize the first word in all sentences that we log? Applies in all places.

Copy link
Member

Choose a reason for hiding this comment

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

In OLM we actually do the opposite -- but we could totally review that convention if you want @awgreene

}
return nil
}

// AddOperatorBundle adds a bundle to l.
func (l *manifestsLoad) AddOperatorBundle(bundle *registry.Bundle) error {
csvRaw, err := bundle.ClusterServiceVersion()
if err != nil {
return errors.Wrap(err, "error getting bundle CSV")
}
csvSpec := operatorsv1alpha1.ClusterServiceVersionSpec{}
if err := json.Unmarshal(csvRaw.Spec, &csvSpec); err != nil {
return errors.Wrap(err, "error unmarshaling CSV spec")
}
bundle.Name = csvSpec.Version.String()
l.bundles[csvSpec.Version.String()] = bundle
return nil
}

// AddOperatorBundle adds the package manifest to l.
func (l *manifestsLoad) AddPackageChannels(pkg registry.PackageManifest) error {
Copy link
Member

Choose a reason for hiding this comment

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

It does not appear that an error can be returned by this method.

Copy link
Member

Choose a reason for hiding this comment

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

This type needs to implement this exact method signature in order to implement the registry.Load interface, which includes the error in case an implementation needs to report that the operation failed in some way.

l.pkg = pkg
return nil
}

// AddBundlePackageChannels is a no-op to implement the registry.Load interface.
func (l *manifestsLoad) AddBundlePackageChannels(manifest registry.PackageManifest, bundle registry.Bundle) error {
Copy link
Member

Choose a reason for hiding this comment

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

Note to self: As we change operator-registry we'll need to be aware of the dependency here on the implementation of its Load interface.

Copy link
Member Author

Choose a reason for hiding this comment

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

In general we're going to have to do something about operator-registry types. If we want this to be a dependency of operator-registry, we're either going to have to copy over types or copy over parse code and use internal types. WDYT? We can probably clean this up in a follow-up PR.

return nil
}

// RmPackageName is a no-op to implement the registry.Load interface.
func (l *manifestsLoad) RmPackageName(packageName string) error {
return nil
}

// ClearNonDefaultBundles is a no-op to implement the registry.Load interface.
func (l *manifestsLoad) ClearNonDefaultBundles(packageName string) error {
return nil
}

// ManifestsStore knows how to query for an operator's package manifest and
// related bundles.
type ManifestsStore interface {
// GetPackageManifest returns the ManifestsStore's registry.PackageManifest.
// The returned object is assumed to be valid.
GetPackageManifest() registry.PackageManifest
// GetBundles returns the ManifestsStore's set of registry.Bundle. These
// bundles are unique by CSV version, since only one operator type should
// exist in one manifests dir.
// The returned objects are assumed to be valid.
GetBundles() []*registry.Bundle
// GetBundleForVersion returns the ManifestsStore's registry.Bundle for a
// given version string. An error should be returned if the passed version
// does not exist in the store.
// The returned object is assumed to be valid.
GetBundleForVersion(string) (*registry.Bundle, error)
}

// manifests implements ManifestsStore
type manifests struct {
pkg registry.PackageManifest
bundles map[string]*registry.Bundle
}

// ManifestsStoreForDir populates a ManifestsStore from the metadata in dir.
// Each bundle and the package manifest are statically validated, and will
// return an error if any are not valid.
func ManifestsStoreForDir(dir string) (ManifestsStore, error) {
load := &manifestsLoad{
dir: dir,
bundles: map[string]*registry.Bundle{},
}
if err := load.populate(); err != nil {
return nil, err
}
return &manifests{
pkg: load.pkg,
bundles: load.bundles,
}, nil
}

func (l manifests) GetPackageManifest() registry.PackageManifest {
return l.pkg
}

func (l manifests) GetBundles() (bundles []*registry.Bundle) {
Copy link
Member

Choose a reason for hiding this comment

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

Couldn't the bundle values contained in the bundles array be changed since they are pointers?

Copy link
Member Author

Choose a reason for hiding this comment

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

operator-registry uses *Bundle, a convention I stuck with here. Also this is an internal package so I figured pointer vs. value wouldn't matter much. Should I make this a slice of values?

Copy link
Member

Choose a reason for hiding this comment

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

This was probably for convenience since the method receivers are of type *registry.Bundle. This lets us avoid needing to get the pointer for each element every time we want to call a method.

for _, bundle := range l.bundles {
bundles = append(bundles, bundle)
}
return bundles
}

func (l manifests) GetBundleForVersion(version string) (*registry.Bundle, error) {
if _, err := semver.Parse(version); err != nil {
return nil, errors.Wrapf(err, "error getting bundle for version %q", version)
Copy link
Member

Choose a reason for hiding this comment

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

Could you share what this error may look like?

Copy link
Member Author

Choose a reason for hiding this comment

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

Any error from Parse(). This invocation is done to validate the version string. We could relax this check if we do not always expect a bundle to be versioned with semver. The way I'm indexing CSV's in this parser is by CSV spec.version.

Copy link
Member

Choose a reason for hiding this comment

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

spec.version is not a required field yet. It will probably be soon, so I think this is okay.

}
bundle, ok := l.bundles[version]
if !ok {
return nil, errors.Errorf("bundle for version %q does not exist", version)
}
return bundle, nil
}
31 changes: 31 additions & 0 deletions pkg/manifests/directory.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,31 @@
package manifests

import (
"fmt"

internal "github.com/operator-framework/api/pkg/internal"
"github.com/operator-framework/api/pkg/validation"
"github.com/operator-framework/api/pkg/validation/errors"

Copy link
Member

Choose a reason for hiding this comment

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

Please remove this newline.

"github.com/operator-framework/operator-registry/pkg/registry"
)

// GetManifestsDir parses all bundles and a package manifest from dir, which
// are returned if found along with any errors or warnings encountered while
// parsing/validating found manifests.
func GetManifestsDir(dir string) (registry.PackageManifest, []*registry.Bundle, []errors.ManifestResult) {
manifests, err := internal.ManifestsStoreForDir(dir)
if err != nil {
result := errors.ManifestResult{}
result.Add(errors.ErrInvalidParse(fmt.Sprintf("parse manifests from %q", dir), err))
return registry.PackageManifest{}, nil, []errors.ManifestResult{result}
}
pkg := manifests.GetPackageManifest()
bundles := manifests.GetBundles()
objs := []interface{}{}
for _, obj := range bundles {
objs = append(objs, obj)
}
results := validation.AllValidators.Validate(objs...)
return pkg, bundles, results
}
15 changes: 15 additions & 0 deletions pkg/validation/doc.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
// This package defines the valid Operator manifests directory format
// by exposing a set of Validator's to verify a directory and
// its constituent manifests. A manifests directory consists of a
// package manifest and a set of versioned Bundles. Each Bundle contains a
// ClusterServiceVersion and one or more CustomResourceDefinition's.
//
// Errors and warnings, both represented by the Error type, are returned
// by exported functions for missing mandatory and optional fields,
// respectively. Each Error implements the error interface.
//
// Manifest and Bundle format: https://github.com/operator-framework/operator-registry/#manifest-format
// ClusterServiceVersion documentation: https://github.com/operator-framework/operator-lifecycle-manager/blob/master/Documentation/design/building-your-csv.md
// Package manifest documentation: https://github.com/operator-framework/operator-lifecycle-manager#discovery-catalogs-and-automated-upgrades
// CustomResourceDefinition documentation: https://kubernetes.io/docs/tasks/access-kubernetes-api/custom-resources/custom-resource-definitions/
package validation
Loading