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

Possibility of splitting out the golang CT client into own repo/module #637

Open
rolandshoemaker opened this issue Jan 8, 2020 · 8 comments
Assignees
Labels
Projects

Comments

@rolandshoemaker
Copy link
Contributor

In a golang modules setup vendoring google/certificate-transparency-go in order to use google/certificate-transparency-go/client is somewhat painful. Because of the extensive number of other projects contained within this repo doing an update of the package can pull in significant unrelated changes and transitive dependencies that make reviewing any changes extremely difficult.

While I understand there are valid reasons for wanting to group together as much of this related code as possible it'd be extremely useful to outside users of these smaller libraries if they could be split out entirely (the more radical approach) or made into independent golang modules (as documented under Multi-Module Repositories in the modules faq).

(Vaguely related to #636)

@RJPercival RJPercival added this to Needs triage in CT via automation Jan 8, 2020
@RJPercival RJPercival moved this from Needs triage to Low priority in CT Jan 8, 2020
@RJPercival
Copy link
Contributor

RJPercival commented Jan 8, 2020

We're supportive of this change, but don't currently have the bandwidth to prioritise it. Would you be willing to undertake it @rolandshoemaker?

@pav-kv
Copy link
Contributor

pav-kv commented Jan 8, 2020

I think we should make it a Golang module. I suspect there will be some dependencies that both client and server need, like merkle package, and don't think it would be convenient to create many repos for those.

@jsha
Copy link
Contributor

jsha commented Jan 8, 2020

I mentioned over on #636 that we're willing to spend some time on this.

If we go the module route, we'd also need to create separate modules for each mutual dependency in order to achieve the goal of minimizing dependencies. That might wind up kind of heavyweight - what do you think?

One thing I noticed when starting to look at this: The ctclient code uses the forked x509 code used by ct-go in general. AFAICT this mainly supports GetEntries being able to return partially parsed results even when Go's x509 package would return a parse error.

It would be nice to switch the client code to use the Go x509 package for simplicity. But I wouldn't want to break the existing behavior of GetEntries, which people probaly rely on. Maybe this suggests that the right split would be to make a separate ct_adder client that is just concerned with submitting certificates? I think this would also allow the client to skip, e.g. the merkle dependency.

@pphaneuf
Copy link
Contributor

pphaneuf commented Jan 9, 2020

A number of users of the client have had problems with the parsing anyway (hence the GetRawEntries method), it would make sense in my opinion to layer on that kind of functionality? Have a low level client that does a minimal job, with equally minimal dependencies, and have a separate, higher level client that might provide that sort of help (or it could be free functions in separate packages/modules that take the low level client as a parameter, whatever is more convenient).

This also applies to the retry behaviour, which has to be avoided for some applications, but could probably easily be done as a wrapper?

@rolandshoemaker
Copy link
Contributor Author

I'm happy to take a crack at it, currently I have a few things on my plate I need to finish first but I should be able to give it a go sometime next week.

@rolandshoemaker
Copy link
Contributor Author

rolandshoemaker commented Feb 5, 2020

I've spent some time trying to work through this, and have come to the opinion that trying to make a number of packages within this current repo modules ends up being rather complex, and doesn't actually provide any real value to third-party users.

The main reasoning for this is that the main repo is already a module, and as such transforming existing packages within the module to their own modules is painful. This is because converting a package within a module to its own module requires doing a reverse-require (i.e. github.com/google/certificate-transparency-go/client would have to require github.com/google/certiifcate-transparency-go, see this section of the go mod faq for more on this) to avoid ambiguous import issues. This means that if we were to make the client libraries their own modules they would still need to pull in everything in the parent repo, which would go against the initial reasoning for wanting to do this in the first place.

I think the best path forward would be to create a new repository, something like github.com/google/ct-client, and move client, jsonclient, dnsclient, x509, x509util, tls, asn1, and loglist into it (possibly as a single module, or as a bunch of submodules, no strong opinion on this to be honest, probably skew towards mono-module). The packages could be left in this repo for a while to aid transition, and then slowly start switching imports from here to the new repo (which should avoid breaking anything, although people still using pre-modules golang might still have issues). This would then leave us with a single paired down client module/repo which wouldn't pull in all the extra bits and bobs the unrelated code in this repo needs (there would still be some cruft pulled in via a transitive dependency on the trillian module, which could be addressed by moving the merkle package into the new repo, but that gets a bit more complicated).

@RJPercival
Copy link
Contributor

That sounds great to me - I was expecting they'd move into a separate repo. I'd recommend moving the x509 and asn1 packages into separate repos though, since they're already used separately from the client and have unique test requirements that could be better accommodated if they were in a separate repo (e.g. OS-dependent build tags). They wouldn't need a dependency on Trillian either.

@rolandshoemaker
Copy link
Contributor Author

Makes sense to me.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
CT
  
Low priority
Development

No branches or pull requests

6 participants