Skip to content

Migrate TestTrustedCreateFromBadTrustServer from moby into cli/e2e #1517

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.

Sign up for GitHub

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

Jump to bottom
Merged
thaJeztah merged 1 commit into from
Nov 26, 2018
Merged

Migrate TestTrustedCreateFromBadTrustServer from moby into cli/e2e #1517

thaJeztah merged 1 commit into from
Nov 26, 2018

Conversation

@glefloch
Copy link
Contributor

@glefloch glefloch commented Nov 13, 2018

Signed-off-by: glefloch glfloch@gmail.com

- What I did
I have migrate TestTrustedCreateFromBadTrustServer from moby/moby repository into docker/cli repository. This aims to closes #1218

- How I did it
I have added a new notary-server into e2e test environment called evil-notary-server. Then duplicate the behavior of the test describe in moby repository.

- How to verify it
You can verify it by running e2e tests.

- Description for the changelog

Add test on container creation using a wrong notary server

- A picture of a cute animal (not mandatory but encouraged)

@GordonTheTurtle
Copy link

GordonTheTurtle commented Nov 14, 2018

Please sign your commits following these rules:
https://github.com/moby/moby/blob/master/CONTRIBUTING.md#sign-your-work
The easiest way to do this is to amend the last commit:

$ git clone -b "migrate-test-from-moby" git@github.com:glefloch/cli.git somewhere
$ cd somewhere
$ git rebase -i HEAD~842353912952
editor opens
change each 'pick' to 'edit'
save the file and quit
$ git commit --amend -s --no-edit
$ git rebase --continue # and repeat the amend for each commit
$ git push -f

Amending updates the existing PR. You DO NOT need to open a new one.

@codecov-io
Copy link

codecov-io commented Nov 14, 2018 •
edited
Loading

Codecov Report

Merging #1517 into master will decrease coverage by 0.07%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #1517      +/-   ##
==========================================
- Coverage   55.22%   55.14%   -0.08%     
==========================================
  Files         289      289              
  Lines       19384    19371      -13     
==========================================
- Hits        10705    10683      -22     
- Misses       7983     7997      +14     
+ Partials      696      691       -5

@glefloch glefloch force-pushed the migrate-test-from-moby branch from d8d09e7 to 436e05c Compare November 14, 2018 11:51
eiais
eiais reviewed Nov 14, 2018
View reviewed changes
Copy link
Contributor

@eiais eiais 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!

eiais
eiais approved these changes Nov 15, 2018
View reviewed changes
Copy link
Contributor

@eiais eiais left a comment

Choose a reason for hiding this comment

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

LGTM. Please squash your commits.

@glefloch glefloch force-pushed the migrate-test-from-moby branch from ea062ca to f568785 Compare November 16, 2018 08:10
@eiais
Copy link
Contributor

eiais commented Nov 20, 2018

@thaJeztah PTAL

silvin-lubecki
silvin-lubecki reviewed Nov 21, 2018
View reviewed changes
e2e/container/create_test.go Outdated
fixtures.WithNotary,
).Assert(t, icmd.Expected{
ExitCode: 1,
Err: "remote trust data does not exist for registry:5000/evil-alpine",
Copy link
Contributor

@silvin-lubecki silvin-lubecki Nov 21, 2018

Choose a reason for hiding this comment

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

When I look at the removed test on moby/moby I can't see the same result as yours.
Can you add some context in the PR comment to explain why you expect this result, and maybe in the test too?

Copy link
Contributor

@eiais eiais Nov 21, 2018

Choose a reason for hiding this comment

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

Good catch! fixtures.WithNotary is using the normal notary server which won't have the evil alpine image.
@glefloch Can you make WithNotary take the notary server as an argument? This will also change the returned error.

Copy link
Contributor Author

@glefloch glefloch Nov 21, 2018

Choose a reason for hiding this comment

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

Yes, I'm going to make this change and re run tests

Copy link
Contributor Author

@glefloch glefloch Nov 21, 2018

Choose a reason for hiding this comment

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

Well, updating the WithNotary ends up with the command exitCode equals to 0. There may be another problem with the configuration.

Copy link
Contributor Author

@glefloch glefloch Nov 22, 2018

Choose a reason for hiding this comment

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

Finally, after looking to the previous test, I was actually missing the first part. I updated the test and report comments from the previous test.

Copy link
Contributor

@silvin-lubecki silvin-lubecki Nov 23, 2018

Choose a reason for hiding this comment

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

I can't see the comments from moby/moby test, did you push them?

Copy link
Contributor Author

@glefloch glefloch Nov 23, 2018

Choose a reason for hiding this comment

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

Sorry for this, I just pushed the update.

silvin-lubecki
silvin-lubecki reviewed Nov 21, 2018
View reviewed changes
Copy link
Contributor

@silvin-lubecki silvin-lubecki left a comment

Choose a reason for hiding this comment

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

LGTM for code, but may need some comments in the tests. Thank you @glefloch for this PR!!

@glefloch glefloch force-pushed the migrate-test-from-moby branch 2 times, most recently from c075c19 to cfc8ebf Compare November 23, 2018 10:46
justincormack
justincormack reviewed Nov 26, 2018
View reviewed changes
e2e/container/create_test.go Outdated
icmd.Command("docker", "create", image),
fixtures.WithConfig(dir.Path()),
fixtures.WithTrust,
fixtures.WithNotaryServer("https://invalidnotaryserver"),
Copy link
Contributor

@justincormack justincormack Nov 26, 2018

Choose a reason for hiding this comment

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

There is an IETF recommendation for invalid domains: https://tools.ietf.org/html/rfc2606#page-2 - so better to use notary.invalid.

Copy link
Contributor Author

@glefloch glefloch Nov 26, 2018

Choose a reason for hiding this comment

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

You right, this is better. I pushed the update.

@justincormack
Copy link
Contributor

justincormack commented Nov 26, 2018

One minor comment but otherwise LGTM

@glefloch
Add missing test
37679bf 
Signed-off-by: glefloch <glfloch@gmail.com>
@glefloch glefloch force-pushed the migrate-test-from-moby branch from cfc8ebf to 37679bf Compare November 26, 2018 14:02
thaJeztah
thaJeztah approved these changes Nov 26, 2018
View reviewed changes
Copy link
Member

@thaJeztah thaJeztah left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@thaJeztah thaJeztah merged commit 2d692ae into docker:master Nov 26, 2018
@GordonTheTurtle GordonTheTurtle added this to the 19.03.0 milestone Nov 26, 2018
@glefloch glefloch deleted the migrate-test-from-moby branch November 26, 2018 17:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@silvin-lubecki silvin-lubecki silvin-lubecki left review comments

@thaJeztah thaJeztah thaJeztah approved these changes

+2 more reviewers

@justincormack justincormack justincormack left review comments

@eiais eiais eiais approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

area/testing area/trust status/2-code-review

Projects

None yet

Milestone

19.03.0

Development

Successfully merging this pull request may close these issues.

Migrate TestTrustedCreateFromBadTrustServer to cli/e2e

7 participants

@glefloch @GordonTheTurtle @codecov-io @eiais @justincormack @thaJeztah @silvin-lubecki