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

Fix/misc #862

Merged
merged 11 commits into from Nov 22, 2017

Conversation

Projects
None yet
3 participants
@gdbelvin
Collaborator

gdbelvin commented Nov 22, 2017

This PR contains a variety of bug fixes for issues encountered during deployment to GCE:

  • SQL that is compatible with MySQL.
    • Use INTEGER which is supported by both SQLite and MySQL.
    • BOOLEAN, TRUE, and FALse are not supported by SQLite.
  • Catch sql.ErrNoRows and return codes.NotFound
  • Skip tests that cannot be run when Trillian is using an SQLite backend.
  • Add URL parameter GET /v1/domains for ListDomains as a convenience / debugging endpoint.
  • Create logs via a connection to the log-server and maps via a connection to the map-server.
    • Previously all CreateTree operations happened through a connection to the map-server.
  • Periodically refresh the list of domains the sequencer operates on.
  • Add the der/proto underscore import to decode VRF keys.

gdbelvin added some commits Nov 15, 2017

Refresh domain list periodically
This change allows the sequencer to detect new domains.
This is especially important when a Key Transparency cluster is
being brought up for the first time since the sequencer starts before
any domains have been initialized.

@gdbelvin gdbelvin requested review from phad and cesarghali Nov 22, 2017

@codecov-io

This comment has been minimized.

Show comment
Hide comment
@codecov-io

codecov-io Nov 22, 2017

Codecov Report

Merging #862 into master will decrease coverage by 0.11%.
The diff coverage is 42.5%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #862      +/-   ##
==========================================
- Coverage   45.93%   45.82%   -0.12%     
==========================================
  Files          34       34              
  Lines        2869     2883      +14     
==========================================
+ Hits         1318     1321       +3     
- Misses       1357     1368      +11     
  Partials      194      194
Impacted Files Coverage Δ
impl/sql/adminstorage/admin_storage.go 63.79% <ø> (ø) ⬆️
core/keyserver/keyserver.go 0% <0%> (ø) ⬆️
core/sequencer/sequencer.go 8.08% <0%> (-0.25%) ⬇️
integration/testutil.go 65.48% <100%> (ø) ⬆️
core/adminserver/admin_server.go 45.21% <100%> (+1.46%) ⬆️

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 0025f9a...4f4158d. Read the comment docs.

codecov-io commented Nov 22, 2017

Codecov Report

Merging #862 into master will decrease coverage by 0.11%.
The diff coverage is 42.5%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #862      +/-   ##
==========================================
- Coverage   45.93%   45.82%   -0.12%     
==========================================
  Files          34       34              
  Lines        2869     2883      +14     
==========================================
+ Hits         1318     1321       +3     
- Misses       1357     1368      +11     
  Partials      194      194
Impacted Files Coverage Δ
impl/sql/adminstorage/admin_storage.go 63.79% <ø> (ø) ⬆️
core/keyserver/keyserver.go 0% <0%> (ø) ⬆️
core/sequencer/sequencer.go 8.08% <0%> (-0.25%) ⬇️
integration/testutil.go 65.48% <100%> (ø) ⬆️
core/adminserver/admin_server.go 45.21% <100%> (+1.46%) ⬆️

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 0025f9a...4f4158d. Read the comment docs.

Show outdated Hide outdated cmd/keytransparency-sequencer/main.go Outdated
Show outdated Hide outdated cmd/keytransparency-sequencer/main.go Outdated
"github.com/google/keytransparency/cmd/serverutil"
ktpb "github.com/google/keytransparency/core/proto/keytransparency_v1_grpc"
gpb "github.com/google/keytransparency/core/proto/keytransparency_v1_grpc"
_ "github.com/google/trillian/crypto/keys/der/proto"

This comment has been minimized.

@phad

phad Nov 22, 2017

Contributor

I'm not sure if it's a Go convention but I think anony imports like this are sometimes separated off into their own block

@phad

phad Nov 22, 2017

Contributor

I'm not sure if it's a Go convention but I think anony imports like this are sometimes separated off into their own block

This comment has been minimized.

@phad

phad Nov 22, 2017

Contributor

However don't bother fighting goimports which has the definitive say on these things.

@phad

phad Nov 22, 2017

Contributor

However don't bother fighting goimports which has the definitive say on these things.

This comment has been minimized.

@gdbelvin

gdbelvin Nov 22, 2017

Collaborator

There's no fighting with goimports here, which keeps imports in their own blocks.
In this change I'm trying to stick with pb and gpb for proto imports, which is a convention.

@gdbelvin

gdbelvin Nov 22, 2017

Collaborator

There's no fighting with goimports here, which keeps imports in their own blocks.
In this change I'm trying to stick with pb and gpb for proto imports, which is a convention.

grpc_prometheus "github.com/grpc-ecosystem/go-grpc-prometheus"
)
var (
addr = flag.String("metrics-addr", ":8081", "The ip:port to publish metrics on")
addr = flag.String("addr", ":8080", "The ip:port to serve on")

This comment has been minimized.

@phad

phad Nov 22, 2017

Contributor

not sure why the default number changed?

@phad

phad Nov 22, 2017

Contributor

not sure why the default number changed?

This comment has been minimized.

@gdbelvin

gdbelvin Nov 22, 2017

Collaborator

Previously, metrics was an http service that just served metrics.
Now, it's an https service that is a proper gRPC and HTTPS / JSON server. The number change is for consistency with the other Key Transparency binaries.

@gdbelvin

gdbelvin Nov 22, 2017

Collaborator

Previously, metrics was an http service that just served metrics.
Now, it's an https service that is a proper gRPC and HTTPS / JSON server. The number change is for consistency with the other Key Transparency binaries.

"github.com/golang/glog"
"github.com/prometheus/client_golang/prometheus/promhttp"
"google.golang.org/grpc"
"google.golang.org/grpc/credentials"
"google.golang.org/grpc/reflection"
"github.com/google/keytransparency/cmd/serverutil"
ktpb "github.com/google/keytransparency/core/proto/keytransparency_v1_grpc"
gpb "github.com/google/keytransparency/core/proto/keytransparency_v1_grpc"

This comment has been minimized.

@phad

phad Nov 22, 2017

Contributor

was serverutil completely unused?

@phad

phad Nov 22, 2017

Contributor

was serverutil completely unused?

This comment has been minimized.

@gdbelvin

gdbelvin Nov 22, 2017

Collaborator

Just moved it up a few lines to keep the following import order:

  • System
  • Key Transparency
  • Third Party
  • Renamed and underscore imports
@gdbelvin

gdbelvin Nov 22, 2017

Collaborator

Just moved it up a few lines to keep the following import order:

  • System
  • Key Transparency
  • Third Party
  • Renamed and underscore imports
@@ -37,6 +38,10 @@ func vrfKeyGen(ctx context.Context, spec *keyspb.Specification) (proto.Message,
}
func TestCreateRead(t *testing.T) {
// We can only run the integration tests if there is a MySQL instance available.
if provider := testdb.Default(); !provider.IsMySQL() {
t.Skipf("Skipping map integration test, SQL driver is %v", provider.Driver)

This comment has been minimized.

@phad

phad Nov 22, 2017

Contributor

I'm beginning to think this idiom is dangerous. It's too easy to forget to set the relevant environment variable then be persuaded all is OK when the test silently passes.
What about t.Errorf() instead?

@phad

phad Nov 22, 2017

Contributor

I'm beginning to think this idiom is dangerous. It's too easy to forget to set the relevant environment variable then be persuaded all is OK when the test silently passes.
What about t.Errorf() instead?

This comment has been minimized.

@gdbelvin

gdbelvin Nov 22, 2017

Collaborator

I agree with you. @codingllama what do you think? If we make it error, we should probably make it error everywhere.

Note that make presubmit sets all the right flags to use MySQL.

@gdbelvin

gdbelvin Nov 22, 2017

Collaborator

I agree with you. @codingllama what do you think? If we make it error, we should probably make it error everywhere.

Note that make presubmit sets all the right flags to use MySQL.

Show outdated Hide outdated core/keyserver/keyserver.go Outdated
for range ticker.C {
domains, err := s.admin.List(ctx, false)
if err != nil {
return fmt.Errorf("admin.List(): %v", err)

This comment has been minimized.

@phad

phad Nov 22, 2017

Contributor

stop the ticker?

@phad

phad Nov 22, 2017

Contributor

stop the ticker?

This comment has been minimized.

@gdbelvin

gdbelvin Nov 22, 2017

Collaborator

Yep.

@gdbelvin

gdbelvin Nov 22, 2017

Collaborator

Yep.

Show outdated Hide outdated core/sequencer/sequencer.go Outdated
@@ -38,7 +38,7 @@ CREATE TABLE IF NOT EXISTS Domains(
VRFPrivateKey MEDIUMBLOB NOT NULL,
MinInterval BIGINT NOT NULL,
MaxInterval BIGINT NOT NULL,
Deleted BOOLEAN,
Deleted INTEGER,

This comment has been minimized.

@phad

phad Nov 22, 2017

Contributor

what motivates this? has Deleted become a multi-state enum rather than a boolean?

@phad

phad Nov 22, 2017

Contributor

what motivates this? has Deleted become a multi-state enum rather than a boolean?

This comment has been minimized.

@phad

phad Nov 22, 2017

Contributor

Oops just read the PR desc, I see it's a SQLite compatibility thing. Ignore.

@phad

phad Nov 22, 2017

Contributor

Oops just read the PR desc, I see it's a SQLite compatibility thing. Ignore.

This comment has been minimized.

@gdbelvin

gdbelvin Nov 22, 2017

Collaborator

💯

@gdbelvin

gdbelvin Nov 22, 2017

Collaborator

💯

Show outdated Hide outdated core/sequencer/sequencer.go Outdated
@phad

phad approved these changes Nov 22, 2017

minor nit with the ticker, then LGTM

@gdbelvin gdbelvin merged commit e715077 into google:master Nov 22, 2017

2 checks passed

cla/google All necessary CLAs are signed
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@gdbelvin gdbelvin deleted the gdbelvin:fix/misc branch Nov 22, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment