Add bundle contents validation #146

exdx · 2020-01-06T22:42:15Z

Description of the change:
Add validation of bundle contents as part of the registry epic. Makes use of the https://github.com/operator-framework/api library for core validation logic. Meant to run along with the bundle image validation. Currently only CSV and CRD manifests are fully validated, other types are simply vetted as valid kube objects.

Signed-off-by: Vu Dinh vdinh@redhat.com
Motivation for the change:

Reviewer Checklist

Implementation matches the proposed design, or proposal is updated to match implementation
Sufficient unit test coverage
Sufficient end-to-end test coverage
Docs updated or added to /docs
Commit messages sensible and descriptive

exdx · 2020-01-06T22:45:57Z

One thing I've noticed is the api validation library throws a fatal error if the CSV installMode is not explicitly set. I think this is the desired behavior, but all our example manifests including the CSVs in this repo do not have an install mode set. The previous PR on bundle image validation included a valid prometheus CSV that was considered invalid by the api library.

exdx · 2020-01-13T15:31:59Z

I think we need to add installMode to the example bundles in this repo (and probably elsewhere) to ensure that our validation passes for our own examples per @ecordell

exdx · 2020-01-13T16:00:55Z

After meeting with the sdk team we want to decouple the image validation and contents validation so that the SDK can import the operator-framework/API directly.
We should

Make a validate bundle command that does not validate image contents (for SDK)
Make a validate bundle command that validates image and image contents (for opm alpha)
cc @kevinrizza

ecordell · 2020-01-15T13:38:04Z

pkg/lib/bundle/validate.go

+		serviceKind:            "",
+		serviceAccountKind:     "",
+		roleKind:               "",
+		roleBindingKind:        "",


This will also need to include the prometheus types, see: operator-framework/operator-lifecycle-manager#1227

can we export these somehow rather than hardcoding them?

even if that is in the API package and not in the registry? the more things like this that we start adding everywhere the harder it is going to be to change them.

At the moment, there is nothing from OLM or registry to export. For OLM, we use a switch statement to handle all supported types. What OLM supports is a selected set of kube objects so there is no standard or logic for how we pick and choose.
I can create some struct with supported types and a function that validates if the type is supported. However, it is still some sort of "hardcoded".

sure yeah, I undestand that. i mostly just mean now we have to keep track of this in two places and when the list updates remember to update them in both. what happens when the API project wants its own version of this validation for static analysis outside the scope of pulling down the image? Then the list is in 3 places.

not sure if we can solve that here right now, but i think this is a problem that we are facing sooner rather than later for a lot of things that are going to go across all of these projects. what is our import relationship between all of these repositories? or are we trying to explicitly avoid that completely by reimplementing the apis everywhere?

I agree. There is a need to consolidate some agreements between OLM and registry. I would expect this change to begin with OLM. OLM needs to define a set of kube objects that it is supporting in a way that registry can import and then registry will enforce it during the bundle packaging/building.

ecordell · 2020-01-15T13:42:34Z

pkg/lib/bundle/validate_test.go

+		numErrors   int
+		errString   string
+	}{
+		{


a test case for "no csv"? It might change in the future, but for now that's a requirement.

I understand this case but not sure if it is helpful in this specific situation. If there is no CSV, it is considered PlainType which we don't validate the contents. So if it is not RegistryV1 format, the ValidateBundleContent func will just return nil.

pkg/lib/bundle/validate_test.go

pkg/lib/bundle/validate.go

cmd/opm/alpha/bundle/validate.go

1. Add bundle content validation 2. Add the bundle validation documentation 3. Add needed vendor code for validation Signed-off-by: Vu Dinh <vdinh@redhat.com>

…tion 1. Seperate bundle format and content validation into 2 seperate functions 2. Remove operator-sdk reference from registry docs. Using `opm` reference instead. 3. Add bundle object validation to improve content validation Signed-off-by: Vu Dinh <vdinh@redhat.com>

dinhxuanvu · 2020-01-16T18:56:49Z

/retest

exdx · 2020-01-21T17:16:53Z

/lgtm

openshift-ci-robot · 2020-01-21T17:16:54Z

@exdx: you cannot LGTM your own PR.

In response to this:

/lgtm

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

dinhxuanvu · 2020-01-21T18:29:41Z

@kevinrizza @ecordell Would like to have the final approval from you :). Thanks

estroz

Two suggestions.

pkg/lib/bundle/validate.go

kevinrizza · 2020-01-21T18:50:47Z

/lgtm

1. Improve errors handling 2. Flattering some nested conditional loops 3. Refactoring supported types 4. Handle plain type validation Signed-off-by: Vu Dinh <vdinh@redhat.com>

kevinrizza · 2020-01-21T18:58:13Z

/lgtm

openshift-ci-robot · 2020-01-21T19:53:14Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dinhxuanvu, exdx, kevinrizza

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [dinhxuanvu]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

openshift-ci-robot · 2020-01-21T19:53:15Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dinhxuanvu, exdx, kevinrizza

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [dinhxuanvu]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

exdx requested a review from dinhxuanvu January 6, 2020 22:42

openshift-ci-robot requested review from ecordell and jpeeler January 6, 2020 22:42

openshift-ci-robot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Jan 6, 2020

exdx requested review from njhale, ecordell and kevinrizza and removed request for ecordell and jpeeler January 6, 2020 22:42

exdx changed the title ~~feat: validate bundle contents~~ [WIP]feat: validate bundle contents Jan 7, 2020

openshift-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jan 7, 2020

exdx force-pushed the feat/validate-bundle-files branch from 6decc3e to a709e08 Compare January 7, 2020 16:46

dinhxuanvu force-pushed the feat/validate-bundle-files branch 4 times, most recently from 6d03ad6 to 701dc76 Compare January 13, 2020 11:28

dinhxuanvu changed the title ~~[WIP]feat: validate bundle contents~~ Add bundle contents validation Jan 13, 2020

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 Jan 13, 2020

dinhxuanvu force-pushed the feat/validate-bundle-files branch from f2adc0b to cbbe884 Compare January 15, 2020 07:50

ecordell reviewed Jan 15, 2020

View reviewed changes

exdx commented Jan 15, 2020

View reviewed changes

pkg/lib/bundle/validate.go Outdated Show resolved Hide resolved

exdx commented Jan 15, 2020

View reviewed changes

pkg/lib/bundle/validate.go Outdated Show resolved Hide resolved

exdx commented Jan 15, 2020

View reviewed changes

pkg/lib/bundle/validate.go Outdated Show resolved Hide resolved

exdx commented Jan 15, 2020

View reviewed changes

pkg/lib/bundle/validate.go Outdated Show resolved Hide resolved

estroz reviewed Jan 16, 2020

View reviewed changes

cmd/opm/alpha/bundle/validate.go Outdated Show resolved Hide resolved

openshift-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 16, 2020

dinhxuanvu force-pushed the feat/validate-bundle-files branch from 72a7841 to 8f88ed8 Compare January 16, 2020 16:07

dinhxuanvu force-pushed the feat/validate-bundle-files branch 3 times, most recently from 51024cf to 649aa9f Compare January 16, 2020 16:26

exdx and others added 2 commits January 16, 2020 13:30

Add validate bundle contents

7673f8e

1. Add bundle content validation 2. Add the bundle validation documentation 3. Add needed vendor code for validation Signed-off-by: Vu Dinh <vdinh@redhat.com>

dinhxuanvu force-pushed the feat/validate-bundle-files branch from 649aa9f to 8e47858 Compare January 16, 2020 18:33

openshift-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 16, 2020

dinhxuanvu force-pushed the feat/validate-bundle-files branch from 8e47858 to dc57d6e Compare January 16, 2020 19:42

estroz mentioned this pull request Jan 17, 2020

cmd/operator-sdk: add bundle validate command operator-framework/operator-sdk#2411

Merged

dinhxuanvu force-pushed the feat/validate-bundle-files branch from dc57d6e to 463dbaa Compare January 21, 2020 05:47

dinhxuanvu requested a review from ecordell January 21, 2020 17:33

estroz reviewed Jan 21, 2020

View reviewed changes

pkg/lib/bundle/validate.go Show resolved Hide resolved

pkg/lib/bundle/validate.go Show resolved Hide resolved

openshift-ci-robot assigned kevinrizza Jan 21, 2020

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jan 21, 2020

Address feedbacks from comments/reviews

0d31556

1. Improve errors handling 2. Flattering some nested conditional loops 3. Refactoring supported types 4. Handle plain type validation Signed-off-by: Vu Dinh <vdinh@redhat.com>

dinhxuanvu force-pushed the feat/validate-bundle-files branch from 463dbaa to 0d31556 Compare January 21, 2020 18:53

dinhxuanvu approved these changes Jan 21, 2020

View reviewed changes

openshift-ci-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. and removed lgtm Indicates that a PR is ready to be merged. labels Jan 21, 2020

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jan 21, 2020

openshift-merge-robot merged commit d8e2ec5 into operator-framework:master Jan 21, 2020

dinhxuanvu mentioned this pull request Jan 22, 2020

Use dynamic client to manage prometheus resources operator-framework/operator-lifecycle-manager#1227

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add bundle contents validation #146

Add bundle contents validation #146

exdx commented Jan 6, 2020 •

edited by dinhxuanvu

exdx commented Jan 6, 2020

exdx commented Jan 13, 2020

exdx commented Jan 13, 2020

ecordell Jan 15, 2020

kevinrizza Jan 15, 2020

kevinrizza Jan 15, 2020

dinhxuanvu Jan 15, 2020

kevinrizza Jan 15, 2020

dinhxuanvu Jan 15, 2020 •

edited

ecordell Jan 15, 2020

dinhxuanvu Jan 15, 2020

dinhxuanvu commented Jan 16, 2020

exdx commented Jan 21, 2020

openshift-ci-robot commented Jan 21, 2020

dinhxuanvu commented Jan 21, 2020

estroz left a comment

kevinrizza commented Jan 21, 2020

kevinrizza commented Jan 21, 2020

openshift-ci-robot commented Jan 21, 2020

openshift-ci-robot commented Jan 21, 2020

Add bundle contents validation #146

Add bundle contents validation #146

Conversation

exdx commented Jan 6, 2020 • edited by dinhxuanvu

exdx commented Jan 6, 2020

exdx commented Jan 13, 2020

exdx commented Jan 13, 2020

ecordell Jan 15, 2020

Choose a reason for hiding this comment

kevinrizza Jan 15, 2020

Choose a reason for hiding this comment

kevinrizza Jan 15, 2020

Choose a reason for hiding this comment

dinhxuanvu Jan 15, 2020

Choose a reason for hiding this comment

kevinrizza Jan 15, 2020

Choose a reason for hiding this comment

dinhxuanvu Jan 15, 2020 • edited

Choose a reason for hiding this comment

ecordell Jan 15, 2020

Choose a reason for hiding this comment

dinhxuanvu Jan 15, 2020

Choose a reason for hiding this comment

dinhxuanvu commented Jan 16, 2020

exdx commented Jan 21, 2020

openshift-ci-robot commented Jan 21, 2020

dinhxuanvu commented Jan 21, 2020

estroz left a comment

Choose a reason for hiding this comment

kevinrizza commented Jan 21, 2020

kevinrizza commented Jan 21, 2020

openshift-ci-robot commented Jan 21, 2020

openshift-ci-robot commented Jan 21, 2020

exdx commented Jan 6, 2020 •

edited by dinhxuanvu

dinhxuanvu Jan 15, 2020 •

edited