Unable to integrate pebble due to vendoring

[bluecmd]

Hi,

I'm trying to integrate Pebble in my integration test like this:

package utils

import (
	"log"
	"net/http"
	"os"

	"github.com/jmhodges/clock"
	"github.com/letsencrypt/pebble/ca"
	"github.com/letsencrypt/pebble/db"
	"github.com/letsencrypt/pebble/va"
	"github.com/letsencrypt/pebble/wfe"
)

type caServer struct {

}

func NewTestCA() *caServer {
	logger := log.New(os.Stdout, "Pebble ", log.LstdFlags)
	clk := clock.New()
	db := db.NewMemoryStore(clk)
	ca := ca.New(logger, db)

	// Enable strict mode to test upcoming API breaking changes
	strictMode := true
	va := va.New(logger, clk, 80, 443, strictMode)
	wfeImpl := wfe.New(logger, clk, db, va, ca, strictMode)
	muxHandler := wfeImpl.Handler()
[..]

When compiling I get these errors:

../../../utils/acme.go:26:25: cannot use clk (type "github.com/jmhodges/clock".Clock) as type "github.com/letsencrypt/pebble/vendor/github.com/jmhodges/clock".Clock in argument to db.NewMemoryStore:
	"github.com/jmhodges/clock".Clock does not implement "github.com/letsencrypt/pebble/vendor/github.com/jmhodges/clock".Clock (wrong type for NewTimer method)
		have NewTimer(time.Duration) *"github.com/jmhodges/clock".Timer
		want NewTimer(time.Duration) *"github.com/letsencrypt/pebble/vendor/github.com/jmhodges/clock".Timer
../../../utils/acme.go:31:14: cannot use clk (type "github.com/jmhodges/clock".Clock) as type "github.com/letsencrypt/pebble/vendor/github.com/jmhodges/clock".Clock in argument to va.New:
	"github.com/jmhodges/clock".Clock does not implement "github.com/letsencrypt/pebble/vendor/github.com/jmhodges/clock".Clock (wrong type for NewTimer method)
		have NewTimer(time.Duration) *"github.com/jmhodges/clock".Timer
		want NewTimer(time.Duration) *"github.com/letsencrypt/pebble/vendor/github.com/jmhodges/clock".Timer
../../../utils/acme.go:32:20: cannot use clk (type "github.com/jmhodges/clock".Clock) as type "github.com/letsencrypt/pebble/vendor/github.com/jmhodges/clock".Clock in argument to wfe.New:
	"github.com/jmhodges/clock".Clock does not implement "github.com/letsencrypt/pebble/vendor/github.com/jmhodges/clock".Clock (wrong type for NewTimer method)
		have NewTimer(time.Duration) *"github.com/jmhodges/clock".Timer
		want NewTimer(time.Duration) *"github.com/letsencrypt/pebble/vendor/github.com/jmhodges/clock".Timer

I believe this is an unfortunate reality of Golang's current vendoring system. Maybe Go modules will solve it in the future - but for now that doesn't help me sadly :-).

Would you consider taking in Clock as an interface instead? Something like:

type Clock interface {
  func Now() something
  func NewTimer() Timer
}

type Timer interface {
  ...
}

That should bypass the vendoring issue.
Thanks for your consideration,

[cpu]

👋 Hi @bluecmd - thanks for opening this issue.

I'm trying to integrate Pebble in my integration test like this:

That's interesting - it isn't how we've imagined Pebble being integrated into integration tests to date. Pebble isn't written to be consumed as a Go library. I suspect there will be many points of refactoring required (and the other issues you've been flagging give me the same sense).

Did you consider running Pebble as an external process for your integration testing? I know this is clunky for a Go project but I think it will be an easier experience with the current codebase.

Would you consider taking in Clock as an interface instead?

I'm on the fence about this. I don't have the cycles to drive the refactoring that would be needed to make Pebble easy to use as a Go library. Taking smaller patches that address one-off problems might be the wrong approach - its a fairly fundamental mismatch between how Pebble was written and how you're using it.

@jsha What do you think?

[bluecmd]

Hi! 👋

I 100% sympathize with you hesitating to add an API guarantee :-).
Let me see if I can convince you there is value in it, and I'll leave the judgement if it's worth to have Pebble offer that kind of API to you.

Did you consider running Pebble as an external process for your integration testing?

Yes, and it's something I'm trying to optimize away from in the integration test suite. Tracking the lifetime of external servers and making sure the state is cleaned up, as well as communicating through temporary files is sadly usually error prone and leads to flaky tests :-(. I explicitly chose Pebble because I'd rather keep up with a unstable API (since I vendor it it's not that big of a deal) than having to debug state across processes.

It's certainly a bit awkward to call os.Setenv in the middle of a test case, but overall it works pretty well :-).

If you want to see the usages with your own eyes, have a look at https://github.com/u-root/u-bmc/blob/master/pkg/bmc/cert/load_test.go and https://github.com/u-root/u-bmc/blob/master/integration/utils/acme.go - one is an unit integration test, and one is an end-to-end test (confusingly enough called "integration", but is more end-to-end).

If I could chose an API freely, from the use cases I have now it would look like:

type PebbleConfig struct {
  Validate bool
  Strict bool
  ..
}

type PebbleServer struct {
  RootCA *tls.Certificate,
  Handler net.Handler,
}
func NewPebbleServer(l Logger, clk Clock, c PebbleConfig) *PebbleServer

It's still a bit early here so I might edit this above if I remember something down the line :-)

To re-iterate: I have no expectations that this will become a supported use of Pebble, I'll make do with the status quo.

[jsha]

Hi @bluecmd,

Welcome to the Pebble repo! Glad you're finding it useful for integration testing.

I definitely think it's worth going through the hassle of spinning up and spinning down a separate process for Pebble. I know it's a hassle, but it produces a more realistic integration test. You're less likely to accidentally hide important behavior by virtue of being in the same process space.

That said, I think your import error is actually solved in current Go vendoring, with or without modules. It's hard to say without seeing everything you're running, but my guess is that somewhere, something is imported using it's vendored path rather than it's real path. I would recommend grepping for "vendor" among your imports. I assume you're on a version of Go that handled vendoring by default?

[bluecmd]

Well, we will have to agree to disagree then :-). When running the project in a simulator Pebble is run in stand alone mode, but when running the 10s of integration tests I'm trying really hard to not pollute state between the processes and having to parse output like when dynamically binding to any open port to allow concurrent tests to run.

Anyway, for the vendoring issue. No, I'm sad to say that this is one of the things I find surprisingly bad about go. Stuff really quite breaks when you do what is currently done in Pebble - but it's fine, as everybody else is doing it. I guess today you more or less have to vendor.

[bluecmd]$ ls -la                                                                       [test] [10:20:45]
total 12
drwxr-xr-x  2 bluecmd primarygroup 4096 Apr  8 10:20 .
drwxr-xr-x 36 bluecmd primarygroup 4096 Apr  8 10:19 ..
-rw-r--r--  1 bluecmd primarygroup  157 Apr  8 10:20 main.go
[bluecmd]$ pwd                                                                          [test] [10:20:47]
/home/bluecmd/go/src/github.com/test
[bluecmd]$ cat main.go                                                                  [test] [10:20:49]
package main

import (
	"github.com/jmhodges/clock"
	"github.com/letsencrypt/pebble/db"
)

func main() {
	clk := clock.New()
	db := db.NewMemoryStore(clk)
}
[bluecmd]$ go vet .                                                                     [test] [10:20:52]
# github.com/test [github.com/test.test]
./main.go:10:25: cannot use clk (type "github.com/jmhodges/clock".Clock) as type "github.com/letsencrypt/pebble/vendor/github.com/jmhodges/clock".Clock in argument to db.NewMemoryStore:
	"github.com/jmhodges/clock".Clock does not implement "github.com/letsencrypt/pebble/vendor/github.com/jmhodges/clock".Clock (wrong type for NewTimer method)
		have NewTimer(time.Duration) *"github.com/jmhodges/clock".Timer
		want NewTimer(time.Duration) *"github.com/letsencrypt/pebble/vendor/github.com/jmhodges/clock".Timer

Edit: Go version reports go version go1.12.1 linux/amd64

[cpu]

One thought: Pebble is using github.com/jmhodges/clock primarily because of its common heredity with Boulder and the thought we'd eventually have tests that needed to mock time.

Pebble doesn't have any tests and it seems like carrying the jmhodges/clock dependency isn't justified. If removing the dependency would also fix this vendoring issue then it seems like a double win. If we add tests I think the vast majority won't require mocking time since Pebble is considerably simpler than Boulder.

Changing to an interface would mostly benefit the library use-case that we can't support and would also leave the dep so I think its a less compelling avenue.

[cpu]

@bluecmd Does this branch #231 integrate better? It isn't my intention to solve the library usage problem with this PR but perhaps it will work better as a happy side-effect.

[bluecmd]

Yep, that works!

[cpu]

@bluecmd Great, thanks for confirming. Since #231 is merged I think we can close this issue out and move on to the locking.