CLOUDP-323995 CLOUDP-321068 Ingress and Egress TLS in mongot #278
Conversation
fealebenpae
force-pushed
the
fealebenpae/search-tls
branch
from
c8447d7 to
af0b239
Compare
fealebenpae
force-pushed
the
fealebenpae/search-tls
branch
from
af0b239 to
56e5519
Compare
| // CertificateKeySecret is a reference to a Secret containing a private key and certificate to use for TLS. | ||
| // The key and cert are expected to be PEM encoded and available at "tls.key" and "tls.crt". | ||
| // This is the same format used for the standard "kubernetes.io/tls" Secret type, but no specific type is required. | ||
| // Alternatively, an entry tls.pem, containing the concatenation of cert and key, can be provided. |
There was a problem hiding this comment.
do we normally support tls.pem AND tls.crt in secrets? I thought we're only supporting standard tls secret types so tls.key and tls.crt only. We might have some leftovers in the codebase handling tls.pem-type secrets but we shouldn't support them anymore for new things. Could you verify it's the case here?
There was a problem hiding this comment.
We’re using the community operator’s tls secret handling behind the scenes which does support the concatenated tls.pem field. I copied this comment directly from the Community CRD. I am happy to drop the tls.pem thing altogether, though.
There was a problem hiding this comment.
Let's drop it and keep it simple, exactly the same as tls-type secrets are defined 👍
| @@ -85,13 +89,28 @@ func (r *MongoDBSearchReconcileHelper) reconcile(ctx context.Context, log *zap.S | |||
| return workflow.Failed(err) | |||
| } | |||
|
|
|||
| mongotConfig := createMongotConfig(r.mdbSearch, r.db) | |||
| configHash, err := r.ensureMongotConfig(ctx, mongotConfig) | |||
| ingressTlsMongotModification, ingressTlsStsModification, err := r.ensureIngressTlsConfig(ctx) | |||
There was a problem hiding this comment.
nit: can we have Tls as uppercased TLS in those names?
There was a problem hiding this comment.
While I agree it's quite nice naming convention specifying mongot's server as ingress and mongot->mongod as egress, I'm a bit worried it's initially misleading/confusing as ingress in k8s is generally considered as a mechanism for exposing workloads externally.
There was a problem hiding this comment.
How about if we call them “query” and “replication” as the search team does?
There was a problem hiding this comment.
replication is regarded also as syncSource, at least in the config schema. Query seems OK, let's see how those work out in the codebase, but it's not super important
There was a problem hiding this comment.
That sounds good to me. I’m not attached to the names I used - I just wanted to see if things worked first.
|
|
||
|
|
||
| @fixture(scope="module") | ||
| def ca_certificate(tmp_path_factory: TempPathFactory): |
There was a problem hiding this comment.
I think we're relying on already generated ca.crt ca.key in the test resource files (look for ca.crt). So if we don't do anything more here than just ca file, we might not need this code
| return ca_cert, ca_private_key, cert_file_path | ||
|
|
||
|
|
||
| def generate_server_certificate( |
There was a problem hiding this comment.
also for this consider using existing code: look for generate_cert, create_tls_certs functions and usages. We use cert-manager in e2e tests, so we don't need this custom RSA code around.
| func getPem(ctx context.Context, getter secret.Getter, secretName types.NamespacedName) string { | ||
| pem, err := secret.ReadKey(ctx, getter, tlsSecretPemName, secretName) | ||
| if err != nil { | ||
| return "" |
There was a problem hiding this comment.
here as well, it doesn't look good to ignore errors like that
| // This is either the tls.pem entry in the given secret, or the concatenation | ||
| // of tls.crt and tls.key | ||
| // It performs a basic validation on the entries. | ||
| func getPemOrConcatenatedCrtAndKey(ctx context.Context, getter secret.Getter, secretName types.NamespacedName) (string, error) { |
There was a problem hiding this comment.
complexity of this function is too much at first glance. Let's see how it will be if we not handle pem keys in the secret
fealebenpae
requested review from
m1kola and
viveksinghggits
and removed request for
a team
Summary
Expose new TLS capabilities in mongot.
Proof of Work
New passing test.
Checklist