External database: unmanaged mode, custom Secret key, postgresql://#456
Open
jalet wants to merge 4 commits into
Open
External database: unmanaged mode, custom Secret key, postgresql://#456jalet wants to merge 4 commits into
jalet wants to merge 4 commits into
Conversation
mm-cloud-bot
added
the
kind/feature
|
@jalet: Adding the "do-not-merge/release-note-label-needed" label because no release-note block was detected, please follow our release note process to remove it. DetailsI understand the commands that are listed here |
mm-cloud-bot
added
do-not-merge/release-note-label-needed
release-note
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds ChangesUnmanaged DB + ExternalKey
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
config/crd/bases/installation.mattermost.com_mattermosts.yaml (1)
104-121:⚠️ Potential issue | 🟡 MinorUpdate the Secret description to include custom keys.
Line 117 still says the Secret should contain
DB_CONNECTION_STRING, but withconnectionStringKeya Secret containing onlyuriis valid.📝 Proposed documentation update
secret: description: |- Secret contains data necessary to connect to the external database. The Kubernetes Secret should contain: - - Key: DB_CONNECTION_STRING | Value: Full database connection string. + - Key: DB_CONNECTION_STRING, or the key configured by connectionStringKey | Value: Full database connection string. It can also contain optional fields, such as:🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@config/crd/bases/installation.mattermost.com_mattermosts.yaml` around lines 104 - 121, Update the Secret description under the "secret" field to reflect that the key for the connection string is configurable via connectionStringKey: state that the default key is "DB_CONNECTION_STRING" but if connectionStringKey is set the operator will use that key (for example "uri"), and that the operator will also fall back to that key for MM_SQLSETTINGS_DATASOURCE when no MM_SQLSETTINGS_DATASOURCE entry exists in the Secret; keep the existing optional keys (MM_SQLSETTINGS_DATASOURCEREPLICAS, DB_CONNECTION_CHECK_URL) unchanged.
♻️ Duplicate comments (1)
apis/mattermost/v1alpha1/zz_generated.openapi.go (1)
20-338:⚠️ Potential issue | 🟠 MajorSame unrelated path-format rewrite as v1beta1; please regenerate both together.
Every change in this file is the same class of rewrite as in
apis/mattermost/v1beta1/zz_generated.openapi.go: schema map keys, factory function names,ref(...)targets, andDependenciesentries moved fromgithub.com/mattermost/mattermost-operator/apis/mattermost/v1alpha1.*to./apis/mattermost/v1alpha1.*, with theschema__apis_mattermost_v1alpha1_*double-underscore form indicating an empty module component.
v1alpha1holds the deprecatedClusterInstallationAPI and is not part of this PR's feature scope (unmanaged DB /ConnectionStringKey/postgresql://all live onv1beta1.Mattermost), so there is no functional reason for this file to have changed. See the detailed review on the v1beta1 file for the full concern about consumer impact and next-make generatethrash — the same remediation applies here: regenerate both files from an environment that produces the previous fully-qualified module-path keys, or make the rename explicit and intentional across both packages.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apis/mattermost/v1alpha1/zz_generated.openapi.go` around lines 20 - 338, Generated openapi keys and refs in zz_generated.openapi.go for package v1alpha1 were rewritten to relative paths (./apis/mattermost/v1alpha1.*) and double-underscore schema function names (schema__apis_mattermost_v1alpha1_*), matching the same unwanted change made in v1beta1; restore the original fully-qualified module-path keys and factory names by regenerating the OpenAPI files for both v1alpha1 and v1beta1 together from the correct generator environment or reverting the changes: ensure functions like schema__apis_mattermost_v1alpha1_ClusterInstallation (and its peers), the ref(...) targets, and the Dependencies entries use the original github.com/mattermost/mattermost-operator/apis/mattermost/v1alpha1.* fully-qualified references instead of ./apis/... so both generated files are consistent and prevent make generate churn.
🧹 Nitpick comments (1)
pkg/mattermost/mattermost_v1beta_test.go (1)
1125-1127: Exercise the unmanaged guard with a non-nil DB config.Passing
nilmeans this test would still pass ifGenerateDeploymentV1BetaignoredDatabase.Unmanagedand only skipped DB injection becausedb == nil.🧪 Proposed test strengthening
- // Passing a nil DatabaseConfig mirrors checkDatabase returning nil for unmanaged. - deployment := GenerateDeploymentV1Beta(mattermost, nil, fileStore, "mm", "", "sa", "") + dbConfig := &ExternalDBConfig{ + secretName: "db-secret", + connectionStringKey: DefaultExternalDBConnectionStringKey, + hasDBCheckURL: true, + dbType: database.PostgreSQLDatabase, + } + deployment := GenerateDeploymentV1Beta(mattermost, dbConfig, fileStore, "mm", "", "sa", "")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/mattermost/mattermost_v1beta_test.go` around lines 1125 - 1127, The test currently passes nil for the DB config so it can't verify the unmanaged guard; instead construct and pass a non-nil database config with Unmanaged set true (e.g., a DatabaseConfig/Database struct instance with Unmanaged: true) into GenerateDeploymentV1Beta and keep the require.NotNil assertion — this ensures GenerateDeploymentV1Beta actually checks Database.Unmanaged rather than simply skipping DB injection due to a nil config.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apis/mattermost/v1beta1/mattermost_types.go`:
- Around line 361-368: Add kubebuilder CRD validation tags to the
ConnectionStringKey field to enforce allowed secret-key names and length:
annotate the ConnectionStringKey string with
+kubebuilder:validation:Pattern="^[A-Za-z0-9._-]+$" and
+kubebuilder:validation:MaxLength=253 (and keep +optional). Update the struct
field `ConnectionStringKey string `json:"connectionStringKey,omitempty"` in
mattermost_types.go to include these validation markers, then regenerate the
CRD/manifests so the schema includes the pattern and maxLength constraints.
---
Outside diff comments:
In `@config/crd/bases/installation.mattermost.com_mattermosts.yaml`:
- Around line 104-121: Update the Secret description under the "secret" field to
reflect that the key for the connection string is configurable via
connectionStringKey: state that the default key is "DB_CONNECTION_STRING" but if
connectionStringKey is set the operator will use that key (for example "uri"),
and that the operator will also fall back to that key for
MM_SQLSETTINGS_DATASOURCE when no MM_SQLSETTINGS_DATASOURCE entry exists in the
Secret; keep the existing optional keys (MM_SQLSETTINGS_DATASOURCEREPLICAS,
DB_CONNECTION_CHECK_URL) unchanged.
---
Duplicate comments:
In `@apis/mattermost/v1alpha1/zz_generated.openapi.go`:
- Around line 20-338: Generated openapi keys and refs in zz_generated.openapi.go
for package v1alpha1 were rewritten to relative paths
(./apis/mattermost/v1alpha1.*) and double-underscore schema function names
(schema__apis_mattermost_v1alpha1_*), matching the same unwanted change made in
v1beta1; restore the original fully-qualified module-path keys and factory names
by regenerating the OpenAPI files for both v1alpha1 and v1beta1 together from
the correct generator environment or reverting the changes: ensure functions
like schema__apis_mattermost_v1alpha1_ClusterInstallation (and its peers), the
ref(...) targets, and the Dependencies entries use the original
github.com/mattermost/mattermost-operator/apis/mattermost/v1alpha1.*
fully-qualified references instead of ./apis/... so both generated files are
consistent and prevent make generate churn.
---
Nitpick comments:
In `@pkg/mattermost/mattermost_v1beta_test.go`:
- Around line 1125-1127: The test currently passes nil for the DB config so it
can't verify the unmanaged guard; instead construct and pass a non-nil database
config with Unmanaged set true (e.g., a DatabaseConfig/Database struct instance
with Unmanaged: true) into GenerateDeploymentV1Beta and keep the require.NotNil
assertion — this ensures GenerateDeploymentV1Beta actually checks
Database.Unmanaged rather than simply skipping DB injection due to a nil config.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 327aae8a-47b6-4b76-a3d9-f24b8c8db087
📒 Files selected for processing (11)
apis/mattermost/v1alpha1/zz_generated.openapi.goapis/mattermost/v1beta1/db_util.goapis/mattermost/v1beta1/db_util_test.goapis/mattermost/v1beta1/mattermost_types.goapis/mattermost/v1beta1/zz_generated.openapi.goconfig/crd/bases/installation.mattermost.com_mattermosts.yamlcontrollers/mattermost/mattermost/database.gopkg/mattermost/database_external.gopkg/mattermost/database_external_test.gopkg/mattermost/mattermost_v1beta.gopkg/mattermost/mattermost_v1beta_test.go
jalet
force-pushed
the
feat/unmanaged-database
branch
from
f79f0db to
129be17
Compare
Contributor
|
This PR has been automatically labelled "stale" because it hasn't had recent activity. |
This was referenced May 4, 2026
Adds two independent mechanisms for connecting a Mattermost installation to a pre-existing database secret (e.g. one generated by cloudnative-pg): - spec.database.unmanaged: when true, the operator skips all database configuration. The user is responsible for supplying MM_SQLSETTINGS_* via spec.mattermostEnv, typically via valueFrom.secretKeyRef pointing at an external secret. - spec.database.external.connectionStringKey: overrides the Secret key the operator reads the connection string from (default DB_CONNECTION_STRING). When set and no MM_SQLSETTINGS_DATASOURCE key is present in the Secret, the same key is reused as the MM_SQLSETTINGS_DATASOURCE source, so a single 'uri' entry is enough. Fixes: mattermost#437
libpq-based clients (including pg_isready, which the operator uses for the external-database readiness init container) accept both "postgres://" and the officially-named "postgresql://" URI form. cloudnative-pg and other postgres operators emit the postgresql:// variant, which was previously rejected by validateDBCheckURL. Add postgresql as an allowed scheme for the postgres and unknown db types. GetTypeFromConnectionString already detects both via its "postgres" prefix check, and the connection string itself is passed through unchanged via secretKeyRef, so no further changes are needed. Fixes: mattermost#438
jalet
force-pushed
the
feat/unmanaged-database
branch
from
129be17 to
3b7bee3
Compare
…base # Conflicts: # config/crd/bases/installation.mattermost.com_mattermosts.yaml
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/mattermost/database_external.go`:
- Around line 199-218: The builtinDBCheckInitContainer function hardcodes
"DB_CONNECTION_STRING" in the EnvVars configuration instead of using the
configurable connectionStringKey, which breaks support for custom connection
string keys. Pass the connectionStringKey parameter through the call chain
starting from line 122 where getDBCheckInitContainer is called. Update the
getDBCheckInitContainer function signature to accept connectionStringKey, update
its call site to pass the connectionStringKey from the context, and update
builtinDBCheckInitContainer to accept and use the connectionStringKey parameter
instead of the hardcoded "DB_CONNECTION_STRING" values on lines 205 and 213.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 26395bcd-3fbb-4b0b-a3c0-6902020a8466
📒 Files selected for processing (5)
apis/mattermost/v1beta1/mattermost_types.goconfig/crd/bases/installation.mattermost.com_mattermosts.yamlpkg/mattermost/database_external.gopkg/mattermost/database_external_test.gopkg/mattermost/mattermost_v1beta_test.go
🚧 Files skipped from review as they are similar to previous changes (4)
- config/crd/bases/installation.mattermost.com_mattermosts.yaml
- apis/mattermost/v1beta1/mattermost_types.go
- pkg/mattermost/database_external_test.go
- pkg/mattermost/mattermost_v1beta_test.go
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/mattermost/database_external.go`:
- Around line 199-218: The builtinDBCheckInitContainer function hardcodes
"DB_CONNECTION_STRING" in the EnvVars configuration instead of using the
configurable connectionStringKey, which breaks support for custom connection
string keys. Pass the connectionStringKey parameter through the call chain
starting from line 122 where getDBCheckInitContainer is called. Update the
getDBCheckInitContainer function signature to accept connectionStringKey, update
its call site to pass the connectionStringKey from the context, and update
builtinDBCheckInitContainer to accept and use the connectionStringKey parameter
instead of the hardcoded "DB_CONNECTION_STRING" values on lines 205 and 213.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 26395bcd-3fbb-4b0b-a3c0-6902020a8466
📒 Files selected for processing (5)
apis/mattermost/v1beta1/mattermost_types.goconfig/crd/bases/installation.mattermost.com_mattermosts.yamlpkg/mattermost/database_external.gopkg/mattermost/database_external_test.gopkg/mattermost/mattermost_v1beta_test.go
🚧 Files skipped from review as they are similar to previous changes (4)
- config/crd/bases/installation.mattermost.com_mattermosts.yaml
- apis/mattermost/v1beta1/mattermost_types.go
- pkg/mattermost/database_external_test.go
- pkg/mattermost/mattermost_v1beta_test.go
🛑 Comments failed to post (1)
pkg/mattermost/database_external.go (1)
199-218:
⚠️ Potential issue | 🟠 Major🧩 Analysis chain
🏁 Script executed:
#!/bin/bash # Verify that connectionStringKey exists on ExternalDBConfig and trace its usage echo "=== Check ExternalDBConfig struct for connectionStringKey field ===" ast-grep --pattern 'type ExternalDBConfig struct { $$$ }' echo "" echo "=== Check if connectionStringKey is used in EnvVars but not in InitContainers path ===" rg -n "connectionStringKey" pkg/mattermost/database_external.goRepository: mattermost/mattermost-operator
Length of output: 1307
🏁 Script executed:
#!/bin/bash # View the InitContainers function and the call to getDBCheckInitContainer echo "=== InitContainers function and call site (around line 122) ===" sed -n '110,135p' pkg/mattermost/database_external.go echo "" echo "=== getDBCheckInitContainer function signature ===" sed -n '135,165p' pkg/mattermost/database_external.go echo "" echo "=== builtinDBCheckInitContainer full implementation ===" sed -n '195,235p' pkg/mattermost/database_external.goRepository: mattermost/mattermost-operator
Length of output: 3589
Builtin init container ignores configured
connectionStringKey, breaking custom-key support.The function hardcodes
"DB_CONNECTION_STRING"on lines 205 and 213, but the PR objective is to support custom keys (e.g.,"uri"from cloudnative-pg secrets). While the main container'sEnvVarscorrectly sources from the configured key, the builtin init container does not receiveconnectionStringKeythrough the call chain (line 122 passes onlye.hasSeparateDatasourceKey), so users configuring a custom key will have a working main container but a failing readiness check.Proposed fix: pass connectionStringKey through the call chain
Update
getDBCheckInitContainersignature and call site (line 122):-func getDBCheckInitContainer( - mattermost *mmv1beta.Mattermost, - secretName, dbType string, - hasSeparateDatasourceKey bool, -) *corev1.Container { +func getDBCheckInitContainer( + mattermost *mmv1beta.Mattermost, + secretName, dbType string, + hasSeparateDatasourceKey bool, + connectionStringKey string, +) *corev1.Container {Update the call in
InitContainers:- container := getDBCheckInitContainer(mattermost, e.secretName, e.dbType, e.hasSeparateDatasourceKey) + container := getDBCheckInitContainer(mattermost, e.secretName, e.dbType, e.hasSeparateDatasourceKey, e.connectionStringKey)Update
builtinDBCheckInitContainersignature and implementation:func builtinDBCheckInitContainer( mattermost *mmv1beta.Mattermost, secretName string, hasSeparateDatasourceKey bool, + connectionStringKey string, timeout time.Duration, ) *corev1.Container { - datasourceKey := "DB_CONNECTION_STRING" + datasourceKey := connectionStringKey if hasSeparateDatasourceKey { datasourceKey = "MM_SQLSETTINGS_DATASOURCE" } envVars := []corev1.EnvVar{ { Name: "MM_CONFIG", - ValueFrom: EnvSourceFromSecret(secretName, "DB_CONNECTION_STRING"), + ValueFrom: EnvSourceFromSecret(secretName, connectionStringKey), },🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/mattermost/database_external.go` around lines 199 - 218, The builtinDBCheckInitContainer function hardcodes "DB_CONNECTION_STRING" in the EnvVars configuration instead of using the configurable connectionStringKey, which breaks support for custom connection string keys. Pass the connectionStringKey parameter through the call chain starting from line 122 where getDBCheckInitContainer is called. Update the getDBCheckInitContainer function signature to accept connectionStringKey, update its call site to pass the connectionStringKey from the context, and update builtinDBCheckInitContainer to accept and use the connectionStringKey parameter instead of the hardcoded "DB_CONNECTION_STRING" values on lines 205 and 213.
The builtin DB readiness init container (mattermost#459) hardcoded DB_CONNECTION_STRING for its MM_CONFIG and MM_SQLSETTINGS_DATASOURCE env vars, ignoring the configurable spec.database.external.connectionStringKey added in this branch. With a custom key (e.g. cloudnative-pg's "uri") and builtin readiness mode, the init container's `mattermost db ping` would read a non-existent Secret key and fail while the main container worked. Thread connectionStringKey through getDBCheckInitContainer to builtinDBCheckInitContainer so its env vars mirror ExternalDBConfig.EnvVars.
marianunez
added
2: Dev Review
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
4 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.
Summary
Two independent improvements to external-database support, aimed at consuming
pre-existing secrets produced by other operators (e.g. cloudnative-pg).
spec.database.unmanaged: true— operator skips all DB config; user suppliesMM_SQLSETTINGS_*viaspec.mattermostEnv.spec.database.external.connectionStringKey— override the Secret key theoperator reads the connection string from (default
DB_CONNECTION_STRING).When set and no
MM_SQLSETTINGS_DATASOURCEkey is present in the Secret, thesame key is reused as the datasource source, so a single
urientry isenough.
postgresql://scheme inDB_CONNECTION_CHECK_URLvalidation —pg_isreadyaccepts both URI forms;GetTypeFromConnectionStringalreadydetected both via its
postgresprefix check.All three are opt-in; default behavior is unchanged.
Ticket Link
Fixes #437
Fixes #438
Release Note