This is an automated email from the ASF dual-hosted git repository.
yuqi4733 pushed a commit to branch branch-0.8
in repository https://gitbox.apache.org/repos/asf/gravitino.git
The following commit(s) were added to refs/heads/branch-0.8 by this push:
new 3e4b13ef4 [#6306] fix(authz): Fix the OOM of JdbcAuthorizationPlugin
(#6316)
3e4b13ef4 is described below
commit 3e4b13ef480af8e31f54134f97f73ce2bfc29dd4
Author: github-actions[bot]
<41898282+github-actions[bot]@users.noreply.github.com>
AuthorDate: Fri Jan 17 16:37:46 2025 +0800
[#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 <[email protected]>
---
.../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 d0a1b0897..2d74bd050 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 6f9f7e29c..8d3788617 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);
}
