-
Notifications
You must be signed in to change notification settings - Fork 7
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
Conversation
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) { |
There was a problem hiding this comment.
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()
}
There was a problem hiding this comment.
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:
- Move more code from
main.go
intoctlog.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 atmain.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 insidectlog.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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
# 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
There was a problem hiding this comment.
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
// systemTimeSource implments scti.TimeSource. | ||
type systemTimeSource struct{} | ||
|
||
// Now returns the true current local time. | ||
func (s systemTimeSource) Now() time.Time { | ||
return time.Now() |
There was a problem hiding this comment.
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) { |
There was a problem hiding this comment.
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
?
There was a problem hiding this comment.
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) { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 :)
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
internal/scti/handlers.go
Outdated
// 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 |
There was a problem hiding this comment.
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 { |
There was a problem hiding this comment.
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
This PR moves as many things as possible under /internal.
To achieve this, it also:
Log
structStorage
interface inhandlers.go
, where they are actually used/internal
storage.go
tostorage/storage.go