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 CI #1164

Merged
merged 13 commits into from Jan 22, 2019

Conversation

Projects
None yet
3 participants
@gdbelvin
Copy link
Collaborator

gdbelvin commented Jan 18, 2019

This PR is best reviewed commit by commit.

There were two essential issues that had the Continuous Integration environment wedged.

  • The kubernetes configs for Trillian had stale paths for the binaries.
  • The key transparency sequencer was opening the same port twice.

Fixing the later involved refactoring the server setup to reuse a common net.Listener.

Contrary to best practice, this PR also includes a few fixups and optimizations:

  • strings.HasPrefix vs. strings.Contains should be faster for matching non-GRPC connections.
  • Split out a few functions to be purpose specific.
  • Use DialContext

gdbelvin added some commits Jan 8, 2019

Remove TLS config from grpc server
We are serving TLS through the http server and forwarding grpc
connections to the grpc server.
serveHTTPGateway with listener and dopts
Rather than opening a socket inside of serveHTTP, reuse a passed in
listener. This allows the server to listen on a predetermined socket.

@gdbelvin gdbelvin requested review from DazWilkin and Martin2112 Jan 18, 2019

@codecov

This comment has been minimized.

Copy link

codecov bot commented Jan 18, 2019

Codecov Report

Merging #1164 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #1164   +/-   ##
=======================================
  Coverage   64.68%   64.68%           
=======================================
  Files          41       41           
  Lines        3112     3112           
=======================================
  Hits         2013     2013           
  Misses        757      757           
  Partials      342      342

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 f0a84c3...e3d391b. Read the comment docs.

@gdbelvin gdbelvin requested review from pavelkalinnikov and jtoohill Jan 18, 2019

@jtoohill
Copy link
Collaborator

jtoohill left a comment

Looks sane to me, I'd like someone more familiar with gRPC APIs to take a look as well though.

tcreds, err := credentials.NewClientTLSFromFile(*certFile, "")
if err != nil {
glog.Exitf("Failed opening cert file %v: %v", *certFile, err)
func serveHTTPMetric(port string) {

This comment has been minimized.

@pavelkalinnikov

pavelkalinnikov Jan 21, 2019

Is it addr rather than port?

This comment has been minimized.

@gdbelvin

gdbelvin Jan 22, 2019

Author Collaborator

done

Show resolved Hide resolved cmd/keytransparency-sequencer/server.go Outdated
tcreds, err := credentials.NewClientTLSFromFile(*certFile, "")
if err != nil {
glog.Exitf("Failed opening cert file %v: %v", *certFile, err)
func serveHTTPMetric(port string) {

This comment has been minimized.

@pavelkalinnikov

pavelkalinnikov Jan 21, 2019

Metric -> Metrics?


glog.Infof("Hosting metrics on %v", port)
if err := http.ListenAndServe(port, metricMux); err != nil {
glog.Fatalf("ListenAndServeTLS(%v): %v", *metricsAddr, err)
}
gwmux, err := serverutil.GrpcGatewayMux(addr, tcreds, services...)

This comment has been minimized.

@pavelkalinnikov

pavelkalinnikov Jan 21, 2019

When is this meant to run? ListenAndServe above returns only when the server is stopped.

This comment has been minimized.

@gdbelvin

gdbelvin Jan 22, 2019

Author Collaborator

It's meant to run in it's own go routine like so:

go serveHTTPMetric(addr)
@@ -140,12 +140,7 @@ func main() {
glog.Exitf("Failed to create directory storage object: %v", err)
}

creds, err := credentials.NewServerTLSFromFile(*certFile, *keyFile)

This comment has been minimized.

@pavelkalinnikov

pavelkalinnikov Jan 21, 2019

Should certFile and keyFile be marked as deprecated?

This comment has been minimized.

@gdbelvin

gdbelvin Jan 22, 2019

Author Collaborator

We still use certFile and keyFile - they just get used as part of http.ServeTLS rather than the go server itself.

if err := signer.RunBatchForAllDirectories(ctx); err != nil {
glog.Errorf("PeriodicallyRun(RunBatchForAllDirectories): %v", err)
}
})

// Shutdown.
httpServer.Shutdown(cctx)

This comment has been minimized.

@pavelkalinnikov

pavelkalinnikov Jan 21, 2019

How is the server shut down now?

This comment has been minimized.

@gdbelvin

gdbelvin Jan 22, 2019

Author Collaborator

Reading the documentation closely, Shutdown looks like it's intended to be called from a separate thread than the serving thread.
If there was a way to trigger a shutdown, we would call it there, but at the moment this open source main function has SIGINT as the only shutdown signal.

@gdbelvin

This comment has been minimized.

Copy link
Collaborator Author

gdbelvin commented Jan 22, 2019

Updated PTAL

gdbelvin added some commits Jan 22, 2019

@gdbelvin gdbelvin merged commit d068f14 into google:master Jan 22, 2019

5 checks passed

GolangCI No issues found!
Details
cla/google All necessary CLAs are signed
codecov/patch Coverage not affected when comparing f0a84c3...e3d391b
Details
codecov/project 64.68% remains the same compared to f0a84c3
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

gdbelvin added a commit to gdbelvin/keytransparency that referenced this pull request Jan 23, 2019

Merge branch 'master' into mutator
* master: (31 commits)
  Fix CI (google#1164)
  Trim trillian.Tree (google#1165)
  Use golangci-lint (google#1166)
  Nits (google#1163)
  Revert "Trim Directory info (google#1157)" (google#1162)
  Trim Directory info (google#1157)
  Remove Ambiguous types.User.DirectoryId (google#1154)
  Update API Documentation (google#1160)
  Lazy load key material (google#1161)
  Fix linter backlog (google#1151)
  Allow batch size to be configurable (google#1147)
  Frontend implementation (google#1153)
  Nonblocking PublishRevisions (google#1149)
  Follow google/trillian#1385 (google#1150)
  Submit SequencedLeaves with Batch API  (google#1148)
  Change (low,high] to [low,high) intervals in HighWatarmarks (google#1144)
  Write to Trillian Map At Revision (google#1146)
  Better sequencer logging  (google#1145)
  Enforce one trillian Map caller at a time with sequencer election (google#1009)
  Client: BatchCreateMutation (google#1142)
  ...

@gdbelvin gdbelvin deleted the gdbelvin:etcd branch Feb 21, 2019

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