-
Notifications
You must be signed in to change notification settings - Fork 14
external mongod #308
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
base: search/public-preview
Are you sure you want to change the base?
external mongod #308
Conversation
MCK 1.2.1 Release NotesOther Changes
|
… anandsyncs/external-mongod
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.
Looks good! I just recommend explicitly using distinct names for the mdbc and mdbs resources to make sure the implicit name matching isn't kicking in at all.
Please resolve the lint issue and merge.
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've left some comments
type ExternalMongodTLS struct { | ||
Enabled bool `json:"enabled"` | ||
// +optional | ||
CASecretRef *userv1.SecretKeyRef `json:"caSecretRef,omitempty"` |
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.
CA doesn't need to be a secret. See TLSConfig
struct used in our CRDs
} | ||
|
||
func (r *externalSearchResource) IsSecurityTLSConfigEnabled() bool { | ||
if r.spec.TLS != nil { |
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.
nit: we can simplify as return r.spec.TLS != nil && r.spec.TLS.Enabled
@@ -31,27 +35,78 @@ const ( | |||
// | |||
// TODO check if we could use already existing interface (DbCommon, MongoDBStatefulSetOwner, etc.) | |||
type SearchSourceDBResource interface { | |||
Name() string |
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.
Could you please check if this removal won't clash with enterprise support in #309?
sample_movies_helper.assert_search_query(retry_timeout=60) | ||
|
||
|
||
def get_connection_string(mdbc: MongoDBCommunity, user_name: str, user_password: str) -> str: |
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.
we should probably make this an utility function as we have few copies of it already
{ | ||
"mongotHost": mongot_host, | ||
"searchIndexManagementHostAndPort": mongot_host, | ||
"skipAuthenticationToSearchIndexManagementServer": 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.
why no searchTLSMode in here? Is it defaulting to true?
|
||
@mark.e2e_search_external_tls | ||
def test_validate_tls_connections(mdbc: MongoDBCommunity, mdbs: MongoDBSearch, namespace: str, issuer_ca_filepath: str): | ||
with pymongo.MongoClient( |
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.
can we use ReplicaSetTester.assert_connectivity
for checking connection to mongodb? It guess it's doing exactly what we do here.
@@ -834,7 +837,8 @@ func getMongodConfigModification(mdb mdbv1.MongoDBCommunity) automationconfig.Mo | |||
// getMongodConfigModification will merge the additional configuration in the CRD | |||
// into the configuration set up by the operator. | |||
func getMongodConfigSearchModification(search *searchv1.MongoDBSearch) automationconfig.Modification { | |||
if search == nil { | |||
// Condition for skipping add parameter if it is external mongod |
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.
nit: unnecessary comment as it's just explain this simple code
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.
ah yes, I simplified the logic but didn't remove the comment, thanks for pointing out.
Summary
Proof of Work
Checklist
skip-changelog
label if not needed