Delete instance.go #89
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
AlCutter
approved these changes
Jan 15, 2025
cmd/gcp/main.go
Outdated
| signer, err := NewSecretManagerSigner(ctx, *signerPublicKeySecretName, *signerPrivateKeySecretName) | ||
| if err != nil { | ||
| klog.Exitf("Can't create secret manager signer: %v", err) | ||
| } | ||
| cpSigner, err := sctfe.NewCpSigner(signer, *origin, timeSource) | ||
| if err != nil { | ||
| klog.Exitf("failed to create checkpoint Signer: %v", err) |
cmd/gcp/main.go
Outdated
|
|
||
| storage, err := newGCPStorage(ctx, cpSigner) | ||
| if err != nil { | ||
| klog.Exitf("failed to initiate storage backend: %v", err) |
cmd/gcp/main.go
Outdated
| Validated: vCfg, | ||
| Deadline: *httpDeadline, | ||
| MetricFactory: prometheus.MetricFactory{}, | ||
| RequestLog: new(sctfe.DefaultRequestLog), |
There was a problem hiding this comment.
nit: match style for MetricFactory:
Suggested change
| RequestLog: new(sctfe.DefaultRequestLog), | |
| RequestLog: &sctfe.DefaultRequestLog{}, |
config.go
Outdated
|
|
||
| // Validate signer that only ECDSA is supported. | ||
| // TODO(phboneff): if this is a library this should also allow RSA as per RFC6962. |
There was a problem hiding this comment.
Devil's avocado: if it supports ECDSA then that's enough to build RFC6962-compliant logs; there's no requirement to support RSA (or, would we recommend anyone to use RSA for a new log, these days?)
config.go
Outdated
| if rejectExtensions != "" { | ||
| lRejectExtensions = strings.Split(rejectExtensions, ",") | ||
| if rejectExpired && rejectUnexpired { | ||
| return nil, errors.New("rejecting all certificates") |
There was a problem hiding this comment.
Suggested change
| return nil, errors.New("rejecting all certificates") | |
| return nil, errors.New("configuration would reject all certificates") |
config.go
Outdated
| Signer: signer, | ||
| // Validate the time interval. | ||
| if notAfterStart != nil && notAfterLimit != nil && (notAfterLimit).Before(*notAfterStart) { | ||
| return nil, errors.New("limit before start") |
There was a problem hiding this comment.
nit: worth doing fmt.Errorf with the dates in?
roger2hk
approved these changes
Jan 16, 2025
config.go
Outdated
| Signer: signer, | ||
| // Validate the time interval. | ||
| if notAfterStart != nil && notAfterLimit != nil && (notAfterLimit).Before(*notAfterStart) { | ||
| return nil, fmt.Errorf("not_after limit %q before start %q", notAfterLimit.Format(time.RFC3339), notAfterStart.Format(time.RFC3339)) |
There was a problem hiding this comment.
Suggested change
| return nil, fmt.Errorf("not_after limit %q before start %q", notAfterLimit.Format(time.RFC3339), notAfterStart.Format(time.RFC3339)) | |
| return nil, fmt.Errorf("not_after_limit %q before not_after_start %q", notAfterLimit.Format(time.RFC3339), notAfterStart.Format(time.RFC3339)) |
There was a problem hiding this comment.
I had done this on purpose to allow for shorter messages, and not to mention the flag there since they belong in main.go. As a compromise to address your comment, I changed "not_after" to "'Not After'", thus matching the common name of this field on certificates.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Towards #5
This PR reorganizes the SCTFE code. It deletes
instance.go, and migrate its content into:main.gofor everything related to setting up the log. Later PR will move some of this into the sctfe library.config.gofor everything related to CT log server logic.handlers.gofor everything related to http serving.It's a stepping stone, more simplifications are to come afterwards (private/public objets and libraries, file renamed, better structures, etc.). To start with, my goal is to architecture the code around three building blocks: