Use the Trillian testing environment #727
Conversation
gdbelvin
force-pushed
the
trillian_verifier
branch
2 times, most recently
from
e010218 to
0ad94ff
Compare
Codecov Report
@@ Coverage Diff @@
## master #727 +/- ##
==========================================
- Coverage 53.22% 48.86% -4.36%
==========================================
Files 38 32 -6
Lines 3207 2566 -641
==========================================
- Hits 1707 1254 -453
+ Misses 1209 1121 -88
+ Partials 291 191 -100
Continue to review full report at Codecov.
|
liamsi
left a comment
liamsi
left a comment
There was a problem hiding this comment.
Awesome! Thanks for this pull request!
Besides few nits and questions this LGTM
| if err != nil { | ||
| glog.Exitf("newMapServer: %v", err) | ||
| } | ||
| mconn, err := grpc.Dial(*mapURL, grpc.WithInsecure()) |
There was a problem hiding this comment.
This is not directly related to this pull but is the grpc-connection to the map-server always unencrypted?
There was a problem hiding this comment.
Yes, Trillian is supposed to be a backend connection, not exposed to the internet so at the moment it's unencrypted.
cmd/keytransparency-server/main.go
Outdated
| if err != nil { | ||
| glog.Exitf("newReadonlyMapServer(): %v", err) | ||
| } | ||
|
|
| // filtering case. | ||
| for i, p := range [][]byte{ | ||
| nil, nil, cp(1), cp(2), nil, nil, cp(3), nil, | ||
| nil, cp(1), cp(2), nil, nil, cp(3), nil, |
There was a problem hiding this comment.
What is the reason for removing this nil?
Also, if you keep it like this, update the comment above to:
// following profiles:
// [nil, 1, 2, 2, 2, 3, 3, 4, 5, 5, 5, 5, 5, 5, 6, 6, 5, 7, 7].There was a problem hiding this comment.
The trillian map server starts at revision 1 rather than revision 0.
This change shifts the test data by one epoch to accomodate that.
There was a problem hiding this comment.
Thanks for the explanation! Could you update the comment to reflect this change as well?
| {24, 10, 0, []int{24}, codes.OK}, // requesting the very last entry. | ||
| {1, 1000000, 17, []int{1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16}, codes.OK}, // DOS prevention. | ||
| {40, 10, 0, []int{}, codes.InvalidArgument}, // start epoch is beyond current epoch. | ||
| {-1, 1, 1, []int{0}, codes.InvalidArgument}, // start epoch is less than 0. |
There was a problem hiding this comment.
Shouldn't this test be moved to integration instead of deleting it? If yes, I'm happy if we merge this pull an open an issue for that (I can give it a shot).
There was a problem hiding this comment.
Ah OK. A similar test (testing the same but from the client's perspective) already exist: https://github.com/google/keytransparency/pull/727/files/0ad94ff088be2e59b6832e5e8b258080bf6e0dc4#diff-f8b79d8d11b0c9af5cfa9f8d823cbb91R204
I've missed it because the test in question wasn't really changed. You can simply ignore this comment.
There was a problem hiding this comment.
Yep. This functionality is more rigorously tested in integration/client_test.go
gdbelvin
force-pushed
the
trillian_verifier
branch
3 times, most recently
from
5d39158 to
ae66d5f
Compare
gdbelvin
force-pushed
the
trillian_verifier
branch
from
ae66d5f to
5a11511
Compare
|
LGTM |
3 participants
This PR transitions Key Transparency from the KT mapserver implementation onto the Trillian MapEnv which runs a proper trillian map server which more closely replicates KT behavior while using a Trillian backend.
This PR also deletes the now unused code. Closes #229