Move code inside internal directory #102

phbnf · 2025-02-05T16:47:16Z

This PR moves as many things as possible under /internal.

To achieve this, it also:

moves the definition of the Log struct Storage interface in handlers.go, where they are actually used
makes a few private fields / structs public, which is ok since they are defined in /internal
moves storage.go to storage/storage.go

AlCutter · 2025-02-05T18:05:32Z

ctlog.go

-	log := &log{}
+var timeSource = systemTimeSource{}
+
+func NewLog(ctx context.Context, origin string, signer crypto.Signer, cfg ChainValidationConfig, cs CreateStorage) (*scti.Log, error) {


Returning instances of structs from an internal package undermines the primary benefit you get from putting things in there, which is that you're free to refactor to your heart's content because nobody can possibly be relying on the APIs inside. If you "smuggle" instances/funcs out, then you can no longer change that surface as it's now public.

You also cause some weird behaviour for users of the package: e.g. this works:

myLog := ctlog.New() myLog.DoThings()

but these won't compile:

struct MyApp { myLog *scti.Log // NO ... } ... myApp := MyApp{ myLog: ctlog.New(), } ...

func main() { myLog := ctlog.New() doSomething(myLog) } func doSomething(l *scti.Log) { // NO log("I'm going to do things") l.DoThings() }

Yeah, I did ponder about it for a long time, and concluded that it was an okay compromise, if only, as a stepping stone. If a struct is defined in the internal directory, it seems reasonable to me to assume that if you're depending on it, you're on your own. The crux of the issue is not really that the return struct is from an internal package, but rather that it has public fields, which could be accessed. If there were all private, it wouldn't be a problem.

Similarly, Tessera public methods export methods which reference private structures in their signature, such as WithCheckpointSigner. The main difference is that these objects can't be used outside of internal because they all take a private object as an argument options.StorageOptions, which can't be created outside of the internal package.

I had three solutions in mind before I sent this PR:

Move more code from main.go into ctlog.go such that it registers the HTTP handlers itself. I'm definitely open to it, I thought I wouldn't move too much code in one go and leave this one for later. It's my preferred follow up (I could even send a PR beforehand and rebase everything). The downside of this is that you don't really see what happens under the hood when you look at main.go, as opposed to seing clearly that a log is created, then handlers are being created. Here's what it could look like.

To make all the fields of this struct private (hence, not accessible), I'd need to move NewLog to the internal directory, and then write a wrapper inside ctlog.go that calls it. That's what we talked about in London, here's a quick dirty shot a it. I didn't pursue this option after our conversation in London.

Leave the Log struct and Storage interface explicitly public, which signs us up to maintaining them. It's my least preferred.

Do you have any preference?

I think (1) is a good plan - returning a http.Handler (which internally could be a http.ServeMux with all the static CT handlers registered) would let alternative main implementations use other stacks (e.g. Gorilla) while being keeping internal stuff private.

I do think that whatever else is returned should also be an exported public thing, this is the public API of the static CT implementation after all (and, otherwise, you'll still have the weirdness of not being able to pass it around, or put it in a declared variable, etc.). But, of course, the public API is not set in stone until we tag a 1.0, so it's fine to evolve it.

Should be better now that #114 has been merged. This likely unblocked further code refactoring, but let's handle them in followup PRs.

# Conflicts: # internal/handlers.go # Conflicts: # cmd/gcp/main.go # ctlog.go # Conflicts: # ctlog.go

# Conflicts: # cmd/gcp/main.go # ctlog.go # ctlog_test.go # Conflicts: # ctlog.go # internal/scti/handlers.go

AlCutter · 2025-02-07T16:49:15Z

cmd/gcp/gcp

I imagine you didn't mean to git add this file? :)

AlCutter · 2025-02-07T16:51:42Z

ctlog.go

+// systemTimeSource implments scti.TimeSource.
+type systemTimeSource struct{}
+
+// Now returns the true current local time.
+func (s systemTimeSource) Now() time.Time {
+	return time.Now()


For follow-up, could this just be a func() time.Time that takes time.Now as a default value?

AlCutter · 2025-02-07T16:54:37Z

ctlog.go


 // newLog instantiates a new log instance, with write endpoints.
 // It initiates chain validation to validate writes, and storage to persist
 // chains.
-func newLog(ctx context.Context, origin string, signer crypto.Signer, cfg ChainValidationConfig, cs CreateStorage) (*log, error) {
-	log := &log{}
+func newLog(ctx context.Context, origin string, signer crypto.Signer, cfg ChainValidationConfig, cs CreateStorage) (*scti.Log, error) {


Should/could newLog live next in scti? It looks a bit like a c'tor for scti.Log?

Moved it there.

AlCutter · 2025-02-07T16:56:36Z

ctlog.go


 	return log, nil
 }

 // newCertValidationOpts checks that a chain validation config is valid,
 // parses it, and loads resources to validate chains.
-func newCertValidationOpts(cfg ChainValidationConfig) (*chainValidationOpts, error) {
+func newCertValidationOpts(cfg ChainValidationConfig) (*scti.ChainValidationOpts, error) {


Could this go in scti too?
Similarly, it's creating and returning structs defined in scti and looks quite like it's part of that package, and I think you wouldn't need to export ChainValidationOpts if it were in there since it's really just an internal detail of Log(?)

Actually, that'd potentially reduce the size of this PR quite a lot since many of the changed lines are just flipping the case of newly exported structs and fields & references to them.

This one is a bit harder to move: the Config struct has to be defined somewhere that's visible to external libraries.

Eventually, this options are used to call validateChain(chain, options). I think the right thing to do is to re-architecture the code for it to call methods (see TODO). I'll come to this later. This is specifically cumbersome to do for this one because these options are also used for the getRoots endpoint.... which is already a tricky one to handle with the x509 migration. So one step at a time :)

What I've done though, is to move the OID and ExtKeyUsage parsing functions to chain_validation.go

In the meantime, to make this PR easier to digest, I can add a constructor in internal. That will save the PR from all the lowercase-->uppercase changes.

AlCutter · 2025-02-07T17:01:41Z

internal/scti/handlers.go

-// pathHandlers maps from a path to the relevant AppHandler instance.
-type pathHandlers map[string]appHandler
+// PathHandlers maps from a path to the relevant AppHandler instance.
+type PathHandlers map[string]appHandler


Do you still need to export this?

AlCutter · 2025-02-07T17:04:40Z

internal/scti/timesource.go

 // specific values.
-type timeSource interface {
+type TimeSource interface {


(follow up fine) I think this could just be type TimeSource func() time.Time

phbnf marked this pull request as ready for review February 5, 2025 17:04

phbnf requested review from AlCutter and roger2hk February 5, 2025 17:04

phbnf assigned AlCutter and roger2hk Feb 5, 2025

AlCutter reviewed Feb 5, 2025

View reviewed changes

phbnf mentioned this pull request Feb 6, 2025

Move more logic into ctlog.go #114

Merged

phbnf added 5 commits February 7, 2025 12:31

move things to internal

e375aea

# Conflicts: # internal/handlers.go # Conflicts: # cmd/gcp/main.go # ctlog.go # Conflicts: # ctlog.go

more internal things, and move things to storage

3146922

# Conflicts: # cmd/gcp/main.go # ctlog.go # ctlog_test.go # Conflicts: # ctlog.go # internal/scti/handlers.go

move structs around

4041ebd

update comment

defb6c9

move testdata to internal

61163e2

phbnf force-pushed the internal branch from d9af84e to 61163e2 Compare February 7, 2025 12:46

AlCutter reviewed Feb 7, 2025

View reviewed changes

phbnf force-pushed the internal branch from 49cb631 to b1f2b28 Compare February 10, 2025 14:46

move newLog to internal

8ee5d18

phbnf force-pushed the internal branch from b1f2b28 to 8ee5d18 Compare February 10, 2025 14:47

AlCutter approved these changes Feb 10, 2025

View reviewed changes

phbnf added 2 commits February 10, 2025 16:09

add constructor, make fields private

536496b

more things private

37c47ed

phbnf force-pushed the internal branch from aa52a22 to 37c47ed Compare February 10, 2025 16:10

AlCutter approved these changes Feb 10, 2025

View reviewed changes

phbnf merged commit caa75b4 into transparency-dev:main Feb 10, 2025
7 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Move code inside internal directory #102

Move code inside internal directory #102

Uh oh!

phbnf commented Feb 5, 2025 •

edited

Loading

Uh oh!

AlCutter Feb 5, 2025

Uh oh!

phbnf Feb 6, 2025

Uh oh!

AlCutter Feb 6, 2025

Uh oh!

phbnf Feb 7, 2025

Uh oh!

AlCutter Feb 7, 2025

Uh oh!

AlCutter Feb 7, 2025

Uh oh!

AlCutter Feb 7, 2025

Uh oh!

phbnf Feb 10, 2025

Uh oh!

AlCutter Feb 7, 2025

Uh oh!

phbnf Feb 10, 2025

Uh oh!

phbnf Feb 10, 2025

Uh oh!

phbnf Feb 10, 2025

Uh oh!

AlCutter Feb 7, 2025

Uh oh!

AlCutter Feb 7, 2025

Uh oh!

Uh oh!

Uh oh!

Move code inside internal directory #102

Move code inside internal directory #102

Uh oh!

Conversation

phbnf commented Feb 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

phbnf commented Feb 5, 2025 •

edited

Loading