diff --git a/README.md b/README.md index 8f5d8bd39..fc46d8472 100644 --- a/README.md +++ b/README.md @@ -13,6 +13,7 @@ Manage external PostgreSQL databases in Kubernetes with ease—supporting AWS RD - [Features](#features) - [Supported Cloud Providers](#supported-cloud-providers) - [Configuration](#configuration) +- [Dedicated Operator Role](#dedicated-operator-role) - [Installation](#installation) - [Custom Resources (CRs)](#custom-resources-crs) - [Multiple Operator Support](#multiple-operator-support) @@ -66,15 +67,69 @@ Set `POSTGRES_CLOUD_PROVIDER` to `AWS` via environment variable, Kubernetes Secr Set environment variables in [`config/manager/operator.yaml`](config/manager/operator.yaml): -| Name | Description | Default | -| --- | --- | --- | -| `WATCH_NAMESPACE` | Namespace to watch. Empty string = all namespaces. | (all namespaces) | -| `POSTGRES_INSTANCE` | Operator identity for multi-instance deployments. | (empty) | -| `KEEP_SECRET_NAME` | Use user-provided secret names instead of auto-generated ones. | disabled | +| Name | Description | Default | +| ------------------- | -------------------------------------------------------------- | ---------------- | +| `WATCH_NAMESPACE` | Namespace to watch. Empty string = all namespaces. | (all namespaces) | +| `POSTGRES_INSTANCE` | Operator identity for multi-instance deployments. | (empty) | +| `KEEP_SECRET_NAME` | Use user-provided secret names instead of auto-generated ones. | disabled | > **Note:** > If enabling `KEEP_SECRET_NAME`, ensure there are no secret name conflicts in your namespace to avoid reconcile loops. +## Dedicated Operator Role + +The operator connects to PostgreSQL using the credentials configured via the `POSTGRES_*` environment variables / Secret (see below). In many setups these credentials are the _server admin_ or _master user_. + +You can also run the operator using a **dedicated operator login role** (recommended for production), for better separation of duties and easier auditing/rotation. + +### What privileges are required? + +This operator manages databases and roles, and also runs some operations inside the created databases. Your operator login role must be able to: + +- Create databases and set database owners (`CREATE DATABASE`, `ALTER DATABASE ... OWNER TO ...`) +- Grant database-level privileges (the operator runs `GRANT CREATE ON DATABASE ...`) +- Create roles/users and manage role membership (`CREATE ROLE`, `DROP ROLE`, `GRANT TO `, `REVOKE ...`) +- Connect to managed databases and: + - Create schemas (`CREATE SCHEMA ... AUTHORIZATION ...`) + - Create extensions (`CREATE EXTENSION ...`) + - Grant privileges / alter default privileges within schemas + +The operator also grants each created role to itself, so it can later revoke privileges, reassign ownership, and drop roles cleanly. + +### Example: creating an operator role + +The exact SQL depends on how your PostgreSQL instance is managed. In plain PostgreSQL (self-hosted), you can often do something like: + +```sql +-- Create a dedicated login for the Kubernetes operator +CREATE ROLE pgoperator WITH + PASSWORD 'YourSecurePassword123!' + LOGIN + CREATEDB + CREATEROLE; +``` + +For managed services, you typically create `ext_postgres_operator` while connected as the platform-provided admin and grant only the capabilities supported by that platform. + +### Cloud provider notes + +Because this is an _external / managed PostgreSQL_ operator, the feasibility of least-privilege depends on your provider. + +- **AWS RDS (PostgreSQL)** + - The initial “master user” is a member of the `rds_superuser` role. + - A dedicated operator role is usually possible: create a login role with `CREATEDB`/`CREATEROLE`, then grant it any extra permissions you need for extensions/schemas. + - Docs: + +- **GCP Cloud SQL (PostgreSQL)** + - Cloud SQL does not expose true `SUPERUSER`. The default `postgres` user is a member of `cloudsqlsuperuser` and has `CREATEROLE` and `CREATEDB`. + - You can create other users/roles with reduced privileges (for example, an operator role with `CREATEROLE`/`CREATEDB`), but some operations (notably certain extensions) may require `cloudsqlsuperuser`. + - Docs: + +- **Azure Database for PostgreSQL  Flexible Server** + - The server admin user is a member of `azure_pg_admin` and has `CREATEDB` and `CREATEROLE`; the `azuresu` superuser role is reserved for Microsoft. + - A dedicated operator role is supported: create a user with `CREATEDB`/`CREATEROLE`, optionally add it to `azure_pg_admin` if you need additional capabilities. + - Docs: + ## Installation ### Install Using Helm (Recommended) @@ -82,11 +137,13 @@ Set environment variables in [`config/manager/operator.yaml`](config/manager/ope The Helm chart for this operator is located in the `charts/ext-postgres-operator` subdirectory. Follow these steps to install: 1. Add the Helm repository: + ```bash helm repo add ext-postgres-operator https://movetokube.github.io/postgres-operator/ ``` 2. Install the operator: + ```bash helm install -n operators ext-postgres-operator ext-postgres-operator/ext-postgres-operator ``` @@ -121,11 +178,13 @@ To install the operator using Kustomize, follow these steps: 1. Configure Postgres credentials for the operator in `config/default/secret.yaml`. 2. Deploy the operator: + ```bash kubectl kustomize config/default/ | kubectl apply -f - ``` Alternatively, use [Kustomize](https://github.com/kubernetes-sigs/kustomize) directly: + ```bash kustomize build config/default/ | kubectl apply -f - ``` @@ -149,11 +208,11 @@ spec: dropOnDelete: false # Set to true if you want the operator to drop the database and role when this CR is deleted (optional) masterRole: test-db-group (optional) schemas: # List of schemas the operator should create in database (optional) - - stores - - customers + - stores + - customers extensions: # List of extensions that should be created in the database (optional) - - fuzzystrmatch - - pgcrypto + - fuzzystrmatch + - pgcrypto ``` This creates a database called `test-db` and a role `test-db-group` that is set as the owner of the database. @@ -173,14 +232,14 @@ metadata: postgres.db.movetokube.com/instance: POSTGRES_INSTANCE spec: role: username - database: my-db # This references the Postgres CR + database: my-db # This references the Postgres CR secretName: my-secret - privileges: OWNER # Can be OWNER/READ/WRITE - annotations: # Annotations to be propagated to the secrets metadata section (optional) + privileges: OWNER # Can be OWNER/READ/WRITE + annotations: # Annotations to be propagated to the secrets metadata section (optional) foo: "bar" labels: - foo: "bar" # Labels to be propagated to the secrets metadata section (optional) - secretTemplate: # Output secrets can be customized using standard Go templates + foo: "bar" # Labels to be propagated to the secrets metadata section (optional) + secretTemplate: # Output secrets can be customized using standard Go templates PQ_URL: "host={{.Host}} user={{.Role}} password={{.Password}} dbname={{.Database}}" ``` @@ -191,22 +250,22 @@ This creates a user role `username-` and grants role `test-db-group`, `tes Two `Postgres` referencing the same database can exist in more than one namespace. The last CR referencing a database will drop the group role and transfer database ownership to the role used by the operator. Every PostgresUser has a generated Kubernetes secret attached to it, which contains the following data (i.e.): -| Key | Comment | -|----------------------|---------------------| -| `DATABASE_NAME` | Name of the database, same as in `Postgres` CR, copied for convenience | -| `HOST` | PostgreSQL server host (including port number) | -| `URI_ARGS` | URI Args, same as in `Postgres` CR, copied for convenience | -| `PASSWORD` | Autogenerated password for user | -| `ROLE` | Autogenerated role with login enabled (user) | -| `LOGIN` | Same as `ROLE`. In case `POSTGRES_CLOUD_PROVIDER` is set to "Azure", `LOGIN` it will be set to `{role}@{serverName}`, serverName is extracted from `POSTGRES_USER` from operator's config. | -| `POSTGRES_URL` | Connection string for Posgres, could be used for Go applications | -| `POSTGRES_JDBC_URL` | JDBC compatible Postgres URI, formatter as `jdbc:postgresql://{POSTGRES_HOST}/{DATABASE_NAME}` | -| `HOSTNAME` | The PostgreSQL server hostname (without port) | -| `PORT` | The PostgreSQL server port | - -| Functions | Meaning | -|----------------|-------------------------------------------------------------------| -| `mergeUriArgs` | Merge any provided uri args with any set in the `Postgres` CR | +| Key | Comment | +| ------------------- | ------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------ | +| `DATABASE_NAME` | Name of the database, same as in `Postgres` CR, copied for convenience | +| `HOST` | PostgreSQL server host (including port number) | +| `URI_ARGS` | URI Args, same as in `Postgres` CR, copied for convenience | +| `PASSWORD` | Autogenerated password for user | +| `ROLE` | Autogenerated role with login enabled (user) | +| `LOGIN` | Same as `ROLE`. In case `POSTGRES_CLOUD_PROVIDER` is set to "Azure", `LOGIN` it will be set to `{role}@{serverName}`, serverName is extracted from `POSTGRES_USER` from operator's config. | +| `POSTGRES_URL` | Connection string for Posgres, could be used for Go applications | +| `POSTGRES_JDBC_URL` | JDBC compatible Postgres URI, formatter as `jdbc:postgresql://{POSTGRES_HOST}/{DATABASE_NAME}` | +| `HOSTNAME` | The PostgreSQL server hostname (without port) | +| `PORT` | The PostgreSQL server port | + +| Functions | Meaning | +| -------------- | ------------------------------------------------------------- | +| `mergeUriArgs` | Merge any provided uri args with any set in the `Postgres` CR | ### Multiple operator support @@ -227,7 +286,7 @@ meeting the specific needs of different applications. Available context: | Variable | Meaning | -|-------------|------------------------------| +| ----------- | ---------------------------- | | `.Host` | Database host | | `.Role` | Generated user/role name | | `.Database` | Referenced database name | @@ -243,12 +302,11 @@ can be found [here](https://github.com/kubernetes/client-go/blob/master/README.m Postgres operator compatibility with Operator SDK version is in the table below | | Operator SDK version | apiextensions.k8s.io | -|---------------------------|----------------------|----------------------| -| `postgres-operator 0.4.x` | v0.17 | v1beta1 | -| `postgres-operator 1.x.x` | v0.18 | v1 | -| `postgres-operator 2.x.x` | v1.39 | v1 | -| `HEAD` | v1.39 | v1 | - +| ------------------------- | -------------------- | -------------------- | +| `postgres-operator 0.4.x` | v0.17 | v1beta1 | +| `postgres-operator 1.x.x` | v0.18 | v1 | +| `postgres-operator 2.x.x` | v1.39 | v1 | +| `HEAD` | v1.39 | v1 | ## Contributing diff --git a/go.mod b/go.mod index 84d3e194c..2cfb7d389 100644 --- a/go.mod +++ b/go.mod @@ -16,6 +16,7 @@ require ( require ( cel.dev/expr v0.24.0 // indirect + github.com/DATA-DOG/go-sqlmock v1.5.2 // indirect github.com/Masterminds/semver/v3 v3.4.0 // indirect github.com/antlr4-go/antlr/v4 v4.13.0 // indirect github.com/beorn7/perks v1.0.1 // indirect diff --git a/go.sum b/go.sum index b1210a71d..ff8603e52 100644 --- a/go.sum +++ b/go.sum @@ -1,5 +1,7 @@ cel.dev/expr v0.24.0 h1:56OvJKSH3hDGL0ml5uSxZmz3/3Pq4tJ+fb1unVLAFcY= cel.dev/expr v0.24.0/go.mod h1:hLPLo1W4QUmuYdA72RBX06QTs6MXw941piREPl3Yfiw= +github.com/DATA-DOG/go-sqlmock v1.5.2 h1:OcvFkGmslmlZibjAjaHm3L//6LiuBgolP7OputlJIzU= +github.com/DATA-DOG/go-sqlmock v1.5.2/go.mod h1:88MAG/4G7SMwSE3CeA0ZKzrT5CiOU3OJ+JlNzwDqpNU= github.com/Masterminds/semver/v3 v3.4.0 h1:Zog+i5UMtVoCU8oKka5P7i9q9HgrJeGzI9SA1Xbatp0= github.com/Masterminds/semver/v3 v3.4.0/go.mod h1:4V+yj/TJE1HU9XfppCwVMZq3I84lprf4nC11bSS5beM= github.com/antlr4-go/antlr/v4 v4.13.0 h1:lxCg3LAv+EUK6t1i0y1V6/SLeUi0eKEKdhQAlS8TVTI= @@ -86,6 +88,7 @@ github.com/json-iterator/go v1.1.12 h1:PV8peI4a0ysnczrg+LtxykD8LfKY9ML6u2jnxaEnr github.com/json-iterator/go v1.1.12/go.mod h1:e30LSqwooZae/UwlEbR2852Gd8hjQvJoHmT4TnhNGBo= github.com/kisielk/errcheck v1.5.0/go.mod h1:pFxgyoBC7bSaBwPgfKdkLd5X25qrDl4LWUI2bnpBCr8= github.com/kisielk/gotool v1.0.0/go.mod h1:XhKaO+MFFWcvkIS/tQcRk01m1F5IRFswLeQ+oQHNcck= +github.com/kisielk/sqlstruct v0.0.0-20201105191214-5f3e10d3ab46/go.mod h1:yyMNCyc/Ib3bDTKd379tNMpB/7/H5TjM2Y9QJ5THLbE= github.com/klauspost/compress v1.18.0 h1:c/Cqfb0r+Yi+JtIEq73FWXVkRonBlf0CRNYc8Zttxdo= github.com/klauspost/compress v1.18.0/go.mod h1:2Pp+KzxcywXVXMr50+X0Q/Lsb43OQHYWRCY2AiWywWQ= github.com/kr/pretty v0.2.1/go.mod h1:ipq/a2n7PKx3OHsz4KJII5eveXtPO4qwEXGdVfWzfnI= diff --git a/pkg/postgres/aws.go b/pkg/postgres/aws.go deleted file mode 100644 index 61e732357..000000000 --- a/pkg/postgres/aws.go +++ /dev/null @@ -1,80 +0,0 @@ -package postgres - -import ( - "fmt" - - "github.com/go-logr/logr" - "github.com/lib/pq" -) - -type awspg struct { - pg -} - -func newAWSPG(postgres *pg) PG { - return &awspg{ - *postgres, - } -} - -func (c *awspg) AlterDefaultLoginRole(role, setRole string) error { - // On AWS RDS the postgres user isn't really superuser so he doesn't have permissions - // to ALTER USER unless he belongs to both roles - err := c.GrantRole(role, c.user) - if err != nil { - return err - } - defer c.RevokeRole(role, c.user) - - return c.pg.AlterDefaultLoginRole(role, setRole) -} - -func (c *awspg) CreateDB(dbname, role string) error { - // Have to add the master role to the group role before we can transfer the database owner - err := c.GrantRole(role, c.user) - if err != nil { - return err - } - - return c.pg.CreateDB(dbname, role) -} - -func (c *awspg) CreateUserRole(role, password string) (string, error) { - returnedRole, err := c.pg.CreateUserRole(role, password) - if err != nil { - return "", err - } - // On AWS RDS the postgres user isn't really superuser so he doesn't have permissions - // to ALTER DEFAULT PRIVILEGES FOR ROLE unless he belongs to the role - err = c.GrantRole(role, c.user) - if err != nil { - return "", err - } - - return returnedRole, nil -} - -func (c *awspg) DropRole(role, newOwner, database string, logger logr.Logger) error { - // On AWS RDS the postgres user isn't really superuser so he doesn't have permissions - // to REASSIGN OWNED BY unless he belongs to both roles - err := c.GrantRole(role, c.user) - if err != nil && err.(*pq.Error).Code != "0LP01" { - if err.(*pq.Error).Code == "42704" { - // The group role does not exist, no point in continuing - return nil - } - return err - } - err = c.GrantRole(newOwner, c.user) - if err != nil && err.(*pq.Error).Code != "0LP01" { - if err.(*pq.Error).Code == "42704" { - // The group role does not exist, no point of granting roles - logger.Info(fmt.Sprintf("not granting %s to %s as %s does not exist", role, newOwner, newOwner)) - return nil - } - return err - } - defer c.RevokeRole(newOwner, c.user) - - return c.pg.DropRole(role, newOwner, database, logger) -} diff --git a/pkg/postgres/azure.go b/pkg/postgres/azure.go deleted file mode 100644 index 99628bcb7..000000000 --- a/pkg/postgres/azure.go +++ /dev/null @@ -1,50 +0,0 @@ -package postgres - -import ( - "github.com/go-logr/logr" - "github.com/lib/pq" -) - -type AzureType string - -type azurepg struct { - pg -} - -func newAzurePG(postgres *pg) PG { - return &azurepg{ - pg: *postgres, - } -} - -func (azpg *azurepg) CreateUserRole(role, password string) (string, error) { - returnedRole, err := azpg.pg.CreateUserRole(role, password) - if err != nil { - return "", err - } - return returnedRole, nil -} - -func (azpg *azurepg) CreateDB(dbname, role string) error { - // This step is necessary before we can set the specified role as the database owner - err := azpg.GrantRole(role, azpg.user) - if err != nil { - return err - } - - return azpg.pg.CreateDB(dbname, role) -} - -func (azpg *azurepg) DropRole(role, newOwner, database string, logger logr.Logger) error { - // Grant the role to the user first - err := azpg.GrantRole(role, azpg.user) - if err != nil && err.(*pq.Error).Code != "0LP01" { - if err.(*pq.Error).Code == "42704" { - return nil - } - return err - } - - // Delegate to parent implementation to perform the actual drop - return azpg.pg.DropRole(role, newOwner, database, logger) -} diff --git a/pkg/postgres/database.go b/pkg/postgres/database.go index 7608f8da3..6b5652b63 100644 --- a/pkg/postgres/database.go +++ b/pkg/postgres/database.go @@ -4,7 +4,6 @@ import ( "fmt" "github.com/go-logr/logr" - "github.com/lib/pq" ) const ( @@ -13,7 +12,7 @@ const ( CREATE_EXTENSION = `CREATE EXTENSION IF NOT EXISTS "%s"` ALTER_DB_OWNER = `ALTER DATABASE "%s" OWNER TO "%s"` REASSIGN_DB_OWNER = `REASSIGN OWNED BY "%s" TO "%s"` - DROP_DATABASE = `DROP DATABASE "%s"` + DROP_DATABASE = `DROP DATABASE "%s" WITH (FORCE)` GRANT_USAGE_SCHEMA = `GRANT USAGE ON SCHEMA "%s" TO "%s"` GRANT_CREATE_TABLE = `GRANT CREATE ON SCHEMA "%s" TO "%s"` GRANT_ALL_TABLES = `GRANT %s ON ALL TABLES IN SCHEMA "%s" TO "%s"` @@ -23,28 +22,40 @@ const ( GRANT_ALL_SEQUENCES = `GRANT %s ON ALL SEQUENCES IN SCHEMA "%s" TO "%s"` DEFAULT_PRIVS_SEQUENCES = `ALTER DEFAULT PRIVILEGES IN SCHEMA "%s" GRANT %s ON SEQUENCES TO "%s"` REVOKE_CONNECT = `REVOKE CONNECT ON DATABASE "%s" FROM public` - TERMINATE_BACKEND = `SELECT pg_terminate_backend(pg_stat_activity.pid) FROM pg_stat_activity WHERE pg_stat_activity.datname = '%s' AND pid <> pg_backend_pid()` GET_DB_OWNER = `SELECT pg_catalog.pg_get_userbyid(d.datdba) FROM pg_catalog.pg_database d WHERE d.datname = '%s'` GRANT_CREATE_SCHEMA = `GRANT CREATE ON DATABASE "%s" TO "%s"` + GRANT_CONNECT = `GRANT CONNECT ON DATABASE "%s" TO "%s"` ) func (c *pg) CreateDB(dbname, role string) error { - _, err := c.db.Exec(fmt.Sprintf(CREATE_DB, dbname)) + // Create database + err := c.execute(fmt.Sprintf(CREATE_DB, dbname)) if err != nil { // eat DUPLICATE DATABASE ERROR - if err.(*pq.Error).Code != "42P04" { + if !isPgError(err, "42P04") { return err } } - _, err = c.db.Exec(fmt.Sprintf(ALTER_DB_OWNER, dbname, role)) + err = c.execute(fmt.Sprintf(ALTER_DB_OWNER, dbname, role)) if err != nil { return err } - _, err = c.db.Exec(fmt.Sprintf(GRANT_CREATE_SCHEMA, dbname, role)) - if err != nil { - return err + // Grant CREATE on database to owner and operator user + usersToGrant := []string{c.user, role} + for _, u := range usersToGrant { + err = c.execute(fmt.Sprintf(GRANT_CREATE_SCHEMA, dbname, u)) + if err != nil { + return fmt.Errorf("failed to grant create schema on %s to %s: %w", dbname, u, err) + } + } + // Grant CONNECT on database to owner and operator user + for _, u := range usersToGrant { + err = c.execute(fmt.Sprintf(GRANT_CONNECT, dbname, u)) + if err != nil { + return fmt.Errorf("failed to grant connect on %s to %s: %w", dbname, u, err) + } } return nil } @@ -54,8 +65,7 @@ func (c *pg) AlterDatabaseOwner(dbname, owner string) error { if owner == "" { return nil } - _, err := c.db.Exec(fmt.Sprintf(ALTER_DB_OWNER, dbname, owner)) - return err + return c.execute(fmt.Sprintf(ALTER_DB_OWNER, dbname, owner)) } func (c *pg) ReassignDatabaseOwner(dbName, currentOwner, newOwner string, logger logr.Logger) error { @@ -63,7 +73,7 @@ func (c *pg) ReassignDatabaseOwner(dbName, currentOwner, newOwner string, logger return nil } - tmpDb, err := GetConnection(c.user, c.pass, c.host, dbName, c.args, logger) + tmpDb, err := getConnection(c.user, c.pass, c.host, dbName, c.args, logger) if err != nil { return err } @@ -71,7 +81,7 @@ func (c *pg) ReassignDatabaseOwner(dbName, currentOwner, newOwner string, logger _, err = tmpDb.Exec(fmt.Sprintf(REASSIGN_DB_OWNER, currentOwner, newOwner)) if err != nil { - if pqErr, ok := err.(*pq.Error); ok && pqErr.Code == "42704" { + if isPgError(err, "42704") { return nil } return err @@ -80,7 +90,7 @@ func (c *pg) ReassignDatabaseOwner(dbName, currentOwner, newOwner string, logger } func (c *pg) CreateSchema(db, role, schema string, logger logr.Logger) error { - tmpDb, err := GetConnection(c.user, c.pass, c.host, db, c.args, logger) + tmpDb, err := getConnection(c.user, c.pass, c.host, db, c.args, logger) if err != nil { return err } @@ -94,20 +104,15 @@ func (c *pg) CreateSchema(db, role, schema string, logger logr.Logger) error { } func (c *pg) DropDatabase(database string, logger logr.Logger) error { - _, err := c.db.Exec(fmt.Sprintf(REVOKE_CONNECT, database)) + err := c.execute(fmt.Sprintf(REVOKE_CONNECT, database)) // Error code 3D000 is returned if database doesn't exist - if err != nil && err.(*pq.Error).Code != "3D000" { + if err != nil && !isPgError(err, "3D000") { return err } - _, err = c.db.Exec(fmt.Sprintf(TERMINATE_BACKEND, database)) - // Error code 3D000 is returned if database doesn't exist - if err != nil && err.(*pq.Error).Code != "3D000" { - return err - } - _, err = c.db.Exec(fmt.Sprintf(DROP_DATABASE, database)) + err = c.execute(fmt.Sprintf(DROP_DATABASE, database)) // Error code 3D000 is returned if database doesn't exist - if err != nil && err.(*pq.Error).Code != "3D000" { + if err != nil && !isPgError(err, "3D000") { return err } @@ -117,7 +122,7 @@ func (c *pg) DropDatabase(database string, logger logr.Logger) error { } func (c *pg) CreateExtension(db, extension string, logger logr.Logger) error { - tmpDb, err := GetConnection(c.user, c.pass, c.host, db, c.args, logger) + tmpDb, err := getConnection(c.user, c.pass, c.host, db, c.args, logger) if err != nil { return err } @@ -131,7 +136,7 @@ func (c *pg) CreateExtension(db, extension string, logger logr.Logger) error { } func (c *pg) SetSchemaPrivileges(schemaPrivileges PostgresSchemaPrivileges, logger logr.Logger) error { - tmpDb, err := GetConnection(c.user, c.pass, c.host, schemaPrivileges.DB, c.args, logger) + tmpDb, err := getConnection(c.user, c.pass, c.host, schemaPrivileges.DB, c.args, logger) if err != nil { return err } diff --git a/pkg/postgres/database_test.go b/pkg/postgres/database_test.go new file mode 100644 index 000000000..fbd17adec --- /dev/null +++ b/pkg/postgres/database_test.go @@ -0,0 +1,320 @@ +package postgres + +import ( + "database/sql" + "regexp" + "testing" + + "github.com/DATA-DOG/go-sqlmock" + "github.com/go-logr/logr" +) + +func TestCreateDB(t *testing.T) { + db, mock, err := sqlmock.New() + if err != nil { + t.Fatalf("sqlmock.New: %v", err) + } + defer db.Close() + + p := &pg{db: db, user: "operator"} + + mock.ExpectExec(regexp.QuoteMeta(`CREATE DATABASE "mydb"`)).WillReturnResult(sqlmock.NewResult(0, 1)) + mock.ExpectExec(regexp.QuoteMeta(`ALTER DATABASE "mydb" OWNER TO "owner"`)).WillReturnResult(sqlmock.NewResult(0, 1)) + mock.ExpectExec(regexp.QuoteMeta(`GRANT CREATE ON DATABASE "mydb" TO "operator"`)).WillReturnResult(sqlmock.NewResult(0, 1)) + mock.ExpectExec(regexp.QuoteMeta(`GRANT CREATE ON DATABASE "mydb" TO "owner"`)).WillReturnResult(sqlmock.NewResult(0, 1)) + mock.ExpectExec(regexp.QuoteMeta(`GRANT CONNECT ON DATABASE "mydb" TO "operator"`)).WillReturnResult(sqlmock.NewResult(0, 1)) + mock.ExpectExec(regexp.QuoteMeta(`GRANT CONNECT ON DATABASE "mydb" TO "owner"`)).WillReturnResult(sqlmock.NewResult(0, 1)) + + if err := p.CreateDB("mydb", "owner"); err != nil { + t.Fatalf("CreateDB: %v", err) + } + + if err := mock.ExpectationsWereMet(); err != nil { + t.Fatalf("expectations: %v", err) + } +} + +func TestRenameGroupRole(t *testing.T) { + db, mock, err := sqlmock.New() + if err != nil { + t.Fatalf("sqlmock.New: %v", err) + } + defer db.Close() + + p := &pg{db: db} + + mock.ExpectExec(regexp.QuoteMeta(`ALTER ROLE "old" RENAME TO "new"`)).WillReturnResult(sqlmock.NewResult(0, 1)) + + if err := p.RenameGroupRole("old", "new"); err != nil { + t.Fatalf("RenameGroupRole: %v", err) + } + if err := mock.ExpectationsWereMet(); err != nil { + t.Fatalf("expectations: %v", err) + } +} + +func TestUpdatePassword(t *testing.T) { + db, mock, err := sqlmock.New() + if err != nil { + t.Fatalf("sqlmock.New: %v", err) + } + defer db.Close() + + p := &pg{db: db} + + mock.ExpectExec(regexp.QuoteMeta(`ALTER ROLE "user" WITH PASSWORD 'newpass'`)).WillReturnResult(sqlmock.NewResult(0, 1)) + + if err := p.UpdatePassword("user", "newpass"); err != nil { + t.Fatalf("UpdatePassword: %v", err) + } + if err := mock.ExpectationsWereMet(); err != nil { + t.Fatalf("expectations: %v", err) + } +} + +func TestGrantRole(t *testing.T) { + db, mock, err := sqlmock.New() + if err != nil { + t.Fatalf("sqlmock.New: %v", err) + } + defer db.Close() + + p := &pg{db: db} + + mock.ExpectExec(regexp.QuoteMeta(`GRANT "role" TO "grantee"`)).WillReturnResult(sqlmock.NewResult(0, 1)) + + if err := p.GrantRole("role", "grantee"); err != nil { + t.Fatalf("GrantRole: %v", err) + } + if err := mock.ExpectationsWereMet(); err != nil { + t.Fatalf("expectations: %v", err) + } +} + +func TestRevokeRole(t *testing.T) { + db, mock, err := sqlmock.New() + if err != nil { + t.Fatalf("sqlmock.New: %v", err) + } + defer db.Close() + + p := &pg{db: db} + + mock.ExpectExec(regexp.QuoteMeta(`REVOKE "role" FROM "revoked"`)).WillReturnResult(sqlmock.NewResult(0, 1)) + + if err := p.RevokeRole("role", "revoked"); err != nil { + t.Fatalf("RevokeRole: %v", err) + } + if err := mock.ExpectationsWereMet(); err != nil { + t.Fatalf("expectations: %v", err) + } +} + +func TestAlterDefaultLoginRole(t *testing.T) { + db, mock, err := sqlmock.New() + if err != nil { + t.Fatalf("sqlmock.New: %v", err) + } + defer db.Close() + + p := &pg{db: db} + + mock.ExpectExec(regexp.QuoteMeta(`ALTER USER "user" SET ROLE "group"`)).WillReturnResult(sqlmock.NewResult(0, 1)) + + if err := p.AlterDefaultLoginRole("user", "group"); err != nil { + t.Fatalf("AlterDefaultLoginRole: %v", err) + } + if err := mock.ExpectationsWereMet(); err != nil { + t.Fatalf("expectations: %v", err) + } +} + +func TestAlterDatabaseOwner(t *testing.T) { + db, mock, err := sqlmock.New() + if err != nil { + t.Fatalf("sqlmock.New: %v", err) + } + defer db.Close() + + p := &pg{db: db} + + mock.ExpectExec(regexp.QuoteMeta(`ALTER DATABASE "mydb" OWNER TO "newowner"`)).WillReturnResult(sqlmock.NewResult(0, 1)) + + if err := p.AlterDatabaseOwner("mydb", "newowner"); err != nil { + t.Fatalf("AlterDatabaseOwner: %v", err) + } + if err := mock.ExpectationsWereMet(); err != nil { + t.Fatalf("expectations: %v", err) + } +} + +func TestAlterDatabaseOwner_EmptyOwnerNoop(t *testing.T) { + p := &pg{} + if err := p.AlterDatabaseOwner("mydb", ""); err != nil { + t.Fatalf("AlterDatabaseOwner: %v", err) + } +} + +func TestDropDatabase(t *testing.T) { + db, mock, err := sqlmock.New() + if err != nil { + t.Fatalf("sqlmock.New: %v", err) + } + defer db.Close() + + p := &pg{db: db} + + mock.ExpectExec(regexp.QuoteMeta(`REVOKE CONNECT ON DATABASE "mydb" FROM public`)).WillReturnResult(sqlmock.NewResult(0, 1)) + mock.ExpectExec(regexp.QuoteMeta(`DROP DATABASE "mydb" WITH (FORCE)`)).WillReturnResult(sqlmock.NewResult(0, 1)) + + if err := p.DropDatabase("mydb", logr.Discard()); err != nil { + t.Fatalf("DropDatabase: %v", err) + } + if err := mock.ExpectationsWereMet(); err != nil { + t.Fatalf("expectations: %v", err) + } +} + +func TestReassignDatabaseOwner(t *testing.T) { + rootDB, _, err := sqlmock.New() + if err != nil { + t.Fatalf("sqlmock.New root: %v", err) + } + defer rootDB.Close() + + perDB, perMock, err := sqlmock.New() + if err != nil { + t.Fatalf("sqlmock.New per-db: %v", err) + } + defer perDB.Close() + + original := getConnection + getConnection = func(user, password, host, database, uriArgs string, logger logr.Logger) (*sql.DB, error) { + return perDB, nil + } + t.Cleanup(func() { getConnection = original }) + + p := &pg{db: rootDB, user: "operator", pass: "pass", host: "host", args: ""} + + perMock.ExpectExec(regexp.QuoteMeta(`REASSIGN OWNED BY "old" TO "new"`)).WillReturnResult(sqlmock.NewResult(0, 1)) + + if err := p.ReassignDatabaseOwner("mydb", "old", "new", logr.Discard()); err != nil { + t.Fatalf("ReassignDatabaseOwner: %v", err) + } + if err := perMock.ExpectationsWereMet(); err != nil { + t.Fatalf("expectations: %v", err) + } +} + +func TestReassignDatabaseOwner_NoOpWhenSame(t *testing.T) { + p := &pg{} + if err := p.ReassignDatabaseOwner("mydb", "owner", "owner", logr.Discard()); err != nil { + t.Fatalf("ReassignDatabaseOwner: %v", err) + } +} + +func TestCreateSchema(t *testing.T) { + rootDB, _, err := sqlmock.New() + if err != nil { + t.Fatalf("sqlmock.New root: %v", err) + } + defer rootDB.Close() + + perDB, perMock, err := sqlmock.New() + if err != nil { + t.Fatalf("sqlmock.New per-db: %v", err) + } + defer perDB.Close() + + original := getConnection + getConnection = func(user, password, host, database, uriArgs string, logger logr.Logger) (*sql.DB, error) { + return perDB, nil + } + t.Cleanup(func() { getConnection = original }) + + p := &pg{db: rootDB, user: "operator", pass: "pass", host: "host", args: ""} + + perMock.ExpectExec(regexp.QuoteMeta(`CREATE SCHEMA IF NOT EXISTS "myschema" AUTHORIZATION "owner"`)).WillReturnResult(sqlmock.NewResult(0, 1)) + + if err := p.CreateSchema("mydb", "owner", "myschema", logr.Discard()); err != nil { + t.Fatalf("CreateSchema: %v", err) + } + if err := perMock.ExpectationsWereMet(); err != nil { + t.Fatalf("expectations: %v", err) + } +} + +func TestCreateExtension(t *testing.T) { + rootDB, _, err := sqlmock.New() + if err != nil { + t.Fatalf("sqlmock.New root: %v", err) + } + defer rootDB.Close() + + perDB, perMock, err := sqlmock.New() + if err != nil { + t.Fatalf("sqlmock.New per-db: %v", err) + } + defer perDB.Close() + + original := getConnection + getConnection = func(user, password, host, database, uriArgs string, logger logr.Logger) (*sql.DB, error) { + return perDB, nil + } + t.Cleanup(func() { getConnection = original }) + + p := &pg{db: rootDB, user: "operator", pass: "pass", host: "host", args: ""} + + perMock.ExpectExec(regexp.QuoteMeta(`CREATE EXTENSION IF NOT EXISTS "pgcrypto"`)).WillReturnResult(sqlmock.NewResult(0, 1)) + + if err := p.CreateExtension("mydb", "pgcrypto", logr.Discard()); err != nil { + t.Fatalf("CreateExtension: %v", err) + } + if err := perMock.ExpectationsWereMet(); err != nil { + t.Fatalf("expectations: %v", err) + } +} + +func TestSetSchemaPrivileges(t *testing.T) { + rootDB, _, err := sqlmock.New() + if err != nil { + t.Fatalf("sqlmock.New root: %v", err) + } + defer rootDB.Close() + + perDB, perMock, err := sqlmock.New() + if err != nil { + t.Fatalf("sqlmock.New per-db: %v", err) + } + defer perDB.Close() + + original := getConnection + getConnection = func(user, password, host, database, uriArgs string, logger logr.Logger) (*sql.DB, error) { + return perDB, nil + } + t.Cleanup(func() { getConnection = original }) + + p := &pg{db: rootDB, user: "operator", pass: "pass", host: "host", args: ""} + + privs := PostgresSchemaPrivileges{ + DB: "mydb", + Role: "app", + Schema: "public", + Privs: "SELECT", + SequencePrivs: "", + FunctionPrivs: "", + CreateSchema: true, + } + + perMock.ExpectExec(regexp.QuoteMeta(`GRANT USAGE ON SCHEMA "public" TO "app"`)).WillReturnResult(sqlmock.NewResult(0, 1)) + perMock.ExpectExec(regexp.QuoteMeta(`GRANT SELECT ON ALL TABLES IN SCHEMA "public" TO "app"`)).WillReturnResult(sqlmock.NewResult(0, 1)) + perMock.ExpectExec(regexp.QuoteMeta(`ALTER DEFAULT PRIVILEGES IN SCHEMA "public" GRANT SELECT ON TABLES TO "app"`)).WillReturnResult(sqlmock.NewResult(0, 1)) + perMock.ExpectExec(regexp.QuoteMeta(`GRANT CREATE ON SCHEMA "public" TO "app"`)).WillReturnResult(sqlmock.NewResult(0, 1)) + + if err := p.SetSchemaPrivileges(privs, logr.Discard()); err != nil { + t.Fatalf("SetSchemaPrivileges: %v", err) + } + if err := perMock.ExpectationsWereMet(); err != nil { + t.Fatalf("expectations: %v", err) + } +} diff --git a/pkg/postgres/gcp.go b/pkg/postgres/gcp.go deleted file mode 100644 index 94e4dd778..000000000 --- a/pkg/postgres/gcp.go +++ /dev/null @@ -1,63 +0,0 @@ -package postgres - -import ( - "fmt" - - "github.com/go-logr/logr" - "github.com/lib/pq" -) - -type gcppg struct { - pg -} - -func newGCPPG(postgres *pg) PG { - return &gcppg{ - *postgres, - } -} - -func (c *gcppg) CreateDB(dbname, role string) error { - - err := c.GrantRole(role, c.user) - if err != nil { - return err - } - err = c.pg.CreateDB(dbname, role) - if err != nil { - return err - } - return nil -} - -func (c *gcppg) DropRole(role, newOwner, database string, logger logr.Logger) error { - if role == "alloydbsuperuser" || role == "postgres" { - logger.Info(fmt.Sprintf("not dropping %s as it is a reserved AlloyDB role", role)) - return nil - } - // On AlloyDB the postgres or other alloydbsuperuser members, aren't really superusers so they don't have permissions - // to REASSIGN OWNED BY unless he belongs to both roles - err := c.GrantRole(role, c.user) - if err != nil && err.(*pq.Error).Code != "0LP01" { - if err.(*pq.Error).Code == "42704" { - // The group role does not exist, no point in continuing - return nil - } - return err - } - defer c.RevokeRole(role, c.user) - if newOwner != c.user { - err = c.GrantRole(newOwner, c.user) - if err != nil && err.(*pq.Error).Code != "0LP01" { - if err.(*pq.Error).Code == "42704" { - // The group role does not exist, no point of granting roles - logger.Info(fmt.Sprintf("not granting %s to %s as %s does not exist", role, newOwner, newOwner)) - return nil - } - return err - } - defer c.RevokeRole(newOwner, c.user) - } - - return c.pg.DropRole(role, newOwner, database, logger) -} diff --git a/pkg/postgres/mock/postgres.go b/pkg/postgres/mock/postgres.go index 3c920cd09..b766bcc71 100644 --- a/pkg/postgres/mock/postgres.go +++ b/pkg/postgres/mock/postgres.go @@ -41,6 +41,20 @@ func (m *MockPG) EXPECT() *MockPGMockRecorder { return m.recorder } +// AlterDatabaseOwner mocks base method. +func (m *MockPG) AlterDatabaseOwner(dbName, owner string) error { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "AlterDatabaseOwner", dbName, owner) + ret0, _ := ret[0].(error) + return ret0 +} + +// AlterDatabaseOwner indicates an expected call of AlterDatabaseOwner. +func (mr *MockPGMockRecorder) AlterDatabaseOwner(dbName, owner any) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "AlterDatabaseOwner", reflect.TypeOf((*MockPG)(nil).AlterDatabaseOwner), dbName, owner) +} + // AlterDefaultLoginRole mocks base method. func (m *MockPG) AlterDefaultLoginRole(role, setRole string) error { m.ctrl.T.Helper() @@ -97,20 +111,6 @@ func (mr *MockPGMockRecorder) CreateGroupRole(role any) *gomock.Call { return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "CreateGroupRole", reflect.TypeOf((*MockPG)(nil).CreateGroupRole), role) } -// RenameGroupRole mocks base method. -func (m *MockPG) RenameGroupRole(currentRole, newRole string) error { - m.ctrl.T.Helper() - ret := m.ctrl.Call(m, "RenameGroupRole", currentRole, newRole) - ret0, _ := ret[0].(error) - return ret0 -} - -// RenameGroupRole indicates an expected call of RenameGroupRole. -func (mr *MockPGMockRecorder) RenameGroupRole(currentRole, newRole any) *gomock.Call { - mr.mock.ctrl.T.Helper() - return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "RenameGroupRole", reflect.TypeOf((*MockPG)(nil).RenameGroupRole), currentRole, newRole) -} - // CreateSchema mocks base method. func (m *MockPG) CreateSchema(db, role, schema string, logger logr.Logger) error { m.ctrl.T.Helper() @@ -210,32 +210,32 @@ func (mr *MockPGMockRecorder) GrantRole(role, grantee any) *gomock.Call { return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "GrantRole", reflect.TypeOf((*MockPG)(nil).GrantRole), role, grantee) } -// AlterDatabaseOwner mocks base method. -func (m *MockPG) AlterDatabaseOwner(dbName, owner string) error { +// ReassignDatabaseOwner mocks base method. +func (m *MockPG) ReassignDatabaseOwner(dbName, currentOwner, newOwner string, logger logr.Logger) error { m.ctrl.T.Helper() - ret := m.ctrl.Call(m, "AlterDatabaseOwner", dbName, owner) + ret := m.ctrl.Call(m, "ReassignDatabaseOwner", dbName, currentOwner, newOwner, logger) ret0, _ := ret[0].(error) return ret0 } -// AlterDatabaseOwner indicates an expected call of AlterDatabaseOwner. -func (mr *MockPGMockRecorder) AlterDatabaseOwner(dbName, owner any) *gomock.Call { +// ReassignDatabaseOwner indicates an expected call of ReassignDatabaseOwner. +func (mr *MockPGMockRecorder) ReassignDatabaseOwner(dbName, currentOwner, newOwner, logger any) *gomock.Call { mr.mock.ctrl.T.Helper() - return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "AlterDatabaseOwner", reflect.TypeOf((*MockPG)(nil).AlterDatabaseOwner), dbName, owner) + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "ReassignDatabaseOwner", reflect.TypeOf((*MockPG)(nil).ReassignDatabaseOwner), dbName, currentOwner, newOwner, logger) } -// ReassignDatabaseOwner mocks base method. -func (m *MockPG) ReassignDatabaseOwner(dbName, currentOwner, newOwner string, logger logr.Logger) error { +// RenameGroupRole mocks base method. +func (m *MockPG) RenameGroupRole(currentRole, newRole string) error { m.ctrl.T.Helper() - ret := m.ctrl.Call(m, "ReassignDatabaseOwner", dbName, currentOwner, newOwner, logger) + ret := m.ctrl.Call(m, "RenameGroupRole", currentRole, newRole) ret0, _ := ret[0].(error) return ret0 } -// ReassignDatabaseOwner indicates an expected call of ReassignDatabaseOwner. -func (mr *MockPGMockRecorder) ReassignDatabaseOwner(dbName, currentOwner, newOwner, logger any) *gomock.Call { +// RenameGroupRole indicates an expected call of RenameGroupRole. +func (mr *MockPGMockRecorder) RenameGroupRole(currentRole, newRole any) *gomock.Call { mr.mock.ctrl.T.Helper() - return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "ReassignDatabaseOwner", reflect.TypeOf((*MockPG)(nil).ReassignDatabaseOwner), dbName, currentOwner, newOwner, logger) + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "RenameGroupRole", reflect.TypeOf((*MockPG)(nil).RenameGroupRole), currentRole, newRole) } // RevokeRole mocks base method. diff --git a/pkg/postgres/postgres.go b/pkg/postgres/postgres.go index c4f12e527..813cd6f7b 100644 --- a/pkg/postgres/postgres.go +++ b/pkg/postgres/postgres.go @@ -2,10 +2,12 @@ package postgres import ( "database/sql" + "errors" "fmt" "log" "github.com/go-logr/logr" + "github.com/lib/pq" "github.com/movetokube/postgres-operator/pkg/config" ) @@ -49,8 +51,14 @@ type PostgresSchemaPrivileges struct { CreateSchema bool } +var ( + getConnection = GetConnection + openSQL = sql.Open + pingDB = func(db *sql.DB) error { return db.Ping() } +) + func NewPG(cfg *config.Cfg, logger logr.Logger) (PG, error) { - db, err := GetConnection( + db, err := getConnection( cfg.PostgresUser, cfg.PostgresPass, cfg.PostgresHost, @@ -71,20 +79,7 @@ func NewPG(cfg *config.Cfg, logger logr.Logger) (PG, error) { default_database: cfg.PostgresDefaultDb, } - switch cfg.CloudProvider { - case "AWS": - logger.Info("Using AWS wrapper") - return newAWSPG(postgres), nil - case "Azure": - logger.Info("Using Azure wrapper") - return newAzurePG(postgres), nil - case "GCP": - logger.Info("Using GCP wrapper") - return newGCPPG(postgres), nil - default: - logger.Info("Using default postgres implementation") - return postgres, nil - } + return postgres, nil } func (c *pg) GetUser() string { @@ -96,10 +91,22 @@ func (c *pg) GetDefaultDatabase() string { } func GetConnection(user, password, host, database, uri_args string, logger logr.Logger) (*sql.DB, error) { - db, err := sql.Open("postgres", fmt.Sprintf("postgresql://%s:%s@%s/%s?%s", user, password, host, database, uri_args)) + db, err := openSQL("postgres", fmt.Sprintf("postgresql://%s:%s@%s/%s?%s", user, password, host, database, uri_args)) if err != nil { - log.Fatal(err) + return nil, err + } + return db, pingDB(db) +} + +func (c *pg) execute(query string) error { + _, err := c.db.Exec(query) + return err +} + +func isPgError(err error, code string) bool { + var pqErr *pq.Error + if errors.As(err, &pqErr) { + return string(pqErr.Code) == code } - err = db.Ping() - return db, err + return false } diff --git a/pkg/postgres/role.go b/pkg/postgres/role.go index 0b4d4b240..7e92cf1d0 100644 --- a/pkg/postgres/role.go +++ b/pkg/postgres/role.go @@ -4,7 +4,6 @@ import ( "fmt" "github.com/go-logr/logr" - "github.com/lib/pq" ) const ( @@ -20,23 +19,34 @@ const ( REASIGN_OBJECTS = `REASSIGN OWNED BY "%s" TO "%s"` ) +var reservedRoles = map[string]struct{}{ + "alloydbsuperuser": {}, // GCP AlloyDB + "cloudsqlsuperuser": {}, // GCP Cloud SQL + "rdsadmin": {}, // AWS RDS + "azuresu": {}, // Azure Database for PostgreSQL +} + func (c *pg) CreateGroupRole(role string) error { // Error code 42710 is duplicate_object (role already exists) - _, err := c.db.Exec(fmt.Sprintf(CREATE_GROUP_ROLE, role)) - if err != nil && err.(*pq.Error).Code != "42710" { + err := c.execute(fmt.Sprintf(CREATE_GROUP_ROLE, role)) + if err != nil && !isPgError(err, "42710") { return err } + // Grant role also to the operator role to be able to manage it + err = c.GrantRole(role, c.user) + if err != nil && !isPgError(err, "0LP01") { + return err + } + return nil } func (c *pg) RenameGroupRole(currentRole, newRole string) error { - _, err := c.db.Exec(fmt.Sprintf(RENAME_GROUP_ROLE, currentRole, newRole)) + err := c.execute(fmt.Sprintf(RENAME_GROUP_ROLE, currentRole, newRole)) if err != nil { - if pqErr, ok := err.(*pq.Error); ok { - // 42704 => role does not exist; treat as success so caller can recreate - if pqErr.Code == "42704" { - return nil - } + // 42704 => role does not exist; treat as success so caller can recreate + if isPgError(err, "42704") { + return nil } return err } @@ -44,42 +54,70 @@ func (c *pg) RenameGroupRole(currentRole, newRole string) error { } func (c *pg) CreateUserRole(role, password string) (string, error) { - _, err := c.db.Exec(fmt.Sprintf(CREATE_USER_ROLE, role, password)) + err := c.execute(fmt.Sprintf(CREATE_USER_ROLE, role, password)) if err != nil { return "", err } + + err = c.GrantRole(role, c.user) + if err != nil && !isPgError(err, "0LP01") { + return "", err + } return role, nil } func (c *pg) GrantRole(role, grantee string) error { - _, err := c.db.Exec(fmt.Sprintf(GRANT_ROLE, role, grantee)) - if err != nil { - return err - } - return nil + return c.execute(fmt.Sprintf(GRANT_ROLE, role, grantee)) } func (c *pg) AlterDefaultLoginRole(role, setRole string) error { - _, err := c.db.Exec(fmt.Sprintf(ALTER_USER_SET_ROLE, role, setRole)) - if err != nil { - return err - } - return nil + return c.execute(fmt.Sprintf(ALTER_USER_SET_ROLE, role, setRole)) } func (c *pg) RevokeRole(role, revoked string) error { - _, err := c.db.Exec(fmt.Sprintf(REVOKE_ROLE, role, revoked)) - if err != nil { - return err - } - return nil + return c.execute(fmt.Sprintf(REVOKE_ROLE, role, revoked)) } func (c *pg) DropRole(role, newOwner, database string, logger logr.Logger) error { + if _, reserved := reservedRoles[role]; reserved || role == c.user { + logger.Info(fmt.Sprintf("not dropping %s as it is a reserved role", role)) + return nil + } + + err := c.GrantRole(role, c.user) + if err != nil && !isPgError(err, "0LP01") { + if isPgError(err, "42704") { + // The group role does not exist, no point in continuing + return nil + } + return err + } + defer func() { + if err := c.RevokeRole(role, c.user); err != nil && !isPgError(err, "0LP01") { + logger.Error(err, "failed to revoke role from operator", "role", role) + } + }() + if newOwner != c.user { + err = c.GrantRole(newOwner, c.user) + if err != nil && !isPgError(err, "0LP01") { + if isPgError(err, "42704") { + // The group role does not exist, no point of granting roles + logger.Info(fmt.Sprintf("not granting %s to %s as %s does not exist", role, newOwner, newOwner)) + return nil + } + return err + } + defer func() { + if err := c.RevokeRole(newOwner, c.user); err != nil && !isPgError(err, "0LP01") { + logger.Error(err, "failed to revoke newOwner role from operator", "role", newOwner) + } + }() + } + // REASSIGN OWNED BY only works if the correct database is selected - tmpDb, err := GetConnection(c.user, c.pass, c.host, database, c.args, logger) + tmpDb, err := getConnection(c.user, c.pass, c.host, database, c.args, logger) if err != nil { - if err.(*pq.Error).Code == "3D000" { + if isPgError(err, "3D000") { return nil // Database is does not exist (anymore) } else { return err @@ -88,30 +126,25 @@ func (c *pg) DropRole(role, newOwner, database string, logger logr.Logger) error _, err = tmpDb.Exec(fmt.Sprintf(REASIGN_OBJECTS, role, newOwner)) defer tmpDb.Close() // Check if error exists and if different from "ROLE NOT FOUND" => 42704 - if err != nil && err.(*pq.Error).Code != "42704" { + if err != nil && !isPgError(err, "42704") { return err } // We previously assigned all objects to the operator's role so DROP OWNED BY will drop privileges of role _, err = tmpDb.Exec(fmt.Sprintf(DROP_OWNED_BY, role)) // Check if error exists and if different from "ROLE NOT FOUND" => 42704 - if err != nil && err.(*pq.Error).Code != "42704" { + if err != nil && !isPgError(err, "42704") { return err } - _, err = c.db.Exec(fmt.Sprintf(DROP_ROLE, role)) + err = c.execute(fmt.Sprintf(DROP_ROLE, role)) // Check if error exists and if different from "ROLE NOT FOUND" => 42704 - if err != nil && err.(*pq.Error).Code != "42704" { + if err != nil && !isPgError(err, "42704") { return err } return nil } func (c *pg) UpdatePassword(role, password string) error { - _, err := c.db.Exec(fmt.Sprintf(UPDATE_PASSWORD, role, password)) - if err != nil { - return err - } - - return nil + return c.execute(fmt.Sprintf(UPDATE_PASSWORD, role, password)) } diff --git a/pkg/postgres/role_test.go b/pkg/postgres/role_test.go new file mode 100644 index 000000000..256938c63 --- /dev/null +++ b/pkg/postgres/role_test.go @@ -0,0 +1,137 @@ +package postgres + +import ( + "database/sql" + "regexp" + "testing" + + "github.com/DATA-DOG/go-sqlmock" + "github.com/go-logr/logr" + "github.com/lib/pq" +) + +func TestDropRole_NoOpForReservedRole(t *testing.T) { + db, _, err := sqlmock.New() + if err != nil { + t.Fatalf("sqlmock.New: %v", err) + } + defer db.Close() + + p := &pg{db: db, user: "operator"} + + if err := p.DropRole("rdsadmin", "newowner", "mydb", logr.Discard()); err != nil { + t.Fatalf("DropRole: %v", err) + } +} + +func TestDropRole_ReassignsAndDrops(t *testing.T) { + rootDB, rootMock, err := sqlmock.New() + if err != nil { + t.Fatalf("sqlmock.New root: %v", err) + } + defer rootDB.Close() + + perDB, perMock, err := sqlmock.New() + if err != nil { + t.Fatalf("sqlmock.New per-db: %v", err) + } + defer perDB.Close() + + original := getConnection + getConnection = func(user, password, host, database, uriArgs string, logger logr.Logger) (*sql.DB, error) { + return perDB, nil + } + t.Cleanup(func() { getConnection = original }) + + p := &pg{db: rootDB, user: "operator", pass: "pass", host: "host", args: ""} + + // Ensure operator can manage the role, and newOwner if needed. + rootMock.ExpectExec(regexp.QuoteMeta(`GRANT "todelete" TO "operator"`)).WillReturnResult(sqlmock.NewResult(0, 1)) + rootMock.ExpectExec(regexp.QuoteMeta(`GRANT "newowner" TO "operator"`)).WillReturnResult(sqlmock.NewResult(0, 1)) + + perMock.ExpectExec(regexp.QuoteMeta(`REASSIGN OWNED BY "todelete" TO "newowner"`)).WillReturnResult(sqlmock.NewResult(0, 1)) + perMock.ExpectExec(regexp.QuoteMeta(`DROP OWNED BY "todelete"`)).WillReturnResult(sqlmock.NewResult(0, 1)) + rootMock.ExpectExec(regexp.QuoteMeta(`DROP ROLE "todelete"`)).WillReturnResult(sqlmock.NewResult(0, 1)) + + if err := p.DropRole("todelete", "newowner", "mydb", logr.Discard()); err != nil { + t.Fatalf("DropRole: %v", err) + } + + if err := rootMock.ExpectationsWereMet(); err != nil { + t.Fatalf("root expectations: %v", err) + } + if err := perMock.ExpectationsWereMet(); err != nil { + t.Fatalf("per-db expectations: %v", err) + } +} + +func TestCreateGroupRole_GrantsRoleToOperatorUser(t *testing.T) { + db, mock, err := sqlmock.New() + if err != nil { + t.Fatalf("sqlmock.New: %v", err) + } + defer db.Close() + + p := &pg{db: db, user: "operator"} + + mock.ExpectExec(regexp.QuoteMeta(`CREATE ROLE "myrole"`)).WillReturnResult(sqlmock.NewResult(0, 1)) + mock.ExpectExec(regexp.QuoteMeta(`GRANT "myrole" TO "operator"`)).WillReturnResult(sqlmock.NewResult(0, 1)) + + if err := p.CreateGroupRole("myrole"); err != nil { + t.Fatalf("CreateGroupRole: %v", err) + } + + if err := mock.ExpectationsWereMet(); err != nil { + t.Fatalf("expectations: %v", err) + } +} + +func TestCreateUserRole_GrantsRoleToOperatorUser(t *testing.T) { + db, mock, err := sqlmock.New() + if err != nil { + t.Fatalf("sqlmock.New: %v", err) + } + defer db.Close() + + p := &pg{db: db, user: "operator"} + + mock.ExpectExec(regexp.QuoteMeta(`CREATE ROLE "app-123" WITH LOGIN PASSWORD 'pass'`)).WillReturnResult(sqlmock.NewResult(0, 1)) + mock.ExpectExec(regexp.QuoteMeta(`GRANT "app-123" TO "operator"`)).WillReturnResult(sqlmock.NewResult(0, 1)) + + role, err := p.CreateUserRole("app-123", "pass") + if err != nil { + t.Fatalf("CreateUserRole: %v", err) + } + if role != "app-123" { + t.Fatalf("CreateUserRole role: got %q want %q", role, "app-123") + } + + if err := mock.ExpectationsWereMet(); err != nil { + t.Fatalf("expectations: %v", err) + } +} + +func TestCreateUserRole_IgnoresAlreadyMemberError(t *testing.T) { + db, mock, err := sqlmock.New() + if err != nil { + t.Fatalf("sqlmock.New: %v", err) + } + defer db.Close() + + p := &pg{db: db, user: "operator"} + + mock.ExpectExec(regexp.QuoteMeta(`CREATE ROLE "app-123" WITH LOGIN PASSWORD 'pass'`)).WillReturnResult(sqlmock.NewResult(0, 1)) + mock.ExpectExec(regexp.QuoteMeta(`GRANT "app-123" TO "operator"`)).WillReturnError(&pq.Error{Code: "0LP01"}) + + role, err := p.CreateUserRole("app-123", "pass") + if err != nil { + t.Fatalf("CreateUserRole: %v", err) + } + if role != "app-123" { + t.Fatalf("CreateUserRole role: got %q want %q", role, "app-123") + } + + if err := mock.ExpectationsWereMet(); err != nil { + t.Fatalf("expectations: %v", err) + } +} diff --git a/tests/kuttl-test-self-hosted-postgres.yaml b/tests/kuttl-test-self-hosted-postgres.yaml index 1f9769f50..5f8e53c9b 100644 --- a/tests/kuttl-test-self-hosted-postgres.yaml +++ b/tests/kuttl-test-self-hosted-postgres.yaml @@ -15,12 +15,24 @@ commands: --set global.postgresql.auth.username=postgres --wait timeout: 120 + - command: >- + kubectl -n $NAMESPACE exec postgresql-0 -- bash -lc + "export PGPASSWORD=postgres; + psql -U postgres -d postgres -v ON_ERROR_STOP=1 -c \" + DO \\$\\$ + BEGIN + IF NOT EXISTS (SELECT 1 FROM pg_roles WHERE rolname = 'pgoperator') THEN + CREATE ROLE pgoperator WITH PASSWORD 'pgoperator' LOGIN CREATEDB CREATEROLE; + END IF; + END + \\$\\$;\"" + timeout: 10 - command: >- helm install -n $NAMESPACE ext-postgres-operator ./charts/ext-postgres-operator --set image.repository=postgres-operator --set image.tag=build --set postgres.host=postgresql - --set postgres.user=postgres - --set postgres.password=postgres + --set postgres.user=pgoperator + --set postgres.password=pgoperator --set postgres.uri_args="sslmode=disable" --wait