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.

Sign up for GitHub

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

connect: persist intermediate CAs on leader change #4379

Merged
merged 4 commits into from
Jul 12, 2018
Merged

connect: persist intermediate CAs on leader change #4379

Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
1d3f4b5
connect: persist intermediate CAs on leader change
kyhavlov Jul 11, 2018
462ace4
connect: update leader initializeCA comment
kyhavlov Jul 11, 2018
4e5fb6b
connect: add provider state to snapshots
kyhavlov Jul 11, 2018
f95c680
connect: use reflect.DeepEqual instead for test
kyhavlov Jul 11, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 4 additions & 2 deletions agent/consul/leader_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ package consul

import (
"os"
"reflect"
"testing"
"time"

Expand All @@ -12,7 +13,6 @@ import (
"github.com/hashicorp/consul/testutil/retry"
"github.com/hashicorp/net-rpc-msgpackrpc"
"github.com/hashicorp/serf/serf"
"github.com/pascaldekloe/goe/verify"
"github.com/stretchr/testify/require"
)

Expand Down Expand Up @@ -1140,6 +1140,8 @@ func TestLeader_PersistIntermediateCAs(t *testing.T) {
}

_, newLeaderRoot := leader.getCAProvider()
verify.Values(r, "", root, newLeaderRoot)
if !reflect.DeepEqual(newLeaderRoot, root) {
r.Fatalf("got %v, want %v", newLeaderRoot, root)
}
Copy link
Member

@banks banks Jul 11, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

(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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member

@banks banks Jul 12, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

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...

})
}