Repository: cloudnative-pg/cloudnative-pg

Issue:
[Bug]: monolith import may create unintended default database
### Is there an existing issue already for this bug?

- [x] I have searched for an existing issue, and could not find anything. I believe this is a new bug.

### I have read the troubleshooting guide

- [x] I have read the troubleshooting guide and I think this is a new bug.

### I am running a supported version of CloudNativePG

- [x] I have read the troubleshooting guide and I think this is a new bug.

### Contact Details

jaime.silvela@mailfence.com

### Version

1.26 (latest patch)

### What version of Kubernetes are you using?

1.33

### What is your Kubernetes environment?

Self-managed: kind (evaluation)

### How did you install the operator?

YAML manifest

### What happened?

Investigating #7447 , the issue opened by @smiyc , I agree with his assessment that it is a bug,
and moreover I find the mis-behavior bigger than just that bug.

In this description I discuss the problem, compare with microservice approach, and propose the fix.
Section headings to hopefully aid navigation.

## monolith import may take in N source databases and create N + 1, or alter the "app" database owner

To recap from #7447 , when using monolith import, a `(database, owner)` of `("app", "xxx")` results in an imported database of `("app", "app")`, whereas a `(database, owner)` of `("foo", "xxx")` is imported, as expected, as `("foo", "xxx")`.

Now, in the second case, the monolith import doesn't create only `("foo", "xxx")`, but adds the default `("app", "app")` too.

Doing a `kubectl get -o yaml` of the running cluster reveals:

``` yaml
spec:
  <snipped>
  bootstrap:
    initdb:
      database: app
      import:
        databases:
        - foo
        roles:
        - xxx
        source:
          externalCluster: cluster-other
        type: monolith
      owner: app
```

In the documentation for the monolith import (https://cloudnative-pg.io/documentation/current/database_import/#the-monolith-type), the expectation is that N `.initdb.import.databases` will result in N databases in the created cluster.
This example shows this is not the case.

The cause is the defaulting webhook, which will create the default (database, user) if there are no `initdb.database` and `initdb.owner` specified.

## comparison with microservice

The defaulting webhook behavior makes sense for the microservice approach.
There, a single DB from the source cluster is chosen to be imported, and in the target cluster will result in `("app", "app")` by default, unless the user specifies a `(database, owner)`.

For monolith this does not make sense. Firstly, monolith is more useful for N databases to import rather than just one.
And in addition, the monolith approach seems tailor made for copying from the source cluster as faithfully as possible.

## proposal

The right fix, in my view, is that the defaulting webhook should not pre-populate the database/owner, if there is a monolith import specified.

Alternatives considered:

Force the user of monolith imports to specify explicitly which of the imported databases is "default" via the `initdb.database` and `initdb.owner`.
 
Also, the documentation needs some updating. And ... we don't have a sample file for monolith import, which shows that it's been taken less seriously than microservice.


### Cluster resource

```shell
apiVersion: postgresql.cnpg.io/v1
kind: Cluster
metadata:
  name: cluster-example-new-monolith
spec:
  instances: 1
  
  storage:
    size: 1Gi

  bootstrap:
    initdb:
      import:
        type: monolith
        databases:
          - foo
        roles:
          - xxx
        source:
          externalCluster: cluster-example

  externalClusters:
    - name: cluster-example
      connectionParameters:
        host: cluster-example-rw
        dbname: postgres
```

### Relevant log output

```shell

```

### Code of Conduct

- [x] I agree to follow this project's Code of Conduct

Discussion:
@dosubot: <!-- Greeting -->
Hi @jsilvela! I'm [Dosu](https://go.dosu.dev/dosubot) and I’m helping the cloudnative-pg team.

<!-- Answer -->
You are correct—there are no existing issues or discussions about this bug. The current implementation of the defaulting webhook sets the default database and owner to "app" if they are not specified, regardless of whether a monolith import is being used. This leads to the unintended creation of an extra default database when using monolith imports, which is not the expected behavior according to the documentation and typical use case for monolith imports ([source](https://github.com/cloudnative-pg/cloudnative-pg/blob/17af8d46922e036a5a07931085ef8c511f71e9d6/api/v1/cluster_defaults.go)).

Your proposal to change the defaulting webhook so it does not pre-populate the database/owner fields when a monolith import is specified is reasonable and aligns with the documented intent of the monolith import process ([source](https://cloudnative-pg.io/documentation/current/database_import)). The documentation also does not mention any defaulting behavior for these fields in the context of monolith imports, which supports your suggestion for both a code and documentation update.

Let me know if you want more details on the relevant code paths or want to discuss possible workarounds until this is addressed.


<!-- Dosu Comment Footer -->
*To reply, just mention [@dosu](https://go.dosu.dev/dosubot).*

---
<sup>How did I do? [Good](https://app.dosu.dev/response-feedback/b4d1f51f-713d-4341-8ae6-a9cc737f679e?feedback_type=great_response) | [Irrelevant](https://app.dosu.dev/response-feedback/b4d1f51f-713d-4341-8ae6-a9cc737f679e?feedback_type=irrelevant_answer) | [Incorrect](https://app.dosu.dev/response-feedback/b4d1f51f-713d-4341-8ae6-a9cc737f679e?feedback_type=incorrect_sources) | [Verbose](https://app.dosu.dev/response-feedback/b4d1f51f-713d-4341-8ae6-a9cc737f679e?feedback_type=too_verbose) | [Hallucination](https://app.dosu.dev/response-feedback/b4d1f51f-713d-4341-8ae6-a9cc737f679e?feedback_type=hallucination) | [Report 🐛](https://app.dosu.dev/response-feedback/b4d1f51f-713d-4341-8ae6-a9cc737f679e?feedback_type=bug_report) | [Other](https://app.dosu.dev/response-feedback/b4d1f51f-713d-4341-8ae6-a9cc737f679e?feedback_type=other)</sup>&nbsp;&nbsp;[![Join Discord](https://img.shields.io/badge/join-5865F2?logo=discord&logoColor=white&label=)](https://go.dosu.dev/discord-bot)&nbsp;[![Share on X](https://img.shields.io/badge/X-share-black)](https://twitter.com/intent/tweet?text=%40dosu_ai%20helped%20me%20solve%20this%20issue!&url=https%3A//github.com/cloudnative-pg/cloudnative-pg/issues/7731)@armru: Hello Jaime, thanks for the Issue and PR! 

I agree that the monolith approach should do a `carbon` copy of the database.

I would love to hear the other mantainers opinion on this, I'm also adding this issue to the 1.27 milestone so we are sure to have this discussion.

Solution patch:
diff --git a/api/v1/cluster_defaults.go b/api/v1/cluster_defaults.go
index 5a5607d8..707b3f42 100644
--- a/api/v1/cluster_defaults.go
+++ b/api/v1/cluster_defaults.go
@@ -238,7 +238,11 @@ func (r *Cluster) defaultInitDB() {
 	}
 
 	if r.Spec.Bootstrap.InitDB.Database == "" {
-		r.Spec.Bootstrap.InitDB.Database = DefaultApplicationDatabaseName
+		// Set the default only if not executing a monolithic import
+		if r.Spec.Bootstrap.InitDB.Import == nil ||
+			r.Spec.Bootstrap.InitDB.Import.Type != MonolithSnapshotType {
+			r.Spec.Bootstrap.InitDB.Database = DefaultApplicationDatabaseName
+		}
 	}
 	if r.Spec.Bootstrap.InitDB.Owner == "" {
 		r.Spec.Bootstrap.InitDB.Owner = r.Spec.Bootstrap.InitDB.Database
diff --git a/docs/src/database_import.md b/docs/src/database_import.md
index f397a6c0..593ea430 100644
--- a/docs/src/database_import.md
+++ b/docs/src/database_import.md
@@ -234,7 +234,8 @@ There are a few things you need to be aware of when using the `monolith` type:
   that needs to run `pg_dump` and retrieve roles information (*superuser* is
   OK)
 - Currently, the `pg_dump -Fd` result is stored temporarily inside the `dumps`
-  folder in the `PGDATA` volume, so there should be enough available space to
+  folder in the `PGDATA` volume of the destination cluster's instances, so
+  there should be enough available space to
   temporarily contain the dump result on the assigned node, as well as the
   restored data and indexes. Once the import operation is completed, this
   folder is automatically deleted by the operator.
@@ -250,7 +251,117 @@ There are a few things you need to be aware of when using the `monolith` type:
   and those databases not allowing connections
 - After the clone procedure is done, `ANALYZE VERBOSE` is executed for every
   database.
-- `postImportApplicationSQL` field is not supported
+- The `postImportApplicationSQL` field is not supported
+
+!!! Hint
+    The databases and their owners are preserved exactly as they exist in the
+    source cluster—no `app` database or user will be created during import. If your
+    `bootstrap.initdb` stanza specifies custom `database` and `owner` values that
+    do not match any of the databases or users being imported, the instance
+    manager will create a new, empty application database and owner role with those
+    specified names, while leaving the imported databases and owners unchanged.
+
+## A practical example
+
+There is nothing to stop you from using the `monolith` approach to import a
+single database. It is interesting to see how the results of doing so would
+differ from using the `microservice` approach.
+
+Given a source cluster, for example the following, with a database named
+`mydb` owned by role `me`:
+
+``` yaml
+apiVersion: postgresql.cnpg.io/v1
+kind: Cluster
+metadata:
+  name: cluster-example
+spec:
+  instances: 1
+
+  postgresql:
+    pg_hba:
+      - host all all all trust
+
+  storage:
+    size: 1Gi
+
+  bootstrap:
+    initdb:
+      database: mydb
+      owner: me
+```
+
+We can import it via `microservice`:
+
+``` yaml
+apiVersion: postgresql.cnpg.io/v1
+kind: Cluster
+metadata:
+  name: cluster-example-microservice
+spec:
+  instances: 1
+  
+  storage:
+    size: 1Gi
+
+  bootstrap:
+    initdb:
+      import:
+        type: microservice
+        databases:
+          - mydb
+        source:
+          externalCluster: cluster-example
+
+  externalClusters:
+    - name: cluster-example
+      connectionParameters:
+        host: cluster-example-rw
+        dbname: postgres
+```
+
+as well as via monolith:
+
+``` yaml
+apiVersion: postgresql.cnpg.io/v1
+kind: Cluster
+metadata:
+  name: cluster-example-monolith
+spec:
+  instances: 1
+  
+  storage:
+    size: 1Gi
+
+  bootstrap:
+    initdb:
+      import:
+        type: monolith
+        databases:
+          - mydb
+        roles:
+          - me
+        source:
+          externalCluster: cluster-example
+
+  externalClusters:
+    - name: cluster-example
+      connectionParameters:
+        host: cluster-example-rw
+        dbname: postgres
+```
+
+In both cases, the database's contents will be imported, but:
+
+- In the microservice case, the imported database's name and owner both become
+  `app`, or whichever configuration for the fields `database` and `owner` are
+  set in the `bootstrap.initdb` stanza.
+- In the monolith case, the database and owner are kept exactly as in the source
+  cluster, i.e. `mydb` and `me` respectively. No `app` database nor user will be
+  created. If there are custom settings for `database` and `owner` in the
+  `bootstrap.initdb` stanza that don't match the source databases/owners to
+  import, the instance manager will create a new empty application database and
+  owner role, but will leave the imported databases/owners intact.
 
 ## Import optimizations
 



Test patch:
diff --git a/tests/e2e/cluster_monolithic_test.go b/tests/e2e/cluster_monolithic_test.go
index 23512785..e12a7407 100644
--- a/tests/e2e/cluster_monolithic_test.go
+++ b/tests/e2e/cluster_monolithic_test.go
@@ -185,6 +185,20 @@ var _ = Describe("Imports with Monolithic Approach", Label(tests.LabelImportingD
 			}
 		})
 
+		By("verifying that no extra application database or owner were created", func() {
+			stmt, err := connTarget.Prepare("SELECT count(*) FROM pg_catalog.pg_database WHERE datname = $1")
+			Expect(err).ToNot(HaveOccurred())
+			var matchCount int
+			err = stmt.QueryRowContext(env.Ctx, "app").Scan(&matchCount)
+			Expect(err).ToNot(HaveOccurred())
+			Expect(matchCount).To(BeZero(), "app database should not exist")
+			stmt, err = connTarget.Prepare("SELECT count(*) from pg_catalog.pg_user WHERE usename = $1")
+			Expect(err).ToNot(HaveOccurred())
+			err = stmt.QueryRowContext(env.Ctx, "app").Scan(&matchCount)
+			Expect(err).ToNot(HaveOccurred())
+			Expect(matchCount).To(BeZero(), "app user should not exist")
+		})
+
 		By(fmt.Sprintf("verifying that the source superuser '%s' became a normal user in target",
 			databaseSuperUser), func() {
 			row := connTarget.QueryRow(fmt.Sprintf(