You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by "mrcnc (via GitHub)" <gi...@apache.org> on 2024/04/11 16:54:56 UTC
[PR] Core: Add property to disable table initialization for JdbcCatalog [iceberg]
mrcnc opened a new pull request, #10124:
URL: https://github.com/apache/iceberg/pull/10124
It would be helpful to disable the creation of the catalog tables used by the JdbcCatalog to maintain the principle of least privilege. In my scenario, I would like to perform database migrations with a migration tool using a user with DDL privileges and use a separate user with only DML privileges for connecting the JdbcCatalog to the database.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Add property to disable table initialization for JdbcCatalog [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra merged PR #10124:
URL: https://github.com/apache/iceberg/pull/10124
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Add property to disable table initialization for JdbcCatalog [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on PR #10124:
URL: https://github.com/apache/iceberg/pull/10124#issuecomment-2053550821
thanks @mrcnc, I'll take another look on monday. @jbonofre could you also take a look please?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Add property to disable table initialization for JdbcCatalog [iceberg]
Posted by "jbonofre (via GitHub)" <gi...@apache.org>.
jbonofre commented on PR #10124:
URL: https://github.com/apache/iceberg/pull/10124#issuecomment-2053962700
This PR is ok for me. I will prepare another pr to provide create statement as sql file and document it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Add property to disable table initialization for JdbcCatalog [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #10124:
URL: https://github.com/apache/iceberg/pull/10124#discussion_r1562361150
##########
core/src/test/java/org/apache/iceberg/jdbc/TestJdbcCatalog.java:
##########
@@ -161,6 +161,19 @@ public void testInitialize() {
jdbcCatalog.initialize("test_jdbc_catalog", properties);
}
+ @Test
+ public void testDisableInitCatalogTables() {
+ Map<String, String> properties = Maps.newHashMap();
+ properties.put(CatalogProperties.WAREHOUSE_LOCATION, this.tableDir.toAbsolutePath().toString());
+ properties.put(CatalogProperties.URI, "jdbc:sqlite:file::memory:?icebergDB");
+ properties.put(JdbcUtil.INIT_CATALOG_TABLES_PROPERTY, "false");
+
+ JdbcCatalog jdbcCatalog = new JdbcCatalog();
+ jdbcCatalog.initialize("test_jdbc_catalog", properties);
+
+ assertThat(jdbcCatalog.getInitializeCatalogTables()).isFalse();
Review Comment:
additionally I think it makes sense to load a namespace or table and assert that this fails via `assertThatThrownBy(() -> catalog.load...).isInstanceOf(..).hasMessage(..)`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Add property to disable table initialization for JdbcCatalog [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #10124:
URL: https://github.com/apache/iceberg/pull/10124#discussion_r1565243207
##########
core/src/test/java/org/apache/iceberg/jdbc/TestJdbcCatalog.java:
##########
@@ -1020,4 +1062,36 @@ private void initLegacySchema(String jdbcUrl) throws SQLException {
.execute();
}
}
+
+ private boolean catalogTablesExist(String jdbcUrl) throws SQLException {
+ SQLiteDataSource dataSource = new SQLiteDataSource();
+ dataSource.setUrl(jdbcUrl);
+
+ boolean catalogTableExists = false;
+ boolean namespacePropertiesTableExists = false;
+
+ try (Connection connection = dataSource.getConnection()) {
+ DatabaseMetaData metadata = connection.getMetaData();
+ if (tableExists(metadata, JdbcUtil.CATALOG_TABLE_VIEW_NAME)) {
+ catalogTableExists = true;
+ }
+ if (tableExists(metadata, JdbcUtil.NAMESPACE_PROPERTIES_TABLE_NAME)) {
+ namespacePropertiesTableExists = true;
+ }
+ }
+
+ return catalogTableExists && namespacePropertiesTableExists;
+ }
+
+ boolean tableExists(DatabaseMetaData metadata, String tableName) throws SQLException {
+ ResultSet resultSet = metadata.getTables(null, null, tableName, new String[] {"TABLE"});
+
+ while (resultSet.next()) {
+ if (resultSet.getString("TABLE_NAME").equals(tableName)) {
Review Comment:
I would switch this around to `tableName.equals(resultSet.getString("TABLE_NAME"))` in case `resultSet.getString("TABLE_NAME")` returns `null`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Add property to disable table initialization for JdbcCatalog [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #10124:
URL: https://github.com/apache/iceberg/pull/10124#discussion_r1562357647
##########
core/src/test/java/org/apache/iceberg/jdbc/TestJdbcCatalog.java:
##########
@@ -161,6 +161,19 @@ public void testInitialize() {
jdbcCatalog.initialize("test_jdbc_catalog", properties);
}
+ @Test
+ public void testDisableInitCatalogTables() {
+ Map<String, String> properties = Maps.newHashMap();
+ properties.put(CatalogProperties.WAREHOUSE_LOCATION, this.tableDir.toAbsolutePath().toString());
+ properties.put(CatalogProperties.URI, "jdbc:sqlite:file::memory:?icebergDB");
+ properties.put(JdbcUtil.INIT_CATALOG_TABLES_PROPERTY, "false");
+
+ JdbcCatalog jdbcCatalog = new JdbcCatalog();
+ jdbcCatalog.initialize("test_jdbc_catalog", properties);
+
+ assertThat(jdbcCatalog.getInitializeCatalogTables()).isFalse();
Review Comment:
instead of checking this flag you might rather want to run a SQL that makes sure the respective catalog tables don't exist. You can do this by creating a connection (similar to how it's done inside `initLegacySchema()`), which then allows to run a SQL.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Add property to disable table initialization for JdbcCatalog [iceberg]
Posted by "jbonofre (via GitHub)" <gi...@apache.org>.
jbonofre commented on PR #10124:
URL: https://github.com/apache/iceberg/pull/10124#issuecomment-2053563062
We already have a property to determine if we create tables or not. But it's not directly exposed easily to the users. Let me prepare a PR to improve this.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Add property to disable table initialization for JdbcCatalog [iceberg]
Posted by "jbonofre (via GitHub)" <gi...@apache.org>.
jbonofre commented on code in PR #10124:
URL: https://github.com/apache/iceberg/pull/10124#discussion_r1565319908
##########
core/src/main/java/org/apache/iceberg/jdbc/JdbcCatalog.java:
##########
@@ -137,6 +137,10 @@ public void initialize(String name, Map<String, String> properties) {
this.connections = new JdbcClientPool(uri, properties);
}
+ String initializeTablesProp = properties.get(JdbcUtil.INIT_CATALOG_TABLES_PROPERTY);
+ if (initializeTablesProp != null) {
+ this.initializeCatalogTables = Boolean.parseBoolean(initializeTablesProp);
Review Comment:
+1
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Add property to disable table initialization for JdbcCatalog [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #10124:
URL: https://github.com/apache/iceberg/pull/10124#discussion_r1561346286
##########
core/src/main/java/org/apache/iceberg/jdbc/JdbcCatalog.java:
##########
@@ -137,6 +137,10 @@ public void initialize(String name, Map<String, String> properties) {
this.connections = new JdbcClientPool(uri, properties);
}
+ String initializeTablesProp = properties.get(PROPERTY_PREFIX + "initialize-catalog-tables");
Review Comment:
you might want to add this rather to `JdbcUtil` (and rename it to `init-catalog-tables`). Also please add some tests to `TestJdbcCatalog`, where the property is set to something different than what is passed to `initializeCatalogTables` via the constructor
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Add property to disable table initialization for JdbcCatalog [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #10124:
URL: https://github.com/apache/iceberg/pull/10124#discussion_r1561346286
##########
core/src/main/java/org/apache/iceberg/jdbc/JdbcCatalog.java:
##########
@@ -137,6 +137,10 @@ public void initialize(String name, Map<String, String> properties) {
this.connections = new JdbcClientPool(uri, properties);
}
+ String initializeTablesProp = properties.get(PROPERTY_PREFIX + "initialize-catalog-tables");
Review Comment:
you might want to add this rather to `JdbcUtil`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Add property to disable table initialization for JdbcCatalog [iceberg]
Posted by "ajantha-bhat (via GitHub)" <gi...@apache.org>.
ajantha-bhat commented on code in PR #10124:
URL: https://github.com/apache/iceberg/pull/10124#discussion_r1565210846
##########
core/src/main/java/org/apache/iceberg/jdbc/JdbcCatalog.java:
##########
@@ -137,6 +137,10 @@ public void initialize(String name, Map<String, String> properties) {
this.connections = new JdbcClientPool(uri, properties);
}
+ String initializeTablesProp = properties.get(JdbcUtil.INIT_CATALOG_TABLES_PROPERTY);
+ if (initializeTablesProp != null) {
+ this.initializeCatalogTables = Boolean.parseBoolean(initializeTablesProp);
Review Comment:
nit: we can directly use
this.initializeCatalogTables = PropertyUtil.propertyAsBoolean(JdbcUtil.INIT_CATALOG_TABLES_PROPERTY, initializeCatalogTables);
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Add property to disable table initialization for JdbcCatalog [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on PR #10124:
URL: https://github.com/apache/iceberg/pull/10124#issuecomment-2058435530
@mrcnc looks like there's a merge conflict. Can you rebase please?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Add property to disable table initialization for JdbcCatalog [iceberg]
Posted by "mrcnc (via GitHub)" <gi...@apache.org>.
mrcnc commented on PR #10124:
URL: https://github.com/apache/iceberg/pull/10124#issuecomment-2052005953
Thanks for the review and suggestions @nastra! I believe I've addressed all the feedback in the latest commit
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Add property to disable table initialization for JdbcCatalog [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #10124:
URL: https://github.com/apache/iceberg/pull/10124#discussion_r1565239656
##########
core/src/test/java/org/apache/iceberg/jdbc/TestJdbcCatalog.java:
##########
@@ -1020,4 +1062,36 @@ private void initLegacySchema(String jdbcUrl) throws SQLException {
.execute();
}
}
+
+ private boolean catalogTablesExist(String jdbcUrl) throws SQLException {
+ SQLiteDataSource dataSource = new SQLiteDataSource();
+ dataSource.setUrl(jdbcUrl);
+
+ boolean catalogTableExists = false;
+ boolean namespacePropertiesTableExists = false;
+
+ try (Connection connection = dataSource.getConnection()) {
+ DatabaseMetaData metadata = connection.getMetaData();
+ if (tableExists(metadata, JdbcUtil.CATALOG_TABLE_VIEW_NAME)) {
+ catalogTableExists = true;
+ }
+ if (tableExists(metadata, JdbcUtil.NAMESPACE_PROPERTIES_TABLE_NAME)) {
+ namespacePropertiesTableExists = true;
+ }
+ }
+
+ return catalogTableExists && namespacePropertiesTableExists;
+ }
+
+ boolean tableExists(DatabaseMetaData metadata, String tableName) throws SQLException {
Review Comment:
can be private
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Add property to disable table initialization for JdbcCatalog [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #10124:
URL: https://github.com/apache/iceberg/pull/10124#discussion_r1565243349
##########
core/src/main/java/org/apache/iceberg/jdbc/JdbcCatalog.java:
##########
@@ -137,6 +137,10 @@ public void initialize(String name, Map<String, String> properties) {
this.connections = new JdbcClientPool(uri, properties);
}
+ String initializeTablesProp = properties.get(JdbcUtil.INIT_CATALOG_TABLES_PROPERTY);
+ if (initializeTablesProp != null) {
+ this.initializeCatalogTables = Boolean.parseBoolean(initializeTablesProp);
Review Comment:
+1
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Add property to disable table initialization for JdbcCatalog [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #10124:
URL: https://github.com/apache/iceberg/pull/10124#discussion_r1562359601
##########
core/src/test/java/org/apache/iceberg/jdbc/TestJdbcCatalog.java:
##########
@@ -161,6 +161,19 @@ public void testInitialize() {
jdbcCatalog.initialize("test_jdbc_catalog", properties);
}
+ @Test
+ public void testDisableInitCatalogTables() {
+ Map<String, String> properties = Maps.newHashMap();
+ properties.put(CatalogProperties.WAREHOUSE_LOCATION, this.tableDir.toAbsolutePath().toString());
+ properties.put(CatalogProperties.URI, "jdbc:sqlite:file::memory:?icebergDB");
+ properties.put(JdbcUtil.INIT_CATALOG_TABLES_PROPERTY, "false");
+
+ JdbcCatalog jdbcCatalog = new JdbcCatalog();
Review Comment:
I think we should also have a test where `initializeCatalogTables` is passed as `false` to the constructor and then set to `true` via the new property
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Add property to disable table initialization for JdbcCatalog [iceberg]
Posted by "jbonofre (via GitHub)" <gi...@apache.org>.
jbonofre commented on PR #10124:
URL: https://github.com/apache/iceberg/pull/10124#issuecomment-2053562342
Sure thing. I can propose a PR during the weekend. Thanks !
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Add property to disable table initialization for JdbcCatalog [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #10124:
URL: https://github.com/apache/iceberg/pull/10124#discussion_r1562355476
##########
core/src/main/java/org/apache/iceberg/jdbc/JdbcCatalog.java:
##########
@@ -843,4 +847,8 @@ public Transaction replaceTransaction() {
return super.replaceTransaction();
}
}
+
+ protected Boolean getInitializeCatalogTables() {
Review Comment:
I don't think we want to expose this
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org