[SECURESIGN-3170] Support PostgreSQL as an optional backend database for Trillian #1483

fghanmi · 2025-12-04T14:51:04Z

PR Type

Enhancement

Description

Add PostgreSQL support as optional backend database for Trillian
Introduce usePostgreSQL flag to switch between MySQL and PostgreSQL
Rename MySQL-specific environment variables to generic database names
Update deployment logic to generate appropriate connection strings
Conditionally apply TLS configuration based on database type

Diagram Walkthrough

flowchart LR
  A["Trillian DB Config"] -->|usePostgreSQL flag| B["Database Type Selection"]
  B -->|true| C["PostgreSQL Connection"]
  B -->|false| D["MySQL Connection"]
  C -->|postgresql_uri| E["PostgreSQL Deployment"]
  D -->|mysql_uri| F["MySQL Deployment"]
  G["Generic DB Variables"] -->|DB_HOST, DB_PORT, DB_USER| E
  G -->|DB_HOST, DB_PORT, DB_USER| F

File Walkthrough

Relevant files

Enhancement

trillian_types.go `Add PostgreSQL flag and update secret documentation` api/v1alpha1/trillian_types.go Add `UsePostgreSQL` boolean field to `TrillianDB` struct with default value false Update database secret documentation to reflect generic database naming (db-host, db-port, db-user, db-password, database) Replace MySQL-specific secret field names with database-agnostic names	+10/-5
constants.go `Rename constants to database-agnostic naming` internal/controller/trillian/actions/constants.go Rename `DbPvcName` from "trillian-mysql" to "trillian-db" Update all secret key constants from MySQL-specific names (mysql-host, mysql-port, mysql-user, mysql-password, mysql-database) to generic database names (db-host, db-port, db-user, db-password, db-name) Add comment clarifying that `SecretRootPassword` is MySQL-only	+7/-7
handle_secret.go `Update secret generation for generic database` internal/controller/trillian/actions/db/handle_secret.go Rename variable `mysqlPass` to `dbPass` for generic database support Update secret data map to use new generic constant names	+3/-3
server-deployment.go `Implement database-agnostic deployment configuration` internal/controller/trillian/utils/server-deployment.go Rename environment variables from MYSQL_* to DB_* (MYSQL_HOSTNAME→DB_HOSTNAME, MYSQL_PORT→DB_PORT, MYSQL_USER→DB_USER, MYSQL_PASSWORD→DB_PASSWORD, MYSQL_DATABASE→DB_NAME) Add conditional logic in `ensureDeployment` to generate PostgreSQL or MySQL connection strings based on `UsePostgreSQL` flag Update init container command to reference generic DB_* variables Modify `WithTlsDB` function to conditionally apply PostgreSQL or MySQL TLS configuration	+36/-21
rhtas.redhat.com_securesigns.yaml `Update SecureSign CRD with PostgreSQL support` config/crd/bases/rhtas.redhat.com_securesigns.yaml Add `usePostgreSQL` field to CRD schema with default value false Update database secret field descriptions to reference generic database naming	+11/-5
rhtas.redhat.com_trillians.yaml `Update Trillian CRD with PostgreSQL support` config/crd/bases/rhtas.redhat.com_trillians.yaml Add `usePostgreSQL` field to CRD schema with default value false Update database secret field descriptions to reference generic database naming	+22/-10

Dependencies

images.env `Update Trillian component image versions` config/default/images.env Update Trillian log signer image SHA256 hash Update Trillian log server image SHA256 hash	+2/-2

qodo-code-review · 2025-12-04T14:51:35Z

PR Compliance Guide 🔍

Below is a summary of compliance checks for this PR:

Security Compliance
🟢	No security concerns identified No security vulnerabilities detected by AI analysis. Human verification advised for critical code.
Ticket Compliance
⚪	🎫 No ticket provided Create ticket/issue
Codebase Duplication Compliance
⚪	Codebase context is not defined Follow the guide to enable codebase context checks.
Custom Compliance
🟢	Generic: Meaningful Naming and Self-Documenting Code Objective: Ensure all identifiers clearly express their purpose and intent, making code self-documenting Status: Passed Learn more about managing compliance generic rules or creating your own custom rules
	Generic: Secure Error Handling Objective: To prevent the leakage of sensitive system information through error messages while providing sufficient detail for internal debugging. Status: Passed Learn more about managing compliance generic rules or creating your own custom rules
	Generic: Secure Logging Practices Objective: To ensure logs are useful for debugging and auditing without exposing sensitive information like PII, PHI, or cardholder data. Status: Passed Learn more about managing compliance generic rules or creating your own custom rules
⚪	Generic: Comprehensive Audit Trails Objective: To create a detailed and reliable record of critical system actions for security analysis and compliance. Status: No audit logs: The new logic for selecting DB backend and constructing URIs does not include any audit logging of critical configuration decisions or outcomes. Referred Code if instance.Spec.Db.UsePostgreSQL { container.Args = append([]string{ "--storage_system=postgresql", "--quota_system=postgresql", "--postgresql_uri=postgresql://$(DB_USER):$(DB_PASSWORD)@$(DB_HOSTNAME):$(DB_PORT)/$(DB_NAME)", }, args...) } else { container.Args = append([]string{ "--storage_system=mysql", "--quota_system=mysql", "--mysql_uri=$(DB_USER):$(DB_PASSWORD)@tcp($(DB_HOSTNAME):$(DB_PORT))/$(DB_NAME)", "--mysql_max_conns=30", "--mysql_max_idle_conns=10", }, args...) } container.Args = append(container.Args, []string{ "--rpc_endpoint=0.0.0.0:" + strconv.Itoa(int(actions.ServerPort)), "--http_endpoint=0.0.0.0:" + strconv.Itoa(int(actions.MetricsPort)), "--alsologtostderr", }...) ... (clipped 2 lines) Learn more about managing compliance generic rules or creating your own custom rules
	Generic: Robust Error Handling and Edge Case Management Objective: Ensure comprehensive error handling that provides meaningful context and graceful degradation Status: Missing validations: The code builds connection args and env refs without validating presence of required secret keys or handling missing/empty values, which could cause runtime failures without context. Referred Code if instance.Spec.MaxRecvMessageSize != nil { container.Args = append(container.Args, "--max_msg_size_bytes", fmt.Sprintf("%d", *instance.Spec.MaxRecvMessageSize)) } //Ports = containerPorts // Env variables from secret trillian-mysql userEnv := kubernetes.FindEnvByNameOrCreate(container, "DB_USER") userEnv.ValueFrom = &core.EnvVarSource{ SecretKeyRef: &core.SecretKeySelector{ Key: actions.SecretUser, LocalObjectReference: core.LocalObjectReference{ Name: instance.Status.Db.DatabaseSecretRef.Name, }, }, } passwordEnv := kubernetes.FindEnvByNameOrCreate(container, "DB_PASSWORD") passwordEnv.ValueFrom = &core.EnvVarSource{ SecretKeyRef: &core.SecretKeySelector{ Key: actions.SecretPassword, LocalObjectReference: core.LocalObjectReference{ ... (clipped 34 lines) Learn more about managing compliance generic rules or creating your own custom rules
	Generic: Security-First Input Validation and Data Handling Objective: Ensure all data inputs are validated, sanitized, and handled securely to prevent vulnerabilities Status: Secret handling: Secrets are interpolated into connection URIs via env vars without visible validation or TLS enforcement for PostgreSQL, which may risk insecure connections or misconfiguration. Referred Code if instance.Spec.Db.UsePostgreSQL { container.Args = append([]string{ "--storage_system=postgresql", "--quota_system=postgresql", "--postgresql_uri=postgresql://$(DB_USER):$(DB_PASSWORD)@$(DB_HOSTNAME):$(DB_PORT)/$(DB_NAME)", }, args...) } else { container.Args = append([]string{ "--storage_system=mysql", "--quota_system=mysql", "--mysql_uri=$(DB_USER):$(DB_PASSWORD)@tcp($(DB_HOSTNAME):$(DB_PORT))/$(DB_NAME)", "--mysql_max_conns=30", "--mysql_max_idle_conns=10", }, args...) } container.Args = append(container.Args, []string{ "--rpc_endpoint=0.0.0.0:" + strconv.Itoa(int(actions.ServerPort)), "--http_endpoint=0.0.0.0:" + strconv.Itoa(int(actions.MetricsPort)), "--alsologtostderr", }...) ... (clipped 2 lines) Learn more about managing compliance generic rules or creating your own custom rules
Update

Compliance status legend

🟢 - Fully Compliant
🟡 - Partial Compliant
🔴 - Not Compliant
⚪ - Requires Further Human Verification
🏷️ - Compliance label

qodo-code-review · 2025-12-04T14:52:55Z

PR Code Suggestions ✨

Explore these optional code suggestions:

Category	Suggestion	Impact
Possible issue	Use correct database connection defaults Introduce PostgreSQL-specific constants for `port`, `host`, and `user`, and use them conditionally to prevent connection failures when creating a PostgreSQL database. internal/controller/trillian/actions/db/handle_secret.go [29-41] const ( - port = 3306 - host = "trillian-mysql" - user = "mysql" + mysqlPort = 3306 + mysqlHost = "trillian-mysql" + mysqlUser = "mysql" + postgresPort = 5432 + postgresHost = "trillian-db" + postgresUser = "postgres" + databaseName = "trillian" dbConnectionResource = "trillian-db-connection" dbConnectionSecretName = "trillian-db-connection-" annotationDatabase = labels.LabelNamespace + "/" + trillian.SecretDatabaseName annotationUser = labels.LabelNamespace + "/" + trillian.SecretUser annotationPort = labels.LabelNamespace + "/" + trillian.SecretPort annotationHost = labels.LabelNamespace + "/" + trillian.SecretHost ) `[To ensure code accuracy, apply this suggestion manually]` Suggestion importance[1-10]: 8 __ Why: The suggestion correctly identifies that hardcoded MySQL default values will cause the new PostgreSQL feature to fail, which is a significant bug in the added functionality.	Medium
	~~Parameterize methods for dynamic configuration~~ Pass the `Trillian` instance to `defaultDBData` and `secretAnnotations` to dynamically configure database secrets for either MySQL or PostgreSQL based on the `UsePostgreSQL` flag. internal/controller/trillian/actions/db/handle_secret.go [173-196] -func (i handleSecretAction) defaultDBData() map[string][]byte { +func (i handleSecretAction) defaultDBData(instance rhtasv1alpha1.Trillian) map[string][]byte { // Define a new Secret object - var rootPass []byte var dbPass []byte - rootPass = utils2.GeneratePassword(12) dbPass = utils2.GeneratePassword(12) + + if instance.Spec.Db.UsePostgreSQL { + return map[string][]byte{ + trillian.SecretPassword: dbPass, + trillian.SecretDatabaseName: []byte(databaseName), + trillian.SecretUser: []byte(postgresUser), + trillian.SecretPort: []byte(strconv.Itoa(postgresPort)), + trillian.SecretHost: []byte(postgresHost), + } + } + + rootPass := utils2.GeneratePassword(12) return map[string][]byte{ trillian.SecretRootPassword: rootPass, trillian.SecretPassword: dbPass, trillian.SecretDatabaseName: []byte(databaseName), - trillian.SecretUser: []byte(user), - trillian.SecretPort: []byte(strconv.Itoa(port)), - trillian.SecretHost: []byte(host), + trillian.SecretUser: []byte(mysqlUser), + trillian.SecretPort: []byte(strconv.Itoa(mysqlPort)), + trillian.SecretHost: []byte(mysqlHost), } } -func (i handleSecretAction) secretAnnotations() map[string]string { +func (i handleSecretAction) secretAnnotations(instance rhtasv1alpha1.Trillian) map[string]string { + if instance.Spec.Db.UsePostgreSQL { + return map[string]string{ + annotationDatabase: databaseName, + annotationUser: postgresUser, + annotationPort: strconv.Itoa(postgresPort), + annotationHost: postgresHost, + } + } return map[string]string{ annotationDatabase: databaseName, - annotationUser: user, - annotationPort: strconv.Itoa(port), - annotationHost: host, + annotationUser: mysqlUser, + annotationPort: strconv.Itoa(mysqlPort), + annotationHost: mysqlHost, } } `[Suggestion processed]` Suggestion importance[1-10]: 8 __ Why: This suggestion provides a correct and necessary implementation to fix a bug where database secret generation would fail for PostgreSQL due to hardcoded MySQL values.	Medium
	Avoid appending arguments multiple times Refactor the container argument construction logic to avoid appending the `args...` slice twice, preventing potential errors from duplicate flags. internal/controller/trillian/utils/server-deployment.go [130-153] - container.Args = append([]string{ - "--storage_system=mysql", - "--quota_system=mysql", - "--mysql_uri=$(MYSQL_USER):$(MYSQL_PASSWORD)@tcp($(MYSQL_HOSTNAME):$(MYSQL_PORT))/$(MYSQL_DATABASE)", - "--mysql_max_conns=30", - "--mysql_max_idle_conns=10", -... - }, args...) + baseArgs := []string{ + "--rpc_endpoint=0.0.0.0:" + strconv.Itoa(int(actions.ServerPort)), + "--http_endpoint=0.0.0.0:" + strconv.Itoa(int(actions.MetricsPort)), + "--alsologtostderr", + } + + if instance.Spec.Db.UsePostgreSQL { + container.Args = append([]string{ + "--storage_system=postgresql", + "--quota_system=postgresql", + "--postgresql_uri=postgresql://$(DB_USER):$(DB_PASSWORD)@$(DB_HOSTNAME):$(DB_PORT)/$(DB_NAME)", + }, baseArgs...) + } else { + container.Args = append([]string{ + "--storage_system=mysql", + "--quota_system=mysql", + "--mysql_uri=$(DB_USER):$(DB_PASSWORD)@tcp($(DB_HOSTNAME):$(DB_PORT))/$(DB_NAME)", + "--mysql_max_conns=30", + "--mysql_max_idle_conns=10", + }, baseArgs...) + } if instance.Spec.MaxRecvMessageSize != nil { container.Args = append(container.Args, "--max_msg_size_bytes", fmt.Sprintf("%d", *instance.Spec.MaxRecvMessageSize)) } + container.Args = append(container.Args, args...) `[To ensure code accuracy, apply this suggestion manually]` Suggestion importance[1-10]: 7 __ Why: The suggestion correctly identifies a logical flaw where arguments are appended twice, which could lead to subtle bugs, and proposes a cleaner, more robust implementation.	Medium
High-level	✅ ~~Refactor database logic using interfaces~~ Suggestion Impact: The commit refactored the DB conditionals from a boolean (UsePostgreSQL) to a provider-based switch and centralized provider-specific secret keys via a map, moving toward an abstraction over multiple database backends. While it did not introduce an interface with separate implementations as suggested, it addressed the core goal of abstracting and generalizing DB handling. code diff: - if instance.Spec.Db.UsePostgreSQL { + switch instance.Spec.Db.Provider { + case "postgresql": container.Args = append([]string{ "--storage_system=postgresql", "--quota_system=postgresql", "--postgresql_uri=postgresql://$(DB_USER):$(DB_PASSWORD)@$(DB_HOSTNAME):$(DB_PORT)/$(DB_NAME)", }, args...) - } else { + case "mysql": container.Args = append([]string{ "--storage_system=mysql", "--quota_system=mysql", @@ -140,6 +149,8 @@ "--mysql_max_conns=30", "--mysql_max_idle_conns=10", }, args...) + default: + return fmt.Errorf("unsupported database provider %s", instance.Spec.Db.Provider) } container.Args = append(container.Args, []string{ @@ -154,10 +165,31 @@ //Ports = containerPorts // Env variables from secret trillian-mysql + dbProviderKeys := map[string]dbKeys{ + "mysql": { + User: actions.SecretUser, + Password: actions.SecretPassword, + Host: actions.SecretHost, + Port: actions.SecretPort, + Database: actions.SecretDatabaseName, + }, + "postgresql": { + User: actions.PgSecretUser, + Password: actions.PgSecretPassword, + Host: actions.PgSecretHost, + Port: actions.PgSecretPort, + Database: actions.PgSecretDatabaseName, + }, + } + keys, ok := dbProviderKeys[instance.Spec.Db.Provider] + if !ok { + return fmt.Errorf("unsupported database provider %s", instance.Spec.Db.Provider) + } + userEnv := kubernetes.FindEnvByNameOrCreate(container, "DB_USER") userEnv.ValueFrom = &core.EnvVarSource{ SecretKeyRef: &core.SecretKeySelector{ - Key: actions.SecretUser, + Key: keys.User, LocalObjectReference: core.LocalObjectReference{ Name: instance.Status.Db.DatabaseSecretRef.Name, }, @@ -167,7 +199,7 @@ passwordEnv := kubernetes.FindEnvByNameOrCreate(container, "DB_PASSWORD") passwordEnv.ValueFrom = &core.EnvVarSource{ SecretKeyRef: &core.SecretKeySelector{ - Key: actions.SecretPassword, + Key: keys.Password, LocalObjectReference: core.LocalObjectReference{ Name: instance.Status.Db.DatabaseSecretRef.Name, }, @@ -177,7 +209,7 @@ hostEnv := kubernetes.FindEnvByNameOrCreate(container, "DB_HOSTNAME") hostEnv.ValueFrom = &core.EnvVarSource{ SecretKeyRef: &core.SecretKeySelector{ - Key: actions.SecretHost, + Key: keys.Host, LocalObjectReference: core.LocalObjectReference{ Name: instance.Status.Db.DatabaseSecretRef.Name, }, @@ -187,7 +219,7 @@ containerPortEnv := kubernetes.FindEnvByNameOrCreate(container, "DB_PORT") containerPortEnv.ValueFrom = &core.EnvVarSource{ SecretKeyRef: &core.SecretKeySelector{ - Key: actions.SecretPort, + Key: keys.Port, LocalObjectReference: core.LocalObjectReference{ Name: instance.Status.Db.DatabaseSecretRef.Name, }, @@ -197,7 +229,7 @@ dbEnv := kubernetes.FindEnvByNameOrCreate(container, "DB_NAME") dbEnv.ValueFrom = &core.EnvVarSource{ SecretKeyRef: &core.SecretKeySelector{ - Key: actions.SecretDatabaseName, + Key: keys.Database, LocalObjectReference: core.LocalObjectReference{ Name: instance.Status.Db.DatabaseSecretRef.Name, }, @@ -236,9 +268,10 @@ func WithTlsDB(instance v1alpha1.Trillian, caPath string, name string) func(apps.Deployment) error { return func(dp apps.Deployment) error { c := kubernetes.FindContainerByNameOrCreate(&dp.Spec.Template.Spec, name) - if instance.Spec.Db.UsePostgreSQL { + switch instance.Spec.Db.Provider { + case "postgresql": c.Args = append(c.Args, "--postgresql_tls_ca", caPath) - } else { + case "mysql": c.Args = append(c.Args, "--mysql_tls_ca", caPath) mysqlServerName := "$(DB_HOSTNAME)." + instance.Namespace + ".svc" @@ -246,6 +279,8 @@ mysqlServerName = "$(DB_HOSTNAME)" } c.Args = append(c.Args, "--mysql_server_name", mysqlServerName) + default: + return fmt.Errorf("unsupported database provider %s", instance.Spec.Db.Provider) } return nil } # File: internal/controller/trillian/utils/server-deployment.go @@ -16,6 +16,14 @@ metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/util/intstr" ) + +type dbKeys struct { + User string + Password string + Host string + Port string + Database string +} func EnsureServerDeployment(instance v1alpha1.Trillian, labels map[string]string) []func(apps.Deployment) error { return []func(deployment apps.Deployment) error{ @@ -126,13 +134,14 @@ container := kubernetes.FindContainerByNameOrCreate(&template.Spec, name) container.Image = image - if instance.Spec.Db.UsePostgreSQL { + switch instance.Spec.Db.Provider { + case "postgresql": container.Args = append([]string{ "--storage_system=postgresql", "--quota_system=postgresql", "--postgresql_uri=postgresql://$(DB_USER):$(DB_PASSWORD)@$(DB_HOSTNAME):$(DB_PORT)/$(DB_NAME)", }, args...) - } else { + case "mysql": container.Args = append([]string{ "--storage_system=mysql", "--quota_system=mysql", @@ -140,6 +149,8 @@ "--mysql_max_conns=30", "--mysql_max_idle_conns=10", }, args...) + default: + return fmt.Errorf("unsupported database provider %s", instance.Spec.Db.Provider) } container.Args = append(container.Args, []string{ @@ -154,10 +165,31 @@ //Ports = containerPorts // Env variables from secret trillian-mysql + dbProviderKeys := map[string]dbKeys{ + "mysql": { + User: actions.SecretUser, + Password: actions.SecretPassword, + Host: actions.SecretHost, + Port: actions.SecretPort, + Database: actions.SecretDatabaseName, + }, + "postgresql": { + User: actions.PgSecretUser, + Password: actions.PgSecretPassword, + Host: actions.PgSecretHost, + Port: actions.PgSecretPort, + Database: actions.PgSecretDatabaseName, + }, + } + keys, ok := dbProviderKeys[instance.Spec.Db.Provider] + if !ok { + return fmt.Errorf("unsupported database provider %s", instance.Spec.Db.Provider) + } + userEnv := kubernetes.FindEnvByNameOrCreate(container, "DB_USER") userEnv.ValueFrom = &core.EnvVarSource{ SecretKeyRef: &core.SecretKeySelector{ - Key: actions.SecretUser, + Key: keys.User, LocalObjectReference: core.LocalObjectReference{ Name: instance.Status.Db.DatabaseSecretRef.Name, }, @@ -167,7 +199,7 @@ passwordEnv := kubernetes.FindEnvByNameOrCreate(container, "DB_PASSWORD") passwordEnv.ValueFrom = &core.EnvVarSource{ SecretKeyRef: &core.SecretKeySelector{ - Key: actions.SecretPassword, + Key: keys.Password, LocalObjectReference: core.LocalObjectReference{ Name: instance.Status.Db.DatabaseSecretRef.Name, }, @@ -177,7 +209,7 @@ hostEnv := kubernetes.FindEnvByNameOrCreate(container, "DB_HOSTNAME") hostEnv.ValueFrom = &core.EnvVarSource{ SecretKeyRef: &core.SecretKeySelector{ - Key: actions.SecretHost, + Key: keys.Host, LocalObjectReference: core.LocalObjectReference{ Name: instance.Status.Db.DatabaseSecretRef.Name, }, @@ -187,7 +219,7 @@ containerPortEnv := kubernetes.FindEnvByNameOrCreate(container, "DB_PORT") containerPortEnv.ValueFrom = &core.EnvVarSource{ SecretKeyRef: &core.SecretKeySelector{ - Key: actions.SecretPort, + Key: keys.Port, LocalObjectReference: core.LocalObjectReference{ Name: instance.Status.Db.DatabaseSecretRef.Name, }, @@ -197,7 +229,7 @@ dbEnv := kubernetes.FindEnvByNameOrCreate(container, "DB_NAME") dbEnv.ValueFrom = &core.EnvVarSource{ SecretKeyRef: &core.SecretKeySelector{ - Key: actions.SecretDatabaseName, + Key: keys.Database, LocalObjectReference: core.LocalObjectReference{ Name: instance.Status.Db.DatabaseSecretRef.Name, }, @@ -236,9 +268,10 @@ func WithTlsDB(instance v1alpha1.Trillian, caPath string, name string) func(apps.Deployment) error { return func(dp apps.Deployment) error { c := kubernetes.FindContainerByNameOrCreate(&dp.Spec.Template.Spec, name) - if instance.Spec.Db.UsePostgreSQL { + switch instance.Spec.Db.Provider { + case "postgresql": c.Args = append(c.Args, "--postgresql_tls_ca", caPath) - } else { + case "mysql": c.Args = append(c.Args, "--mysql_tls_ca", caPath) mysqlServerName := "$(DB_HOSTNAME)." + instance.Namespace + ".svc" @@ -246,6 +279,8 @@ mysqlServerName = "$(DB_HOSTNAME)" } c.Args = append(c.Args, "--mysql_server_name", mysqlServerName) + default: + return fmt.Errorf("unsupported database provider %s", instance.Spec.Db.Provider) } Refactor the conditional logic for MySQL and PostgreSQL into a `DatabaseProvider` interface with separate implementations. This will abstract database-specific configurations, improving maintainability and simplifying the addition of new database backends. Examples: internal/controller/trillian/utils/server-deployment.go [129-143] if instance.Spec.Db.UsePostgreSQL { container.Args = append([]string{ "--storage_system=postgresql", "--quota_system=postgresql", "--postgresql_uri=postgresql://$(DB_USER):$(DB_PASSWORD)@$(DB_HOSTNAME):$(DB_PORT)/$(DB_NAME)", }, args...) } else { container.Args = append([]string{ "--storage_system=mysql", "--quota_system=mysql", ... (clipped 5 lines) internal/controller/trillian/utils/server-deployment.go [239-249] if instance.Spec.Db.UsePostgreSQL { c.Args = append(c.Args, "--postgresql_tls_ca", caPath) } else { c.Args = append(c.Args, "--mysql_tls_ca", caPath) mysqlServerName := "$(DB_HOSTNAME)." + instance.Namespace + ".svc" if !instance.Spec.Db.Create { mysqlServerName = "$(DB_HOSTNAME)" } c.Args = append(c.Args, "--mysql_server_name", mysqlServerName) ... (clipped 1 lines) Solution Walkthrough: Before: // internal/controller/trillian/utils/server-deployment.go func ensureDeployment(instance v1alpha1.Trillian, ...) { // ... if instance.Spec.Db.UsePostgreSQL { container.Args = //... postgresql args } else { container.Args = //... mysql args } // ... } func WithTlsDB(instance v1alpha1.Trillian, ...) { // ... if instance.Spec.Db.UsePostgreSQL { c.Args = append(c.Args, "--postgresql_tls_ca", caPath) } else { c.Args = append(c.Args, "--mysql_tls_ca", caPath) // ... more mysql specific args } // ... } After: // internal/controller/trillian/db_provider.go type DatabaseProvider interface { GetArgs(instance v1alpha1.Trillian, args ...string) []string GetTlsArgs(instance v1alpha1.Trillian, caPath string) []string } // ... MySQLProvider and PostgreSQLProvider implementations // internal/controller/trillian/utils/server-deployment.go func getDbProvider(usePostgreSQL bool) DatabaseProvider { if usePostgreSQL { return &PostgreSQLProvider{} } return &MySQLProvider{} } func ensureDeployment(instance v1alpha1.Trillian, ...) { provider := getDbProvider(instance.Spec.Db.UsePostgreSQL) container.Args = provider.GetArgs(instance, args...) } func WithTlsDB(instance v1alpha1.Trillian, ...) { provider := getDbProvider(instance.Spec.Db.UsePostgreSQL) c.Args = append(c.Args, provider.GetTlsArgs(instance, caPath)...) } Suggestion importance[1-10]: 7 __ Why: The suggestion correctly identifies repeated conditional logic for database configuration and proposes a valid abstraction using an interface, which significantly improves maintainability and extensibility for future database support.	Medium
Update

internal/controller/trillian/actions/db/handle_secret.go

osmman · 2025-12-08T08:51:41Z

internal/controller/trillian/actions/constants.go

-	SecretUser         = "mysql-user"
-	SecretPort         = "mysql-port"
-	SecretHost         = "mysql-host"
+	SecretPassword     = "db-password"


You unable to simple rename these values, it broke existing instances on upgrade. You have to keep both options for backward compatibility.

osmman · 2025-12-08T08:57:08Z

api/v1alpha1/trillian_types.go

+	// If false (default), MySQL/MariaDB is expected.
+	//+kubebuilder:default:=false
+	//+optional
+	UsePostgreSQL bool `json:"usePostgreSQL"`


Trillian support multiple storage systems so it is not right way to specify which to use. It should be enum type which has these options allowed mysql and postgresql. Please name the field provider so it is aligned with other CRDs standard.

bouskaJ · 2025-12-08T13:58:07Z

api/v1alpha1/trillian_types.go

 	// mysql-password: The password to connect to the MySQL server
 	// mysql-database: The database to connect to
-	//+optional
+	// For PostgreSQL, only connection is supported, the secret must contain:


I'm concerned about the new set of variables. Could we consider using the data source URL, similar to how we handle the search index? https://github.com/securesign/secure-sign-operator/blob/main/api/v1alpha1/rekor_types.go#L154

Envs are mounted by https://github.com/securesign/secure-sign-operator/blob/main/api/v1alpha1/rekor_types.go#L55
Any changes must be implemented carefully to maintain backward compatibility.

Could we consider using the data source URL:

Do you mean for both providers (mysql and postgresql) ?

If it's only for postgresql, I have added env variables for postgresql to have a similar config as mysql to be able to re-use the existing code in server-deployment.go

yes, for both. I would like to have unified API for all components. The searchIndex uses the following format (see the data source spec https://jdbc.postgresql.org/documentation/use/#connecting-to-the-database):
"$(MYSQL_USER):$(MYSQL_PASSWORD)@tcp(my-mysql.$(NAMESPACE).svc:3300)/$(MYSQL_DB)"
https://github.com/securesign/secure-sign-operator/blob/main/test/e2e/byodb_test.go#L43

I know, that this could mean an API change that we can't afford in a minor version release. I just want to bring this to the table and explore.

what about changing searchIndex config to be as trillian DB's ? I find it easier to be handled by the customers.
I think providing the full database URL puts too much responsibility on the customer and increases the chance of mistakes. Supplying only the essential variables allows the operator to assemble the URL correctly and consistently.

Utilizing a URL to pass the datasource is a widely accepted convention. This method provides the flexibility to embed numerous configuration parameters, such as those relating to TLS (Transport Layer Security). Conversely, the discovery of proprietary parameter names may present challenges and is susceptible to error.

so if we apply it on the trillian mysql db, won't this break upgrade scenario and backward compatibility ?

I was thinking about providing a default URL that will read content from the secret. The secret needs to be mounted as Auth (https://github.com/securesign/secure-sign-operator/blob/main/internal/controller/rekor/actions/server/deployment.go#L82). Do you think it is doable?

but this would break the upgrade scenario anyway, right ?

I don't think so.

The URL will have default value -> no braking change

the credentials file will be just mapped diferently to the deployment but it will be still working

This comment was marked as outdated.

Sign in to view

qodo-code-review bot added the Review effort 2/5 label Dec 4, 2025

This comment was marked as outdated.

Sign in to view

Support PostgreSQL as an optional backend database for Trillian

5a541ac

fghanmi force-pushed the SECURESIGN-3170 branch from 153996a to 5a541ac Compare December 4, 2025 14:56

fghanmi added 3 commits December 4, 2025 17:05

update var names

a03496c

updates

b80c022

update DbPvcName

1a10ce0

qodo-code-review bot reviewed Dec 5, 2025

View reviewed changes

internal/controller/trillian/actions/db/handle_secret.go Show resolved Hide resolved

updates

2a844ca

osmman requested changes Dec 8, 2025

View reviewed changes

fghanmi added 4 commits December 8, 2025 12:00

review updates: ensure backward compatibility

3094cfe

fix manifests

826435f

fix go lint

1bde570

update trillian/utils/server-deployment.go

0307a95

fghanmi requested a review from osmman December 8, 2025 12:44

bouskaJ reviewed Dec 8, 2025

View reviewed changes

[SECURESIGN-3170] Support PostgreSQL as an optional backend database for Trillian #1483

Are you sure you want to change the base?

[SECURESIGN-3170] Support PostgreSQL as an optional backend database for Trillian #1483

Uh oh!

Conversation

fghanmi commented Dec 4, 2025 • edited by qodo-code-review bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Type

Description

Diagram Walkthrough

File Walkthrough

Uh oh!

This comment was marked as outdated.

qodo-code-review bot commented Dec 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Compliance Guide 🔍

Uh oh!

This comment was marked as outdated.

Uh oh!

qodo-code-review bot commented Dec 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Code Suggestions ✨

Examples:

Solution Walkthrough:

Before:

After:

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

fghanmi commented Dec 4, 2025 •

edited by qodo-code-review bot

Loading

qodo-code-review bot commented Dec 4, 2025 •

edited

Loading

qodo-code-review bot commented Dec 4, 2025 •

edited

Loading