-
Notifications
You must be signed in to change notification settings - Fork 3.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: snowflake key auth server #34548
Conversation
…keypair-validation
…keypair-validation
WalkthroughThe recent changes primarily enhance the SnowflakePlugin with improved authentication handling, including key pair and basic authentication methods. Additional utility functions were introduced to manage encrypted keys. The DatasourcesApi now ensures proper cleaning of authentication objects. Updates also included new error messages and adjustments to test configurations. Changes
Poem
Warning Review ran into problemsProblems (1)
Tip Early access features: enabledWe are currently testing the following features in early access:
Note:
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
/build-deploy-preview skip-tests=true |
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/9693303074. |
Deploy-Preview-URL: https://ce-34548.dp.appsmith.com |
/build-deploy-preview skip-tests=true recreate=true |
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/9694068032. |
Deploy-Preview-URL: https://ce-34548.dp.appsmith.com |
/build-deploy-preview skip-tests=true recreate=true |
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/9694734021. |
Deploy-Preview-URL: https://ce-34548.dp.appsmith.com |
...ns/snowflakePlugin/src/main/java/com/external/plugins/exceptions/SnowflakeErrorMessages.java
Outdated
Show resolved
Hide resolved
...ver/appsmith-plugins/snowflakePlugin/src/main/java/com/external/plugins/SnowflakePlugin.java
Outdated
Show resolved
Hide resolved
...ver/appsmith-plugins/snowflakePlugin/src/main/java/com/external/plugins/SnowflakePlugin.java
Outdated
Show resolved
Hide resolved
...ver/appsmith-plugins/snowflakePlugin/src/main/java/com/external/plugins/SnowflakePlugin.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, can we please add some tests for the following cases:
- when passphrase is wrong for encrypted private key.
- when private key or passphrase is wrong
- positive successful test case with all the fields and no errors.
@NilanshBansal Will take this up separately as it may not be as straightforward as it looks. |
/build-deploy-preview skip-tests=true recreate=true |
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/9740276453. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (4)
- app/client/src/api/DatasourcesApi.ts (1 hunks)
- app/server/appsmith-plugins/snowflakePlugin/src/main/java/com/external/plugins/SnowflakePlugin.java (5 hunks)
- app/server/appsmith-plugins/snowflakePlugin/src/main/java/com/external/plugins/exceptions/SnowflakeErrorMessages.java (1 hunks)
- app/server/appsmith-plugins/snowflakePlugin/src/main/java/com/external/utils/SnowflakeKeyUtils.java (1 hunks)
Files skipped from review as they are similar to previous changes (2)
- app/server/appsmith-plugins/snowflakePlugin/src/main/java/com/external/plugins/exceptions/SnowflakeErrorMessages.java
- app/server/appsmith-plugins/snowflakePlugin/src/main/java/com/external/utils/SnowflakeKeyUtils.java
Additional context used
Learnings (1)
app/server/appsmith-plugins/snowflakePlugin/src/main/java/com/external/plugins/SnowflakePlugin.java (1)
Learnt from: AmanAgarwal041 PR: appsmithorg/appsmith#34491 File: app/server/appsmith-plugins/snowflakePlugin/src/main/java/com/external/plugins/SnowflakePlugin.java:320-338 Timestamp: 2024-06-26T11:16:09.077Z Learning: The `encryptedPassphrase` field in the `KeyPairAuth` class for Snowflake authentication is optional and does not require validation for emptiness.
Biome
app/client/src/api/DatasourcesApi.ts
[error] 24-243: Avoid classes that contain only static members.
Prefer using simple functions instead of classes with only static members.
(lint/complexity/noStaticOnlyClass)
Additional comments not posted (8)
app/client/src/api/DatasourcesApi.ts (2)
Line range hint
15-45
: LGTM! Ensure thecleanAuthenticationObject
method is thoroughly tested.The changes sanitize the
authentication
object before sending it to the server, which is a good practice. Ensure that thecleanAuthenticationObject
method is thoroughly tested to handle all possible authentication types and edge cases.
146-148
: LGTM! Ensure thecleanAuthenticationObject
method is thoroughly tested.The changes sanitize the
authentication
object before sending it to the server, which is a good practice. Ensure that thecleanAuthenticationObject
method is thoroughly tested to handle all possible authentication types and edge cases.app/server/appsmith-plugins/snowflakePlugin/src/main/java/com/external/plugins/SnowflakePlugin.java (6)
159-182
: LGTM! Ensure thegetHikariConfig
method is thoroughly tested.The changes set up the HikariConfig and connection properties for the HikariDataSource. Ensure that the
getHikariConfig
method is thoroughly tested to handle all possible configurations and edge cases.
206-215
: LGTM! Ensure all authentication types are thoroughly tested.The changes add a switch statement to handle different authentication types and set properties accordingly. Ensure that all authentication types are thoroughly tested to handle all possible configurations and edge cases.
463-483
: LGTM! Ensure all authentication types are thoroughly tested.The changes add a switch statement to handle different authentication types and return the appropriate HikariConfig. Ensure that all authentication types are thoroughly tested to handle all possible configurations and edge cases.
485-515
: Ensure secure handling of private keys.The changes set up the SnowflakeBasicDataSource with the private key and other properties. Ensure that the private key handling is secure and that error handling is robust to prevent any security vulnerabilities.
517-529
: LGTM! Ensure the authentication details are thoroughly tested.The changes set the username and password for the HikariConfig. Ensure that the authentication details are thoroughly tested to handle all possible configurations and edge cases.
546-562
: LGTM! Ensure the exception handling is robust.The changes handle exceptions and return the private key. Ensure that the exception handling is robust to prevent any security vulnerabilities and that the private key is correctly retrieved.
Failed server tests
|
...ver/appsmith-plugins/snowflakePlugin/src/main/java/com/external/plugins/SnowflakePlugin.java
Outdated
Show resolved
Hide resolved
...ver/appsmith-plugins/snowflakePlugin/src/main/java/com/external/plugins/SnowflakePlugin.java
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor qs, rest lgtm
Deploy-Preview-URL: https://ce-34548.dp.appsmith.com |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (2)
- app/server/appsmith-plugins/snowflakePlugin/src/main/java/com/external/plugins/SnowflakePlugin.java (7 hunks)
- app/server/appsmith-plugins/snowflakePlugin/src/test/java/com/external/plugins/SnowflakePluginTest.java (1 hunks)
Additional context used
Learnings (1)
app/server/appsmith-plugins/snowflakePlugin/src/main/java/com/external/plugins/SnowflakePlugin.java (1)
Learnt from: AmanAgarwal041 PR: appsmithorg/appsmith#34491 File: app/server/appsmith-plugins/snowflakePlugin/src/main/java/com/external/plugins/SnowflakePlugin.java:320-338 Timestamp: 2024-06-26T11:16:09.077Z Learning: The `encryptedPassphrase` field in the `KeyPairAuth` class for Snowflake authentication is optional and does not require validation for emptiness.
Additional comments not posted (9)
app/server/appsmith-plugins/snowflakePlugin/src/test/java/com/external/plugins/SnowflakePluginTest.java (1)
105-105
: Verify the correctness of the authentication type.Ensure that the
dbAuth
authentication type is correct and consistent with the rest of the codebase.Verification successful
The authentication type
dbAuth
is correct and consistent across the codebase.The usage of
dbAuth
in the codebase is consistent and aligns with the established authentication practices.
app/server/appsmith-interfaces/src/main/java/com/appsmith/external/constants/Authentication.java
: The constantDB_AUTH
is defined here, confirming its legitimacy.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the correctness of the authentication type `dbAuth`. # Test: Search for the usage of `dbAuth` in the codebase. Expect: Consistent usage across the codebase. rg --type java $'dbAuth'Length of output: 8111
app/server/appsmith-plugins/snowflakePlugin/src/main/java/com/external/plugins/SnowflakePlugin.java (8)
215-223
: Verify the correctness of the authentication type handling.Ensure that the authentication type handling is correct and consistent with the rest of the codebase.
471-491
: Verify the correctness of the Hikari configuration handling.Ensure that the Hikari configuration handling based on the authentication type is correct and consistent with the rest of the codebase.
Verification successful
Verify the correctness of the Hikari configuration handling.
The
getHikariConfig
function is invoked within thecreateConnectionClient
method, where the Hikari configuration is further processed by setting the JDBC URL. This confirms that the Hikari configuration handling is consistent within the context of setting up a connection client.
- The
getHikariConfig
method correctly handles different authentication types and returns aMono<HikariConfig>
which is then used to create aMono<HikariDataSource>
.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the correctness of the Hikari configuration handling. # Test: Search for the usage of `getHikariConfig` in the codebase. Expect: Consistent handling of Hikari configuration. rg --type java $'getHikariConfig'Length of output: 356
Script:
#!/bin/bash # Description: Verify the correctness of the Hikari configuration handling by examining the usage of `getHikariConfig`. # Test: Search for the invocation of `getHikariConfig` within `SnowflakePlugin.java` and examine the context of its usage. rg 'getHikariConfig' -A 5 -B 5 app/server/appsmith-plugins/snowflakePlugin/src/main/java/com/external/plugins/SnowflakePlugin.javaLength of output: 1096
555-571
: Verify the correctness of the private key handling.Ensure that the private key handling, including exception handling, is correct and consistent with the rest of the codebase.
Verification successful
Verify the correctness of the private key handling.
The function
getPrivateKeyFromBase64
is used withingetKeyPairAuthConfig
, where keyBytes and passphrase are retrieved fromKeyPairAuth
and the result is used to set up aSnowflakeBasicDataSource
. The private key handling and exception handling appear to be correct and consistent with the rest of the codebase.
getPrivateKeyFromBase64
correctly handles exceptions and converts them intoMono.error
with appropriate error messages.- The usage of
getPrivateKeyFromBase64
ingetKeyPairAuthConfig
ensures that the private key is properly decoded and used for setting up the data source.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the correctness of the private key handling. # Test: Search for the usage of `getPrivateKeyFromBase64` in the codebase. Expect: Correct handling of private keys. rg --type java $'getPrivateKeyFromBase64'Length of output: 400
Script:
#!/bin/bash # Description: Extract context around the usage of `getPrivateKeyFromBase64` in `SnowflakePlugin.java`. # Get the lines around the usage of `getPrivateKeyFromBase64` to understand how it's called. rg --type java -A 5 -B 5 'getPrivateKeyFromBase64' app/server/appsmith-plugins/snowflakePlugin/src/main/java/com/external/plugins/SnowflakePlugin.javaLength of output: 1483
494-524
: Verify the correctness of the key-pair authentication configuration.Ensure that the key-pair authentication configuration is correct and consistent with the rest of the codebase.
Verification successful
The key-pair authentication configuration appears to be correctly implemented.
The function
getKeyPairAuthConfig
is invoked appropriately within a switch-case structure handling different authentication types. The configuration process within the function appears to be properly set up with the necessary parameters.
getKeyPairAuthConfig
is called whenSNOWFLAKE_KEY_PAIR_AUTH
is the selected authentication type.- The function correctly configures the
HikariConfig
with the required parameters.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the correctness of the key-pair authentication configuration. # Test: Search for the usage of `getKeyPairAuthConfig` in the codebase. Expect: Correct configuration of key-pair authentication. rg --type java $'getKeyPairAuthConfig'Length of output: 392
Script:
#!/bin/bash # Description: Extract context around the usage of `getKeyPairAuthConfig` in SnowflakePlugin.java. # Extract 5 lines before and after the usage of `getKeyPairAuthConfig` to understand the context rg --type java -C 5 'getKeyPairAuthConfig' app/server/appsmith-plugins/snowflakePlugin/src/main/java/com/external/plugins/SnowflakePlugin.javaLength of output: 1127
526-538
: Verify the correctness of the basic authentication configuration.Ensure that the basic authentication configuration is correct and consistent with the rest of the codebase.
573-577
: Verify the correctness of the JDBC URL formation.Ensure that the JDBC URL formation is correct and consistent with the rest of the codebase.
Verification successful
Verify the correctness of the JDBC URL formation.
Based on the context provided, the function
getJDBCUrl
constructs the JDBC URL using the base URL from theDatasourceConfiguration
and appends the domain.snowflakecomputing.com
. This approach appears to be consistent with Snowflake's JDBC URL format.
getJDBCUrl
is used to set the JDBC URL in the HikariConfig object.- The URL formation logic seems correct as per Snowflake's documentation.
No inconsistencies or errors were found in the construction of the JDBC URL.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the correctness of the JDBC URL formation. # Test: Search for the usage of `getJDBCUrl` in the codebase. Expect: Correct formation of JDBC URL. rg --type java $'getJDBCUrl'Length of output: 561
Script:
#!/bin/bash # Extract lines around the usage of `getJDBCUrl` to understand the context. # Print 5 lines before and after the first usage of `getJDBCUrl` rg --type java -A 5 -B 5 $'String jdbcUrl = getJDBCUrl(datasourceConfiguration);' app/server/appsmith-plugins/snowflakePlugin/src/main/java/com/external/plugins/SnowflakePlugin.java # Print 5 lines before and after the second usage of `getJDBCUrl` rg --type java -A 5 -B 5 $'String jdbcUrl = getJDBCUrl(datasourceConfiguration);' app/server/appsmith-plugins/snowflakePlugin/src/main/java/com/external/plugins/SnowflakePlugin.javaLength of output: 365
Script:
#!/bin/bash # Extract broader lines around the usage of `getJDBCUrl` to understand the context. # Print 10 lines before and after the first usage of `getJDBCUrl` rg --type java -A 10 -B 10 $'String jdbcUrl = getJDBCUrl(datasourceConfiguration);' app/server/appsmith-plugins/snowflakePlugin/src/main/java/com/external/plugins/SnowflakePlugin.java # Print 10 lines before and after the second usage of `getJDBCUrl` rg --type java -A 10 -B 10 $'String jdbcUrl = getJDBCUrl(datasourceConfiguration);' app/server/appsmith-plugins/snowflakePlugin/src/main/java/com/external/plugins/SnowflakePlugin.javaLength of output: 369
Script:
#!/bin/bash # Extract the entire content of SnowflakePlugin.java to understand the context and verify the JDBC URL formation. cat app/server/appsmith-plugins/snowflakePlugin/src/main/java/com/external/plugins/SnowflakePlugin.javaLength of output: 30979
297-297
: Verify the correctness of the validation logic for key-pair authentication.Ensure that the validation logic for key-pair authentication is correct and consistent with the rest of the codebase.
168-172
: Verify the correctness of the JDBC URL.Ensure that the JDBC URL is correctly set based on the datasource configuration.
Verification successful
The correctness of the JDBC URL is verified.
The
getJDBCUrl
method in theSnowflakePlugin
class constructs the JDBC URL using theurl
field from theDatasourceConfiguration
class. Theurl
field is directly accessed and used to form the JDBC URL correctly.
DatasourceConfiguration
class definition:app/server/appsmith-interfaces/src/main/java/com/appsmith/external/models/DatasourceConfiguration.java
getJDBCUrl
method definition:app/server/appsmith-plugins/snowflakePlugin/src/main/java/com/external/plugins/SnowflakePlugin.java
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the correctness of the JDBC URL. # Test: Search for the `getJDBCUrl` method implementation and usage in the codebase. Expect: Correct URL formation. rg --type java $'getJDBCUrl'Length of output: 561
Script:
#!/bin/bash # Extract the implementation of the `getJDBCUrl` method for detailed analysis. ast-grep --lang java --pattern $'private String getJDBCUrl(DatasourceConfiguration dsConfig) { $$$ }'Length of output: 904
Script:
#!/bin/bash # Search for the implementation of the `getUrl` method in the `DatasourceConfiguration` class. ast-grep --lang java --pattern $'class DatasourceConfiguration { $$$ String getUrl() { $$$ } $$$ }'Length of output: 112
Script:
#!/bin/bash # Search for the `DatasourceConfiguration` class and list its methods. ast-grep --lang java --pattern $'class DatasourceConfiguration { $$$ }' --debugLength of output: 300
Script:
#!/bin/bash # Search for the `DatasourceConfiguration` class and list its methods. ast-grep --lang java --pattern $'class DatasourceConfiguration { $$$ }' --debug-queryLength of output: 154
Script:
#!/bin/bash # Search for the `DatasourceConfiguration` class definition and inspect its contents. rg --type java 'class DatasourceConfiguration'Length of output: 216
Script:
#!/bin/bash # Extract the `getUrl` method from the `DatasourceConfiguration` class. ast-grep --lang java --pattern $'class DatasourceConfiguration { $$$ String getUrl() { $$$ } $$$ }' app/server/appsmith-interfaces/src/main/java/com/appsmith/external/models/DatasourceConfiguration.javaLength of output: 215
Script:
#!/bin/bash # List all methods in the `DatasourceConfiguration` class. ast-grep --lang java --pattern $'class DatasourceConfiguration { $$$ }' app/server/appsmith-interfaces/src/main/java/com/appsmith/external/models/DatasourceConfiguration.javaLength of output: 177
Script:
#!/bin/bash # Display the contents of the `DatasourceConfiguration.java` file. cat app/server/appsmith-interfaces/src/main/java/com/appsmith/external/models/DatasourceConfiguration.javaLength of output: 1540
} catch (HikariPool.PoolInitializationException e) { | ||
throw new AppsmithPluginException( | ||
AppsmithPluginError.PLUGIN_DATASOURCE_ARGUMENT_ERROR, e.getMessage()); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Improve exception handling for Hikari pool initialization.
Consider providing more context in the exception message or using a custom exception to improve error handling.
- throw new AppsmithPluginException(
- AppsmithPluginError.PLUGIN_DATASOURCE_ARGUMENT_ERROR, e.getMessage());
+ throw new CustomHikariPoolException(
+ AppsmithPluginError.PLUGIN_DATASOURCE_ARGUMENT_ERROR, "Error initializing Hikari pool: " + e.getMessage());
Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
} catch (HikariPool.PoolInitializationException e) { | |
throw new AppsmithPluginException( | |
AppsmithPluginError.PLUGIN_DATASOURCE_ARGUMENT_ERROR, e.getMessage()); | |
} | |
} catch (HikariPool.PoolInitializationException e) { | |
throw new CustomHikariPoolException( | |
AppsmithPluginError.PLUGIN_DATASOURCE_ARGUMENT_ERROR, "Error initializing Hikari pool: " + e.getMessage()); | |
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- app/server/appsmith-plugins/snowflakePlugin/src/main/java/com/external/plugins/SnowflakePlugin.java (6 hunks)
Additional context used
Learnings (1)
app/server/appsmith-plugins/snowflakePlugin/src/main/java/com/external/plugins/SnowflakePlugin.java (1)
Learnt from: AmanAgarwal041 PR: appsmithorg/appsmith#34491 File: app/server/appsmith-plugins/snowflakePlugin/src/main/java/com/external/plugins/SnowflakePlugin.java:320-338 Timestamp: 2024-06-26T11:16:09.077Z Learning: The `encryptedPassphrase` field in the `KeyPairAuth` class for Snowflake authentication is optional and does not require validation for emptiness.
Additional comments not posted (6)
app/server/appsmith-plugins/snowflakePlugin/src/main/java/com/external/plugins/SnowflakePlugin.java (6)
205-213
: LGTM!The code changes correctly handle different authentication types.
Line range hint
287-299
: LGTM!The code changes correctly validate different authentication types and required fields.
462-482
: LGTM!The code changes correctly handle different authentication types and return a
Mono<HikariConfig>
.
484-514
: LGTM!The code changes correctly handle the configuration for key-pair authentication and handle errors using
Mono
.
516-528
: LGTM!The code changes correctly handle the configuration for basic authentication and return a
Mono<HikariConfig>
.
545-561
: LGTM!The code changes correctly handle the reading of encrypted private keys and return a
Mono<PrivateKey>
.
return getHikariConfig(datasourceConfiguration, properties) | ||
.flatMap(config -> Mono.fromCallable(() -> { | ||
// Set up the connection URL | ||
String jdbcUrl = getJDBCUrl(datasourceConfiguration); | ||
config.setJdbcUrl(jdbcUrl); | ||
|
||
config.setDataSourceProperties(properties); | ||
config.setDataSourceProperties(properties); | ||
|
||
// Now create the connection pool from the configuration | ||
HikariDataSource datasource = null; | ||
try { | ||
datasource = new HikariDataSource(config); | ||
} catch (HikariPool.PoolInitializationException e) { | ||
throw new AppsmithPluginException( | ||
AppsmithPluginError.PLUGIN_DATASOURCE_ARGUMENT_ERROR, e.getMessage()); | ||
} | ||
// Now create the connection pool from the configuration | ||
HikariDataSource datasource = null; | ||
try { | ||
datasource = new HikariDataSource(config); | ||
} catch (HikariPool.PoolInitializationException e) { | ||
throw new AppsmithPluginException( | ||
AppsmithPluginError.PLUGIN_DATASOURCE_ARGUMENT_ERROR, e.getMessage()); | ||
} | ||
|
||
return datasource; | ||
}) | ||
.subscribeOn(scheduler); | ||
return datasource; | ||
}) | ||
.subscribeOn(scheduler)) | ||
.onErrorMap( | ||
AppsmithPluginException.class, | ||
error -> new AppsmithPluginException( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Improve error handling for Hikari pool initialization.
Consider providing more context in the exception message or using a custom exception to improve error handling.
- throw new AppsmithPluginException(
- AppsmithPluginError.PLUGIN_DATASOURCE_ARGUMENT_ERROR, e.getMessage());
+ throw new CustomHikariPoolException(
+ AppsmithPluginError.PLUGIN_DATASOURCE_ARGUMENT_ERROR, "Error initializing Hikari pool: " + e.getMessage());
Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
return getHikariConfig(datasourceConfiguration, properties) | |
.flatMap(config -> Mono.fromCallable(() -> { | |
// Set up the connection URL | |
String jdbcUrl = getJDBCUrl(datasourceConfiguration); | |
config.setJdbcUrl(jdbcUrl); | |
config.setDataSourceProperties(properties); | |
config.setDataSourceProperties(properties); | |
// Now create the connection pool from the configuration | |
HikariDataSource datasource = null; | |
try { | |
datasource = new HikariDataSource(config); | |
} catch (HikariPool.PoolInitializationException e) { | |
throw new AppsmithPluginException( | |
AppsmithPluginError.PLUGIN_DATASOURCE_ARGUMENT_ERROR, e.getMessage()); | |
} | |
// Now create the connection pool from the configuration | |
HikariDataSource datasource = null; | |
try { | |
datasource = new HikariDataSource(config); | |
} catch (HikariPool.PoolInitializationException e) { | |
throw new AppsmithPluginException( | |
AppsmithPluginError.PLUGIN_DATASOURCE_ARGUMENT_ERROR, e.getMessage()); | |
} | |
return datasource; | |
}) | |
.subscribeOn(scheduler); | |
return datasource; | |
}) | |
.subscribeOn(scheduler)) | |
.onErrorMap( | |
AppsmithPluginException.class, | |
error -> new AppsmithPluginException( | |
return getHikariConfig(datasourceConfiguration, properties) | |
.flatMap(config -> Mono.fromCallable(() -> { | |
// Set up the connection URL | |
String jdbcUrl = getJDBCUrl(datasourceConfiguration); | |
config.setJdbcUrl(jdbcUrl); | |
config.setDataSourceProperties(properties); | |
// Now create the connection pool from the configuration | |
HikariDataSource datasource = null; | |
try { | |
datasource = new HikariDataSource(config); | |
} catch (HikariPool.PoolInitializationException e) { | |
throw new CustomHikariPoolException( | |
AppsmithPluginError.PLUGIN_DATASOURCE_ARGUMENT_ERROR, "Error initializing Hikari pool: " + e.getMessage()); | |
} | |
return datasource; | |
}) | |
.subscribeOn(scheduler)) | |
.onErrorMap( | |
AppsmithPluginException.class, | |
error -> new AppsmithPluginException( |
Failed server tests
|
/build-deploy-preview skip-tests=true recreate=true |
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/9741684547. |
Deploy-Preview-URL: https://ce-34548.dp.appsmith.com |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changes LGTM. Also tested on DP
## Description This PR adds new authentication method for snowflake: Use of public and private keys to authorise a snowflake database. Snowflake provides a couple of authentication mechanisms in order to authenticate the DB. Currently appsmith only provides a way to authorise using username and password. This PR adds support for private key authentication, where by user can set a public key on their DB and they can use corresponding private key in appsmith to authorise the datasource. In snowflake DB form, we have added a new field for authentication type which has two options: Basic and Key pair. Basic will allows us to authenticate using username and password. Key pair will provide us a way to upload the private key along with input field for adding passphrase (In case of encrypted private key). More info: https://docs.snowflake.com/en/user-guide/key-pair-auth https://github.com/appsmithorg/appsmith/assets/30018882/99774925-12a3-4cc0-af0a-614c3574cdc3 Fixes #`Issue Number` _or_ Fixes `Issue URL` > [!WARNING] > _If no issue exists, please create an issue first, and check with the maintainers if the issue is valid._ ## Automation /ok-to-test tags="@tag.Datasource" ### 🔍 Cypress test results <!-- This is an auto-generated comment: Cypress test results --> > [!TIP] > 🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉 > Workflow run: <https://github.com/appsmithorg/appsmith/actions/runs/9743830603> > Commit: cb64fff > <a href="https://internal.appsmith.com/app/cypress-dashboard/rundetails-65890b3c81d7400d08fa9ee5?branch=master&workflowId=9743830603&attempt=1" target="_blank">Cypress dashboard</a>. > Tags: `@tag.Datasource` <!-- end of auto-generated comment: Cypress test results --> ## Communication Should the DevRel and Marketing teams inform users about this change? - [ ] Yes - [ ] No <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **New Features** - Enhanced Snowflake integration with improved connection creation and authentication handling (key pair and basic auth types). - **Bug Fixes** - Fixed issues related to handling authentication objects within datasource configuration to ensure more reliable connections. - **Documentation** - Added new error messages for passphrase requirements and incorrect passphrase or private key for encrypted private keys. - **Tests** - Updated tests to verify the correct setting of authentication types in the Snowflake plugin. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Co-authored-by: Rishabh-Rathod <rishabh.rathod@appsmith.com> Co-authored-by: Aman Agarwal <aman@appsmith.com> Co-authored-by: “sneha122” <“sneha@appsmith.com”>
## Description Provide a concise summary of the changes made in this pull request - ## Pull request type Check the appropriate box: - [ ] Review Fixes - [ ] Documentation Overhaul - [x] Feature/Story - Link one or more Engineering Tickets * appsmithorg/appsmith#34548 - [ ] A-Force - [ ] Error in documentation - [ ] Maintenance ## Documentation tickets Link to one or more documentation tickets: - ## Checklist From the below options, select the ones that are applicable: - [ ] Checked for Grammarly suggestions. - [ ] Adhered to the writing checklist. - [ ] Adhered to the media checklist. - [ ] Verified and updated cross-references or added redirect rules. - [ ] Tested the redirect rules on deploy preview. - [ ] Validated the modifications made to the content on the deploy preview. - [ ] Validated the CSS modifications on different screen sizes.
Description
This PR adds new authentication method for snowflake: Use of public and private keys to authorise a snowflake database.
Snowflake provides a couple of authentication mechanisms in order to authenticate the DB. Currently appsmith only provides a way to authorise using username and password. This PR adds support for private key authentication, where by user can set a public key on their DB and they can use corresponding private key in appsmith to authorise the datasource.
In snowflake DB form, we have added a new field for authentication type which has two options: Basic and Key pair. Basic will allows us to authenticate using username and password. Key pair will provide us a way to upload the private key along with input field for adding passphrase (In case of encrypted private key).
More info: https://docs.snowflake.com/en/user-guide/key-pair-auth
Screen.Recording.2024-07-01.at.1.16.05.PM.mov
Fixes #
Issue Number
or
Fixes
Issue URL
Warning
If no issue exists, please create an issue first, and check with the maintainers if the issue is valid.
Automation
/ok-to-test tags="@tag.Datasource"
🔍 Cypress test results
Tip
🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/9743830603
Commit: cb64fff
Cypress dashboard.
Tags:
@tag.Datasource
Communication
Should the DevRel and Marketing teams inform users about this change?
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Tests