From 3e4b13ef480af8e31f54134f97f73ce2bfc29dd4 Mon Sep 17 00:00:00 2001 From: "github-actions[bot]" <41898282+github-actions[bot]@users.noreply.github.com> Date: Fri, 17 Jan 2025 16:37:46 +0800 Subject: [PATCH] [#6306] fix(authz): Fix the OOM of JdbcAuthorizationPlugin (#6316) ### What changes were proposed in this pull request? Fix the OOM of JdbcAuthorizationPlugin. In old implement, we will call roleUpdate in the roleCreate. We will repeated call, they will cause oom. ### Why are the changes needed? Fix: #6306 ### Does this PR introduce _any_ user-facing change? No. ### How was this patch tested? Added a new UT. Co-authored-by: roryqi --- .../jdbc/JdbcAuthorizationPlugin.java | 13 +++++--- .../jdbc/TestJdbcAuthorizationPlugin.java | 30 ++++++++++++------- 2 files changed, 28 insertions(+), 15 deletions(-) diff --git a/authorizations/authorization-common/src/main/java/org/apache/gravitino/authorization/jdbc/JdbcAuthorizationPlugin.java b/authorizations/authorization-common/src/main/java/org/apache/gravitino/authorization/jdbc/JdbcAuthorizationPlugin.java index d0a1b0897ec..2d74bd050e3 100644 --- a/authorizations/authorization-common/src/main/java/org/apache/gravitino/authorization/jdbc/JdbcAuthorizationPlugin.java +++ b/authorizations/authorization-common/src/main/java/org/apache/gravitino/authorization/jdbc/JdbcAuthorizationPlugin.java @@ -110,8 +110,13 @@ public Boolean onMetadataUpdated(MetadataObjectChange... changes) throws Runtime @Override public Boolean onRoleCreated(Role role) throws AuthorizationPluginException { List sqls = getCreateRoleSQL(role.name()); + boolean createdNewly = false; for (String sql : sqls) { - executeUpdateSQL(sql, "already exists"); + createdNewly = executeUpdateSQL(sql, "already exists"); + } + + if (!createdNewly) { + return true; } if (role.securableObjects() != null) { @@ -140,7 +145,6 @@ public Boolean onRoleDeleted(Role role) throws AuthorizationPluginException { @Override public Boolean onRoleUpdated(Role role, RoleChange... changes) throws AuthorizationPluginException { - onRoleCreated(role); for (RoleChange change : changes) { if (change instanceof RoleChange.AddSecurableObject) { SecurableObject object = ((RoleChange.AddSecurableObject) change).getSecurableObject(); @@ -381,14 +385,15 @@ protected AuthorizationPluginException toAuthorizationPluginException(SQLExcepti "JDBC authorization plugin fail to execute SQL, error code: %d", se.getErrorCode()); } - public void executeUpdateSQL(String sql, String ignoreErrorMsg) { + public boolean executeUpdateSQL(String sql, String ignoreErrorMsg) { try (final Connection connection = getConnection()) { try (final Statement statement = connection.createStatement()) { statement.executeUpdate(sql); + return true; } } catch (SQLException se) { if (ignoreErrorMsg != null && se.getMessage().contains(ignoreErrorMsg)) { - return; + return false; } LOG.error("JDBC authorization plugin exception: ", se); throw toAuthorizationPluginException(se); diff --git a/authorizations/authorization-common/src/test/java/org/apache/gravitino/authorization/jdbc/TestJdbcAuthorizationPlugin.java b/authorizations/authorization-common/src/test/java/org/apache/gravitino/authorization/jdbc/TestJdbcAuthorizationPlugin.java index 6f9f7e29c37..8d3788617cd 100644 --- a/authorizations/authorization-common/src/test/java/org/apache/gravitino/authorization/jdbc/TestJdbcAuthorizationPlugin.java +++ b/authorizations/authorization-common/src/test/java/org/apache/gravitino/authorization/jdbc/TestJdbcAuthorizationPlugin.java @@ -74,9 +74,10 @@ public List getSetOwnerSQL( return Collections.emptyList(); } - public void executeUpdateSQL(String sql, String ignoreErrorMsg) { + public boolean executeUpdateSQL(String sql, String ignoreErrorMsg) { Assertions.assertEquals(expectSQLs.get(currentSQLIndex), sql); currentSQLIndex++; + return true; } }; @@ -148,23 +149,21 @@ public void testPermissionManagement() { // Test metalake object and different role change resetSQLIndex(); - expectSQLs = Lists.newArrayList("CREATE ROLE tmp", "GRANT SELECT ON TABLE *.* TO ROLE tmp"); + expectSQLs = Lists.newArrayList("GRANT SELECT ON TABLE *.* TO ROLE tmp"); SecurableObject metalakeObject = SecurableObjects.ofMetalake("metalake", Lists.newArrayList(Privileges.SelectTable.allow())); RoleChange roleChange = RoleChange.addSecurableObject("tmp", metalakeObject); plugin.onRoleUpdated(role, roleChange); resetSQLIndex(); - expectSQLs = Lists.newArrayList("CREATE ROLE tmp", "REVOKE SELECT ON TABLE *.* FROM ROLE tmp"); + expectSQLs = Lists.newArrayList("REVOKE SELECT ON TABLE *.* FROM ROLE tmp"); roleChange = RoleChange.removeSecurableObject("tmp", metalakeObject); plugin.onRoleUpdated(role, roleChange); resetSQLIndex(); expectSQLs = Lists.newArrayList( - "CREATE ROLE tmp", - "REVOKE SELECT ON TABLE *.* FROM ROLE tmp", - "GRANT CREATE ON TABLE *.* TO ROLE tmp"); + "REVOKE SELECT ON TABLE *.* FROM ROLE tmp", "GRANT CREATE ON TABLE *.* TO ROLE tmp"); SecurableObject newMetalakeObject = SecurableObjects.ofMetalake("metalake", Lists.newArrayList(Privileges.CreateTable.allow())); roleChange = RoleChange.updateSecurableObject("tmp", metalakeObject, newMetalakeObject); @@ -175,7 +174,7 @@ public void testPermissionManagement() { SecurableObject catalogObject = SecurableObjects.ofCatalog("catalog", Lists.newArrayList(Privileges.SelectTable.allow())); roleChange = RoleChange.addSecurableObject("tmp", catalogObject); - expectSQLs = Lists.newArrayList("CREATE ROLE tmp", "GRANT SELECT ON TABLE *.* TO ROLE tmp"); + expectSQLs = Lists.newArrayList("GRANT SELECT ON TABLE *.* TO ROLE tmp"); plugin.onRoleUpdated(role, roleChange); // Test schema object @@ -184,8 +183,7 @@ public void testPermissionManagement() { SecurableObjects.ofSchema( catalogObject, "schema", Lists.newArrayList(Privileges.SelectTable.allow())); roleChange = RoleChange.addSecurableObject("tmp", schemaObject); - expectSQLs = - Lists.newArrayList("CREATE ROLE tmp", "GRANT SELECT ON TABLE schema.* TO ROLE tmp"); + expectSQLs = Lists.newArrayList("GRANT SELECT ON TABLE schema.* TO ROLE tmp"); plugin.onRoleUpdated(role, roleChange); // Test table object @@ -194,8 +192,18 @@ public void testPermissionManagement() { SecurableObjects.ofTable( schemaObject, "table", Lists.newArrayList(Privileges.SelectTable.allow())); roleChange = RoleChange.addSecurableObject("tmp", tableObject); - expectSQLs = - Lists.newArrayList("CREATE ROLE tmp", "GRANT SELECT ON TABLE schema.table TO ROLE tmp"); + expectSQLs = Lists.newArrayList("GRANT SELECT ON TABLE schema.table TO ROLE tmp"); + plugin.onRoleUpdated(role, roleChange); + + // Test the role with objects + resetSQLIndex(); + role = + RoleEntity.builder() + .withId(-1L) + .withName("tmp") + .withSecurableObjects(Lists.newArrayList(tableObject)) + .withAuditInfo(AuditInfo.EMPTY) + .build(); plugin.onRoleUpdated(role, roleChange); }