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

Add clock mock plumbing throughout the project #408

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

pzduniak
Copy link

@pzduniak pzduniak commented May 30, 2023

Hey, it appears that the repo included github.com/jmhodges/clock as a dependency at some point, but it was removed (#231).

As I was trying to integrate pebble into our integration tests, I ran into the issue of the lack of the clock mocks, which was a dealbreaker. I reintroduced the dependency here, unfortunately it breaks the external API and it's probably not done the way you, the maintainers, would approach it. I'm open to spending some time on fine tuning this PR to better match the rest of the codebase stylistically. This was the bare minimum of changes I had to make to make it all work.

Copy link
Contributor

@aarongable aarongable left a comment

Choose a reason for hiding this comment

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

Thanks for the PR, this generally looks good to me! Just a few small comments, but nothing big and no concerns from me. Thanks!

@@ -8,6 +8,7 @@ import (
"os"
"strconv"

"github.com/jmhodges/clock"
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
"github.com/jmhodges/clock"
"github.com/jmhodges/clock"

@@ -34,6 +36,7 @@ type CAImpl struct {
log *log.Logger
db *db.MemoryStore
ocspResponderURL string
clockSource clock.Clock
Copy link
Contributor

Choose a reason for hiding this comment

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

Here and throughout, let's rename clockSource to clk, to match Boulder.

Comment on lines +360 to +362
if clockSource == nil {
ca.clockSource = clock.New()
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Two comments:

  1. Here and throughout, honestly I think it's fine to leave this check/default off. The clk can just be a required argument and pebble can just panic if it isn't provided. Just like it would if the db argument was nil.
  2. But if you really do want to keep it optional, do this check/default before the creation of the CAImpl on line 349.

db := db.NewMemoryStore()
ca := ca.New(logger, db, c.Pebble.OCSPResponderURL, alternateRoots, chainLength, c.Pebble.CertificateValidityPeriod)
va := va.New(logger, c.Pebble.HTTPPort, c.Pebble.TLSPort, *strictMode, *resolverAddress)
clockSource := clock.New()
Copy link
Contributor

Choose a reason for hiding this comment

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

Based on your PR description (you mention "breaking the external API") it sounds like you're using Pebble as a library, rather than as a standalone binary? In that case, your changes here make sense.

But if we're going to go to the trouble of giving Pebble full fake clock support, we should give it to everyone, including people who use Pebble as a binary (which I think is most users of Pebble).

To that end, let's replace this line with something a bit more like this, so that the fake clock can be initialized from an environment variable if someone wants to, or defaults to the system clock otherwise.

@pzduniak
Copy link
Author

pzduniak commented May 8, 2024

Thanks for the review, I'll address your comments when I have some free time.

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.

2 participants