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

[e2e]: Add tests for dockerv2 #309

Merged
merged 7 commits into from Dec 23, 2019
Merged

Conversation

@erbesharat
Copy link
Member

erbesharat commented Dec 19, 2019

What this PR does / why we need it:
Adds E2E tests for dockerv2 type

Special notes for your reviewer:
Not ready yet, we have to add the custom registry as an insecure registry to the docker daemon config.

Release note:


@deepcode-ci-bot

This comment has been minimized.

Copy link

deepcode-ci-bot bot commented Dec 19, 2019

DeepCode's analysis on #d02a2c found:

  • 0 critical issues. ⚠️ 0 warnings and 0 minor issues. ✔️ 0 issues were fixed.

💬 This comment has been generated by the DeepCode bot, installed by the owner of the repository. The DeepCode bot protects your repository by detecting and commenting on security vulnerabilities or other critical issues.


☺️ If you want to provide feedback on our bot, here is how to contact us.

@stp-ip

This comment has been minimized.

Copy link
Member

stp-ip commented Dec 19, 2019

When all pulls and pushes go through crane, why does the docker daemon need to know about the unsecure registry?

@erbesharat

This comment has been minimized.

Copy link
Member Author

erbesharat commented Dec 20, 2019

My bad, forgot to use crane instead of docker for pushing the test image to the registry.

.travis.yml Show resolved Hide resolved
@erbesharat

This comment has been minimized.

Copy link
Member Author

erbesharat commented Dec 21, 2019

I'm having some trouble with writings tests for the push.
This is what I have in mind to do:

  1. Pull the image that we pushed to the registry in manager-script before running the dockerv2 tests. (This should be done in dockerv2's testcase)
  2. Tag the pulled image to the zone address so we can push it
  3. Push the image using crane

But the issue is crane needs a tarball of the image to push it to the registry and we have to tag the image so we can push it using txtdirect. But to create a tarball for the image we have to use docker save command and I couldn't find anything in crane to do that.

So I guess we have two options:

  1. Find a fix for dind issues so we can install docker in the tester image
  2. Implement a subcommand in crane to create tarballs for images
@erbesharat

This comment has been minimized.

Copy link
Member Author

erbesharat commented Dec 21, 2019

Tried to use the crane export command but still can't find out why it doesn't work with the custom registry. It doesn't return any errors and just stays frozen.

Update:
crane export returned the following error:

2019/12/22 15:44:34 No matching credentials were found, falling back on anonymous
2019/12/22 15:44:34 MANIFEST_UNKNOWN: manifest unknown; map[Tag:latest]
@erbesharat

This comment has been minimized.

Copy link
Member Author

erbesharat commented Dec 22, 2019

The issue is crane only exports remote images into a tarball.
So when we pull the testing image and tag it to push it again to the registry, crane can't export the newly tagged image into a tarball.

@stp-ip

This comment has been minimized.

Copy link
Member

stp-ip commented Dec 22, 2019

As we have the image in the registry wouldn't it be possible to use crane copy to test push?
To be fair this is not perfect as push will fail, if the pull itself fails.

What's your stance on pushing the e2e docker push tests to a separate PR? That way we can merge this in to have test coverage for the currently supported dockerv2 parts. Will also look into why our current dockerv2 usage on c.txtdirect.org etc. isn't working as expected.

@erbesharat

This comment has been minimized.

Copy link
Member Author

erbesharat commented Dec 22, 2019

I agree with a separate PR for the push tests. Have I missed anything or the current fallback tests and the 1 pull test are fine for now? Couldn't think of any other scenario to cover right now.

Copy link
Member

stp-ip left a comment

I think the test cases are fine for now and a good start. Test coverage \o/

A few small things, but overall almost ready to merge.

result[false] = append(result[false], test)
log.Printf("[%s]: Couldn't send the request: %s", test.name, err.Error())
continue
if test.fallback {

This comment has been minimized.

Copy link
@stp-ip

stp-ip Dec 22, 2019

Member

I like the restructure on the fallback.

@@ -4,4 +4,6 @@ RUN apk --no-cache add git

WORKDIR /e2e

RUN GO111MODULE=on go get -u github.com/google/go-containerregistry/cmd/crane

This comment has been minimized.

Copy link
@stp-ip

stp-ip Dec 22, 2019

Member

This should be pinnned to a specific version I think.
Two options:

Create a go.mod you use to track your globally installed tools, such as in ~/global-tools/go.mod, and cd to that directory prior to running go get or go install for any globally installed tools.

Create a go.mod for each tool in separate directories, such as ~/tools/gorename/go.mod and ~/tools/goimports/go.mod, and cd to that appropriate directory prior to running go get or go install for the tool.

This comment has been minimized.

Copy link
@erbesharat

erbesharat Dec 23, 2019

Author Member

Can't we control the versions using the @Version at the end of go get command and manage them only inside the Dockerfile? Since we're only using crane's binary, I'm not sure if go.mod is needed.

This comment has been minimized.

Copy link
@stp-ip

stp-ip Dec 23, 2019

Member

Good idea. Simpler and same effect. +1

This comment has been minimized.

Copy link
@erbesharat

erbesharat Dec 23, 2019

Author Member

Since crane doesn't have specific versions, I used 34fb8ff which is the hash for insecure flag PR's merge commit.

Makefile Outdated
@@ -36,6 +36,7 @@ endtoend-test: docker-build
docker build -t $(IMAGE)-dirty .
cd e2e && \
docker build -t c.txtdirect.org/tester:dirty . && \
docker run -d -p 5000:5000 --name registry registry:2 && \

This comment has been minimized.

Copy link
@stp-ip

stp-ip Dec 22, 2019

Member

Can we pin this to a specific docker registry version?

This comment has been minimized.

Copy link
@erbesharat

erbesharat Dec 23, 2019

Author Member

The currently available tags are:

  • 2.7.1, 2.7, 2, latest
  • 2.6.2, 2.6

Since "2.7.1, 2.7, 2" are all the same image, is it needed to switch the tag?

This comment has been minimized.

Copy link
@stp-ip

stp-ip Dec 23, 2019

Member

Question is how much we trust Docker not to break during minor versions.
We could be specific and use 2.7.1, but happy to keep it as is. Up to you.

This comment has been minimized.

Copy link
@erbesharat

erbesharat Dec 23, 2019

Author Member

Yeah I agree that Docker may mess up something during minor versions.
Will change it to 2.7.1.

}

type Kind string

const (

This comment has been minimized.

Copy link
@stp-ip

stp-ip Dec 22, 2019

Member

Why the abstraction of Kind?

This comment has been minimized.

Copy link
@erbesharat

erbesharat Dec 23, 2019

Author Member

Just thought it may look better than direct string comparison. Will change it if it's unnecessary.

This comment has been minimized.

Copy link
@stp-ip

stp-ip Dec 23, 2019

Member

It makes the overall code a bit harder to understand in my view as the string for each type wouldn't change frequently anyway. Ok to keep it as is, but feel like this might be an over optimization for nicer code > readability. What do you think?

This comment has been minimized.

Copy link
@erbesharat

erbesharat Dec 23, 2019

Author Member

Well, It would've been useful if we had lots of types and not only pull and push. And since they're not gonna get changed at all I guess going with strings instead of Kind type is better rn.

@stp-ip

This comment has been minimized.

Copy link
Member

stp-ip commented Dec 22, 2019

Also still marked as a draft PR

@erbesharat erbesharat marked this pull request as ready for review Dec 23, 2019
@stp-ip
stp-ip approved these changes Dec 23, 2019
@stp-ip stp-ip merged commit f4318a5 into txtdirect:master Dec 23, 2019
2 checks passed
2 checks passed
DeepCode Found no critical issues.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
2 participants
You can’t perform that action at this time.