Skip to content
This repository was archived by the owner on Oct 11, 2024. It is now read-only.

Comments

Use a simple grpc connection to Trillian backends#760

Closed
gdbelvin wants to merge 2 commits intogoogle:masterfrom
gdbelvin:refactor/sig
Closed

Use a simple grpc connection to Trillian backends#760
gdbelvin wants to merge 2 commits intogoogle:masterfrom
gdbelvin:refactor/sig

Conversation

@gdbelvin
Copy link
Contributor

Previously, the signer used a verifying trillian log client in the
signer. This:

  • Required a separate build step to acquire the verifying key
  • Was tied to a confusing and only partially implemented multi-tennant
    configuration system.

This PR simplifies the situation to make a pure grpc call to the
trillian log backend, simplifying debugging, and makes things consistent
consistent with the way we are querying the trillian map server, and is
also consistent with the way the Certificate Transparency frontends
treat their own trillian backends.

This PR also removes the dead code associated with these defunct interfaces.

Previously, the signer used a verifying trillian log client in the
signer. This:
- Required a separate build step to acquire the verifying key
- Was tied to a confusing and only partially implemented multi-tennant
  configuration system.

This commit simplifies the situation to make a pure grpc call to the
trillian log backend, simplifying debugging, and makes things consistent
consistent with the way we are querying the trillian map server, and is
also consistent with the way the Certificate Transparency frontends
treat their own trillian backends.
The appender interface is a defunct wrapper for the trillian log.

The admin interface is a defunct api for multi-tennant configuration.
@gdbelvin gdbelvin requested review from cesarghali and taknira August 17, 2017 13:46
@codecov-io
Copy link

Codecov Report

Merging #760 into master will decrease coverage by 0.12%.
The diff coverage is 7.14%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #760      +/-   ##
==========================================
- Coverage   49.18%   49.05%   -0.13%     
==========================================
  Files          30       28       -2     
  Lines        2507     2446      -61     
==========================================
- Hits         1233     1200      -33     
+ Misses       1077     1061      -16     
+ Partials      197      185      -12
Impacted Files Coverage Δ
core/signer/signer.go 10.94% <0%> (-0.46%) ⬇️
integration/testutil.go 70% <100%> (+1.11%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5a851ef...bcd17bd. Read the comment docs.


// Connection to append only log
tlog, err := config.LogClient(*logID, *logURL, *logPubKey)
lconn, err := grpc.Dial(*logURL, grpc.WithInsecure())
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this on the same machine? If yes, will it always be on the same machine? If not, why did we switch to an insecure connection?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The trillian log and map are not always on the same machine.

Using grpc.WithInsecure() is not a change. It was previously hidden behind config. This makes it a bit more obvious and consistent.

return err
}
// TODO(gbelvin): Add leaf at a specific index. trillian#423
// TODO(gdbelvin): If the log doesn't do this, we need to generate an emergency alert.
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: gbelvin? :)

@gdbelvin gdbelvin closed this Aug 18, 2017
@gdbelvin gdbelvin deleted the refactor/sig branch August 18, 2017 08:02
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants