validation library #3

estroz · 2019-09-24T17:50:02Z

Moved from dweepgogia/new-manifest-verification.

Implementation of the library described in #2.

Notes:

See my comment about api repo structure, which is not followed here but could/should be.
Quite a bit more validation logic needs to be ported from operator-courier.

/cc @gallettilance @shawn-hurley @ecordell

- Checking CSV yaml for data type mismatch against OLM's CSV type

- Adding Command Line Tool for the library

- To avoid "usage" and "flags" getting printed every time there's an error

- For reporting warnings and errors in case of optional and mandatory fields missing, respectively. - To enable users to extract more information from an error/warning.

validation logic, refactoring around interface cmd/verify.go: Verify() name change

- To implement a generic custom error type which is also compatible with upstream.

- To have all the csv checks in one file (csv_validator.go).

- To avoid redundancy while using the cli (operator-verify manifest) instead of (operator-verify verify).

- To verify the operator manifest.

- To compare example annotations to the provided APIs list and report appropriate errors.

- To check for the manifest format and validate the manifest and individual operator files.

- To support validating the CRDs present in the manifest.

- To support validating package yaml present in the manifest.

- To perform validation across manifest files.

- To improve the design.

awgreene

A number of nits - but the logic seems good.

README.md

awgreene · 2019-09-25T15:33:42Z

cmd/operator-verify/main.go

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


Is this newline necessary?

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

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?

cmd/operator-verify/main.go

awgreene · 2019-09-25T15:35:21Z

cmd/operator-verify/manifests/cmd.go

+	"os"
+
+	"github.com/operator-framework/api/pkg/manifests"
+


Is this newline necessary?

cmd/operator-verify/manifests/cmd.go

pkg/validation/validation.go

pkg/validation/internal/test_suite.go

awgreene · 2019-09-25T17:34:56Z

pkg/validation/internal/package_manifest.go

+	seen := map[string]struct{}{}
+	for i, c := range pkg.Channels {
+		if c.Name == "" {
+			errs = append(errs, errors.ErrInvalidPackageManifest(fmt.Sprintf("channel %d name is empty", i), pkg.PackageName))


fmt.Sprintf("channel %d name is empty", i), pkg.PackageName) isn't i just a integer for each channel?

Yes, here I assume there's no other identifying information for a channel so channel number is the only piece of information available to identify which channel is invalid.

njhale

Thanks for adding this! It's looking good so far. I was only able to get half way through, but I figured it was a good place to stop and get some questions out.

README.md

cmd/operator-verify/main.go

go.mod

pkg/validation/errors/error.go

pkg/validation/interfaces/validator.go

pkg/validation/internal/bundle.go

go.mod

njhale · 2019-10-03T14:20:54Z

go.mod

+	github.com/ghodss/yaml v1.0.0
+	github.com/hashicorp/golang-lru v0.5.3 // indirect
+	github.com/modern-go/concurrent v0.0.0-20180306012644-bacd9c7ef1dd // indirect
+	github.com/operator-framework/operator-lifecycle-manager v0.0.0-20190725173916-b56e63a643cc


Can we avoid the OLM dependency? Eventually all the types could live in this repo anyway, and it would help to avoid circular dependency issues when OLM inevitably imports validation.

I would have to duplicate the ClusterServiceVersion and related types here to remove this dependency. Are you ok with this?

We may have to do the same thing for operator-registry (registry.Bundle and some parsing types).

That was the initial idea. Ideally, as soon as we moved something to this repo, we'd switch over in OLM. That whole plan has been put on hold since it makes releasing API changes more difficult; e.g. we would need to release API changes ahead of the controller logic that handles them.

@ecordell, I think we need to make a decision ASAP to either move validation to its own repo or move the OLM APIs here.

pkg/internal/bundle.go

njhale · 2019-10-03T14:31:06Z

pkg/internal/bundle.go

+func (l *manifestsLoad) populate() error {
+	dl := sqlite.NewSQLLoaderForDirectory(l, l.dir)
+	if err := dl.Populate(); err != nil {
+		return errors.Wrapf(err, "error getting bundles from manifests dir %q", l.dir)


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

njhale · 2019-10-03T14:33:37Z

pkg/internal/bundle.go

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


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.

pkg/validation/interfaces/validator.go

njhale · 2019-10-28T14:57:56Z

@kevinrizza @ecordell, did we decide if we're abandoning this repo? If so, we should make plans to move this PR into its own validation repo or OLM proper.

(hopefully this question doesn't rekindle the entire conversation...)

kevinrizza · 2019-10-28T15:37:00Z

@kevinrizza @ecordell, did we decide if we're abandoning this repo? If so, we should make plans to move this PR into its own validation repo or OLM proper.

(hopefully this question doesn't rekindle the entire conversation...)

There is an active effort to create an API version for the entire operator-framework. I think once that conversation gets further along we may end up wanting to merge this (either in the current state or otherwise) so for now maybe let's just leave it open and reevaluate later?

njhale · 2019-10-28T16:57:43Z

There is an active effort to create an API version for the entire operator-framework. I think once that conversation gets further along we may end up wanting to merge this (either in the current state or otherwise) so for now maybe let's just leave it open and reevaluate later?

Sure, we can leave this here for now -- my comment was spurred by the decision to place net-new OLM APIs directly in its repository (instead of here).

estroz · 2019-10-30T19:51:53Z

@njhale @awgreene comments addressed, PTAL

njhale

Thanks again for your patience. I'm going to be in transit soon, so I decided to post the first half of the review in advance. It's looking solid so far, mostly just nits and questions

README.md

go.mod

njhale · 2019-11-06T21:12:01Z

go.mod

+	github.com/ghodss/yaml v1.0.0
+	github.com/hashicorp/golang-lru v0.5.3 // indirect
+	github.com/modern-go/concurrent v0.0.0-20180306012644-bacd9c7ef1dd // indirect
+	github.com/operator-framework/operator-lifecycle-manager v0.0.0-20190725173916-b56e63a643cc


That was the initial idea. Ideally, as soon as we moved something to this repo, we'd switch over in OLM. That whole plan has been put on hold since it makes releasing API changes more difficult; e.g. we would need to release API changes ahead of the controller logic that handles them.

@ecordell, I think we need to make a decision ASAP to either move validation to its own repo or move the OLM APIs here.

go.mod

njhale · 2019-11-06T21:31:15Z

pkg/internal/bundle.go

+	return l.pkg
+}
+
+func (l manifests) GetBundles() (bundles []*registry.Bundle) {


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.

njhale · 2019-11-06T22:11:04Z

pkg/validation/internal/csv.go

+	"k8s.io/apimachinery/pkg/util/yaml"
+)
+
+type CSVValidator struct{}


nit: If you make a function type that implements Validator, then you won't need this empty struct. A good example is in the go standard library; see https://golang.org/src/net/http/server.go?s=61509:61556#L1993

So something like

type csvv1alpha1Validator func(*v1alpha1.ClusterServiceVersion) errors.ManifestResult func (f csvv1alpha1Validator) Validate(objs ...interface{}) (results []errors.ManifestResult) { for _, obj := range objs { switch v := obj.(type) { case *v1alpha1.ClusterServiceVersion: results = append(results, f(v)) } } return results }

Then export functions that actually validate the CSV object:

var Validatev1alpha1CSV csvv1alpha1Validator = func(*v1alpha1.ClusterServiceVersion) errors.ManifestResult { ... }

Almost. What I'm proposing just allows functions to implement the Validator interface:

type ValidatorFunc Validate(...interface{}) []errors.ManifestResult func (v ValidatorFunc) Validate(objs ...interface{}) (results []errors.ManifestResult) { return v(objs...) } var CSVValidator Validator = ValidatorFunc(validateCSVs) func validateCSVs(objs ...interface{}) (results []errors.ManifestResult) { for _, obj := range objs { switch v := obj.(type) { case *v1alpha1.ClusterServiceVersion: results = append(results, validateCSV(v)) case *registry.ClusterServiceVersion: results = append(results, validateCSVRegistry(v)) } } return results }

pkg/validation/internal/csv.go

njhale · 2019-11-06T22:40:41Z

pkg/validation/internal/csv.go

+
+		// Omitted field tags will contain ",omitempty", and ignored tags will
+		// match "-" exactly, respectively.
+		isOptionalField := strings.Contains(tag, ",omitempty") || tag == "-"


follow-up: If we could somehow get the OpenAPI spec for CRDs not included in the bundle, could we use a pre-existing schema validator to handle checking for missing required fields?

Can you elaborate on what you mean by "OpenAPI spec for CRDs not included in the bundle" a bit? What manifest kind are we checking for missing fields in the context of your question?

One example would be the OpenAPI spec for a Deployment. We would validate the manifest against the OpenAPI schema, which would tell us when fields are missing (due to required directives, etc...). Does that make more sense?

Yes that does. I will implement this in a follow-up.

pkg/validation/internal/csv.go

gallettilance

Looks great!!
Just a few questions (could be a follow up PR). Also we still need to resolve where the OLM APIs live

gallettilance · 2019-11-07T14:30:24Z

README.md

+
+## `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.


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

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

This might make a nice follow-up.

gallettilance · 2019-11-07T14:36:32Z

cmd/operator-verify/main.go

+		Short: "Operator manifest validation tool",
+		Long:  `operator-verify is a CLI tool that calls functions in pkg/validation.`,
+	}
+


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

I also think this can be a follow-up.

gallettilance · 2019-11-07T14:41:56Z

pkg/internal/bundle.go

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


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.

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.

njhale

Here's the rest of my review. Mostly follow-ups and nits:

pkg/validation/internal/csv.go

njhale · 2019-11-07T12:29:04Z

pkg/validation/internal/manifests.go

+	"github.com/operator-framework/operator-registry/pkg/registry"
+)
+
+type ManifestsValidator struct{}


PackageUpdateGraphValidator?

I suppose this is a fairer name given this validator's current logic. However I'd rather keep the name general in case the constituent manifest types of a manifests directory are changed, or other, non-update graph validation logic is added in the future. How about VersionedManifestSetValidator or ManifestsDirValidator?

Alternatively we could add future logic as described above in another validator and reserve this one for validating the update graph.

Alternatively we could add future logic as described above in another validator and reserve this one for validating the update graph.

This is more where my head is at, since I view validators as composable units that form larger validators (if Validators implemented Validator). I can also see where it can be preferable to make larger units to avoid duplication of work or handle interdependencies. If that's the case, then either of the names you've suggested are fine with me.

Ehh because this code is only validating the update graph right now, we can name this PackageUpdateGraphValidator and add another validator later. These names are subject to change since they're internal types.

pkg/validation/internal/manifests.go

pkg/validation/internal/test_suite.go

pkg/validation/validation.go

errors.ManifestResult. pkg/validation/internal: default validator implementations pkg/validation/errors: result and error types for validators pkg/internal: bundle parser pkg/manifests: GetManifestsDir() parses and validates a directory of Operator manifests *: update import paths to operator-framework/api go.*: revendor README.md: concise description of pkg/validation

njhale

It's super close. I just don't want to merge it now only to move it to another repo immediately after. A big chunk of our team is out, so I'll confer with @shawn-hurley and @dmesser soon to settle things.

njhale · 2019-11-13T05:20:52Z

pkg/validation/internal/csv.go

+	"k8s.io/apimachinery/pkg/util/yaml"
+)
+
+type CSVValidator struct{}


Almost. What I'm proposing just allows functions to implement the Validator interface:

type ValidatorFunc Validate(...interface{}) []errors.ManifestResult func (v ValidatorFunc) Validate(objs ...interface{}) (results []errors.ManifestResult) { return v(objs...) } var CSVValidator Validator = ValidatorFunc(validateCSVs) func validateCSVs(objs ...interface{}) (results []errors.ManifestResult) { for _, obj := range objs { switch v := obj.(type) { case *v1alpha1.ClusterServiceVersion: results = append(results, validateCSV(v)) case *registry.ClusterServiceVersion: results = append(results, validateCSVRegistry(v)) } } return results }

pkg/validation/internal/csv.go

pkg/validation/validation.go

njhale · 2019-11-13T05:45:34Z

pkg/validation/internal/manifests.go

+	"github.com/operator-framework/operator-registry/pkg/registry"
+)
+
+type ManifestsValidator struct{}


Alternatively we could add future logic as described above in another validator and reserve this one for validating the update graph.

This is more where my head is at, since I view validators as composable units that form larger validators (if Validators implemented Validator). I can also see where it can be preferable to make larger units to avoid duplication of work or handle interdependencies. If that's the case, then either of the names you've suggested are fine with me.

…pend non-default Validators to the Validator; ValidatorFunc implements Validator

njhale · 2019-11-25T18:46:12Z

/hold cancel
/lgtm

dweepgogia and others added 18 commits August 26, 2019 16:20

[lib] Adding serialization for CSV type

66d1dd8

- Checking CSV yaml for data type mismatch against OLM's CSV type

[cli] Adding verify command

41cd8df

- Adding Command Line Tool for the library

[cli] Handling error reporting through cli

d0170bc

- To avoid "usage" and "flags" getting printed every time there's an error

[lib] Handling mandatory/optional fields and reporting custom errors

0953276

- For reporting warnings and errors in case of optional and mandatory fields missing, respectively. - To enable users to extract more information from an error/warning.

pkg/validate: add Validator interface to add k8s objects and run

2b156e3

validation logic, refactoring around interface cmd/verify.go: Verify() name change

refactor Validator and error/result types into pkg/validate/validator

d80f5d6

fix comment

f527665

go.mod,go.sum: add modfiles

ccf242d

[lib] refactoring the library

3638614

- To implement a generic custom error type which is also compatible with upstream.

[lib] refactor logic for csv validation

5efb42a

- To have all the csv checks in one file (csv_validator.go).

[cli] change from verify to manifest command

03c49b5

- To avoid redundancy while using the cli (operator-verify manifest) instead of (operator-verify verify).

[lib] add support for handling manifest

a5bb22e

- To verify the operator manifest.

[lib] csv validator for example annotations

968a8b4

- To compare example annotations to the provided APIs list and report appropriate errors.

[lib] parse operator manifest directory

ffa1afa

- To check for the manifest format and validate the manifest and individual operator files.

[lib] add crd validator

c7ab3cb

- To support validating the CRDs present in the manifest.

[lib] add package validator

9d8d567

- To support validating package yaml present in the manifest.

[lib] add bundle validator

8ee9933

- To perform validation across manifest files.

[lib] add two methods to validator interface

3fe9a43

- To improve the design.

openshift-ci-robot requested review from ecordell, gallettilance and shawn-hurley September 24, 2019 17:50

openshift-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Sep 24, 2019

awgreene requested changes Sep 26, 2019

View reviewed changes

njhale reviewed Sep 26, 2019

View reviewed changes

openshift-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 26, 2019

njhale reviewed Oct 3, 2019

View reviewed changes

go.mod Show resolved Hide resolved

njhale reviewed Oct 3, 2019

View reviewed changes

njhale mentioned this pull request Oct 28, 2019

Add Marketplace apis #1

Closed

estroz mentioned this pull request Oct 30, 2019

doc/proposals: proposal template and validator library proposal #2

Closed

camilamacedo86 mentioned this pull request Nov 1, 2019

feat(olm-catalog) : add missing fields with placehoders in the CSV operator-framework/operator-sdk#2095

Closed

njhale reviewed Nov 6, 2019

View reviewed changes

estroz force-pushed the validator-interface branch from f23e367 to 689a491 Compare November 7, 2019 00:27

gallettilance approved these changes Nov 7, 2019

View reviewed changes

njhale reviewed Nov 7, 2019

View reviewed changes

estroz force-pushed the validator-interface branch from 4a3cc94 to 02fc1ea Compare November 9, 2019 02:46

estroz force-pushed the validator-interface branch from 02fc1ea to 439c769 Compare November 9, 2019 03:01

njhale reviewed Nov 13, 2019

View reviewed changes

pkg/validation/interfaces: Validator's implement WithValidators to ap…

02bfec5

…pend non-default Validators to the Validator; ValidatorFunc implements Validator

estroz changed the title ~~[WIP] validation library~~ validation library Nov 15, 2019

openshift-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Nov 15, 2019

estroz requested review from njhale and awgreene November 15, 2019 18:39

openshift-ci-robot assigned njhale Nov 25, 2019

openshift-ci-robot added lgtm Indicates that a PR is ready to be merged. and removed do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. labels Nov 25, 2019

njhale merged commit 94d8034 into operator-framework:master Nov 25, 2019

		"os"

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


		## `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.

validation library #3

validation library #3

Uh oh!

Conversation

estroz commented Sep 24, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

awgreene left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

njhale left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

njhale commented Oct 28, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kevinrizza commented Oct 28, 2019

Uh oh!

njhale commented Oct 28, 2019

Uh oh!

estroz commented Oct 30, 2019

Uh oh!

njhale left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

estroz Nov 7, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

estroz commented Sep 24, 2019 •

edited

Loading

njhale commented Oct 28, 2019 •

edited

Loading

estroz Nov 7, 2019 •

edited

Loading

njhale Nov 6, 2019 •

edited

Loading