This is an automated email from the ASF dual-hosted git repository.
yuqi4733 pushed a commit to branch main
in repository https://gitbox.apache.org/repos/asf/gravitino.git
The following commit(s) were added to refs/heads/main by this push:
new 7945692385 [#6306] fix(authz): Fix the OOM of JdbcAuthorizationPlugin
(#6312)
7945692385 is described below
commit 79456923856f58c6be79ba8ce68b18cf4c14f978
Author: roryqi <ror...@apache.org>
AuthorDate: Fri Jan 17 16:09:16 2025 +0800
[#6306] fix(authz): Fix the OOM of JdbcAuthorizationPlugin (#6312)
### 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.
---
.../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 d0a1b0897e..2d74bd050e 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 abstract class JdbcAuthorizationPlugin implements
AuthorizationPlugin, Jd
@Override
public Boolean onRoleCreated(Role role) throws AuthorizationPluginException {
List<String> 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 abstract class JdbcAuthorizationPlugin implements
AuthorizationPlugin, Jd
@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 @@ public abstract class JdbcAuthorizationPlugin implements
AuthorizationPlugin, Jd
"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 6f9f7e29c3..8d3788617c 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 class TestJdbcAuthorizationPlugin {
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 class TestJdbcAuthorizationPlugin {
// 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 class TestJdbcAuthorizationPlugin {
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 class TestJdbcAuthorizationPlugin {
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 class TestJdbcAuthorizationPlugin {
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);
}
