feat: add MongoDB Atlas support via system TLS and URI passthrough#1069
Open
drubinstein wants to merge 2 commits intoNVIDIA:mainfrom
Open
feat: add MongoDB Atlas support via system TLS and URI passthrough#1069drubinstein wants to merge 2 commits intoNVIDIA:mainfrom
drubinstein wants to merge 2 commits intoNVIDIA:mainfrom
Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdd conditional injection of a datastore URI Secret into multiple Helm Deployment envFrom entries, document MongoDB connection options in values, and propagate a new UseSystemTLS flag from env config through adapters into the MongoDB client/watcher to enable system-CA TLS selection. Changes
Sequence Diagram(s)sequenceDiagram
participant Helm as "Helm (templates)"
participant K8s as "Kubernetes (Deployment/Pod)"
participant App as "Application Container"
participant StoreClient as "store-client (DatabaseConfig)"
participant MongoWatcher as "MongoDB Watcher / Client"
participant MongoDB as "MongoDB"
Helm->>K8s: Render Deployment with envFrom (configMap + conditional secretRef)
K8s->>App: Pod starts with env (MONGODB_URI, MONGODB_USE_SYSTEM_TLS)
App->>StoreClient: Initialize DatabaseConfig (reads env vars)
StoreClient->>MongoWatcher: NewMongoDBClient(..., UseSystemTLS)
MongoWatcher->>MongoWatcher: constructMongoClientOptions -> branch on UseSystemTLS
alt UseSystemTLS == true
MongoWatcher->>MongoDB: Connect using system CA pool TLS (no client cert)
else UseSystemTLS == false
MongoWatcher->>MongoWatcher: build cert-based TLS (dynamic or static)
MongoWatcher->>MongoDB: Connect using client-certificate TLS (MONGODB-X509 possible)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.11.3)level=error msg="[linters_context] typechecking error: pattern ./...: directory prefix . does not contain main module or its selected dependencies" Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@distros/kubernetes/nvsentinel/templates/configmap-datastore.yaml`:
- Around line 84-88: The inline comment next to the MONGODB_URI block references
global.datastore.uriSecret but the templates actually use
.Values.global.datastore.uriSecretName; update the comment to reference
global.datastore.uriSecretName (or vice‑versa if you prefer renaming the Helm
value) so the comment matches the actual Helm value name used by the template
(.Values.global.datastore.connection.uri and
.Values.global.datastore.uriSecretName) and avoid confusion.
In `@distros/kubernetes/nvsentinel/values.yaml`:
- Line 37: The docs comment mistakenly references uriSecret while the actual
configuration key is uriSecretName; update the comment(s) in values.yaml to use
uriSecretName (and any example snippets) so they match the real key, and scan
the file for any other occurrences (e.g., the other instance noted) to ensure
all comments and examples consistently reference uriSecretName.
In `@store-client/pkg/client/mongodb_client.go`:
- Line 347: NewChangeStreamWatcher builds a separate mongoConfig that omits
dbConfig.GetUseSystemTLS(), causing mixed TLS behavior; update
NewChangeStreamWatcher to propagate UseSystemTLS from the provided dbConfig (or
NewMongoDBClient) into its local mongoConfig and ensure any logic that performs
certificate rotation/management is disabled when UseSystemTLS is true so system
CA paths are used consistently; look for NewMongoDBClient,
NewChangeStreamWatcher, and the local mongoConfig variable and add UseSystemTLS:
dbConfig.GetUseSystemTLS() (or equivalent) and guard cert-rotation calls behind
a !UseSystemTLS condition.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: b5486c71-6048-4bae-9350-9d8c1161a7a7
📒 Files selected for processing (14)
distros/kubernetes/nvsentinel/charts/csp-health-monitor/templates/deployment.yamldistros/kubernetes/nvsentinel/charts/event-exporter/templates/deployment.yamldistros/kubernetes/nvsentinel/charts/fault-quarantine/templates/deployment.yamldistros/kubernetes/nvsentinel/charts/fault-remediation/templates/deployment.yamldistros/kubernetes/nvsentinel/charts/health-events-analyzer/templates/deployment.yamldistros/kubernetes/nvsentinel/charts/node-drainer/templates/deployment.yamldistros/kubernetes/nvsentinel/templates/configmap-datastore.yamldistros/kubernetes/nvsentinel/values.yamlnode-drainer/pkg/reconciler/reconciler_integration_test.gostore-client/pkg/adapter/legacy_adapter.gostore-client/pkg/client/convenience.gostore-client/pkg/client/mongodb_client.gostore-client/pkg/config/env_loader.gostore-client/pkg/datastore/providers/mongodb/watcher/watch_store.go
Comment on lines
+84
to
+88
| {{- if .Values.global.datastore.connection.uri }} | ||
| # Direct URI provided (e.g. MongoDB Atlas mongodb+srv://...). Password should be | ||
| # supplied via global.datastore.uriSecret to avoid storing credentials in a ConfigMap. | ||
| MONGODB_URI: {{ .Values.global.datastore.connection.uri | quote }} | ||
| {{- else }} |
Contributor
There was a problem hiding this comment.
Minor comment inconsistency: uriSecret vs uriSecretName.
The comment references global.datastore.uriSecret but the actual Helm value used in the deployment templates is global.datastore.uriSecretName. Consider updating for consistency.
- # supplied via global.datastore.uriSecret to avoid storing credentials in a ConfigMap.
+ # supplied via global.datastore.uriSecretName to avoid storing credentials in a ConfigMap.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| {{- if .Values.global.datastore.connection.uri }} | |
| # Direct URI provided (e.g. MongoDB Atlas mongodb+srv://...). Password should be | |
| # supplied via global.datastore.uriSecret to avoid storing credentials in a ConfigMap. | |
| MONGODB_URI: {{ .Values.global.datastore.connection.uri | quote }} | |
| {{- else }} | |
| {{- if .Values.global.datastore.connection.uri }} | |
| # Direct URI provided (e.g. MongoDB Atlas mongodb+srv://...). Password should be | |
| # supplied via global.datastore.uriSecretName to avoid storing credentials in a ConfigMap. | |
| MONGODB_URI: {{ .Values.global.datastore.connection.uri | quote }} | |
| {{- else }} |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@distros/kubernetes/nvsentinel/templates/configmap-datastore.yaml` around
lines 84 - 88, The inline comment next to the MONGODB_URI block references
global.datastore.uriSecret but the templates actually use
.Values.global.datastore.uriSecretName; update the comment to reference
global.datastore.uriSecretName (or vice‑versa if you prefer renaming the Helm
value) so the comment matches the actual Helm value name used by the template
(.Values.global.datastore.connection.uri and
.Values.global.datastore.uriSecretName) and avoid confusion.
| # | ||
| # # MongoDB Atlas / external MongoDB: set uri directly instead of host/port. | ||
| # # The URI must include credentials. To avoid storing the password in a | ||
| # # ConfigMap, create a Secret and reference it via uriSecret below instead. |
Contributor
There was a problem hiding this comment.
Fix the option name typo in the docs (uriSecret vs uriSecretName).
The comment references uriSecret, but the documented key is uriSecretName. This can cause user misconfiguration.
📝 Suggested doc fix
- # # ConfigMap, create a Secret and reference it via uriSecret below instead.
+ # # ConfigMap, create a Secret and reference it via uriSecretName below instead.Also applies to: 54-54
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@distros/kubernetes/nvsentinel/values.yaml` at line 37, The docs comment
mistakenly references uriSecret while the actual configuration key is
uriSecretName; update the comment(s) in values.yaml to use uriSecretName (and
any example snippets) so they match the real key, and scan the file for any
other occurrences (e.g., the other instance noted) to ensure all comments and
examples consistently reference uriSecretName.
| ChangeStreamRetryDeadlineSeconds: dbConfig.GetTimeoutConfig().GetChangeStreamRetryDeadlineSeconds(), | ||
| ChangeStreamRetryIntervalSeconds: dbConfig.GetTimeoutConfig().GetChangeStreamRetryIntervalSeconds(), | ||
| AppName: dbConfig.GetAppName(), | ||
| UseSystemTLS: dbConfig.GetUseSystemTLS(), |
Contributor
There was a problem hiding this comment.
UseSystemTLS is only wired for one MongoDB path, causing split TLS behavior.
NewMongoDBClient now forwards UseSystemTLS, but NewChangeStreamWatcher builds a separate mongoConfig without it. This can break Atlas/public-CA setups when change streams start. Also, cert rotation should be skipped when system TLS is enabled.
🔧 Suggested fix
func NewMongoDBClient(ctx context.Context, dbConfig config.DatabaseConfig) (*MongoDBClient, error) {
+ useSystemTLS := dbConfig.GetUseSystemTLS()
+
// Convert to the existing MongoDBConfig format for backward compatibility
mongoConfig := mongoWatcher.MongoDBConfig{
@@
- UseSystemTLS: dbConfig.GetUseSystemTLS(),
+ UseSystemTLS: useSystemTLS,
}
@@
- if config.IsCertRotationEnabled() {
+ if config.IsCertRotationEnabled() && !useSystemTLS {
slog.Info("Certificate rotation enabled, initializing certificate watcher")
@@
func (c *MongoDBClient) NewChangeStreamWatcher(ctx context.Context, tokenConfig TokenConfig,
pipeline interface{}) (ChangeStreamWatcher, error) {
@@
mongoConfig := mongoWatcher.MongoDBConfig{
@@
ChangeStreamRetryDeadlineSeconds: c.config.GetTimeoutConfig().GetChangeStreamRetryDeadlineSeconds(),
ChangeStreamRetryIntervalSeconds: c.config.GetTimeoutConfig().GetChangeStreamRetryIntervalSeconds(),
CertWatcher: c.certWatcher,
+ UseSystemTLS: c.config.GetUseSystemTLS(),
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@store-client/pkg/client/mongodb_client.go` at line 347,
NewChangeStreamWatcher builds a separate mongoConfig that omits
dbConfig.GetUseSystemTLS(), causing mixed TLS behavior; update
NewChangeStreamWatcher to propagate UseSystemTLS from the provided dbConfig (or
NewMongoDBClient) into its local mongoConfig and ensure any logic that performs
certificate rotation/management is disabled when UseSystemTLS is true so system
CA paths are used consistently; look for NewMongoDBClient,
NewChangeStreamWatcher, and the local mongoConfig variable and add UseSystemTLS:
dbConfig.GetUseSystemTLS() (or equivalent) and guard cert-rotation calls behind
a !UseSystemTLS condition.
Adds MONGODB_USE_SYSTEM_TLS support so NVSentinel can connect to MongoDB
Atlas (and any deployment using publicly trusted certificates) without
requiring custom CA cert files mounted in pods.
Go changes:
- New MONGODB_USE_SYSTEM_TLS env var: when true, uses x509.SystemCertPool()
instead of loading CA certs from disk, skipping X.509 client auth
- Added GetUseSystemTLS() to DatabaseConfig interface and all implementations
- Wired UseSystemTLS through MongoDBConfig, NewMongoDBClient, and
ConvertToMongoDBConfig
Helm changes:
- configmap-datastore.yaml: supports global.datastore.connection.uri for
direct URI passthrough (e.g. mongodb+srv://), emits MONGODB_USE_SYSTEM_TLS
- values.yaml: documents uri, useSystemTLS, and uriSecretName options
- All 6 datastore-consuming deployment templates: inject optional Secret via
envFrom when global.datastore.uriSecretName is set, keeping passwords out
of ConfigMaps
Usage with Atlas:
kubectl create secret generic nvsentinel-atlas-uri \
--from-literal=MONGODB_URI='mongodb+srv://user:pass@cluster.mongodb.net/'
helm upgrade nvsentinel ... \
--set global.datastore.provider=mongodb \
--set global.datastore.connection.database=nvsentinel \
--set global.datastore.useSystemTLS=true \
--set global.datastore.uriSecretName=nvsentinel-atlas-uri
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Signed-off-by: David Rubinstein <david@reflection.ai>
drubinstein
force-pushed
the
david/external-mongo
branch
from
ab8e294 to
3439a4b
Compare
Collaborator
|
/ok to test 3439a4b |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
store-client/pkg/client/mongodb_client.go (1)
347-347:⚠️ Potential issue | 🔴 CriticalPropagate
UseSystemTLSconsistently to change streams and skip cert rotation in system-TLS mode.Line 347 wires
UseSystemTLSonly for one config path.NewChangeStreamWatcherstill rebuilds a config without this flag, and cert rotation is still initialized when system TLS is enabled. That can cause split behavior/failures for Atlas-style setups without mounted cert/key files.🔧 Proposed fix
func NewMongoDBClient(ctx context.Context, dbConfig config.DatabaseConfig) (*MongoDBClient, error) { + useSystemTLS := dbConfig.GetUseSystemTLS() + // Convert to the existing MongoDBConfig format for backward compatibility mongoConfig := mongoWatcher.MongoDBConfig{ @@ - UseSystemTLS: dbConfig.GetUseSystemTLS(), + UseSystemTLS: useSystemTLS, } @@ - if config.IsCertRotationEnabled() { + if config.IsCertRotationEnabled() && !useSystemTLS { slog.Info("Certificate rotation enabled, initializing certificate watcher") @@ func (c *MongoDBClient) NewChangeStreamWatcher(ctx context.Context, tokenConfig TokenConfig, pipeline interface{}) (ChangeStreamWatcher, error) { @@ mongoConfig := mongoWatcher.MongoDBConfig{ @@ ChangeStreamRetryDeadlineSeconds: c.config.GetTimeoutConfig().GetChangeStreamRetryDeadlineSeconds(), ChangeStreamRetryIntervalSeconds: c.config.GetTimeoutConfig().GetChangeStreamRetryIntervalSeconds(), CertWatcher: c.certWatcher, + UseSystemTLS: c.config.GetUseSystemTLS(), }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@store-client/pkg/client/mongodb_client.go` at line 347, The UseSystemTLS flag is only set on one config path causing NewChangeStreamWatcher to rebuild a config without it and the certificate-rotation path to run even when system TLS is enabled; update NewChangeStreamWatcher and any code that constructs a new db config for change streams to copy dbConfig.GetUseSystemTLS() into the rebuilt config (the same config key used on line where UseSystemTLS is currently set) and modify the cert-rotation initialization (the function that starts/initializes rotation) to no-op when UseSystemTLS is true so cert/key rotation is skipped for system-TLS setups.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@store-client/pkg/adapter/legacy_adapter.go`:
- Around line 147-149: Add a Go doc comment for the exported method
GetUseSystemTLS on LegacyDatabaseConfigAdapter: a one-line comment that starts
with "GetUseSystemTLS" and briefly explains that the method returns whether
system TLS should be used (reads the environment variable
config.EnvMongoDBUseSystemTLS and interprets it via stringutil.IsTruthyValue),
and mention the bool return for clarity; place the comment immediately above the
GetUseSystemTLS method declaration.
In `@store-client/pkg/config/env_loader.go`:
- Around line 515-517: Add a concise Go doc comment for the exported method
GetUseSystemTLS on type StandardDatabaseConfig: place a comment immediately
above the method that begins with "GetUseSystemTLS" and briefly describes what
it returns (e.g., whether the database connection should use system TLS
certificates), mentioning the receiver type StandardDatabaseConfig if helpful;
update the comment to follow Go doc conventions (full sentence, starts with the
method name).
---
Duplicate comments:
In `@store-client/pkg/client/mongodb_client.go`:
- Line 347: The UseSystemTLS flag is only set on one config path causing
NewChangeStreamWatcher to rebuild a config without it and the
certificate-rotation path to run even when system TLS is enabled; update
NewChangeStreamWatcher and any code that constructs a new db config for change
streams to copy dbConfig.GetUseSystemTLS() into the rebuilt config (the same
config key used on line where UseSystemTLS is currently set) and modify the
cert-rotation initialization (the function that starts/initializes rotation) to
no-op when UseSystemTLS is true so cert/key rotation is skipped for system-TLS
setups.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 9803dec3-4740-4e63-9eae-ea22eee710f0
📒 Files selected for processing (14)
distros/kubernetes/nvsentinel/charts/csp-health-monitor/templates/deployment.yamldistros/kubernetes/nvsentinel/charts/event-exporter/templates/deployment.yamldistros/kubernetes/nvsentinel/charts/fault-quarantine/templates/deployment.yamldistros/kubernetes/nvsentinel/charts/fault-remediation/templates/deployment.yamldistros/kubernetes/nvsentinel/charts/health-events-analyzer/templates/deployment.yamldistros/kubernetes/nvsentinel/charts/node-drainer/templates/deployment.yamldistros/kubernetes/nvsentinel/templates/configmap-datastore.yamldistros/kubernetes/nvsentinel/values.yamlnode-drainer/pkg/reconciler/reconciler_integration_test.gostore-client/pkg/adapter/legacy_adapter.gostore-client/pkg/client/convenience.gostore-client/pkg/client/mongodb_client.gostore-client/pkg/config/env_loader.gostore-client/pkg/datastore/providers/mongodb/watcher/watch_store.go
✅ Files skipped from review due to trivial changes (4)
- distros/kubernetes/nvsentinel/values.yaml
- distros/kubernetes/nvsentinel/charts/event-exporter/templates/deployment.yaml
- distros/kubernetes/nvsentinel/charts/csp-health-monitor/templates/deployment.yaml
- distros/kubernetes/nvsentinel/templates/configmap-datastore.yaml
🚧 Files skipped from review as they are similar to previous changes (4)
- store-client/pkg/client/convenience.go
- distros/kubernetes/nvsentinel/charts/health-events-analyzer/templates/deployment.yaml
- node-drainer/pkg/reconciler/reconciler_integration_test.go
- distros/kubernetes/nvsentinel/charts/node-drainer/templates/deployment.yaml
Comment on lines
+147
to
+149
| func (l *LegacyDatabaseConfigAdapter) GetUseSystemTLS() bool { | ||
| return stringutil.IsTruthyValue(os.Getenv(config.EnvMongoDBUseSystemTLS)) | ||
| } |
Contributor
There was a problem hiding this comment.
Add a doc comment for exported method GetUseSystemTLS.
Line 147 introduces an exported method without a Go doc comment.
✏️ Suggested doc comment
+// GetUseSystemTLS returns true when MongoDB should use the system CA trust store.
func (l *LegacyDatabaseConfigAdapter) GetUseSystemTLS() bool {
return stringutil.IsTruthyValue(os.Getenv(config.EnvMongoDBUseSystemTLS))
}As per coding guidelines: "Function comments required for all exported Go functions".
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| func (l *LegacyDatabaseConfigAdapter) GetUseSystemTLS() bool { | |
| return stringutil.IsTruthyValue(os.Getenv(config.EnvMongoDBUseSystemTLS)) | |
| } | |
| // GetUseSystemTLS returns true when MongoDB should use the system CA trust store. | |
| func (l *LegacyDatabaseConfigAdapter) GetUseSystemTLS() bool { | |
| return stringutil.IsTruthyValue(os.Getenv(config.EnvMongoDBUseSystemTLS)) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@store-client/pkg/adapter/legacy_adapter.go` around lines 147 - 149, Add a Go
doc comment for the exported method GetUseSystemTLS on
LegacyDatabaseConfigAdapter: a one-line comment that starts with
"GetUseSystemTLS" and briefly explains that the method returns whether system
TLS should be used (reads the environment variable config.EnvMongoDBUseSystemTLS
and interprets it via stringutil.IsTruthyValue), and mention the bool return for
clarity; place the comment immediately above the GetUseSystemTLS method
declaration.
Comment on lines
+515
to
+517
| func (c *StandardDatabaseConfig) GetUseSystemTLS() bool { | ||
| return c.useSystemTLS | ||
| } |
Contributor
There was a problem hiding this comment.
Add a doc comment for exported method GetUseSystemTLS.
Line 515 adds an exported getter without a Go doc comment.
✏️ Suggested doc comment
+// GetUseSystemTLS returns whether system CA trust should be used for MongoDB TLS.
func (c *StandardDatabaseConfig) GetUseSystemTLS() bool {
return c.useSystemTLS
}As per coding guidelines: "Function comments required for all exported Go functions".
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| func (c *StandardDatabaseConfig) GetUseSystemTLS() bool { | |
| return c.useSystemTLS | |
| } | |
| // GetUseSystemTLS returns whether system CA trust should be used for MongoDB TLS. | |
| func (c *StandardDatabaseConfig) GetUseSystemTLS() bool { | |
| return c.useSystemTLS | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@store-client/pkg/config/env_loader.go` around lines 515 - 517, Add a concise
Go doc comment for the exported method GetUseSystemTLS on type
StandardDatabaseConfig: place a comment immediately above the method that begins
with "GetUseSystemTLS" and briefly describes what it returns (e.g., whether the
database connection should use system TLS certificates), mentioning the receiver
type StandardDatabaseConfig if helpful; update the comment to follow Go doc
conventions (full sentence, starts with the method name).
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
store-client/pkg/datastore/providers/mongodb/watcher/watch_store.go (1)
613-631: Wrap TLS branch errors with explicit context.These branches return raw errors, which makes failure origin harder to identify in logs and call chains.
🔧 Suggested change
switch { case mongoConfig.UseSystemTLS: // Use the system CA trust store — no cert files required. // Credentials (e.g. SCRAM username/password) are encoded in the URI. tlsConfig, err = constructSystemTLSConfig() if err != nil { - return nil, err + return nil, fmt.Errorf("construct system TLS config: %w", err) } case mongoConfig.CertWatcher != nil: // Dynamic TLS with automatic client certificate rotation. tlsConfig, err = constructDynamicTLSConfig(mongoConfig) if err != nil { - return nil, err + return nil, fmt.Errorf("construct dynamic TLS config: %w", err) } default: // Static TLS loaded from cert files on disk. tlsConfig, err = constructStaticTLSConfig(mongoConfig) if err != nil { - return nil, err + return nil, fmt.Errorf("construct static TLS config: %w", err) } }As per coding guidelines, "Wrap errors with context using
fmt.Errorf("context: %w", err)in Go code".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@store-client/pkg/datastore/providers/mongodb/watcher/watch_store.go` around lines 613 - 631, The error returns from the TLS selection branches should be wrapped with contextual messages; update the cases that call constructSystemTLSConfig, constructDynamicTLSConfig, and constructStaticTLSConfig inside the switch on mongoConfig (including the mongoConfig.UseSystemTLS and mongoConfig.CertWatcher branches) to return fmt.Errorf with context (e.g., "constructSystemTLSConfig failed: %w") instead of returning the raw err so callers can identify which TLS path failed.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@store-client/pkg/datastore/providers/mongodb/watcher/watch_store.go`:
- Around line 613-631: The error returns from the TLS selection branches should
be wrapped with contextual messages; update the cases that call
constructSystemTLSConfig, constructDynamicTLSConfig, and
constructStaticTLSConfig inside the switch on mongoConfig (including the
mongoConfig.UseSystemTLS and mongoConfig.CertWatcher branches) to return
fmt.Errorf with context (e.g., "constructSystemTLSConfig failed: %w") instead of
returning the raw err so callers can identify which TLS path failed.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 30440fdc-9791-473a-8556-5ad7902f5ec6
📒 Files selected for processing (1)
store-client/pkg/datastore/providers/mongodb/watcher/watch_store.go
Signed-off-by: David Rubinstein <david@reflection.ai>
drubinstein
force-pushed
the
david/external-mongo
branch
from
1009494 to
46b3f45
Compare
Collaborator
|
/ok to test 46b3f45 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Adds MONGODB_USE_SYSTEM_TLS support so NVSentinel can connect to MongoDB Atlas (and any deployment using publicly trusted certificates) without requiring custom CA cert files mounted in pods.
Go changes:
Helm changes:
Usage with Atlas:
kubectl create secret generic nvsentinel-atlas-uri \ --from-literal=MONGODB_URI='mongodb+srv://user:pass@cluster.mongodb.net/' helm upgrade nvsentinel ... \ --set global.datastore.provider=mongodb \ --set global.datastore.connection.database=nvsentinel \ --set global.datastore.useSystemTLS=true \ --set global.datastore.uriSecretName=nvsentinel-atlas-uri
Summary
Type of Change
Component(s) Affected
Testing
Checklist
Summary by CodeRabbit
New Features
Documentation
Tests