diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml index f700a6a9f..b9557d743 100644 --- a/.github/workflows/test.yml +++ b/.github/workflows/test.yml @@ -32,3 +32,31 @@ jobs: go clean -modcache make build file bin/manager + permission-repair: + name: Permission repair (PostgreSQL ${{ matrix.postgres }}) + runs-on: ubuntu-latest + timeout-minutes: 15 + strategy: + matrix: + postgres: ["16", "18"] + services: + postgres: + image: postgres:${{ matrix.postgres }} + env: + POSTGRES_HOST_AUTH_METHOD: trust + ports: + - 5432:5432 + options: >- + --health-cmd "pg_isready -U postgres" + --health-interval 5s + --health-timeout 5s + --health-retries 10 + steps: + - uses: actions/checkout@v7 + - uses: actions/setup-go@v7 + with: + go-version-file: go.mod + - name: Verify permission recovery and rollback + env: + PERMISSION_REPAIR_TEST_DSN: postgresql://postgres@localhost:5432/postgres?sslmode=disable + run: go test -race ./pkg/postgres -run TestPermissionRepairIntegration -count=1 diff --git a/README.md b/README.md index 8f5d8bd39..b617fd57d 100644 --- a/README.md +++ b/README.md @@ -18,6 +18,7 @@ Manage external PostgreSQL databases in Kubernetes with ease—supporting AWS RD - [Multiple Operator Support](#multiple-operator-support) - [Secret Templating](#secret-templating) - [Compatibility](#compatibility) +- [Additional Features](#additional-features) - [Contributing](#contributing) - [License](#license) @@ -66,11 +67,11 @@ 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. @@ -82,11 +83,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 +124,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 +154,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 +178,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 +196,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 +232,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 +248,62 @@ 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 | + +## Additional Features + +### AWS specific features (`cloud_provider: "AWS"`) + +- Enable IAM authentication for this user (PostgreSQL on AWS RDS only) + + ```yaml + kind: PostgresUser + .... + spec: + aws: + enableIamAuth: false # (by Default false) + ``` + +- AWS `pg_repack` extension installation / properly alter privileges for the owner user if `cloud_provider: "AWS"` + + ```yaml + kind: Postgres + --- + spec: + extensions: + - pg_repack + ``` +### Scheduled permission repair + +Permission repair is optional per `Postgres`. It adds missing grants; it never +revokes custom grants, changes ownership, recreates roles or rotates credentials. + +What is covered by permissionRepair: + +- All application schemas. +- Tables, partitions, views, materialized views, and foreign tables. +- Sequences. +- Functions and procedures. +- Owner-managed types, domains, and large objects. +- Default privileges for future objects created by the owner role. + +See [docs/permissionRepair.md](docs/permissionRepair.md) for the full reference. + +```yaml +kind: Postgres +.... +spec: + # Keep the existing database, masterRole and schema configuration. + permissionRepair: + schedule: "0 2 * * *" # Five cron fields; every day at 02:00 UTC + windowDuration: "30m" # Latest allowed start/end; defaults to 30m + timeout: "5m" # Maximum transaction duration; defaults to 5m +``` ## Contributing diff --git a/api/v1alpha1/postgres_types.go b/api/v1alpha1/postgres_types.go index 8ce68189e..2196cc1b6 100644 --- a/api/v1alpha1/postgres_types.go +++ b/api/v1alpha1/postgres_types.go @@ -9,7 +9,10 @@ import ( // PostgresSpec defines the desired state of Postgres type PostgresSpec struct { - Database string `json:"database"` + // PermissionRepair enables scheduled, additive permission repair. Omit to disable. + // +optional + PermissionRepair *PermissionRepairSpec `json:"permissionRepair,omitempty"` + Database string `json:"database"` // +optional MasterRole string `json:"masterRole,omitempty"` // +optional @@ -22,10 +25,42 @@ type PostgresSpec struct { Extensions []string `json:"extensions,omitempty"` } +// PermissionRepairSpec schedules maintenance using a five-field UTC cron expression. +type PermissionRepairSpec struct { + // Schedule uses minute, hour, day of month, month, day of week, always in UTC. + // +kubebuilder:validation:MinLength=9 + Schedule string `json:"schedule"` + // WindowDuration limits how late an occurrence can start, including after restart. + // +kubebuilder:default="30m" + // +optional + WindowDuration string `json:"windowDuration,omitempty"` + // Timeout bounds each transaction, also capped by the end of the window. + // +kubebuilder:default="5m" + // +optional + Timeout string `json:"timeout,omitempty"` +} + +// PermissionRepairStatus persists scheduling across restarts and leader changes. +type PermissionRepairStatus struct { + // Configuration identifies the schedule configuration used for NextRunTime. + Configuration string `json:"configuration,omitempty"` + // +optional + NextRunTime *metav1.Time `json:"nextRunTime,omitempty"` + // +optional + LastAttemptTime *metav1.Time `json:"lastAttemptTime,omitempty"` + // +optional + LastSuccessTime *metav1.Time `json:"lastSuccessTime,omitempty"` + // Error is empty after a successful repair. It never contains connection credentials. + // +optional + Error string `json:"error,omitempty"` +} + // PostgresStatus defines the observed state of Postgres type PostgresStatus struct { - Succeeded bool `json:"succeeded"` - Roles PostgresRoles `json:"roles"` + // +optional + PermissionRepair *PermissionRepairStatus `json:"permissionRepair,omitempty"` + Succeeded bool `json:"succeeded"` + Roles PostgresRoles `json:"roles"` // +optional // +listType=set Schemas []string `json:"schemas,omitempty"` diff --git a/api/v1alpha1/zz_generated.deepcopy.go b/api/v1alpha1/zz_generated.deepcopy.go index c21128086..dbe284a35 100644 --- a/api/v1alpha1/zz_generated.deepcopy.go +++ b/api/v1alpha1/zz_generated.deepcopy.go @@ -8,6 +8,48 @@ import ( runtime "k8s.io/apimachinery/pkg/runtime" ) +// DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. +func (in *PermissionRepairSpec) DeepCopyInto(out *PermissionRepairSpec) { + *out = *in +} + +// DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new PermissionRepairSpec. +func (in *PermissionRepairSpec) DeepCopy() *PermissionRepairSpec { + if in == nil { + return nil + } + out := new(PermissionRepairSpec) + in.DeepCopyInto(out) + return out +} + +// DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. +func (in *PermissionRepairStatus) DeepCopyInto(out *PermissionRepairStatus) { + *out = *in + if in.NextRunTime != nil { + in, out := &in.NextRunTime, &out.NextRunTime + *out = (*in).DeepCopy() + } + if in.LastAttemptTime != nil { + in, out := &in.LastAttemptTime, &out.LastAttemptTime + *out = (*in).DeepCopy() + } + if in.LastSuccessTime != nil { + in, out := &in.LastSuccessTime, &out.LastSuccessTime + *out = (*in).DeepCopy() + } +} + +// DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new PermissionRepairStatus. +func (in *PermissionRepairStatus) DeepCopy() *PermissionRepairStatus { + if in == nil { + return nil + } + out := new(PermissionRepairStatus) + in.DeepCopyInto(out) + return out +} + // DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. func (in *Postgres) DeepCopyInto(out *Postgres) { *out = *in @@ -85,6 +127,11 @@ func (in *PostgresRoles) DeepCopy() *PostgresRoles { // DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. func (in *PostgresSpec) DeepCopyInto(out *PostgresSpec) { *out = *in + if in.PermissionRepair != nil { + in, out := &in.PermissionRepair, &out.PermissionRepair + *out = new(PermissionRepairSpec) + **out = **in + } if in.Schemas != nil { in, out := &in.Schemas, &out.Schemas *out = make([]string, len(*in)) @@ -110,6 +157,11 @@ func (in *PostgresSpec) DeepCopy() *PostgresSpec { // DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. func (in *PostgresStatus) DeepCopyInto(out *PostgresStatus) { *out = *in + if in.PermissionRepair != nil { + in, out := &in.PermissionRepair, &out.PermissionRepair + *out = new(PermissionRepairStatus) + (*in).DeepCopyInto(*out) + } out.Roles = in.Roles if in.Schemas != nil { in, out := &in.Schemas, &out.Schemas diff --git a/charts/ext-postgres-operator/Chart.yaml b/charts/ext-postgres-operator/Chart.yaml index 1407a4fd0..7359664a0 100644 --- a/charts/ext-postgres-operator/Chart.yaml +++ b/charts/ext-postgres-operator/Chart.yaml @@ -8,5 +8,5 @@ description: | type: application -version: 3.0.0 -appVersion: "2.4.0" +version: 3.1.0 +appVersion: "2.6.0" diff --git a/charts/ext-postgres-operator/crds/db.movetokube.com_postgres_crd.yaml b/charts/ext-postgres-operator/crds/db.movetokube.com_postgres_crd.yaml index 4977deff6..bfa6f2466 100644 --- a/charts/ext-postgres-operator/crds/db.movetokube.com_postgres_crd.yaml +++ b/charts/ext-postgres-operator/crds/db.movetokube.com_postgres_crd.yaml @@ -42,6 +42,28 @@ spec: x-kubernetes-list-type: set masterRole: type: string + permissionRepair: + description: PermissionRepair enables scheduled, additive permission + repair. Omit to disable. + properties: + schedule: + description: Schedule uses minute, hour, day of month, month, + day of week, always in UTC. + minLength: 9 + type: string + timeout: + default: 5m + description: Timeout bounds each transaction, also capped by the + end of the window. + type: string + windowDuration: + default: 30m + description: WindowDuration limits how late an occurrence can + start, including after restart. + type: string + required: + - schedule + type: object schemas: items: type: string @@ -58,6 +80,28 @@ spec: type: string type: array x-kubernetes-list-type: set + permissionRepair: + description: PermissionRepairStatus persists scheduling across restarts + and leader changes. + properties: + configuration: + description: Configuration identifies the schedule configuration + used for NextRunTime. + type: string + error: + description: Error is empty after a successful repair. It never + contains connection credentials. + type: string + lastAttemptTime: + format: date-time + type: string + lastSuccessTime: + format: date-time + type: string + nextRunTime: + format: date-time + type: string + type: object roles: description: PostgresRoles stores the different group roles for database properties: diff --git a/config/crd/bases/db.movetokube.com_postgres.yaml b/config/crd/bases/db.movetokube.com_postgres.yaml index 10b1f2585..748f75bdc 100644 --- a/config/crd/bases/db.movetokube.com_postgres.yaml +++ b/config/crd/bases/db.movetokube.com_postgres.yaml @@ -50,6 +50,28 @@ spec: x-kubernetes-list-type: set masterRole: type: string + permissionRepair: + description: PermissionRepair enables scheduled, additive permission + repair. Omit to disable. + properties: + schedule: + description: Schedule uses minute, hour, day of month, month, + day of week, always in UTC. + minLength: 9 + type: string + timeout: + default: 5m + description: Timeout bounds each transaction, also capped by the + end of the window. + type: string + windowDuration: + default: 30m + description: WindowDuration limits how late an occurrence can + start, including after restart. + type: string + required: + - schedule + type: object schemas: items: type: string @@ -66,6 +88,28 @@ spec: type: string type: array x-kubernetes-list-type: set + permissionRepair: + description: PermissionRepairStatus persists scheduling across restarts + and leader changes. + properties: + configuration: + description: Configuration identifies the schedule configuration + used for NextRunTime. + type: string + error: + description: Error is empty after a successful repair. It never + contains connection credentials. + type: string + lastAttemptTime: + format: date-time + type: string + lastSuccessTime: + format: date-time + type: string + nextRunTime: + format: date-time + type: string + type: object roles: description: PostgresRoles stores the different group roles for database properties: diff --git a/config/samples/db_v1alpha1_postgres.yaml b/config/samples/db_v1alpha1_postgres.yaml index a1d0525c3..0e7667c1e 100644 --- a/config/samples/db_v1alpha1_postgres.yaml +++ b/config/samples/db_v1alpha1_postgres.yaml @@ -12,3 +12,8 @@ spec: schemas: # List of schemas the operator should create in database - stores - customers + # Optional: reapply application permissions daily in a bounded window. + # permissionRepair: + # schedule: "0 2 * * *" # Every day at 02:00 UTC + # windowDuration: "30m" + # timeout: "5m" diff --git a/docs/permissionRepair.md b/docs/permissionRepair.md new file mode 100644 index 000000000..3230c113f --- /dev/null +++ b/docs/permissionRepair.md @@ -0,0 +1,97 @@ +# Scheduled permission repair + +Permission repair is optional per `Postgres`. It adds missing grants; it never +revokes custom grants, changes ownership, recreates roles or rotates credentials. + +What is covered by permissionRepair: + +- All application schemas. +- Tables, partitions, views, materialized views, and foreign tables. +- Sequences. +- Functions and procedures. +- Owner-managed types, domains, and large objects. +- Default privileges for future objects created by the owner role. + +Note: Time format is UTC + +This is part of Postgres object spec + +```yaml +apiVersion: db.movetokube.com/v1alpha1 +kind: Postgres +metadata: + name: my-db + namespace: app + annotations: + # OPTIONAL + # use this to target which instance of operator should process this CR. See General config + postgres.db.movetokube.com/instance: POSTGRES_INSTANCE +spec: + database: test-db # Name of database created in PostgreSQL + 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) + permissionRepair: # If that field is omitted permissionRepair will be disabled + schedule: "0 2 * * *" # Five cron fields; every day at 02:00 UTC + windowDuration: "30m" # Latest allowed start/end; defaults to 30m + timeout: "5m" # Maximum transaction duration; defaults to 5m + schemas: # List of schemas the operator should create in database (optional) + - stores + - customers + extensions: # List of extensions that should be created in the database (optional) + - fuzzystrmatch + - pgcrypto +``` + +The schedule accepts five-field cron syntax Schedules always run in UTC. + +With a schedule configured, initial provisioning still applies normal grants. +Subsequent permission repair runs only in its window, using `spec.schemas` and the +existing roles in `status.roles`. Keep the full list of application schemas in +`spec.schemas`; system schemas are rejected. Removing `permissionRepair` restores +the original event-driven schema-grant behavior. + +Repair covers database `CONNECT`, schema `USAGE` (and writer `CREATE`), tables, +partitions, views, materialized views, foreign tables, sequences, functions, +procedures, types/domains and owner-managed large objects. Reader gets table/large +object `SELECT` and type `USAGE`. Writer gets table CRUD, sequence `USAGE, SELECT`, +routine `EXECUTE`, type `USAGE`, and large-object `SELECT, UPDATE`. It does not grant +reader execution of routines or writer `TRUNCATE`/ownership rights. + +Existing application objects must belong to the stable owner. Extension-managed objects and PostgreSQL-generated internal routines (such as +range constructors) retain their existing policy. RLS policies, foreign-server credentials +and user mappings are not repaired. Large objects are scoped by owner in the +current database because they do not belong to schemas. + +Default privileges target the stable owner explicitly, in each configured schema. +They therefore apply to migration objects created as that role, not objects +created as an administrator or another login/group. Large-object defaults require +PostgreSQL 18; earlier servers receive existing large-object grants only. New +schemas must be added to `spec.schemas`. Reconcile extensions through their normal +configuration. + +The controller persists `nextRunTime`, `lastAttemptTime`, `lastSuccessTime` and +`error` in `status.permissionRepair`. Provisioning `status.succeeded` stays separate +from repair failures. Enabling/changing the schedule (or its database/schema/role +scope) schedules the next future occurrence. After a restart, a pending occurrence +runs only if still inside its window; expired occurrences are skipped. Each +occurrence is claimed before SQL, attempted at most once, and failures wait until +the next occurrence. A crash after claiming can skip that attempt; interrupted +attempts remain visible in status. This avoids immediate retry loops and repairs +outside maintenance windows. + +Repairs run in a transaction, use cancellation/timeouts capped at the window end, +and take a PostgreSQL advisory lock to prevent concurrent repairs of the same +database. Run the operator with its default leader election enabled. Error status +never includes connection strings or passwords. + +Upgrade the installed CRD as well as the controller before configuring the new +fields. Helm does not automatically upgrade CRDs from a chart's `crds/` directory. + +To run the real privilege/rollback tests against a **disposable** PostgreSQL server: + +```bash +PERMISSION_REPAIR_TEST_DSN='postgresql://postgres@127.0.0.1:55439/postgres?sslmode=disable' go test ./pkg/postgres -run TestPermissionRepairIntegration -count=1 +``` + +The integration test creates and removes a temporary database and roles. CI runs +it on PostgreSQL 16 and 18; scheduling/controller tests require no live cluster. diff --git a/go.mod b/go.mod index c89f8640c..4a738fad0 100644 --- a/go.mod +++ b/go.mod @@ -4,9 +4,11 @@ go 1.26.0 require ( github.com/go-logr/logr v1.4.4 + github.com/DATA-DOG/go-sqlmock v1.5.2 github.com/lib/pq v1.12.3 github.com/onsi/ginkgo/v2 v2.32.0 github.com/onsi/gomega v1.42.1 + github.com/robfig/cron/v3 v3.0.1 go.uber.org/mock v0.6.0 k8s.io/api v0.37.0 k8s.io/apimachinery v0.37.0 diff --git a/go.sum b/go.sum index 4bc62390d..37169a270 100644 --- a/go.sum +++ b/go.sum @@ -1,5 +1,7 @@ cel.dev/expr v0.25.1 h1:1KrZg61W6TWSxuNZ37Xy49ps13NUovb66QLprthtwi4= cel.dev/expr v0.25.1/go.mod h1:hrXvqGP6G6gyx8UAHSHJ5RGk//1Oj5nXQ2NI02Nrsg4= +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.1 h1:SqQKkuVZ+zWkMMNkjy5FZe5mr5WURWnlpmOuzYWrPrQ= @@ -103,6 +105,7 @@ github.com/joshdk/go-junit v1.0.0 h1:S86cUKIdwBHWwA6xCmFlf3RTLfVXYQfvanM5Uh+K6GE github.com/joshdk/go-junit v1.0.0/go.mod h1:TiiV0PqkaNfFXjEiyjWM3XXrhVyCa1K4Zfga6W52ung= github.com/json-iterator/go v1.1.12 h1:PV8peI4a0ysnczrg+LtxykD8LfKY9ML6u2jnxaEnrnM= github.com/json-iterator/go v1.1.12/go.mod h1:e30LSqwooZae/UwlEbR2852Gd8hjQvJoHmT4TnhNGBo= +github.com/kisielk/sqlstruct v0.0.0-20201105191214-5f3e10d3ab46/go.mod h1:yyMNCyc/Ib3bDTKd379tNMpB/7/H5TjM2Y9QJ5THLbE= github.com/klauspost/compress v1.19.0 h1:sXLILfc9jV2QYWkzFOPWStmcUVH2RHEB1JCdY2oVvCQ= github.com/klauspost/compress v1.19.0/go.mod h1:cwPg85FWrGar70rWktvGQj8/hthj3wpl0PGDogxkrSQ= github.com/kr/pretty v0.3.1 h1:flRD4NNwYAUpkphVc1HcthR4KEIFJ65n8Mw5qdRn3LE= @@ -142,6 +145,8 @@ github.com/prometheus/common v0.70.0 h1:bcpru3tWPVnxGnETLgOV5jbp/JRXgYEyv65CuBLA github.com/prometheus/common v0.70.0/go.mod h1:S/SFasQmgGiYH6C81LKCtYa8QACgthGg5zxL2udV7SY= github.com/prometheus/procfs v0.21.1 h1:GljZCt+zSTS+NZq88cyQ1LjZ+RCHp3uVuabBWA5+OJI= github.com/prometheus/procfs v0.21.1/go.mod h1:aB55Cww9pdSJVHk0hUf0inxWyyjPogFIjmHKYgMKmtY= +github.com/robfig/cron/v3 v3.0.1 h1:WdRxkvbJztn8LMz/QEvLN5sBU+xKpSqwwUO1Pjr4qDs= +github.com/robfig/cron/v3 v3.0.1/go.mod h1:eQICP3HwyT7UooqI/z+Ov+PtYAWygg1TEWWzGIFLtro= github.com/rogpeppe/go-internal v1.14.1 h1:UQB4HGPB6osV0SQTLymcB4TgvyWu6ZyliaW0tI/otEQ= github.com/rogpeppe/go-internal v1.14.1/go.mod h1:MaRKkUm5W0goXpeCfT7UZI6fk/L7L7so1lCWt35ZSgc= github.com/russross/blackfriday/v2 v2.1.0/go.mod h1:+Rmxgy9KzJVeS9/2gXHxylqXiyQDYRxCVz55jmeOWTM= diff --git a/internal/controller/permission_repair.go b/internal/controller/permission_repair.go new file mode 100644 index 000000000..54cbbe815 --- /dev/null +++ b/internal/controller/permission_repair.go @@ -0,0 +1,143 @@ +package controller + +import ( + "context" + "crypto/sha256" + "fmt" + "strings" + "time" + + dbv1alpha1 "github.com/movetokube/postgres-operator/api/v1alpha1" + "github.com/movetokube/postgres-operator/pkg/postgres" + "github.com/robfig/cron/v3" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + ctrl "sigs.k8s.io/controller-runtime" + "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/log" +) + +type repairSchedule struct { + cron.Schedule + window, timeout time.Duration + key string +} + +func parseRepairSchedule(spec *dbv1alpha1.PermissionRepairSpec, now time.Time) (repairSchedule, error) { + var result repairSchedule + if len(strings.Fields(spec.Schedule)) != 5 || strings.Contains(spec.Schedule, "=") { + return result, fmt.Errorf("schedule must have exactly five cron fields") + } + parser := cron.NewParser(cron.Minute | cron.Hour | cron.Dom | cron.Month | cron.Dow) + schedule, err := parser.Parse("CRON_TZ=UTC " + spec.Schedule) + if err != nil { + return result, fmt.Errorf("invalid cron schedule") + } + if schedule.Next(now).IsZero() { + return result, fmt.Errorf("cron schedule has no next occurrence") + } + window := spec.WindowDuration + if window == "" { + window = "30m" + } + timeout := spec.Timeout + if timeout == "" { + timeout = "5m" + } + result.window, err = time.ParseDuration(window) + if err != nil || result.window <= 0 || result.window > 24*time.Hour { + return result, fmt.Errorf("windowDuration must be positive and at most 24h") + } + result.timeout, err = time.ParseDuration(timeout) + if err != nil || result.timeout <= 0 || result.timeout > result.window { + return result, fmt.Errorf("timeout must be positive and no longer than windowDuration") + } + result.Schedule = schedule + result.key = fmt.Sprintf("%x", sha256.Sum256([]byte(spec.Schedule+"|UTC|"+window+"|"+timeout))) + return result, nil +} + +func (r *PostgresReconciler) repairNow() time.Time { + if r.now != nil { + return r.now().UTC() + } + return time.Now().UTC() +} + +func (r *PostgresReconciler) reconcilePermissionRepair(ctx context.Context, instance *dbv1alpha1.Postgres) (ctrl.Result, error) { + before := instance.DeepCopy() + if instance.Spec.PermissionRepair == nil { + if instance.Status.PermissionRepair != nil { + instance.Status.PermissionRepair = nil + return ctrl.Result{}, r.Status().Patch(ctx, instance, client.MergeFrom(before)) + } + return ctrl.Result{}, nil + } + now := r.repairNow() + schedule, err := parseRepairSchedule(instance.Spec.PermissionRepair, now) + if instance.Status.PermissionRepair == nil { + instance.Status.PermissionRepair = &dbv1alpha1.PermissionRepairStatus{} + } + status := instance.Status.PermissionRepair + if err != nil { + status.Error = err.Error() + status.NextRunTime = nil + status.Configuration = "" + return ctrl.Result{}, r.Status().Patch(ctx, instance, client.MergeFrom(before)) + } + // A schema/role change starts a new schedule instead of applying stale work. + schedule.key = fmt.Sprintf("%x", sha256.Sum256([]byte(fmt.Sprintf("%s|%s|%v|%v", schedule.key, instance.Spec.Database, instance.Spec.Schemas, instance.Status.Roles)))) + if status.Configuration != schedule.key || status.NextRunTime == nil { + next := metav1.NewTime(schedule.Next(now)) + status.NextRunTime = &next + status.Configuration = schedule.key + status.Error = "" + return ctrl.Result{RequeueAfter: next.Sub(now)}, r.Status().Patch(ctx, instance, client.MergeFrom(before)) + } + due := status.NextRunTime.Time + if now.Before(due) { + return ctrl.Result{RequeueAfter: due.Sub(now)}, nil + } + next := metav1.NewTime(schedule.Next(now)) + status.NextRunTime = &next + end := due.Add(schedule.window) + if !now.Before(end) { + // Do not catch up outside the maintenance window. + return ctrl.Result{RequeueAfter: next.Sub(now)}, r.Status().Patch(ctx, instance, client.MergeFrom(before)) + } + // Persist the occurrence claim before SQL. A restart will not repeat this occurrence. + attempt := metav1.NewTime(now) + status.LastAttemptTime = &attempt + status.Error = "repair interrupted before completion" + if err := r.Status().Patch(ctx, instance, client.MergeFromWithOptions(before, client.MergeFromWithOptimisticLock{})); err != nil { + return ctrl.Result{}, err + } + before = instance.DeepCopy() + status = instance.Status.PermissionRepair + deadline := now.Add(schedule.timeout) + if end.Before(deadline) { + deadline = end + } + repairCtx, cancel := context.WithTimeout(ctx, deadline.Sub(now)) + defer cancel() + err = r.pg.RepairPermissions(repairCtx, postgres.PermissionRepair{ + Database: instance.Spec.Database, Schemas: instance.Spec.Schemas, + Owner: instance.Status.Roles.Owner, Reader: instance.Status.Roles.Reader, Writer: instance.Status.Roles.Writer, + }) + if err != nil { + // Connection errors can contain credentials. Store/log a bounded safe diagnostic. + status.Error = postgres.PermissionRepairError(err) + log.FromContext(ctx).Info("Permission repair failed", "reason", status.Error) + } else { + success := metav1.NewTime(r.repairNow()) + status.LastSuccessTime = &success + status.Error = "" + } + if err := r.Status().Patch(ctx, instance, client.MergeFromWithOptions(before, client.MergeFromWithOptimisticLock{})); err != nil { + return ctrl.Result{}, err + } + delay := status.NextRunTime.Sub(r.repairNow()) + if delay <= 0 { + delay = time.Second + } + return ctrl.Result{RequeueAfter: delay}, nil +} diff --git a/internal/controller/permission_repair_test.go b/internal/controller/permission_repair_test.go new file mode 100644 index 000000000..1b2db3c73 --- /dev/null +++ b/internal/controller/permission_repair_test.go @@ -0,0 +1,243 @@ +package controller + +import ( + "context" + "errors" + "strings" + "testing" + "time" + + db "github.com/movetokube/postgres-operator/api/v1alpha1" + "github.com/movetokube/postgres-operator/pkg/postgres" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/runtime" + "k8s.io/apimachinery/pkg/types" + ctrl "sigs.k8s.io/controller-runtime" + "sigs.k8s.io/controller-runtime/pkg/client/fake" +) + +type repairSpy struct { + postgres.PG + calls int + request postgres.PermissionRepair + err error + timeout time.Duration +} + +func (s *repairSpy) RepairPermissions(ctx context.Context, p postgres.PermissionRepair) error { + s.calls++ + s.request = p + if deadline, ok := ctx.Deadline(); ok { + s.timeout = time.Until(deadline) + } + return s.err +} +func repairFixture(t *testing.T) (*PostgresReconciler, *db.Postgres, *repairSpy, *time.Time) { + t.Helper() + now := time.Date(2026, 9, 9, 0, 0, 0, 0, time.UTC) + instance := &db.Postgres{ObjectMeta: metav1.ObjectMeta{Name: "app", Namespace: "test", Finalizers: []string{"finalizer.db.movetokube.com"}}, + Spec: db.PostgresSpec{Database: "app-db", MasterRole: "app-owner", Schemas: []string{"public", "billing"}, PermissionRepair: &db.PermissionRepairSpec{Schedule: "0 2 * * *"}}, + Status: db.PostgresStatus{Succeeded: true, Schemas: []string{"public", "billing"}, Roles: db.PostgresRoles{Owner: "app-owner", Reader: "app-reader", Writer: "app-writer"}}} + scheme := runtime.NewScheme() + if err := db.AddToScheme(scheme); err != nil { + t.Fatal(err) + } + spy := &repairSpy{} + r := &PostgresReconciler{Client: fake.NewClientBuilder().WithScheme(scheme).WithStatusSubresource(instance).WithObjects(instance).Build(), pg: spy, now: func() time.Time { return now }} + return r, instance, spy, &now +} +func runRepair(t *testing.T, r *PostgresReconciler, p *db.Postgres) ctrl.Result { + t.Helper() + result, err := r.Reconcile(context.Background(), ctrl.Request{NamespacedName: types.NamespacedName{Name: p.Name, Namespace: p.Namespace}}) + if err != nil { + t.Fatal(err) + } + if err = r.Get(context.Background(), types.NamespacedName{Name: p.Name, Namespace: p.Namespace}, p); err != nil { + t.Fatal(err) + } + return result +} +func TestPermissionRepairSchedule(t *testing.T) { + now := time.Date(2026, 9, 9, 0, 0, 0, 0, time.UTC) + tests := []struct { + name string + spec db.PermissionRepairSpec + want string + bad bool + }{ + {"UTC", db.PermissionRepairSpec{Schedule: "0 2 * * *"}, "2026-09-09T02:00:00Z", false}, + {"steps", db.PermissionRepairSpec{Schedule: "*/15 * * * *"}, "2026-09-09T00:15:00Z", false}, + {"six fields", db.PermissionRepairSpec{Schedule: "0 0 2 * * *"}, "", true}, + {"descriptor", db.PermissionRepairSpec{Schedule: "@daily"}, "", true}, + {"range", db.PermissionRepairSpec{Schedule: "65 2 * * *"}, "", true}, + {"impossible", db.PermissionRepairSpec{Schedule: "0 2 31 2 *"}, "", true}, + {"timeout", db.PermissionRepairSpec{Schedule: "0 2 * * *", Timeout: "0s"}, "", true}, + {"oversized timeout", db.PermissionRepairSpec{Schedule: "0 2 * * *", Timeout: "31m"}, "", true}, + {"window", db.PermissionRepairSpec{Schedule: "0 2 * * *", WindowDuration: "48h"}, "", true}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + s, err := parseRepairSchedule(&tt.spec, now) + if (err != nil) != tt.bad { + t.Fatalf("error=%v", err) + } + if err == nil && s.Next(now).UTC().Format(time.RFC3339) != tt.want { + t.Fatalf("next=%v", s.Next(now)) + } + }) + } + +} +func TestPermissionRepairLifecycle(t *testing.T) { + r, p, spy, now := repairFixture(t) + result := runRepair(t, r, p) + if spy.calls != 0 || result.RequeueAfter != 2*time.Hour { + t.Fatalf("initial: calls=%d result=%v", spy.calls, result) + } + // A fresh reconciler uses persisted state, not an in-memory cron job. + r = &PostgresReconciler{Client: r.Client, pg: spy, now: r.now} + *now = now.Add(2*time.Hour + time.Minute) + runRepair(t, r, p) + status := p.Status.PermissionRepair + if spy.calls != 1 || status.LastSuccessTime == nil || status.Error != "" || !p.Status.Succeeded { + t.Fatalf("status=%+v calls=%d", status, spy.calls) + } + if spy.request.Owner != "app-owner" || len(spy.request.Schemas) != 2 || spy.timeout <= 0 || spy.timeout > 5*time.Minute { + t.Fatalf("request=%+v timeout=%v", spy.request, spy.timeout) + } + runRepair(t, r, p) + if spy.calls != 1 { + t.Fatal("status event repeated SQL") + } + // Failure preserves last success and provisioning state; retry is next cron occurrence. + success := status.LastSuccessTime.DeepCopy() + *now = status.NextRunTime.Time + spy.err = errors.New("postgresql://user:secret@host/db") + runRepair(t, r, p) + if spy.calls != 2 || !p.Status.Succeeded || !p.Status.PermissionRepair.LastSuccessTime.Equal(success) || strings.Contains(p.Status.PermissionRepair.Error, "secret") || p.Status.PermissionRepair.Error == "" { + t.Fatalf("failure status=%+v", p.Status) + } + runRepair(t, r, p) + if spy.calls != 2 { + t.Fatal("failure retried outside schedule") + } +} +func TestPermissionRepairMissedWindowAndChanges(t *testing.T) { + r, p, spy, now := repairFixture(t) + runRepair(t, r, p) + *now = now.Add(3 * time.Hour) + runRepair(t, r, p) + if spy.calls != 0 || p.Status.PermissionRepair.NextRunTime.Day() != 10 { + t.Fatal("missed window executed") + } + p.Spec.PermissionRepair.Schedule = "0 4 * * *" + if err := r.Update(context.Background(), p); err != nil { + t.Fatal(err) + } + runRepair(t, r, p) + if p.Status.PermissionRepair.NextRunTime.UTC().Hour() != 4 { + t.Fatalf("schedule change not applied: spec=%+v status=%+v", p.Spec.PermissionRepair, p.Status.PermissionRepair) + } + p.Spec.PermissionRepair.Schedule = "invalid" + if err := r.Update(context.Background(), p); err != nil { + t.Fatal(err) + } + result := runRepair(t, r, p) + if result.RequeueAfter != 0 || p.Status.PermissionRepair.Error == "" || spy.calls != 0 { + t.Fatal("invalid config not rejected") + } + // Removing schedule clears state (helper avoids intentional legacy schema grants). + p.Spec.PermissionRepair = nil + if _, err := r.reconcilePermissionRepair(context.Background(), p); err != nil { + t.Fatal(err) + } + if p.Status.PermissionRepair != nil { + t.Fatal("disabled schedule retained") + } +} +func TestPermissionRepairDeadlineAtWindowEnd(t *testing.T) { + r, p, spy, now := repairFixture(t) + runRepair(t, r, p) + *now = now.Add(2*time.Hour + 29*time.Minute) + runRepair(t, r, p) + if spy.timeout <= 0 || spy.timeout > time.Minute { + t.Fatalf("timeout=%v", spy.timeout) + } +} + +func TestPermissionRepairClaimConflict(t *testing.T) { + r, p, spy, now := repairFixture(t) + runRepair(t, r, p) + stale := p.DeepCopy() + *now = now.Add(2 * time.Hour) + if _, err := r.reconcilePermissionRepair(context.Background(), p); err != nil { + t.Fatal(err) + } + if _, err := r.reconcilePermissionRepair(context.Background(), stale); err == nil { + t.Fatal("stale claim should conflict") + } + if spy.calls != 1 { + t.Fatal("conflicting claim executed SQL") + } +} +func TestPermissionRepairExcludedResources(t *testing.T) { + t.Run("other instance", func(t *testing.T) { + r, p, spy, _ := repairFixture(t) + r.instanceFilter = "other" + runRepair(t, r, p) + if spy.calls != 0 || p.Status.PermissionRepair != nil { + t.Fatal("wrong instance scheduled") + } + }) + t.Run("deleted", func(t *testing.T) { + r, p, spy, _ := repairFixture(t) + if err := r.Delete(context.Background(), p); err != nil { + t.Fatal(err) + } + _, err := r.Reconcile(context.Background(), ctrl.Request{NamespacedName: types.NamespacedName{Name: p.Name, Namespace: p.Namespace}}) + if err != nil { + t.Fatal(err) + } + if spy.calls != 0 { + t.Fatal("deleted resource repaired") + } + }) +} + +// A host-local timezone or daylight-saving boundary must never move the UTC window. +func TestPermissionRepairAlwaysUTC(t *testing.T) { + previousLocal := time.Local + time.Local = time.FixedZone("operator-local", -7*60*60) + t.Cleanup(func() { time.Local = previousLocal }) + spec := &db.PermissionRepairSpec{Schedule: "0 2 * * *"} + for _, day := range []string{"2026-03-28", "2026-03-29", "2026-10-24", "2026-10-25"} { + t.Run(day, func(t *testing.T) { + midnight, err := time.Parse("2006-01-02", day) + if err != nil { + t.Fatal(err) + } + now := midnight.In(time.Local) + schedule, err := parseRepairSchedule(spec, now) + if err != nil { + t.Fatal(err) + } + if got := schedule.Next(now); !got.Equal(midnight.Add(2 * time.Hour)) { + t.Fatalf("next=%v, expected 02:00 UTC", got) + } + r, p, spy, clock := repairFixture(t) + *clock = now + if r.repairNow().Location() != time.UTC { + t.Fatal("controller clock is not UTC") + } + result := runRepair(t, r, p) + if result.RequeueAfter != 2*time.Hour || !p.Status.PermissionRepair.NextRunTime.Equal(&metav1.Time{Time: midnight.Add(2 * time.Hour)}) || spy.calls != 0 { + t.Fatalf("wrong UTC scheduling: %+v", p.Status.PermissionRepair) + } + *clock = now.Add(2 * time.Hour) + runRepair(t, r, p) + if spy.calls != 1 { + t.Fatal("repair did not run at 02:00 UTC") + } + }) + } +} diff --git a/internal/controller/postgres_controller.go b/internal/controller/postgres_controller.go index da19b93e2..dce1cc99a 100644 --- a/internal/controller/postgres_controller.go +++ b/internal/controller/postgres_controller.go @@ -4,6 +4,7 @@ import ( "context" "fmt" "slices" + "time" "k8s.io/apimachinery/pkg/api/errors" "k8s.io/apimachinery/pkg/runtime" @@ -26,6 +27,7 @@ type PostgresReconciler struct { client.Client Scheme *runtime.Scheme pg postgres.PG + now func() time.Time // pgHost string instanceFilter string } @@ -131,6 +133,7 @@ func (r *PostgresReconciler) Reconcile(ctx context.Context, req ctrl.Request) (c return ctrl.Result{Requeue: true}, err } + wasProvisioned := instance.Status.Succeeded // creation logic if !instance.Status.Succeeded { owner := instance.Spec.MasterRole @@ -181,8 +184,8 @@ func (r *PostgresReconciler) Reconcile(ctx context.Context, req ctrl.Request) (c if err != nil { return requeue(errors.NewInternalError(err)) } - // Alter database owner if the owner role was changed - err = r.pg.AlterDatabaseOwner(instance.Spec.Database, instance.Status.Roles.Owner) + // Alter database owner to desiredOwner if the owner role was changed + err = r.pg.AlterDatabaseOwner(instance.Spec.Database, desiredOwner) if err != nil { return requeue(errors.NewInternalError(err)) } @@ -232,50 +235,58 @@ func (r *PostgresReconciler) Reconcile(ctx context.Context, req ctrl.Request) (c instance.Status.Schemas = append(instance.Status.Schemas, schema) } - // Set privileges on schemas during every reconcile to ensure privileges are correct - for _, schema := range instance.Spec.Schemas { + // Without a schedule preserve event-driven grants. With a schedule, grant + // during initial provisioning; subsequent repair is restricted to its window. + if instance.Spec.PermissionRepair == nil || !wasProvisioned { + // Set privileges on schemas during every reconcile to ensure privileges are correct + for _, schema := range instance.Spec.Schemas { - // Set privileges on schema - schemaPrivilegesReader := postgres.PostgresSchemaPrivileges{ - DB: database, - Role: reader, - Schema: schema, - Privs: readerPrivs, - CreateSchema: false, - } - err = r.pg.SetSchemaPrivileges(schemaPrivilegesReader) - if err != nil { - reqLogger.Error(err, fmt.Sprintf("Could not give %s permissions \"%s\"", reader, readerPrivs)) - continue - } - schemaPrivilegesWriter := postgres.PostgresSchemaPrivileges{ - DB: database, - Role: writer, - Schema: schema, - Privs: writerPrivs, - SequencePrivs: writerSequencePrivs, - FunctionPrivs: writerFunctionPrivs, - CreateSchema: true, - } - err = r.pg.SetSchemaPrivileges(schemaPrivilegesWriter) - if err != nil { - reqLogger.Error(err, fmt.Sprintf("Could not give %s permissions \"%s\", sequence privileges \"%s\", and function privileges \"%s\"", writer, writerPrivs, writerSequencePrivs, writerFunctionPrivs)) - continue - } - schemaPrivilegesOwner := postgres.PostgresSchemaPrivileges{ - DB: database, - Role: owner, - Schema: schema, - Privs: ownerPrivs, - SequencePrivs: ownerSequencePrivs, - FunctionPrivs: ownerFunctionPrivs, - CreateSchema: true, - } - err = r.pg.SetSchemaPrivileges(schemaPrivilegesOwner) - if err != nil { - reqLogger.Error(err, fmt.Sprintf("Could not give %s permissions \"%s\", sequence privileges \"%s\", and function privileges \"%s\"", owner, ownerPrivs, ownerSequencePrivs, ownerFunctionPrivs)) - continue + // Set privileges on schema + schemaPrivilegesReader := postgres.PostgresSchemaPrivileges{ + Owner: owner, + DB: database, + Role: reader, + Schema: schema, + Privs: readerPrivs, + CreateSchema: false, + } + err = r.pg.SetSchemaPrivileges(schemaPrivilegesReader) + if err != nil { + reqLogger.Error(err, fmt.Sprintf("Could not give %s permissions \"%s\"", reader, readerPrivs)) + continue + } + schemaPrivilegesWriter := postgres.PostgresSchemaPrivileges{ + Owner: owner, + DB: database, + Role: writer, + Schema: schema, + Privs: writerPrivs, + SequencePrivs: writerSequencePrivs, + FunctionPrivs: writerFunctionPrivs, + CreateSchema: true, + } + err = r.pg.SetSchemaPrivileges(schemaPrivilegesWriter) + if err != nil { + reqLogger.Error(err, fmt.Sprintf("Could not give %s permissions \"%s\", sequence privileges \"%s\", and function privileges \"%s\"", writer, writerPrivs, writerSequencePrivs, writerFunctionPrivs)) + continue + } + schemaPrivilegesOwner := postgres.PostgresSchemaPrivileges{ + Owner: owner, + DB: database, + Role: owner, + Schema: schema, + Privs: ownerPrivs, + SequencePrivs: ownerSequencePrivs, + FunctionPrivs: ownerFunctionPrivs, + CreateSchema: true, + } + err = r.pg.SetSchemaPrivileges(schemaPrivilegesOwner) + if err != nil { + reqLogger.Error(err, fmt.Sprintf("Could not give %s permissions \"%s\", sequence privileges \"%s\", and function privileges \"%s\"", owner, ownerPrivs, ownerSequencePrivs, ownerFunctionPrivs)) + continue + } } + } err = r.Status().Patch(ctx, instance, client.MergeFrom(before)) @@ -291,7 +302,7 @@ func (r *PostgresReconciler) Reconcile(ctx context.Context, req ctrl.Request) (c } reqLogger.Info("Reconciling done") - return ctrl.Result{}, nil + return r.reconcilePermissionRepair(ctx, instance) } func (r *PostgresReconciler) addFinalizer(reqLogger logr.Logger, m *dbv1alpha1.Postgres) error { diff --git a/internal/controller/postgres_controller_test.go b/internal/controller/postgres_controller_test.go index 2f9d62b02..949f72fba 100644 --- a/internal/controller/postgres_controller_test.go +++ b/internal/controller/postgres_controller_test.go @@ -71,8 +71,6 @@ var _ = Describe("PostgresReconciler", func() { // Gomock mockCtrl = gomock.NewController(GinkgoT()) pg = mockpg.NewMockPG(mockCtrl) - pg.EXPECT().AlterDatabaseOwner(gomock.Any(), gomock.Any()).Return(nil).AnyTimes() - pg.EXPECT().ReassignDatabaseOwner(gomock.Any(), gomock.Any(), gomock.Any()).Return(nil).AnyTimes() cl = k8sClient // Create runtime scheme sc = scheme.Scheme @@ -365,6 +363,32 @@ var _ = Describe("PostgresReconciler", func() { }) }) + Context("MasterRole is changed for existing database", func() { + BeforeEach(func() { + modPostgres := postgresCR.DeepCopy() + modPostgres.Spec.MasterRole = "new-master-role" + modPostgres.Status = v1alpha1.PostgresStatus{ + Succeeded: true, + Roles: v1alpha1.PostgresRoles{ + Owner: "old-master-role", + }, + } + initClient(modPostgres, false) + }) + + It("should alter database owner to the desired role", func() { + pg.EXPECT().RenameGroupRole("old-master-role", "new-master-role").Return(nil).Times(1) + pg.EXPECT().AlterDatabaseOwner(name, "new-master-role").Return(nil).Times(1) + + err := runReconcile(rp, ctx, req) + Expect(err).NotTo(HaveOccurred()) + + foundPostgres := &v1alpha1.Postgres{} + Expect(cl.Get(ctx, types.NamespacedName{Name: name, Namespace: namespace}, foundPostgres)).To(BeNil()) + Expect(foundPostgres.Status.Roles.Owner).To(Equal("new-master-role")) + }) + }) + Context("Correct annotation filter is set", func() { BeforeEach(func() { // Create client diff --git a/pkg/postgres/aws.go b/pkg/postgres/aws.go index a27167a4d..a606ebbf9 100644 --- a/pkg/postgres/aws.go +++ b/pkg/postgres/aws.go @@ -2,6 +2,7 @@ package postgres import ( "fmt" + "strings" "github.com/lib/pq" ) @@ -10,6 +11,14 @@ type awspg struct { pg } +const ( + AWS_ALTER_REPACK_DEFAULT_PRIVS_TABLES = `ALTER DEFAULT PRIVILEGES FOR ROLE "%s" IN SCHEMA "repack" GRANT INSERT ON TABLES TO PUBLIC` + AWS_ALTER_REPACK_DEFAULT_PRIVS_SEQUENCES = `ALTER DEFAULT PRIVILEGES FOR ROLE "%s" IN SCHEMA "repack" GRANT USAGE, SELECT ON SEQUENCES TO PUBLIC` +) + +// defaults to GetConnection in production, but can be overridden in unit tests. +var awsGetConnection = GetConnection + func newAWSPG(postgres *pg) PG { return &awspg{ *postgres, @@ -38,6 +47,49 @@ func (c *awspg) CreateDB(dbname, role string) error { return c.pg.CreateDB(dbname, role) } +func (c *awspg) CreateExtension(dbname, extension string) error { + // Keep standard extension creation behavior for AWS as well. + err := c.pg.CreateExtension(dbname, extension) + if err != nil { + return err + } + + // AWS-specific workaround is only required for pg_repack. + if !strings.EqualFold(extension, "pg_repack") { + return nil + } + + return c.applyPgRepackPrivileges(dbname) +} + +func (c *awspg) applyPgRepackPrivileges(dbname string) error { + var owner string + // Resolve current database owner role to target ALTER DEFAULT PRIVILEGES FOR ROLE. + err := c.db.QueryRow(fmt.Sprintf(GET_DB_OWNER, dbname)).Scan(&owner) + if err != nil { + return err + } + + // Execute pg_repack privilege statements in the target database. + tmpDb, err := awsGetConnection(c.user, c.pass, c.host, dbname, c.args) + if err != nil { + return err + } + defer tmpDb.Close() + + _, err = tmpDb.Exec(fmt.Sprintf(AWS_ALTER_REPACK_DEFAULT_PRIVS_TABLES, owner)) + if err != nil { + return err + } + + _, err = tmpDb.Exec(fmt.Sprintf(AWS_ALTER_REPACK_DEFAULT_PRIVS_SEQUENCES, owner)) + if err != nil { + return err + } + + return nil +} + func (c *awspg) CreateUserRole(role, password string) (string, error) { returnedRole, err := c.pg.CreateUserRole(role, password) if err != nil { diff --git a/pkg/postgres/aws_test.go b/pkg/postgres/aws_test.go new file mode 100644 index 000000000..736284f34 --- /dev/null +++ b/pkg/postgres/aws_test.go @@ -0,0 +1,68 @@ +package postgres + +import ( + "database/sql" + "fmt" + "regexp" + "testing" + + sqlmock "github.com/DATA-DOG/go-sqlmock" +) + +func TestApplyPgRepackPrivileges(t *testing.T) { + originalGetConnection := awsGetConnection + defer func() { + awsGetConnection = originalGetConnection + }() + + mainDB, mainMock, err := sqlmock.New() + if err != nil { + t.Fatalf("failed to create main sqlmock: %v", err) + } + defer mainDB.Close() + + tmpDB, tmpMock, err := sqlmock.New() + if err != nil { + t.Fatalf("failed to create tmp sqlmock: %v", err) + } + defer tmpDB.Close() + + dbname := "test-db-dev" + owner := "test-db-dev-owner" + + mainMock.ExpectQuery(regexp.QuoteMeta(fmt.Sprintf(GET_DB_OWNER, dbname))). + WillReturnRows(sqlmock.NewRows([]string{"pg_get_userbyid"}).AddRow(owner)) + + awsGetConnection = func(user, password, host, database, uriArgs string) (*sql.DB, error) { + if database != dbname { + t.Fatalf("expected database %s, got %s", dbname, database) + } + return tmpDB, nil + } + + tmpMock.ExpectExec(regexp.QuoteMeta(fmt.Sprintf(AWS_ALTER_REPACK_DEFAULT_PRIVS_TABLES, owner))). + WillReturnResult(sqlmock.NewResult(0, 0)) + tmpMock.ExpectExec(regexp.QuoteMeta(fmt.Sprintf(AWS_ALTER_REPACK_DEFAULT_PRIVS_SEQUENCES, owner))). + WillReturnResult(sqlmock.NewResult(0, 0)) + + c := &awspg{ + pg: pg{ + db: mainDB, + host: "localhost:5432", + user: "postgres", + pass: "postgres", + args: "sslmode=disable", + }, + } + + if err := c.applyPgRepackPrivileges(dbname); err != nil { + t.Fatalf("expected nil error, got %v", err) + } + + if err := mainMock.ExpectationsWereMet(); err != nil { + t.Fatalf("main DB expectations were not met: %v", err) + } + if err := tmpMock.ExpectationsWereMet(); err != nil { + t.Fatalf("tmp DB expectations were not met: %v", err) + } +} diff --git a/pkg/postgres/database.go b/pkg/postgres/database.go index 11fe8c432..6d19d17bd 100644 --- a/pkg/postgres/database.go +++ b/pkg/postgres/database.go @@ -136,6 +136,22 @@ func (c *pg) SetSchemaPrivileges(schemaPrivileges PostgresSchemaPrivileges) erro } defer tmpDb.Close() + // Keep default privileges aligned with the current database owner and configured access roles + if schemaPrivileges.Owner != "" && schemaPrivileges.Owner != c.user { + for _, grant := range []struct{ objects, privileges string }{ + {"TABLES", schemaPrivileges.Privs}, {"SEQUENCES", schemaPrivileges.SequencePrivs}, {"FUNCTIONS", schemaPrivileges.FunctionPrivs}, + } { + if grant.privileges == "" { + continue + } + _, err = tmpDb.Exec(fmt.Sprintf("ALTER DEFAULT PRIVILEGES FOR ROLE %s IN SCHEMA %s GRANT %s ON %s TO %s", + pq.QuoteIdentifier(schemaPrivileges.Owner), pq.QuoteIdentifier(schemaPrivileges.Schema), grant.privileges, grant.objects, pq.QuoteIdentifier(schemaPrivileges.Role))) + if err != nil { + return err + } + } + } + // Grant role usage on schema _, err = tmpDb.Exec(fmt.Sprintf(GRANT_USAGE_SCHEMA, schemaPrivileges.Schema, schemaPrivileges.Role)) if err != nil { diff --git a/pkg/postgres/mock/postgres.go b/pkg/postgres/mock/postgres.go index 23cfdba8f..a663abe73 100644 --- a/pkg/postgres/mock/postgres.go +++ b/pkg/postgres/mock/postgres.go @@ -3,13 +3,14 @@ // // Generated by this command: // -// mockgen -source pkg/postgres/postgres.go +// mockgen -source=pkg/postgres/postgres.go -destination=pkg/postgres/mock/postgres.go -package=mock // -// Package mock_postgres is a generated GoMock package. -package mock_postgres +// Package mock is a generated GoMock package. +package mock import ( + context "context" reflect "reflect" postgres "github.com/movetokube/postgres-operator/pkg/postgres" @@ -237,6 +238,20 @@ func (mr *MockPGMockRecorder) RenameGroupRole(currentRole, newRole any) *gomock. return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "RenameGroupRole", reflect.TypeOf((*MockPG)(nil).RenameGroupRole), currentRole, newRole) } +// RepairPermissions mocks base method. +func (m *MockPG) RepairPermissions(arg0 context.Context, arg1 postgres.PermissionRepair) error { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "RepairPermissions", arg0, arg1) + ret0, _ := ret[0].(error) + return ret0 +} + +// RepairPermissions indicates an expected call of RepairPermissions. +func (mr *MockPGMockRecorder) RepairPermissions(arg0, arg1 any) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "RepairPermissions", reflect.TypeOf((*MockPG)(nil).RepairPermissions), arg0, arg1) +} + // RevokeRole mocks base method. func (m *MockPG) RevokeRole(role, revoked string) error { m.ctrl.T.Helper() diff --git a/pkg/postgres/permission_repair.go b/pkg/postgres/permission_repair.go new file mode 100644 index 000000000..264103842 --- /dev/null +++ b/pkg/postgres/permission_repair.go @@ -0,0 +1,182 @@ +package postgres + +import ( + "context" + "database/sql" + "errors" + "fmt" + "net/url" + "strings" + + "github.com/lib/pq" +) + +// PermissionRepair uses existing roles and only the explicitly configured schemas. +type PermissionRepair struct { + Database, Owner, Reader, Writer string + Schemas []string +} + +// PermissionRepairError deliberately excludes connection strings and server detail. +func PermissionRepairError(err error) string { + if errors.Is(err, context.DeadlineExceeded) { + return "permission repair timed out" + } + if errors.Is(err, context.Canceled) { + return "permission repair canceled" + } + var pgErr *pq.Error + if errors.As(err, &pgErr) { + return "permission repair failed (SQLSTATE " + string(pgErr.Code) + ")" + } + return "permission repair failed; check database connectivity, configured schemas and owner privileges" +} + +func (c *pg) RepairPermissions(ctx context.Context, repair PermissionRepair) error { + // Use a URL encoder and PingContext: both connection setup and SQL are bounded. + uri := &url.URL{Scheme: "postgresql", User: url.UserPassword(c.user, c.pass), Host: c.host, Path: "/" + repair.Database, RawQuery: c.args} + db, err := sql.Open("postgres", uri.String()) + if err != nil { + return err + } + defer db.Close() + if err := db.PingContext(ctx); err != nil { + return err + } + return repairPermissions(ctx, db, repair) +} + +func repairPermissions(ctx context.Context, db *sql.DB, repair PermissionRepair) error { + if repair.Database == "" || repair.Owner == "" || repair.Reader == "" || repair.Writer == "" || len(repair.Schemas) == 0 { + return fmt.Errorf("database, all roles and at least one schema are required") + } + if repair.Owner == repair.Reader || repair.Owner == repair.Writer || repair.Reader == repair.Writer { + return fmt.Errorf("repair roles must be distinct") + } + for _, name := range append([]string{repair.Database, repair.Owner, repair.Reader, repair.Writer}, repair.Schemas...) { + if strings.ContainsRune(name, 0) { + return fmt.Errorf("identifiers cannot contain NUL") + } + } + for _, schema := range repair.Schemas { + if schema == "" || schema == "information_schema" || strings.HasPrefix(schema, "pg_") { + return fmt.Errorf("system or empty schema cannot be repaired") + } + } + tx, err := db.BeginTx(ctx, nil) + if err != nil { + return err + } + defer tx.Rollback() + // Transaction-scoped lock also prevents overlap between CRs referring to one database. + var locked bool + if err := tx.QueryRowContext(ctx, "SELECT pg_try_advisory_xact_lock(hashtextextended(current_database(), 716913))").Scan(&locked); err != nil { + return err + } + if !locked { + return fmt.Errorf("another permission repair is running") + } + q := pq.QuoteIdentifier + exec := func(statement string) error { _, err := tx.ExecContext(ctx, statement); return err } + if err := exec("SET LOCAL ROLE " + q(repair.Owner)); err != nil { + return err + } + // Fail instead of silently accepting GRANT warnings on objects belonging to another owner. + var invalid bool + if err := tx.QueryRowContext(ctx, repairOwnershipCheck, pq.Array(repair.Schemas)).Scan(&invalid); err != nil { + return err + } + if invalid { + return fmt.Errorf("configured schemas contain application objects owned by another role") + } + if err := exec(fmt.Sprintf("GRANT CONNECT ON DATABASE %s TO %s, %s", q(repair.Database), q(repair.Reader), q(repair.Writer))); err != nil { + return err + } + for _, schema := range repair.Schemas { + for _, statement := range []string{ + fmt.Sprintf("GRANT USAGE ON SCHEMA %s TO %s, %s", q(schema), q(repair.Reader), q(repair.Writer)), + fmt.Sprintf("GRANT CREATE ON SCHEMA %s TO %s", q(schema), q(repair.Writer)), + } { + if err := exec(statement); err != nil { + return err + } + } + } + // Enumerate objects to avoid extensions, internal routines and automatic types. + // Multiranges inherit range ACLs; arrays and table row types use their parent ACLs. + rows, err := tx.QueryContext(ctx, repairObjectGrants, pq.Array(repair.Schemas), repair.Reader, repair.Writer) + if err != nil { + return err + } + var statements []string + for rows.Next() { + var statement string + if err := rows.Scan(&statement); err != nil { + rows.Close() + return err + } + statements = append(statements, statement) + } + err = rows.Err() + rows.Close() + if err != nil { + return err + } + for _, statement := range statements { + if err := exec(statement); err != nil { + return err + } + } + // Only the stable owner creates migration objects. Do not change administrator defaults. + for _, schema := range repair.Schemas { + for _, grant := range []struct{ objects, privileges, role string }{ + {"TABLES", "SELECT", repair.Reader}, {"TABLES", "SELECT, INSERT, UPDATE, DELETE", repair.Writer}, + {"SEQUENCES", "USAGE, SELECT", repair.Writer}, {"ROUTINES", "EXECUTE", repair.Writer}, + {"TYPES", "USAGE", repair.Reader}, {"TYPES", "USAGE", repair.Writer}, + } { + if err := exec(fmt.Sprintf("ALTER DEFAULT PRIVILEGES FOR ROLE %s IN SCHEMA %s GRANT %s ON %s TO %s", q(repair.Owner), q(schema), grant.privileges, grant.objects, q(grant.role))); err != nil { + return err + } + } + } + // Large objects are database-local, not schema-local. Their defaults require PG18. + var version int + if err := tx.QueryRowContext(ctx, "SELECT current_setting('server_version_num')::int").Scan(&version); err != nil { + return err + } + if version >= 180000 { + if err := exec(fmt.Sprintf("ALTER DEFAULT PRIVILEGES FOR ROLE %s GRANT SELECT ON LARGE OBJECTS TO %s", q(repair.Owner), q(repair.Reader))); err != nil { + return err + } + if err := exec(fmt.Sprintf("ALTER DEFAULT PRIVILEGES FOR ROLE %s GRANT SELECT, UPDATE ON LARGE OBJECTS TO %s", q(repair.Owner), q(repair.Writer))); err != nil { + return err + } + } + return tx.Commit() +} + +// System and extension objects retain their own privilege policy. Non-extension +// application objects must belong to the stable owner before adoption is repaired. +const repairOwnershipCheck = `SELECT EXISTS ( + SELECT 1 FROM ( + SELECT 'pg_class'::regclass AS classid, c.oid, c.relowner AS owner FROM pg_class c JOIN pg_namespace n ON n.oid=c.relnamespace WHERE n.nspname=ANY($1) AND c.relkind IN ('r','p','v','m','f','S') + UNION ALL SELECT 'pg_proc'::regclass, p.oid, p.proowner FROM pg_proc p JOIN pg_namespace n ON n.oid=p.pronamespace WHERE n.nspname=ANY($1) + UNION ALL SELECT 'pg_type'::regclass, t.oid, t.typowner FROM pg_type t JOIN pg_namespace n ON n.oid=t.typnamespace LEFT JOIN pg_class c ON c.oid=t.typrelid WHERE n.nspname=ANY($1) AND t.typisdefined AND t.typtype NOT IN ('p','m') AND (t.typrelid=0 OR c.relkind='c') AND NOT EXISTS (SELECT 1 FROM pg_type a WHERE a.typarray=t.oid) + ) obj WHERE owner<>current_user::regrole AND NOT EXISTS (SELECT 1 FROM pg_depend d WHERE d.classid=obj.classid AND d.objid=obj.oid AND (d.deptype='e' OR (obj.classid='pg_proc'::regclass AND d.deptype='i'))) +) OR EXISTS (SELECT 1 FROM pg_database WHERE datname=current_database() AND datdba<>current_user::regrole) OR EXISTS (SELECT 1 FROM pg_namespace WHERE nspname=ANY($1) AND nspowner NOT IN (current_user::regrole, 'pg_database_owner'::regrole))` + +const repairObjectGrants = `SELECT statement FROM ( + SELECT format('GRANT SELECT ON TABLE %I.%I TO %I; GRANT SELECT, INSERT, UPDATE, DELETE ON TABLE %I.%I TO %I', n.nspname,c.relname,$2::text,n.nspname,c.relname,$3::text) AS statement + FROM pg_class c JOIN pg_namespace n ON n.oid=c.relnamespace + WHERE n.nspname=ANY($1) AND c.relowner=current_user::regrole AND c.relkind IN ('r','p','v','m','f') AND NOT EXISTS (SELECT 1 FROM pg_depend d WHERE d.classid='pg_class'::regclass AND d.objid=c.oid AND d.deptype='e') + UNION ALL SELECT format('GRANT USAGE, SELECT ON SEQUENCE %I.%I TO %I',n.nspname,c.relname,$3::text) + FROM pg_class c JOIN pg_namespace n ON n.oid=c.relnamespace + WHERE n.nspname=ANY($1) AND c.relowner=current_user::regrole AND c.relkind='S' AND NOT EXISTS (SELECT 1 FROM pg_depend d WHERE d.classid='pg_class'::regclass AND d.objid=c.oid AND d.deptype='e') + UNION ALL SELECT format('GRANT EXECUTE ON ROUTINE %I.%I(%s) TO %I',n.nspname,p.proname,pg_get_function_identity_arguments(p.oid),$3::text) + FROM pg_proc p JOIN pg_namespace n ON n.oid=p.pronamespace + WHERE n.nspname=ANY($1) AND p.proowner=current_user::regrole AND NOT EXISTS (SELECT 1 FROM pg_depend d WHERE d.classid='pg_proc'::regclass AND d.objid=p.oid AND d.deptype IN ('e','i')) + UNION ALL SELECT format('GRANT USAGE ON TYPE %I.%I TO %I, %I',n.nspname,t.typname,$2::text,$3::text) + FROM pg_type t JOIN pg_namespace n ON n.oid=t.typnamespace LEFT JOIN pg_class c ON c.oid=t.typrelid + WHERE n.nspname=ANY($1) AND t.typowner=current_user::regrole AND t.typisdefined AND t.typtype NOT IN ('p','m') AND (t.typrelid=0 OR c.relkind='c') AND NOT EXISTS (SELECT 1 FROM pg_type a WHERE a.typarray=t.oid) AND NOT EXISTS (SELECT 1 FROM pg_depend d WHERE d.classid='pg_type'::regclass AND d.objid=t.oid AND d.deptype='e') + UNION ALL SELECT format('GRANT SELECT ON LARGE OBJECT %s TO %I; GRANT SELECT, UPDATE ON LARGE OBJECT %s TO %I',oid,$2::text,oid,$3::text) FROM pg_largeobject_metadata WHERE lomowner=current_user::regrole +) grants ORDER BY statement` diff --git a/pkg/postgres/permission_repair_integration_test.go b/pkg/postgres/permission_repair_integration_test.go new file mode 100644 index 000000000..bdfbbdaca --- /dev/null +++ b/pkg/postgres/permission_repair_integration_test.go @@ -0,0 +1,214 @@ +package postgres + +import ( + "context" + "database/sql" + "fmt" + "net/url" + "os" + "strings" + "testing" + "time" + + "github.com/lib/pq" +) + +// Run only against a disposable PostgreSQL server; this test creates and drops its own database/roles. +func TestPermissionRepairIntegration(t *testing.T) { + dsn := os.Getenv("PERMISSION_REPAIR_TEST_DSN") + if dsn == "" { + t.Skip("set PERMISSION_REPAIR_TEST_DSN to a disposable PostgreSQL server") + } + admin, err := sql.Open("postgres", dsn) + if err != nil { + t.Fatal(err) + } + t.Cleanup(func() { admin.Close() }) + suffix := fmt.Sprintf("%d", time.Now().UnixNano()) + database := "repair_" + suffix + owner := "owner_\"" + suffix + reader := "reader_" + suffix + writer := "writer_" + suffix + q := pq.QuoteIdentifier + mustExec := func(db *sql.DB, query string) { + t.Helper() + if _, err := db.Exec(query); err != nil { + t.Fatal(err) + } + } + for _, role := range []string{owner, reader, writer} { + mustExec(admin, "CREATE ROLE "+q(role)) + } + t.Cleanup(func() { + if _, err := admin.Exec("DROP DATABASE IF EXISTS " + q(database) + " WITH (FORCE)"); err != nil { + t.Error(err) + } + for _, role := range []string{owner, reader, writer} { + if _, err := admin.Exec("DROP ROLE " + q(role)); err != nil { + t.Error(err) + } + } + }) + mustExec(admin, "CREATE DATABASE "+q(database)+" OWNER "+q(owner)) + uri, err := url.Parse(dsn) + if err != nil { + t.Fatal(err) + } + uri.Path = "/" + database + target, err := sql.Open("postgres", uri.String()) + if err != nil { + t.Fatal(err) + } + defer target.Close() + target.SetMaxOpenConns(3) + schema := "billing\"items" + mustExec(target, "CREATE EXTENSION hstore; CREATE EXTENSION postgres_fdw; CREATE SERVER dummy FOREIGN DATA WRAPPER postgres_fdw") + mustExec(target, "GRANT USAGE ON FOREIGN SERVER dummy TO "+q(owner)) + mustExec(target, "SET ROLE "+q(owner)+`; CREATE SCHEMA `+q(schema)+`; CREATE SCHEMA private; + CREATE TABLE public.existing(id int, value text); + CREATE TABLE public.partitioned(id int) PARTITION BY RANGE(id); + CREATE TABLE public.partition_1 PARTITION OF public.partitioned FOR VALUES FROM (0) TO (10); + CREATE VIEW public.a_view AS SELECT * FROM public.existing; + CREATE MATERIALIZED VIEW public.a_matview AS SELECT * FROM public.existing; + CREATE SEQUENCE public.counter; + CREATE FUNCTION public.work(i int) RETURNS int LANGUAGE sql AS 'SELECT i'; + CREATE FUNCTION public.work(i text) RETURNS text LANGUAGE sql AS 'SELECT i'; + CREATE PROCEDURE public.proc() LANGUAGE sql AS 'SELECT 1'; + CREATE TYPE public.mood AS ENUM ('ok'); + CREATE DOMAIN public.positive AS int CHECK(VALUE>0); + CREATE TYPE public.pair AS (a int,b text); + CREATE TYPE public.custom_range AS RANGE (subtype=integer); + CREATE FOREIGN TABLE public.foreign_t(id int) SERVER dummy; + CREATE TABLE `+q(schema)+`.other(id int); + CREATE TABLE private.hidden(id int); + REVOKE ALL ON ALL ROUTINES IN SCHEMA public FROM PUBLIC; + ALTER DEFAULT PRIVILEGES REVOKE EXECUTE ON ROUTINES FROM PUBLIC; + ALTER DEFAULT PRIVILEGES REVOKE USAGE ON TYPES FROM PUBLIC; + RESET ROLE;`) + // Preserve custom grants, while repairing only the expected baseline. + mustExec(target, "GRANT TRUNCATE ON public.existing TO "+q(writer)) + createLO := func() uint32 { + t.Helper() + tx, err := target.Begin() + if err != nil { + t.Fatal(err) + } + defer tx.Rollback() + if _, err = tx.Exec("SET LOCAL ROLE " + q(owner)); err != nil { + t.Fatal(err) + } + var oid uint32 + if err = tx.QueryRow("SELECT lo_create(0)").Scan(&oid); err != nil { + t.Fatal(err) + } + if err = tx.Commit(); err != nil { + t.Fatal(err) + } + return oid + } + lo := createLO() + repair := PermissionRepair{Database: database, Owner: owner, Reader: reader, Writer: writer, Schemas: []string{"public", schema}} + ctx, cancel := context.WithTimeout(context.Background(), 30*time.Second) + defer cancel() + password, _ := uri.User.Password() + pgClient := &pg{host: uri.Host, user: uri.User.Username(), pass: password, args: uri.RawQuery} + if err = pgClient.RepairPermissions(ctx, repair); err != nil { + t.Fatal(err) + } + // Idempotent repair and custom grants must survive repeated runs. + if err = repairPermissions(ctx, target, repair); err != nil { + t.Fatal(err) + } + check := func(query string, want bool, args ...any) { + t.Helper() + var got bool + if err := target.QueryRow(query, args...).Scan(&got); err != nil { + t.Fatal(err) + } + if got != want { + t.Fatalf("%s args=%v: got %v want %v", query, args, got, want) + } + } + for _, table := range []string{"public.existing", "public.partitioned", "public.partition_1", "public.a_view", "public.a_matview", "public.foreign_t", q(schema) + ".other"} { + check("SELECT has_table_privilege($1,$2,'SELECT')", true, reader, table) + for _, priv := range []string{"SELECT", "INSERT", "UPDATE", "DELETE"} { + check("SELECT has_table_privilege($1,$2,$3)", true, writer, table, priv) + } + check("SELECT has_table_privilege($1,$2,'INSERT')", false, reader, table) + } + check("SELECT has_table_privilege($1,'public.existing','TRUNCATE')", true, writer) + check("SELECT has_table_privilege($1,'private.hidden','SELECT')", false, reader) + check("SELECT has_sequence_privilege($1,'public.counter','USAGE')", true, writer) + check("SELECT has_sequence_privilege($1,'public.counter','UPDATE')", false, writer) + for _, routine := range []string{"public.work(integer)", "public.work(text)", "public.proc()"} { + check("SELECT has_function_privilege($1,$2,'EXECUTE')", true, writer, routine) + check("SELECT has_function_privilege($1,$2,'EXECUTE')", false, reader, routine) + } + for _, typ := range []string{"public.mood", "public.positive", "public.pair", "public.custom_range", "public.custom_multirange"} { + check("SELECT has_type_privilege($1,$2,'USAGE')", true, reader, typ) + } + check("SELECT EXISTS (SELECT 1 FROM pg_largeobject_metadata l, LATERAL aclexplode(l.lomacl) a WHERE l.oid=$2 AND a.grantee=$1::regrole AND a.privilege_type='SELECT')", true, reader, lo) + check("SELECT EXISTS (SELECT 1 FROM pg_largeobject_metadata l, LATERAL aclexplode(l.lomacl) a WHERE l.oid=$2 AND a.grantee=$1::regrole AND a.privilege_type='UPDATE')", true, writer, lo) + // Normal provisioning must also repair the stable owner's defaults. + mustExec(target, "ALTER DEFAULT PRIVILEGES FOR ROLE "+q(owner)+" IN SCHEMA public REVOKE SELECT ON TABLES FROM "+q(reader)) + if err := pgClient.SetSchemaPrivileges(PostgresSchemaPrivileges{DB: database, Owner: owner, Role: reader, Schema: "public", Privs: "SELECT"}); err != nil { + t.Fatal(err) + } + // Defaults must be on the owner, even though repair was invoked through an admin connection. + mustExec(target, "SET ROLE "+q(owner)+`; CREATE TABLE public.future(id int); CREATE SEQUENCE public.future_seq; + CREATE PROCEDURE public.future_proc() LANGUAGE sql AS 'SELECT 1'; CREATE TYPE public.future_type AS ENUM ('new'); RESET ROLE;`) + check("SELECT has_table_privilege($1,'public.future','SELECT')", true, reader) + check("SELECT has_table_privilege($1,'public.future','UPDATE')", true, writer) + check("SELECT has_sequence_privilege($1,'public.future_seq','USAGE')", true, writer) + check("SELECT has_function_privilege($1,'public.future_proc()','EXECUTE')", true, writer) + check("SELECT has_type_privilege($1,'public.future_type','USAGE')", true, reader) + var version int + if err = target.QueryRow("SHOW server_version_num").Scan(&version); err != nil { + t.Fatal(err) + } + if version >= 180000 { + check("SELECT EXISTS (SELECT 1 FROM pg_largeobject_metadata l, LATERAL aclexplode(l.lomacl) a WHERE l.oid=$2 AND a.grantee=$1::regrole AND a.privilege_type='SELECT')", true, reader, createLO()) + } + // Explicit drift repair. + mustExec(target, "REVOKE SELECT ON public.existing FROM "+q(reader)+"; REVOKE USAGE ON public.counter FROM "+q(writer)) + if err = repairPermissions(ctx, target, repair); err != nil { + t.Fatal(err) + } + check("SELECT has_table_privilege($1,'public.existing','SELECT')", true, reader) + check("SELECT has_sequence_privilege($1,'public.counter','USAGE')", true, writer) + // A second CR/operator cannot overlap while the transaction lock is held. + tx, err := target.Begin() + if err != nil { + t.Fatal(err) + } + if _, err = tx.Exec("SELECT pg_advisory_xact_lock(hashtextextended(current_database(),716913))"); err != nil { + t.Fatal(err) + } + err = repairPermissions(ctx, target, repair) + tx.Rollback() + if err == nil || !strings.Contains(err.Error(), "another permission repair") { + t.Fatalf("lock: %v", err) + } + // A failed transaction cannot leave grants on earlier objects committed. + mustExec(target, "REVOKE SELECT ON public.existing, public.future FROM "+q(reader)) + tx, err = target.Begin() + if err != nil { + t.Fatal(err) + } + if _, err = tx.Exec("SELECT oid FROM pg_class WHERE oid = 'public.future'::regclass FOR UPDATE"); err != nil { + t.Fatal(err) + } + short, cancelShort := context.WithTimeout(context.Background(), 100*time.Millisecond) + err = repairPermissions(short, target, repair) + cancelShort() + tx.Rollback() + if err == nil { + t.Fatal("expected timeout") + } + check("SELECT has_table_privilege($1,'public.existing','SELECT')", false, reader) + // Wrong ownership must not be silently treated as repaired. + mustExec(target, "CREATE TABLE public.not_adopted(id int)") + if err = repairPermissions(ctx, target, repair); err == nil { + t.Fatal("expected ownership failure") + } +} diff --git a/pkg/postgres/permission_repair_test.go b/pkg/postgres/permission_repair_test.go new file mode 100644 index 000000000..c2f9b47f7 --- /dev/null +++ b/pkg/postgres/permission_repair_test.go @@ -0,0 +1,56 @@ +package postgres + +import ( + "context" + "errors" + "strings" + "testing" + + "github.com/DATA-DOG/go-sqlmock" + "github.com/lib/pq" +) + +func TestPermissionRepairValidation(t *testing.T) { + cases := []PermissionRepair{ + {}, {Database: "db", Owner: "o", Reader: "r", Writer: "w"}, + {Database: "db", Owner: "o", Reader: "o", Writer: "w", Schemas: []string{"public"}}, + {Database: "db", Owner: "o", Reader: "r", Writer: "w", Schemas: []string{"pg_catalog"}}, + {Database: "db", Owner: "o", Reader: "r", Writer: "w", Schemas: []string{"information_schema"}}, + {Database: "db", Owner: "o", Reader: "r", Writer: "w", Schemas: []string{"pg_temp_2"}}, + {Database: "db", Owner: "o", Reader: "r", Writer: "w", Schemas: []string{""}}, + {Database: "db", Owner: "o\x00", Reader: "r", Writer: "w", Schemas: []string{"public"}}, + } + for _, p := range cases { + if err := repairPermissions(context.Background(), nil, p); err == nil { + t.Fatalf("accepted %+v", p) + } + } +} +func TestPermissionRepairErrorsAreSafe(t *testing.T) { + for _, err := range []error{errors.New("postgresql://admin:secret@example/db"), &pq.Error{Code: "42501", Message: "secret"}, context.Canceled, context.DeadlineExceeded} { + got := PermissionRepairError(err) + if got == "" || strings.Contains(got, "secret") { + t.Fatalf("unsafe diagnostic %q", got) + } + } +} +func TestPermissionRepairRollbackOnGrantFailure(t *testing.T) { + db, m, err := sqlmock.New() + if err != nil { + t.Fatal(err) + } + defer db.Close() + m.ExpectBegin() + m.ExpectQuery("SELECT pg_try_advisory").WillReturnRows(sqlmock.NewRows([]string{"locked"}).AddRow(true)) + m.ExpectExec("SET LOCAL ROLE").WillReturnResult(sqlmock.NewResult(0, 0)) + m.ExpectQuery("SELECT EXISTS").WillReturnRows(sqlmock.NewRows([]string{"invalid"}).AddRow(false)) + m.ExpectExec("GRANT CONNECT").WillReturnResult(sqlmock.NewResult(0, 0)) + m.ExpectExec("GRANT USAGE ON SCHEMA").WillReturnError(&pq.Error{Code: "42501"}) + m.ExpectRollback() + if err := repairPermissions(context.Background(), db, PermissionRepair{Database: "db", Owner: "o", Reader: "r", Writer: "w", Schemas: []string{"public"}}); err == nil { + t.Fatal("failure reported as success") + } + if err := m.ExpectationsWereMet(); err != nil { + t.Fatal(err) + } +} diff --git a/pkg/postgres/postgres.go b/pkg/postgres/postgres.go index dd5886a53..ab8d51c6b 100644 --- a/pkg/postgres/postgres.go +++ b/pkg/postgres/postgres.go @@ -1,6 +1,7 @@ package postgres import ( + "context" "database/sql" "fmt" @@ -9,6 +10,7 @@ import ( ) type PG interface { + RepairPermissions(context.Context, PermissionRepair) error CreateDB(dbname, username string) error CreateSchema(db, role, schema string) error CreateExtension(db, extension string) error @@ -39,6 +41,7 @@ type pg struct { } type PostgresSchemaPrivileges struct { + Owner string DB string Role string Schema string