Scorecard2 stub test binary #2829

jmccormick2001 · 2020-04-13T18:15:21Z

Description of the change:
this PR stubs out the Basic and OLM tests into the scorecard2 alpha design, it includes a unit test and a unit test data directory, it also includes a stubbed out scorecard test binary which will eventually execute the tests within an image, for now it just runs the stubbed out tests from the command line.

The stubbed out tests produce a sample ScorecardTestResult that has a Pass state, eventually this will be replaced by a real test implementation.

Motivation for the change:

part of the overall scorecard2 implementation

joelanford · 2020-04-14T02:58:41Z

internal/scorecard/alpha/tests/basic_tests.go

+	r.Name = BasicCheckStatusTest
+	r.Description = "Custom Resource has a Status Block"


Will this cause competing name and description fields between the test image output and the test config file? We probably want one source of truth for each field in the --list output and actual result output.

We need to answer a fundamental question I think:

What guarantee, if any, should scorecard make that a test that claims a certain name is actually the test that the runner expects it to be?

Consider the scenario of a pipeline that implements a policy that says "require that 'basic-check-status' runs and passes. What stops someone from editing their config to use a different image for that test and having it output a result that says "basic-check-status" passed?

It seems like the only way for the runner to really be able to trust the results is for it to be able to trust the image and entrypoint, since it would be very difficult to impersonate an image registry and convince a pipeline cluster to run your image instead of the trusted image, especially if SHAs are used in the image reference.

So:

Is this scenario a real use-case?

Should we document that image reference and entrypoint are the fields that drive test policies and "name" and "description" are merely there for helpful context for the test?

Thoughts @shawn-hurley @dmesser @jmccormick2001?

the description in the config.yaml is probably redundant since its included into the ScorecardTestResult by the test itself, I'll remove that in a future PR. I suppose however, having it in the config file gives you the ability to customize the test description (e.g. Spanish translation). The name in the config file serves as the entrypoint so those match what the image is expecting or else the test image will not know what test to run.

I think documenting image/entrypoint fields makes sense (option 2). I was planning on doing the documentation in a separate PR/story currently.

I think the only way you can trust test results is to insure you are running a valid version of the test images or else you can spoof results, I'd consider that the domain of the pipeline/CI maintainers.

I think the only way you can trust test results is to insure you are running a valid version of the test images or else you can spoof results, I'd consider that the domain of the pipeline/CI maintainers.

But what is the method by with the pipeline/CI maintainers know which images are being used? With the current design, the pipeline runs operator-sdk scorecard --bundle <bundle> and then the output shows the results. The image and entrypoint are never surfaced to the caller.

WDYT about having the config file contribute some of the fields to the result output and the test image log contributing the test result fields:

config.yaml contributes:

image

entrypoint

labels

name

description

test log contributes:

state (pass/fail/error)

errors

suggestions

log

So with this --list would output just fields from config.yaml and running the test would output fields from both.

--list currently works off of the config file only, I was going to keep it that way....when the integration to the test image is coded, I think we'll need to decide to pull the name/description from either the config file or the image itself, not sure at this point.

Sounds good. I can see arguments for name and description come from either config or the log output. What I'm after is that we're crystal clear how that works, and we probably need to document it once we have a decision if it isn't immediately obvious.

images/scorecard-test/bin/user_setup

joelanford · 2020-04-14T03:04:19Z

internal/scorecard/alpha/tests/test_bundle.go

+	logrus.SetOutput(validationLogOutput)
+	defer logrus.SetOutput(origOutput)
+
+	cfg.PackageManifest, cfg.Bundles, cfg.BundleErrors = manifests.GetManifestsDir(bundlePath)


I could be wrong, but I think the bundlePath will look like this (from the OLM operator bundle enhancement proposal):

$ tree / ├── manifests │ ├── testbackup.crd.yaml │ ├── testcluster.crd.yaml │ ├── testoperator.v0.1.0.clusterserviceversion.yaml │ └── testrestore.crd.yaml └── metadata └── annotations.yaml

I think we need to find a different function that loads a single bundle from disk.

@estroz @kevinrizza @ecordell does something exist that turns a bundle directory like this into a registry.Bundle? I don't see anything here that seems to help, but I'm probably overlooking something.

If we have that, I think most of this file may go away. We could basically use registry.Bundle directly, possibly wrapping it to expose the CRs from the CSVs alm-examples.

This function, which is called by an SQLPopulator from operator-registry, will return a single registry.Bundle if bundlePath is a bundle directory, i.e. len(cfg.Bundles) == 1.

@estroz Does that help us? It seems like we're actually after is something like loadBundle, but for that, it seems like we would pass /bundle/manifests and then /bundle/metadata would be ignored.

We probably also don't want all the logrus logging.

The logging isn't great, and I have a PR upstream to configure it. For now turning it off when running GetManifestsDir should be enough. Do we care about metadata in this PR?

Maybe not. Just trying to capture the potential issues I see and want to make sure we don't let tech debt creep in from the beginning. Totally fine by me to add a TODO comment and a follow-on task to address this later.

I added a TODO around changing the get bundle API call, there is a story to handle the bundle image case so thats probably a good time to address this API.

internal/scorecard/alpha/testdata/memcached-operator.package.yaml

images/scorecard-test/bin/entrypoint

… another PR

joelanford · 2020-04-15T18:04:57Z

images/scorecard-test/cmd/test/main.go

+)
+
+func main() {
+	entrypoint := os.Args[1:]


Non-blocking nit: It would be helpful for this program to print out the full list of test names it supports if argv[0] is empty or unknown. Maybe add a TODO comment?

agree, I added a TODO to not forget it.

joelanford · 2020-04-15T18:09:00Z

internal/scorecard/alpha/testdata/0.0.1/cache.example.com_memcacheds_crd.yaml

@@ -0,0 +1,63 @@
+apiVersion: apiextensions.k8s.io/v1beta1


Do we want the directory structure for this testdata to match the bundle directory format we expect?

testdata └── bundle ├── manifests │ ├── cache.example.com_memcacheds_crd.yaml │ ├── memcached-operator.v0.0.1.clusterserviceversion.yaml │ └── memcached-operator.v0.0.1.clusterserviceversion.yaml.orig └── metadata └── annotations.yaml

I updated the testdata to use the new bundle format. I need to add a correct annotations.yaml file at some point, I was unsure of the format of that, figured it could be done in another PR.

For reference about annotations.yaml: https://github.com/openshift/enhancements/blob/master/enhancements/olm/operator-bundle.md#docker

sweet, just added a sample annotations.yaml file.

joelanford · 2020-04-15T18:19:18Z

internal/scorecard/alpha/tests/test_bundle.go

+type TestBundle struct {
+	BundleErrors []errors.ManifestResult
+	Bundles      []*registry.Bundle
+	CRs          []unstructured.Unstructured
+}


Can we make this struct look like we expect it to look once the API we want for parsing a bundle directory exists?

My guess is that something like the following will exist:

func LoadBundleFromDirectory(bundlePath string) (*registry.Bundle, error) { ... }

With that in mind, can we just pass a registry.Bundle to the tests?

For getting CRs, we could either wrap registry.Bundle in a new type that has a method for getting the CRs, or we can just have a helper function that returns the CRs given a registry.Bundle.

This is related to the other convo here.

Again, totally fine to add a TODO to get back to this later so we're not blocked.

Just want to make sure the final thing we ship uses the new bundle format and has a straightforward/intuitive API.

I added a TODO to address the API changes, at that point I like the idea of adding a helper to get the CRs if that is not part of the new API we eventually use.

estroz · 2020-04-15T18:31:54Z

internal/scorecard/alpha/tests/test_bundle.go

+		return cfg, fmt.Errorf("error in csv retrieval %s", err.Error())
+	}
+
+	almExamples := csv.ObjectMeta.Annotations["alm-examples"]


Check if csv.GetAnnotations() == nil.

good catch, fixed.

joelanford

/lgtm

jmccormick2001 added 6 commits April 8, 2020 13:59

initial

978828e

add test switch

78dd9e0

add json output, clean up stubbed tests

1abc461

cleanup

a3b7d1f

add unit test and test data

a505f59

make error messages uppercase for linter

131289c

openshift-ci-robot requested review from camilamacedo86 and hasbro17 April 13, 2020 18:15

jmccormick2001 requested review from estroz and jmrodri April 13, 2020 19:25

joelanford reviewed Apr 14, 2020

View reviewed changes

estroz reviewed Apr 14, 2020

View reviewed changes

internal/scorecard/alpha/testdata/memcached-operator.package.yaml Outdated Show resolved Hide resolved

remove chmod on /etc/passwd

bfd8468

joelanford reviewed Apr 14, 2020

View reviewed changes

images/scorecard-test/bin/entrypoint Outdated Show resolved Hide resolved

jmccormick2001 added 2 commits April 14, 2020 14:10

removing test Dockerfile and associated scripts since its included in…

1c18fcf

… another PR

remove use of package manifests as they are being deprecated

859e14e

joelanford reviewed Apr 15, 2020

View reviewed changes

estroz reviewed Apr 15, 2020

View reviewed changes

jmccormick2001 added 2 commits April 15, 2020 13:42

address review comments, add TODOs, change testdata to new bundle format

4456f96

add testdata annotations.yaml file

442775a

joelanford approved these changes Apr 15, 2020

View reviewed changes

openshift-ci-robot assigned joelanford Apr 15, 2020

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

jmccormick2001 merged commit 5479d95 into operator-framework:master Apr 16, 2020

		r.Name = BasicCheckStatusTest
		r.Description = "Custom Resource has a Status Block"

Scorecard2 stub test binary #2829

Scorecard2 stub test binary #2829

Uh oh!

Conversation

jmccormick2001 commented Apr 13, 2020

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!

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!

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!

Choose a reason for hiding this comment

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!

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!

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

joelanford left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants