make cafilepath configurable#856
make cafilepath configurable#856filipcirtog wants to merge 2 commits intovm-migration-feature-branchfrom
Conversation
There was a problem hiding this comment.
Pull request overview
This PR introduces a new optional spec.security.tls.caFilePath field to let users choose a custom in-container CA certificate path, and wires that path into the Ops Manager automation config for MongoDB resources. It also updates pod volume mounts to expose the CA ConfigMap at the requested file path and adds an E2E test plus Evergreen plumbing.
Changes:
- Extend CRDs and API types with
security.tls.caFilePathfor MongoDB (and related) resources. - Use
caFilePath(with a default) when configuring TLS CAFilePath in Ops Manager automation config for ReplicaSet/MultiCluster/ ShardedCluster. - Mount the CA ConfigMap at the custom file path via an additional
subPathvolume mount; add an E2E test and Evergreen task.
Reviewed changes
Copilot reviewed 16 out of 16 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
api/v1/mdb/mongodb_types.go |
Adds TLSConfig.CAFilePath plus helper getters to surface it with defaults. |
controllers/operator/mongodbreplicaset_controller.go |
Uses GetTLSCAFilePath(...) when setting CAFilePath for replica sets. |
controllers/operator/mongodbmultireplicaset_controller.go |
Uses GetTLSCAFilePath(...) for multi-cluster replica sets. |
controllers/operator/mongodbshardedcluster_controller.go |
Uses GetTLSCAFilePath(...) for sharded clusters; removes old cert-type-based CA path selection. |
controllers/operator/construct/database_volumes.go |
Adds an extra CA ConfigMap mount at the custom path using subPath. |
public/crds.yaml |
CRD schema update to include security.tls.caFilePath. |
config/crd/bases/*.yaml |
Generated CRD bases updated with caFilePath. |
helm_chart/crds/*.yaml |
Helm CRDs updated with caFilePath. |
docker/mongodb-kubernetes-tests/tests/tls/tls_rs_requiressl_custom_ca_path.py |
New E2E test for TLS requireSSL with a custom CA file path. |
.evergreen.yml / .evergreen-tasks.yml |
Adds a new E2E task to CI. |
controllers/operator/appdbreplicaset_controller.go |
Minor refactor (local variable for CA path) in AppDB monitoring/TLS config. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
| volumeMounts = append(volumeMounts, corev1.VolumeMount{ | ||
| MountPath: customPath, | ||
| Name: caVolume.Name, | ||
| SubPath: filepath.Base(customPath), | ||
| ReadOnly: true, |
There was a problem hiding this comment.
Nit: customPath is a container path (POSIX-style). Using path.Base (instead of filepath.Base) avoids OS-dependent path semantics and matches the rest of the file’s use of path for in-container paths.
| workflowStatus, _ := r.ensureSSLCertificates(ctx, sc, log) | ||
| if !workflowStatus.IsOK() { |
There was a problem hiding this comment.
ensureSSLCertificates’ second return value (certSecretTypes) is now ignored, but the helper still builds and returns the map. Consider simplifying ensureSSLCertificates to return only workflow.Status (and remove the now-dead certSecretTypes plumbing) to reduce confusion and maintenance overhead.
| // CAFilePath allows specifying a custom path for the CA certificate file instead of using the default | ||
| // ConfigMap mount path. If not specified, the default /mongodb-automation/tls/ca/ca-pem is used | ||
| // (or /mongodb-automation/ca.pem for AppDB). When set, the operator creates an additional mount at | ||
| // that path using subPath from the CA ConfigMap (tls.CA). The ConfigMap must have a key matching | ||
| // the filename (e.g. ca-pem). | ||
| CAFilePath string `json:"caFilePath,omitempty"` |
There was a problem hiding this comment.
The new CAFilePath documentation is misleading for AppDB/OpsManager: AppDB CA is not mounted under /mongodb-automation, and this field isn’t currently wired for AppDB. Either remove the AppDB mention here or update it to the actual AppDB CA path and clarify scope/behavior so users don’t assume this works for AppDB.
| func (s *Security) GetTLSCAFilePath(defaultPath string) string { | ||
| if s == nil || s.TLSConfig == nil { | ||
| return defaultPath | ||
| } | ||
| return s.TLSConfig.GetCAFilePath(defaultPath) | ||
| } |
There was a problem hiding this comment.
CAFilePath is added on the API type, but not all MongoDB topologies are wired to use it yet (e.g., the standalone controller still hard-codes util.CAFilePathInContainer). If this field is meant to be supported for Standalone as well, update the remaining controllers/call sites to use Security.GetTLSCAFilePath so behavior is consistent across resource types.
| """E2E tests for replica set TLS with requireSSL using CAFilePath custom path. | ||
|
|
||
| When caFilePath is specified, the operator mounts the CA from the ConfigMap | ||
| (tls.ca) at the custom path instead of the default.""" |
There was a problem hiding this comment.
This docstring says the CA is mounted at the custom path “instead of the default”, but the implementation adds an additional mount while keeping the default /mongodb-automation/tls/ca mount. Consider rewording to avoid implying the default mount is removed.
| (tls.ca) at the custom path instead of the default.""" | |
| (tls.ca) at the custom path, in addition to the default /mongodb-automation/tls/ca mount.""" |
| if tlsConfig != nil && tlsConfig.CAFilePath != "" { | ||
| customPath := tlsConfig.CAFilePath | ||
| volumeMounts = append(volumeMounts, corev1.VolumeMount{ | ||
| MountPath: customPath, | ||
| Name: caVolume.Name, | ||
| SubPath: filepath.Base(customPath), | ||
| ReadOnly: true, | ||
| }) |
There was a problem hiding this comment.
When CAFilePath is set, this adds a subPath mount that requires the referenced file to exist inside the ConfigMap volume. Because the ConfigMap volume is marked optional, a missing ConfigMap (or missing key matching the filename) can cause pod start failures due to subPath resolution. Consider validating the ConfigMap/key exists before adding the subPath mount, or making the CA ConfigMap non-optional when CAFilePath is specified.
| if tlsConfig != nil && tlsConfig.CAFilePath != "" { | ||
| customPath := tlsConfig.CAFilePath | ||
| volumeMounts = append(volumeMounts, corev1.VolumeMount{ | ||
| MountPath: customPath, | ||
| Name: caVolume.Name, | ||
| SubPath: filepath.Base(customPath), | ||
| ReadOnly: true, | ||
| }) | ||
| } |
There was a problem hiding this comment.
Test coverage: CAFilePath introduces new volume-mount behavior (extra mount + subPath). There are unit tests in this package, but none asserting the expected mounts for CAFilePath. Please add a unit test that verifies the custom mount path/subPath is present when spec.security.tls.caFilePath is set.
Summary
Previously, the operator automatically used fixed CA certificate mount paths:
/mongodb-automation/tls/ca/ca-pem for MongoDB deployments/mongodb-automation/ca.pem for AppDB(not scope of this changes)This was a blocker for VM-to-Kubernetes migrations and custom security requirements because:
Solution: New optional CRD field spec.security.tls.caFilePath allows specifying a custom CA certificate path. When set, the operator:
Checklist
skip-changeloglabel if not needed