persist cloud config #76

dentiny · 2025-09-13T00:35:26Z

Summary

Briefly explain what this PR does.

Related Issues

Closes # or links to related issues.

Changes

Checklist

Code builds correctly
Tests have been added or updated
Documentation updated if necessary
I have reviewed my own changes

gemini-code-assist

Summary of Changes

Hello @dentiny, I'm Gemini Code Assist¹! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!

This pull request significantly enhances the system's ability to manage and persist cloud-specific security configurations for tables. It introduces a flexible, extensible structure for handling credentials from various cloud providers, starting with AWS, and centralizes the management of these secrets within the metadata store. This change improves the robustness and adaptability of how table configurations and their associated security details are stored and retrieved.

Highlights

Refactored Cloud Security Configuration: The AwsSecurityConfig has been moved and integrated into a new CloudSecurityConfig enum. This provides a more extensible structure for handling various cloud provider credentials, starting with AWS, and allows for easier future expansion.
Centralized Secret Management: A new TableConfigEntry struct is introduced to consolidate and manage different types of secrets (cloud vendor, Iceberg data access, WAL) associated with a table configuration during persistence. This streamlines the handling of sensitive information.
Database Schema Update for Secrets: The secrets table in the SQLite metadata store has been updated. It now supports a new cloud usage type for secrets and uses a more generic provider field, accommodating the new CloudSecurityConfig.
API and Internal Logic Updates: Public APIs in lib.rs and storage.rs now expose the new CloudSecurityConfig and AwsSecurityConfig types. Internal logic in iceberg_table_config.rs and config_utils.rs has been adapted to utilize these new structures for configuration parsing and persistence.

Using Gemini Code Assist

The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.

Invoking Gemini

You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.

Feature	Command	Description
Code Review	`/gemini review`	Performs a code review for the current pull request in its current state.
Pull Request Summary	`/gemini summary`	Provides a summary of the current pull request in its current state.
Comment	@gemini-code-assist	Responds in comments when explicitly tagged, both in issue comments and review comments.
Help	`/gemini help`	Displays a list of available commands.

Customization

To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.

Limitations & Feedback

Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here.

You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.

Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩

gemini-code-assist

Code Review

This pull request refactors the cloud security configuration to make it more extensible and attempts to persist it. While the refactoring is a good step, the implementation is incomplete and introduces several critical bugs, particularly in the database persistence logic. There are compilation errors, runtime panics, and incorrect SQL queries that need to be addressed. The logic for reading the persisted configuration also appears to be missing.

gemini-code-assist · 2025-09-13T00:39:41Z

src/moonlink/src/storage/iceberg/iceberg_table_config.rs

@@ -1,5 +1,5 @@
 #[cfg(feature = "catalog-glue")]
-use crate::storage::iceberg::aws_security_config::AwsSecurityConfig;
+use crate::{storage::iceberg::aws_security_config::AwsSecurityConfig, CloudSecurityConfig};


This use statement is incorrect. It refers to storage::iceberg::aws_security_config::AwsSecurityConfig, but the aws_security_config module has been removed. Furthermore, AwsSecurityConfig is not directly used in this file. The import should only be for CloudSecurityConfig.

Suggested change

use crate::{storage::iceberg::aws_security_config::AwsSecurityConfig, CloudSecurityConfig};

use crate::CloudSecurityConfig;

gemini-code-assist · 2025-09-13T00:39:41Z

src/moonlink_metadata_store/src/sqlite/sql/create_secrets.sql

-    usage_type TEXT CHECK (usage_type IN ('iceberg', 'wal')),            -- Purpose of secret: 'iceberg' or 'wal'.
-    storage_provider TEXT CHECK (storage_provider IN ('s3', 'gcs')),     -- One of ('s3', 'gcs')
+    usage_type TEXT CHECK (usage_type IN ('cloud', 'iceberg_storage', 'wal_storage')),
+    provider TEXT CHECK (storage_provider IN ('aws', 's3', 'gcs')),


There is a typo in the CHECK constraint. It refers to storage_provider, but the column has been renamed to provider. This will cause a SQL error when creating the table or inserting data.

provider TEXT CHECK (provider IN ('aws', 's3', 'gcs')),

gemini-code-assist · 2025-09-13T00:39:41Z

src/moonlink_metadata_store/src/sqlite/sqlite_metadata_store.rs

+                s_ice.provider          AS iceberg_storage_provider,
                s_ice.key_id            AS iceberg_key_id,
                s_ice.secret            AS iceberg_secret,
                s_ice.endpoint          AS iceberg_endpoint,
                s_ice.region            AS iceberg_region,
                s_ice.project           AS iceberg_project,
-                s_wal.storage_provider  AS wal_storage_provider,
+                s_wal.provider          AS wal_storage_provider,


The LEFT JOIN conditions for s_ice and s_wal use usage_type = 'iceberg' and usage_type = 'wal' respectively. However, in create_secrets.sql, these usage_type values have been changed to 'iceberg_storage' and 'wal_storage'. This query will now fail to retrieve any secrets for iceberg and WAL. The usage_type values in the ON clauses must be updated to match the new schema.

gemini-code-assist · 2025-09-13T00:39:41Z

src/moonlink_metadata_store/src/sqlite/sqlite_metadata_store.rs

+        let table_config_entry = config_utils::parse_moonlink_table_config(moonlink_table_config)?;
+        let serialized_config = serde_json::to_string(&table_config_entry.serialized_config)?;


This refactoring is incomplete and has introduced several critical issues that will cause compile-time and runtime errors:

The variables iceberg_secret and wal_secret, used later in the function, are no longer defined after this change.

The new cloud_vendor_secret is extracted into table_config_entry but is never persisted to the database.

The subsequent INSERT statements into the secrets table are incorrect as they use:

Hardcoded usage_type values ('iceberg', 'wal') that are now invalid according to the updated schema. They should be 'iceberg_storage' and 'wal_storage'.

The old column name storage_provider which has been renamed to provider.

The entire function needs to be fixed to correctly extract secrets from table_config_entry and persist them using the correct SQL.

gemini-code-assist · 2025-09-13T00:39:42Z

src/moonlink/src/storage/iceberg/cloud_security_config.rs

+                    secret: aws_security_config.security_access_key.clone(),
+                    project: None,
+                    endpoint: None,
+                    region: None,


The region from AwsSecurityConfig is not being used when creating the MoonlinkTableSecret. The region field is being set to None, but it should be populated from aws_security_config.region. This could lead to issues when interacting with AWS services that require a region.

Suggested change

region: None,

region: Some(aws_security_config.region.clone()),

gemini-code-assist · 2025-09-13T00:39:42Z

src/moonlink/src/storage/iceberg/cloud_security_config.rs

+                    region: None,
+                })
+            }
+            _ => None


Using a wildcard _ => None makes this match statement less robust. If new variants are added to CloudSecurityConfig (e.g., for another cloud provider), they will be silently ignored and return None instead of causing a compile error for an unhandled variant. It's better to have an exhaustive match to catch this at compile time and ensure all variants are handled correctly.

gemini-code-assist · 2025-09-13T00:39:42Z

src/moonlink/src/storage/iceberg/iceberg_table_config.rs

+            IcebergCatalogConfig::Glue { glue_catalog_config } => {
+                Some(glue_catalog_config.cloud_secret_config.clone())
+            }
+            _ => None


Using a wildcard _ => None makes this match statement less robust. If new variants are added to IcebergCatalogConfig that contain cloud secrets, they will be silently ignored instead of causing a compile error for an unhandled variant. It's better to have an exhaustive match to ensure all cases are handled.

gemini-code-assist · 2025-09-13T00:39:42Z

src/moonlink_metadata_store/src/config_utils.rs

+    let cloud_secret_config = if let Some(cloud_secret_config) = iceberg_config.metadata_accessor_config.get_cloud_secret_config() {
+        cloud_secret_config.extract_security_metadata_entry()
+    } else {
+        None
+    };


This if let ... else block can be written more concisely and idiomatically using Option::and_then.

let cloud_secret_config = iceberg_config .metadata_accessor_config .get_cloud_secret_config() .and_then(|config| config.extract_security_metadata_entry());

persist cloud config

35fdcd9

gemini-code-assist bot reviewed Sep 13, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

persist cloud config #76

persist cloud config #76

Uh oh!

dentiny commented Sep 13, 2025

Uh oh!

gemini-code-assist bot left a comment

Uh oh!

gemini-code-assist bot left a comment

Uh oh!

gemini-code-assist bot Sep 13, 2025

Uh oh!

gemini-code-assist bot Sep 13, 2025

Uh oh!

gemini-code-assist bot Sep 13, 2025

Uh oh!

gemini-code-assist bot Sep 13, 2025

Uh oh!

gemini-code-assist bot Sep 13, 2025

Uh oh!

gemini-code-assist bot Sep 13, 2025

Uh oh!

gemini-code-assist bot Sep 13, 2025

Uh oh!

gemini-code-assist bot Sep 13, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

	use crate::{storage::iceberg::aws_security_config::AwsSecurityConfig, CloudSecurityConfig};
	use crate::CloudSecurityConfig;

		let table_config_entry = config_utils::parse_moonlink_table_config(moonlink_table_config)?;
		let serialized_config = serde_json::to_string(&table_config_entry.serialized_config)?;

	region: None,
	region: Some(aws_security_config.region.clone()),

persist cloud config #76

Are you sure you want to change the base?

persist cloud config #76

Uh oh!

Conversation

dentiny commented Sep 13, 2025

Summary

Related Issues

Changes

Checklist

Uh oh!

gemini-code-assist bot left a comment

Choose a reason for hiding this comment

Summary of Changes

Highlights

Footnotes

Uh oh!

gemini-code-assist bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

gemini-code-assist bot Sep 13, 2025

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist bot Sep 13, 2025

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist bot Sep 13, 2025

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist bot Sep 13, 2025

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist bot Sep 13, 2025

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist bot Sep 13, 2025

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist bot Sep 13, 2025

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist bot Sep 13, 2025

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants