diff --git a/operator/src/main/java/it/aboutbits/postgresql/core/BaseReconciler.java b/operator/src/main/java/it/aboutbits/postgresql/core/BaseReconciler.java index fe7515f..6acd76c 100644 --- a/operator/src/main/java/it/aboutbits/postgresql/core/BaseReconciler.java +++ b/operator/src/main/java/it/aboutbits/postgresql/core/BaseReconciler.java @@ -86,13 +86,33 @@ public Optional getReferencedClusterConnection( return Optional.of(clusterConnection); } + /// Sets the phase to [CRPhase#ERROR] for a failure without an exception, and logs the message. + public void setErrorPhase( + CR resource, + S status, + String message + ) { + log.warn( + "Failed to reconcile {} [resource={}/{}, message={}]", + resource.getKind(), + resource.getMetadata().getNamespace(), + resource.getMetadata().getName(), + message + ); + + status.setPhase(CRPhase.ERROR) + .setMessage(message); + } + public UpdateControl handleError( CR resource, S status, E exception ) { log.error( - "Failed to reconcile resource [resource={}]", + "Failed to reconcile {} [resource={}/{}]", + resource.getKind(), + resource.getMetadata().getNamespace(), resource.getMetadata().getName(), exception ); diff --git a/operator/src/main/java/it/aboutbits/postgresql/crd/clusterconnection/ClusterConnectionReconciler.java b/operator/src/main/java/it/aboutbits/postgresql/crd/clusterconnection/ClusterConnectionReconciler.java index 529af3d..d958019 100644 --- a/operator/src/main/java/it/aboutbits/postgresql/crd/clusterconnection/ClusterConnectionReconciler.java +++ b/operator/src/main/java/it/aboutbits/postgresql/crd/clusterconnection/ClusterConnectionReconciler.java @@ -43,8 +43,6 @@ public UpdateControl reconcile( return UpdateControl.patchStatus(resource); } catch (RuntimeException e) { - log.error("Failed to check database connectivity", e); - return handleError( resource, status, diff --git a/operator/src/main/java/it/aboutbits/postgresql/crd/defaultprivilege/DefaultPrivilegeReconciler.java b/operator/src/main/java/it/aboutbits/postgresql/crd/defaultprivilege/DefaultPrivilegeReconciler.java index 5c331c0..f2becae 100644 --- a/operator/src/main/java/it/aboutbits/postgresql/crd/defaultprivilege/DefaultPrivilegeReconciler.java +++ b/operator/src/main/java/it/aboutbits/postgresql/crd/defaultprivilege/DefaultPrivilegeReconciler.java @@ -68,14 +68,17 @@ public UpdateControl reconcile( invalid.removeAll(allowedPrivilegesForObjectType); - status.setPhase(CRPhase.ERROR) - .setMessage("DefaultPrivilege contains invalid privileges for the specified objectType. [resource=%s/%s, objectType=%s, invalidPrivileges=%s, allowedPrivilegesForObjectType=%s]".formatted( - getResourceNamespaceOrOwn(resource, clusterRef.getNamespace()), - clusterRef.getName(), + setErrorPhase( + resource, + status, + "DefaultPrivilege contains invalid privileges for the specified objectType. [resource=%s/%s, objectType=%s, invalidPrivileges=%s, allowedPrivilegesForObjectType=%s]".formatted( + namespace, + name, objectType, invalid, objectType.privileges() - )); + ) + ); return UpdateControl.patchStatus(resource); } @@ -188,7 +191,8 @@ public DeleteControl cleanup( "Failed to delete DefaultPrivilege [resource={}/{}, status.phase={}]", namespace, name, - status.getPhase() + status.getPhase(), + e ); status.setMessage("Deletion failed: %s".formatted(e.getMessage())); @@ -232,13 +236,16 @@ private UpdateControl reconcileInTransaction( )); if (!unsupportedPrivileges.isEmpty()) { - status.setPhase(CRPhase.ERROR) - .setMessage("The following privileges require a newer PostgreSQL version (current: %d): %s [resource=%s/%s]".formatted( + setErrorPhase( + resource, + status, + "The following privileges require a newer PostgreSQL version (current: %d): %s [resource=%s/%s]".formatted( databaseMajorVersion, unsupportedPrivileges, - getResourceNamespaceOrOwn(resource, namespace), + namespace, name - )); + ) + ); return UpdateControl.patchStatus(resource); } @@ -269,6 +276,20 @@ private UpdateControl reconcileInTransaction( ); } + if (privilegesToRevoke.isEmpty() && privilegesToGrant.isEmpty()) { + log.info( + "DefaultPrivilege up-to-date [resource={}/{}]", + namespace, + name + ); + } else { + log.info( + "Updating DefaultPrivilege [resource={}/{}]", + namespace, + name + ); + } + status.setPhase(CRPhase.READY) .setMessage(null); diff --git a/operator/src/main/java/it/aboutbits/postgresql/crd/grant/GrantReconciler.java b/operator/src/main/java/it/aboutbits/postgresql/crd/grant/GrantReconciler.java index 5024420..fb8a594 100644 --- a/operator/src/main/java/it/aboutbits/postgresql/crd/grant/GrantReconciler.java +++ b/operator/src/main/java/it/aboutbits/postgresql/crd/grant/GrantReconciler.java @@ -73,14 +73,17 @@ public UpdateControl reconcile( invalid.removeAll(allowedPrivilegesForObjectType); - status.setPhase(CRPhase.ERROR) - .setMessage("Grant contains invalid privileges for the specified objectType. [resource=%s/%s, objectType=%s, invalidPrivileges=%s, allowedPrivilegesForObjectType=%s]".formatted( - getResourceNamespaceOrOwn(resource, clusterRef.getNamespace()), - clusterRef.getName(), + setErrorPhase( + resource, + status, + "Grant contains invalid privileges for the specified objectType. [resource=%s/%s, objectType=%s, invalidPrivileges=%s, allowedPrivilegesForObjectType=%s]".formatted( + namespace, + name, objectType, invalid, objectType.privileges() - )); + ) + ); return UpdateControl.patchStatus(resource); } @@ -197,7 +200,8 @@ public DeleteControl cleanup( "Failed to delete Grant [resource={}/{}, status.phase={}]", namespace, name, - status.getPhase() + status.getPhase(), + e ); status.setMessage("Deletion failed: %s".formatted(e.getMessage())); @@ -252,13 +256,16 @@ private UpdateControl reconcileInTransaction( )); if (!unsupportedPrivileges.isEmpty()) { - status.setPhase(CRPhase.ERROR) - .setMessage("The following privileges require a newer PostgreSQL version (current: %d): %s [resource=%s/%s]".formatted( + setErrorPhase( + resource, + status, + "The following privileges require a newer PostgreSQL version (current: %d): %s [resource=%s/%s]".formatted( databaseMajorVersion, unsupportedPrivileges, - getResourceNamespaceOrOwn(resource, namespace), + namespace, name - )); + ) + ); return UpdateControl.patchStatus(resource); } @@ -284,18 +291,24 @@ private UpdateControl reconcileInTransaction( }); if (!missingObjects.isEmpty()) { - status.setPhase(CRPhase.ERROR) - .setMessage("Did not grant or revoke any privileges as the listed %s objects do not exist [resource=%s/%s]%n%s".formatted( + setErrorPhase( + resource, + status, + "Did not grant or revoke any privileges as the listed %s objects do not exist [resource=%s/%s]%n • %s".formatted( objectType, - getResourceNamespaceOrOwn(resource, namespace), + namespace, name, String.join("\n • ", missingObjects) - )); + ) + ); return UpdateControl.patchStatus(resource) .rescheduleAfter(Duration.ofSeconds(60)); } + // Whether any grant or revoke statement changes a privilege, only used for the log + var updated = false; + // 1. Reconcile objects explicitly listed in the Spec (processObjects). // We know these are NOT owned (filtered above) and ARE in the spec. for (var object : processObjects) { @@ -312,6 +325,8 @@ private UpdateControl reconcileInTransaction( object, privilegesToRevoke ); + + updated = true; } // If we are not in the "ALL" mode, e.g. objects is an empty List, do explicit grants @@ -331,17 +346,27 @@ private UpdateControl reconcileInTransaction( object, privilegesToGrant ); + + updated = true; } } } // 2. Bulk grant ("ALL" mode only) if (isAllMode) { - grantService.grantOnAll( + var executed = grantService.grantOnAll( tx, spec, expectedPrivileges ); + + // The bulk grant changes something only if a non-owned object misses an expected privilege + if (executed) { + updated |= processObjects.stream() + .anyMatch(object -> !currentObjectPrivileges.getOrDefault(object, Set.of()) + .containsAll(expectedPrivileges) + ); + } } // 2. Revoke orphaned object privileges (Objects with privileges but not in Spec) @@ -367,14 +392,30 @@ private UpdateControl reconcileInTransaction( object, privilegesToRevoke ); + + updated = true; } } + if (updated) { + log.info( + "Updating Grant [resource={}/{}]", + namespace, + name + ); + } else { + log.info( + "Grant up-to-date [resource={}/{}]", + namespace, + name + ); + } + String message = null; if (!ownedObjects.isEmpty()) { - message = "The role is the owner of the listed %s objects and thus we did not need to grant or revoke any privileges from them. [resource=%s/%s]%n%s".formatted( + message = "The role is the owner of the listed %s objects and thus we did not need to grant or revoke any privileges from them. [resource=%s/%s]%n • %s".formatted( objectType, - getResourceNamespaceOrOwn(resource, namespace), + namespace, name, String.join("\n • ", ownedObjects) ); diff --git a/operator/src/main/java/it/aboutbits/postgresql/crd/grant/GrantService.java b/operator/src/main/java/it/aboutbits/postgresql/crd/grant/GrantService.java index 8c59e17..3b1c0d4 100644 --- a/operator/src/main/java/it/aboutbits/postgresql/crd/grant/GrantService.java +++ b/operator/src/main/java/it/aboutbits/postgresql/crd/grant/GrantService.java @@ -420,7 +420,11 @@ public void grant( tx.execute(statement); } - public void grantOnAll( + /// Grants the privileges on all objects of the objectType in the schema. + /// Only `TABLE` and `SEQUENCE` support this, for all other objectTypes nothing is executed. + /// + /// @return `true` if the statement was executed + public boolean grantOnAll( DSLContext tx, GrantSpec spec, Set privilegesToGrant @@ -432,7 +436,7 @@ public void grantOnAll( // grant on all s in schema to ; // is only supported by TABLE, SEQUENCE, FUNCTION, PROCEDURE and ROUTINE if (objectType != TABLE && objectType != SEQUENCE) { - return; + return false; } var privileges = privilegesToGrant.stream() @@ -448,6 +452,8 @@ public void grantOnAll( ); tx.execute(statement); + + return true; } public void revoke( diff --git a/operator/src/test/java/it/aboutbits/postgresql/crd/defaultprivilege/DefaultPrivilegeReconcilerTest.java b/operator/src/test/java/it/aboutbits/postgresql/crd/defaultprivilege/DefaultPrivilegeReconcilerTest.java index 5d92980..0451b7e 100644 --- a/operator/src/test/java/it/aboutbits/postgresql/crd/defaultprivilege/DefaultPrivilegeReconcilerTest.java +++ b/operator/src/test/java/it/aboutbits/postgresql/crd/defaultprivilege/DefaultPrivilegeReconcilerTest.java @@ -295,7 +295,12 @@ void errorWhenInvalidPrivileges() { .extracting(DefaultPrivilege::getStatus) .satisfies(status -> { assertThat(status.getPhase()).isEqualTo(CRPhase.ERROR); - assertThat(status.getMessage()).startsWith("DefaultPrivilege contains invalid privileges for the specified objectType"); + assertThat(status.getMessage()) + .startsWith("DefaultPrivilege contains invalid privileges for the specified objectType") + .contains("[resource=%s/%s,".formatted( + defaultPrivilege.getMetadata().getNamespace(), + defaultPrivilege.getMetadata().getName() + )); }); } diff --git a/operator/src/test/java/it/aboutbits/postgresql/crd/grant/GrantReconcilerTest.java b/operator/src/test/java/it/aboutbits/postgresql/crd/grant/GrantReconcilerTest.java index 29cc12d..f09ed14 100644 --- a/operator/src/test/java/it/aboutbits/postgresql/crd/grant/GrantReconcilerTest.java +++ b/operator/src/test/java/it/aboutbits/postgresql/crd/grant/GrantReconcilerTest.java @@ -290,7 +290,12 @@ void errorWhenInvalidPrivileges() { .extracting(Grant::getStatus) .satisfies(status -> { assertThat(status.getPhase()).isEqualTo(CRPhase.ERROR); - assertThat(status.getMessage()).startsWith("Grant contains invalid privileges for the specified objectType"); + assertThat(status.getMessage()) + .startsWith("Grant contains invalid privileges for the specified objectType") + .contains("[resource=%s/%s,".formatted( + grant.getMetadata().getNamespace(), + grant.getMetadata().getName() + )); }); } @@ -908,6 +913,177 @@ void grantOnAllTables( ); } + @Test + @DisplayName("Should reconcile to ERROR and list the missing tables when a listed table does not exist") + void errorWhenTableDoesNotExist() { + // given + var clusterConnectionMain = given.one() + .clusterConnection() + .returnFirst(); + + var database = given.one() + .database() + .withClusterConnectionName(clusterConnectionMain.getMetadata().getName()) + .withReclaimPolicy(DELETE) + .returnFirst(); + + var clusterConnectionDb = given.one() + .clusterConnection() + .withDatabase(database.getSpec().getName()) + .returnFirst(); + + var schema = given.one() + .schema() + .withClusterConnectionName(clusterConnectionDb.getMetadata().getName()) + .withDatabase(database.getSpec().getName()) + .withReclaimPolicy(DELETE) + .returnFirst(); + + var role = given.one() + .role() + .withClusterConnectionName(clusterConnectionMain.getMetadata().getName()) + .returnFirst(); + + var tableName = "test_table"; + var missingTableName = "missing_table"; + createTable( + clusterConnectionDb, + database.getSpec().getName(), + schema.getSpec().getName(), + tableName + ); + + // when + var grant = given.one() + .grant() + .withClusterConnectionName(clusterConnectionDb.getMetadata().getName()) + .withDatabase(database.getSpec().getName()) + .withSchema(schema.getSpec().getName()) + .withRole(role.getSpec().getName()) + .withObjectType(TABLE) + .withObjects(tableName, missingTableName) + .withPrivileges(SELECT) + .returnFirst(); + + // then + assertThat(grant) + .isNotNull() + .extracting(Grant::getStatus) + .satisfies(status -> { + assertThat(status.getPhase()).isEqualTo(CRPhase.ERROR); + assertThat(status.getMessage()) + .startsWith("Did not grant or revoke any privileges") + .contains("[resource=%s/%s]".formatted( + grant.getMetadata().getNamespace(), + grant.getMetadata().getName() + )) + .endsWith("%n • \"%s\".\"%s\"".formatted( + schema.getSpec().getName(), + missingTableName + )) + .doesNotContain("\"%s\"".formatted(tableName)); + }); + + assertThatNoPrivileges( + clusterConnectionDb, + grant, + tableName + ); + + // cleanup + deleteTable( + clusterConnectionDb, + database.getSpec().getName(), + schema.getSpec().getName(), + tableName + ); + } + + @Test + @DisplayName("Should reconcile to READY and list the owned tables when the role is the owner of a listed table") + void readyWhenRoleOwnsTable() { + // given + var clusterConnectionMain = given.one() + .clusterConnection() + .returnFirst(); + + var database = given.one() + .database() + .withClusterConnectionName(clusterConnectionMain.getMetadata().getName()) + .withReclaimPolicy(DELETE) + .returnFirst(); + + var clusterConnectionDb = given.one() + .clusterConnection() + .withDatabase(database.getSpec().getName()) + .returnFirst(); + + var schema = given.one() + .schema() + .withClusterConnectionName(clusterConnectionDb.getMetadata().getName()) + .withDatabase(database.getSpec().getName()) + .withReclaimPolicy(DELETE) + .returnFirst(); + + var role = given.one() + .role() + .withClusterConnectionName(clusterConnectionMain.getMetadata().getName()) + .returnFirst(); + + var tableName = "test_table"; + createTable( + clusterConnectionDb, + database.getSpec().getName(), + schema.getSpec().getName(), + tableName + ); + changeTableOwner( + clusterConnectionDb, + database.getSpec().getName(), + schema.getSpec().getName(), + tableName, + role.getSpec().getName() + ); + + // when + var grant = given.one() + .grant() + .withClusterConnectionName(clusterConnectionDb.getMetadata().getName()) + .withDatabase(database.getSpec().getName()) + .withSchema(schema.getSpec().getName()) + .withRole(role.getSpec().getName()) + .withObjectType(TABLE) + .withObjects(tableName) + .withPrivileges(SELECT) + .returnFirst(); + + // then + assertThat(grant) + .isNotNull() + .extracting(Grant::getStatus) + .satisfies(status -> { + assertThat(status.getPhase()).isEqualTo(CRPhase.READY); + assertThat(status.getMessage()) + .startsWith("The role is the owner of the listed") + .contains("[resource=%s/%s]".formatted( + grant.getMetadata().getNamespace(), + grant.getMetadata().getName() + )) + .endsWith("%n • \"%s\".\"%s\"".formatted( + schema.getSpec().getName(), + tableName + )); + }); + + // cleanup: the role cannot be dropped while it owns the table + deleteTable( + clusterConnectionDb, + database.getSpec().getName(), + schema.getSpec().getName(), + tableName + ); + } + static Stream> provideAllSupportedPrivileges() { int version = ConfigProvider.getConfig().getValue("test.postgresql-major-version", Integer.class); @@ -1350,6 +1526,22 @@ private void createTableWithSerial( } } + private void changeTableOwner( + ClusterConnection clusterConnection, + String databaseName, + String schemaName, + String tableName, + String owner + ) { + try (var dsl = postgreSQLContextFactory.getDSLContext(clusterConnection, databaseName)) { + dsl.execute( + "alter table {0} owner to {1}", + quotedName(schemaName, tableName), + quotedName(owner) + ); + } + } + private void deleteTable( ClusterConnection clusterConnection, String databaseName,