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

Fix validation of references to CA certificate in TLS config #1119

Conversation

adamliesko
Copy link
Contributor

@adamliesko adamliesko commented Sep 17, 2022

Closes #1114 and #1054 on GH.

All Submissions:

  • Have you opened an Issue before filing this PR?

The issue has existed beforehand.

  • Have you signed our CLA?

Yes.

  • Have you checked to ensure there aren't other open Pull Requests for the same update/change?

#1115 Exists, however, is incomplete and is mixing the TLS validations into an (at least in my opinion) undesired place of different validations. But maybe you/we would like to actually broaden the scope of validations, happy to hear your preferred choice.

  • Put closes #XXXX in your comment to auto-close the issue that your PR fixes (if such).

@@ -87,6 +87,15 @@ func newScramReplicaSet(users ...mdbv1.MongoDBUser) mdbv1.MongoDBCommunity {
}

func newTestReplicaSetWithTLS() mdbv1.MongoDBCommunity {
return newTestReplicaSetWithTLSCaCertificateReferences(&mdbv1.LocalObjectReference{
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you please help me understand what this change bring to the table? I'm not sure if it changes anything?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It allows us to create test fixtures, where the caller passes custom arguments.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok, understood. Thanks

})
t.Run("Failure if reference to CA cert is missing", func(t *testing.T) {
mdb := newTestReplicaSetWithTLSCaCertificateReferences(nil, nil)

Copy link
Contributor

Choose a reason for hiding this comment

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

There's quite a lot of code duplication within this test. Can I ask you to re-implement it to the parameters-driven test, like https://github.com/mongodb/mongodb-kubernetes-operator/blob/master/test/e2e/replica_set_arbiter/replica_set_arbiter_test.go#L28 ?

Copy link
Contributor Author

@adamliesko adamliesko Oct 3, 2022

Choose a reason for hiding this comment

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

Yes sure. I initially started it as a table-driven test, but skimming through the codebase it didn't seem to me like a prevalent pattern.

@adamliesko adamliesko requested a review from slaskawi October 5, 2022 06:55
@slaskawi slaskawi added the safe-to-test Add this label to PRs from forks to trigger E2E tests label Oct 20, 2022
@slaskawi slaskawi merged commit cece58a into mongodb:master Oct 21, 2022
@slaskawi
Copy link
Contributor

Integrated, thanks @adamliesko

@dan-mckean
Copy link
Collaborator

Hey @adamliesko - I'm the PM for our MongoDB Kubernetes operators.

Wondered if you or any of your colleagues would be up for a chat about how you're getting on running MongoDB in Kubernetes?

[email protected]

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLOUDP-136914 safe-to-test Add this label to PRs from forks to trigger E2E tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[BUG] operator crashes if tls config is invalid
4 participants