connect: persist intermediate CAs on leader change

[kyhavlov]

This fixes the issue with intermediate CA certs not being present on the new leader after a leader change.

[banks]

Good job, I thought this was going to be a lot more hairy to fix for some reason but seems good.

[banks]

This comment is now misleading - we don't just exit if the CA is already bootstrapped, we still importantly need to load the intermediates into the CA instance.

[mkeeler]

Is this somehow better than require.Equal(r, root, newLeaderRoot).

Regardless of what we choose, we should decide on which testing helper framework to use and stick with it. I had been encouraged to use the github.com/stretchr/testify/require package when I started so all the tests I have been writing use that.

[kyhavlov]

retry.R doesn't quite satisfy the TestingT interface there - i'll just change this to reflect.DeepEqual instead.

[banks]

See discussion on #4379 (comment)

[banks]

(Bikeshed alert) Why not require? The error message is much better. Does it not with Retry?

@mkeeler I agree testify is our current preference. I've never worked out if verify does something that makes it compatible with retry as I tend to see them used together but if not I agree with require in general and prefer it to DeepEqual in pretty much every case.

[kyhavlov]

require.Equal doesn't take retry.R, so it doesn't work here. I agree that'd be nicer than DeepEqual if it worked.

[banks]

Gotcha. no need to change this again for now but I was assuming this was the case and that verify was something we'd written to work with retry.

When Matt pointed out it's thirdparty I wondered if I'd just been wrong about testify not working in retry funcs. I guess it works with retry because it has an interface that mimics *testing.T which retry.R implements whereas testify actually requires a real *testing.T.

IMO, verify is still fine to use in retry funcs given that it produces nicer output and actually works. I think long-term it would be nice to come up with a wrapper in retry that allows it to work with testify and then remove all use of verify in the code base but that's not a thing for this PR so my $0.02 is that verify is still fair game only inside retry loops until we fix retry to be compatible with testify.

But land this as it is and we can take this discussion elsewhere if people feel it's important to clean that up.

[banks]

https://github.com/stretchr/testify/blob/f35b8ab0b5a2cef36673838d662e249dd9c94686/require/requirements.go#L4-L7

Ah so testify uses an interface too so it should actually be trivial to make retry.R compatible. I propose we make a task to do that and clean up all uses of verify separate from this PR.

It's always bugged me I can't use assert/require in retry loops but I assumed it was going to be a non-trivial fix.

[banks]

Hmm, wait:

goe/testing.TB (used by verify.Values:

type TB interface {
    Error(args ...interface{})
    Errorf(format string, args ...interface{})
    Fail()
    FailNow()
    Failed() bool
    Fatal(args ...interface{})
    Fatalf(format string, args ...interface{})
    Log(args ...interface{})
    Logf(format string, args ...interface{})
    Name() string
    Skip(args ...interface{})
    SkipNow()
    Skipf(format string, args ...interface{})
    Skipped() bool
    Helper()
    // contains filtered or unexported methods
}

require.TestingT:

type TestingT interface {
	Errorf(format string, args ...interface{})
	FailNow()
}

The second is a strict subset of the first as far as I can see. Anything that works with verify.Values should work with require.

...

Ah mystery solved. We are using an old version of verify that takes:

type Errorer interface {
	Error(args ...interface{})
}

// Values verifies that got has all the content, and only the content, defined by want.
// Note that NaN always results in a mismatch.
func Values(r Errorer

retry.R implements that, but not Errorf. So just making an Errorf would be all that's needed to make require work in retry loops.

There are lots of old places using verify though so it's a big clean up...