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

feat: add --image flag in gator test|expand #2398

Merged
merged 32 commits into from
Dec 14, 2022

Conversation

davis-haba
Copy link
Contributor

@davis-haba davis-haba commented Nov 10, 2022

Signed-off-by: davis-haba davishaba@google.com

Adds --image flag to gator test|expand, which supports pulling OCI Artifacts containing policy bundles for ingestion by gator. #2333

The flag is mixable with the existing --filename flag, as well as with stdin in the case of gator test. For example:

gator test --image=localhost:5000/gator/my-bundle:v1 --filename=my-manifest.yaml

The documentation has been updated with the new --image flag, as well as some information on creating OCI Artifacts with policy bundles.

This PR also introduces duplicate-detection logic in gator test|expand, which will log a warning if resources with the same GKNN are found across any of the input methods (--filename, --image, or stdin). In the case of duplicates, the command does not fail closed, and we cannot make any guarantees about which copy will be used.

Some example warnings:

WARNING - Resource named 'my-resource' (from file: 'file.yaml', image: 'gcr.io/this/that:latest') is already defined in file: 'another.yaml', image:'gcr.io/that/this:v1'

WARNING - Resource named 'my-resource' (from stdin) is already defined in file: 'file.yaml'

WARNING - Resource named 'my-resource' (from file: 'file.yaml') is already defined in stdin

WARNING - Resource named 'my-resource' (from file: 'file.yaml') is already defined in file: 'another.yaml', image: 'gcr.io/this/that:latest'

The gator tests have been updated to run in a container, which is executable via make test-gator-containerized`. The approach creates a "test runner" container to run the tests, and another container inside the test runner to act as the image registry.

The Gator CLI code has also been refactored such that code for each command has its own package, and common code is in gator/reader and gator/. #1779

@codecov-commenter
Copy link

codecov-commenter commented Nov 10, 2022

Codecov Report

Base: 53.52% // Head: 53.38% // Decreases project coverage by -0.14% ⚠️

Coverage data is based on head (a7aade3) compared to base (92841ff).
Patch coverage: 27.30% of modified lines in pull request are covered.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #2398      +/-   ##
==========================================
- Coverage   53.52%   53.38%   -0.15%     
==========================================
  Files         117      115       -2     
  Lines       10281    10170     -111     
==========================================
- Hits         5503     5429      -74     
+ Misses       4354     4326      -28     
+ Partials      424      415       -9     
Flag Coverage Δ
unittests 53.38% <27.30%> (-0.15%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
pkg/gator/reader/filereader.go 0.00% <0.00%> (ø)
pkg/gator/reader/read_constraints.go 12.50% <0.00%> (ø)
pkg/gator/verify/printer_go.go 46.78% <ø> (ø)
pkg/gator/verify/result.go 72.72% <ø> (ø)
pkg/gator/reader/conflictdetector.go 66.66% <66.66%> (ø)
pkg/gator/verify/runner.go 85.54% <90.32%> (ø)
pkg/gator/test/test.go 61.90% <100.00%> (ø)
pkg/gator/verify/assertion.go 100.00% <100.00%> (ø)
pkg/gator/verify/filter.go 100.00% <100.00%> (ø)
pkg/gator/verify/read_suites.go 88.34% <100.00%> (ø)
... and 7 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

Copy link
Contributor

@acpana acpana left a comment

Choose a reason for hiding this comment

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

Thanks for working on this 💯 ; left some nits and questions, nothing blocking from me

.github/workflows/workflow.yaml Outdated Show resolved Hide resolved
build/testing/Dockerfile Outdated Show resolved Hide resolved
build/testing/startup.sh Outdated Show resolved Hide resolved
build/testing/startup.sh Outdated Show resolved Hide resolved
go.mod Show resolved Hide resolved
pkg/gator/oci.go Outdated Show resolved Hide resolved
pkg/gator/oci.go Outdated Show resolved Hide resolved
website/docs/gator.md Outdated Show resolved Hide resolved
pkg/gator/oci.go Outdated Show resolved Hide resolved
pkg/gator/oci.go Outdated Show resolved Hide resolved
pkg/gator/oci.go Outdated Show resolved Hide resolved
pkg/gator/oci.go Outdated Show resolved Hide resolved
pkg/gator/oci.go Outdated Show resolved Hide resolved
test/gator/test/test.bats Outdated Show resolved Hide resolved
@davis-haba davis-haba requested review from maxsmythe and acpana and removed request for acpana November 15, 2022 19:14
Copy link
Contributor

@maxsmythe maxsmythe left a comment

Choose a reason for hiding this comment

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

Nice refactor!

Basically LGTM modulo some nits, and a more substantial comment that I think we might be able to consolidate Dockerfiles to have only one test image.

Makefile Outdated Show resolved Hide resolved
build/testing/startup.sh Outdated Show resolved Hide resolved
pkg/gator/reader/conflictdetector_test.go Show resolved Hide resolved
pkg/gator/reader/filereader.go Show resolved Hide resolved
Copy link
Contributor

@acpana acpana left a comment

Choose a reason for hiding this comment

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

some light nits, UX questions and string change callouts;

otherwise LGTM! Thanks for following up here.

cmd/gator/expand/expand.go Outdated Show resolved Hide resolved
cmd/gator/test/test.go Outdated Show resolved Hide resolved
cmd/gator/expand/expand.go Show resolved Hide resolved
pkg/gator/errors.go Show resolved Hide resolved
pkg/oci/oci.go Show resolved Hide resolved
website/docs/gator.md Outdated Show resolved Hide resolved
website/docs/gator.md Outdated Show resolved Hide resolved
website/docs/gator.md Show resolved Hide resolved
@davis-haba davis-haba requested a review from maxsmythe November 16, 2022 21:35
Copy link
Contributor

@maxsmythe maxsmythe left a comment

Choose a reason for hiding this comment

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

LGTM

@davis-haba davis-haba requested a review from sozercan December 6, 2022 23:41
@sozercan sozercan added this to the v3.11.0 milestone Dec 7, 2022
@@ -116,6 +117,11 @@ test-e2e:
.PHONY: test-gator
test-gator: gator test-gator-verify test-gator-test test-gator-expand

.PHONY: test-gator-containerized
test-gator-containerized: __test-image
docker run --privileged -v $(shell pwd):/app -v /var/lib/docker \
Copy link
Member

Choose a reason for hiding this comment

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

this is going to make artifacts created by this (like gator binary) owned by root. can we do this without privileged?

Copy link
Contributor Author

@davis-haba davis-haba Dec 13, 2022

Choose a reason for hiding this comment

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

The test needs to run docker-inside-docker in order to spin up a local container registry to push to test bundles to. Unfortunately, --privileged is necessary to allow the inner container (the container registry) to be able to access the file system of the outer container (the test runner).

I'm not sure I understand why this would make the gator binary owned by root, as this privileged execution is just for a test image. Is there someway we can exclude it? Otherwise, if this is a problem, we might have to explore other methods of e2e testing this, like perhaps using an external public registry for the test images instead of spinning up a local one.

test/gator/test/test.bats Outdated Show resolved Hide resolved
Copy link
Member

@sozercan sozercan left a comment

Choose a reason for hiding this comment

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

@davis-haba overall LGTM, added a few comments and looks like there are merge conflicts now

Copy link
Contributor

@acpana acpana left a comment

Choose a reason for hiding this comment

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

LGTM !! (modulo merge conflicts)

Signed-off-by: davis-haba <davishaba@google.com>
davis-haba and others added 14 commits December 13, 2022 12:44
Signed-off-by: davis-haba <davishaba@google.com>
Signed-off-by: davis-haba <davishaba@google.com>
Signed-off-by: davis-haba <davishaba@google.com>
Signed-off-by: davis-haba <davishaba@google.com>
Signed-off-by: davis-haba <davishaba@google.com>
Signed-off-by: davis-haba <davishaba@google.com>
Co-authored-by: alex <8968914+acpana@users.noreply.github.com>
Signed-off-by: davis-haba <52938648+davis-haba@users.noreply.github.com>
Co-authored-by: alex <8968914+acpana@users.noreply.github.com>
Signed-off-by: davis-haba <52938648+davis-haba@users.noreply.github.com>
Co-authored-by: alex <8968914+acpana@users.noreply.github.com>
Signed-off-by: davis-haba <52938648+davis-haba@users.noreply.github.com>
Co-authored-by: alex <8968914+acpana@users.noreply.github.com>
Signed-off-by: davis-haba <52938648+davis-haba@users.noreply.github.com>
Signed-off-by: davis-haba <davishaba@google.com>
Signed-off-by: davis-haba <davishaba@google.com>
Signed-off-by: davis-haba <davishaba@google.com>
Signed-off-by: davis-haba <davishaba@google.com>
davis-haba and others added 5 commits December 13, 2022 13:41
Co-authored-by: Sertaç Özercan <852750+sozercan@users.noreply.github.com>
Signed-off-by: davis-haba <52938648+davis-haba@users.noreply.github.com>
Co-authored-by: Sertaç Özercan <852750+sozercan@users.noreply.github.com>
Signed-off-by: davis-haba <52938648+davis-haba@users.noreply.github.com>
Signed-off-by: davis-haba <davishaba@google.com>
Signed-off-by: davis-haba <davishaba@google.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants