-
Notifications
You must be signed in to change notification settings - Fork 481
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
CLOUDP-66230: Add E2E tests for TLS #81
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is looking great! I think as we add more and more functionality, we need to figure out a cleaner way of structuring our tests. I think this is out of scope for this PR, but it's something we should have a think about!
*metav1.NewControllerRef(&mdb, schema.GroupVersionKind{ | ||
Group: mdbv1.SchemeGroupVersion.Group, | ||
Version: mdbv1.SchemeGroupVersion.Version, | ||
Kind: mdb.Kind, | ||
}))) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe we should extract this into a defaultOwnerReference
function or similar. I think we have this call to metav1.NewControllerRef
in several places now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed this by refactoring all tests to use the BasicFunctionality
test. Now this owner reference only appears there.
mdb := e2eutil.NewTestMongoDB("mdb0") | ||
mdb.Spec.Security.TLS = e2eutil.NewTestTLSConfig(false) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel like we're going to need a better way of building MongoDB resources, perhaps a builder or even for now just a separate function to fully produce a TLS enabled MDB resource. I would also suggest maybe making the resource name a bit more meaningful, even just "mdb-tls" or "mdb-tls-upgrade"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Have updated the tests to use the name "mdb-tls" instead. As the name is embedded in the certificate I've opted to use the same for both, otherwise we would need one certificate for each test.
We discussed a builder separately and agreed it would be a nice addition. That work is tracked by a different ticket.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed this in person, make sure the E2E test pass and are stable before merging (they failed in the previous run)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks great! Thanks for all the hard work on this one. Great job keeping as much as possible unexported and removing duplication with the grouped test functions for common test cases. 👍
Good to merge once all the tests pass!
return func(t *testing.T) { | ||
ctx, cancel := context.WithCancel(context.Background()) // start a go routine which will periodically check basic MongoDB connectivity | ||
defer cancel() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
@rodrigovalin I believe the test failure was caused by what I fixed in "Move context cancel outside goroutine". I'm going to run it a few times now to see if I can trigger another failure, otherwise I'll go ahead and merge. |
All Submissions:
This PR adds two E2E tests for TLS, they are:
replica_set_tls
: A test for creating a new TLS-enabled cluster including a connectivity test over TLS.replica_set_tls_upgrade
: A test which enables TLS on an existing cluster and ensures the cluster is reachable during the upgrade.The test certificates and keys are pre-generated and stored in
testdata/tls
. This PR doesn't contain the CRD updates as it was already getting a bit big. Those changes will be a separate PR.