Skip to content
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

Updated Trillian/etcd/grpc to latest version #774

Merged
merged 2 commits into from
Mar 3, 2021

Conversation

mhutchinson
Copy link
Contributor

@mhutchinson mhutchinson commented Mar 2, 2021

Most of this is pretty straightforwarded, just bending the code to the slightly different shape of the dependencies. While the etcd announcements look quite different in code, I've confirmed that the output of etcdctl get trillian-ctfe --prefix is the same before/after this change, e.g.

trillian-ctfe-http/localhost:6965
{"Op":0,"Addr":"localhost:6965","Metadata":null}
trillian-ctfe-metrics-http/localhost:6965
{"Op":0,"Addr":"localhost:6965","Metadata":null}

Most of this is pretty straightforwarded, just bending the code to the slightly different shape of the dependencies. The one part that looks like a functional change is in `trillian/ctfe/ct_server/main.go`; etcd has changed service discovery to allow multiple endpoints and this entails us advertising services under a slightly different key. Previously it looks like we just announced the http & metrics services under the bare names provided by the flags (e.g. `trillian-ctfe-http`). Now that etcd has endpoints, it looks like this isn't directly possible and thus I've needed to add a trailing slash (`trillian-ctfe-http/`). This gets around the check (https://github.com/etcd-io/etcd/blob/77e6df28cf18fcf6c25eeb18c98304fb8e9026cd/client/v3/naming/endpoints/endpoints_impl.go#L45) but feels a bit dodgy. What would seem more natural to me is to have a single service (e.g. `trillian-ctfe`) with two endpoints (`http`, and `metrics`). I'd appreciate someone with more familiarity with etcd usage advising on this though.
@mhutchinson mhutchinson requested a review from pav-kv March 2, 2021 14:57
@google-cla google-cla bot added the cla: yes label Mar 2, 2021
@codecov
Copy link

codecov bot commented Mar 2, 2021

Codecov Report

Merging #774 (f8c02bd) into master (3d1e6e0) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #774   +/-   ##
=======================================
  Coverage   72.94%   72.94%           
=======================================
  Files          81       81           
  Lines        7842     7842           
=======================================
  Hits         5720     5720           
  Misses       1696     1696           
  Partials      426      426           

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 3d1e6e0...f8c02bd. Read the comment docs.

@mhutchinson
Copy link
Contributor Author

Hi @pgporada do you mind taking a look at this PR and letting us know if & how it'll affect you? I've tried to keep the changes minimal, but I've been forced to make a minor functional change, and I don't have the context to know how big a deal this will be.

@pgporada
Copy link
Contributor

pgporada commented Mar 3, 2021

Thanks for pinging me about this. Turns out that we're doing something goofy in our ctfe configuration. I wasn't aware that I could or even should be using etcd for the ctfe. From what I can tell our ctfe's are hitting the kubernetes services api

} else {
glog.Infof("Using regular DNS resolver")
dialOpts = append(dialOpts, grpc.WithBalancerName(roundrobin.Name))
}

ctfe config

    spec:
      containers:
        - name: trillian-ctfe
          args: [
          "-http_endpoint=0.0.0.0:6962",
          "-metrics_endpoint=0.0.0.0:6963",
          "-log_rpc_server=dns:///log-server:8090",  <==== this thing
          "-log_config=/ct-server.cfg",
          "-logtostderr",
          "-rpc_deadline=30s",
          "-max_get_entries=256",
          "-by_range=true",
          "-align_getentries=true",
          "-mask_internal_errors=true"
          ]

Our trillian log_server and log_signer's contain the following config. Will a similar code change to what you've done here also need to be done for the log_server and log_signer when a similar PR lands in the trillian repo?

        "-etcd_servers=etcd-cluster-client:2379",
        "-etcd_http_service=logserver-http",

Assuming that I switch my ctfe's to use etcd, am I correctly understanding that the config would look like:

        "-etcd_http_service=logserver-http/",
        "-etcd_metrics_service=logserver-metrics/",

You're right that feels a bit dodgy. I am onboard with a single service with two endpoints.

Revert the changelog as there is no delta now.
Copy link
Contributor

@pav-kv pav-kv left a comment

Choose a reason for hiding this comment

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

This is backward-compatible, as far as I can tell. LGTM.

trillian/ctfe/ct_server/main.go Show resolved Hide resolved
@pav-kv
Copy link
Contributor

pav-kv commented Mar 3, 2021

Update the PR desc?

@mhutchinson
Copy link
Contributor Author

I've updated the PR description 👍

@mhutchinson
Copy link
Contributor Author

@pgporada thanks for your comments here. I went back to look at Trillian and then found out my expectation of the previous behaviour didn't match the reality. See google/trillian#2381. It turns out that the endpoints were already announced under a foo/bar format, and I've changed this PR to update etcd APIs without any functional changes.

@mhutchinson mhutchinson merged commit 1df04e9 into google:master Mar 3, 2021
@mhutchinson mhutchinson deleted the updateEmAll branch March 3, 2021 15:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants