Skip to content

Move code inside internal directory #102

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

Merged
merged 8 commits into from
Feb 10, 2025
Merged

Conversation

phbnf
Copy link
Collaborator

@phbnf phbnf commented Feb 5, 2025

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

@phbnf phbnf marked this pull request as ready for review February 5, 2025 17:04
@phbnf phbnf requested review from AlCutter and roger2hk February 5, 2025 17:04
ctlog.go Outdated
log := &log{}
var timeSource = systemTimeSource{}

func NewLog(ctx context.Context, origin string, signer crypto.Signer, cfg ChainValidationConfig, cs CreateStorage) (*scti.Log, error) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

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()
} 
   

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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:

  1. 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.
  2. 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.
  3. 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?

Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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

phbnf added 5 commits February 7, 2025 12:31
# 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
cmd/gcp/gcp Outdated
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

ctlog.go Outdated
Comment on lines 73 to 78
// systemTimeSource implments scti.TimeSource.
type systemTimeSource struct{}

// Now returns the true current local time.
func (s systemTimeSource) Now() time.Time {
return time.Now()
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

ctlog.go Outdated

// 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) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Moved it there.


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) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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 :)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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.

// 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
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do you still need to export this?

// specific values.
type timeSource interface {
type TimeSource interface {
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

@phbnf phbnf merged commit caa75b4 into transparency-dev:main Feb 10, 2025
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants