feat: snowflake key pair #4781
feat: snowflake key pair #4781
Conversation
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThe updates primarily focus on enhancing the handling of test credentials across various integrations by switching to Changes
Sequence Diagram(s)sequenceDiagram
participant Test as Test Suite
participant Env as Environment
participant Snowflake as Snowflake Integration
participant Utils as Utils
Test->>Env: os.LookupEnv("TEST_KEY")
Env-->>Test: Return value and existence status
Test->>Snowflake: Initialize with config
Snowflake->>Utils: ParsePrivateKey(privateKey, passPhrase)
Utils-->>Snowflake: Return parsed key
Snowflake-->>Test: Snowflake initialized with key pair auth
Poem
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 (
|
achettyiitr
force-pushed
the
feat.snowflake-key-pair
branch
from
57c34e6 to
25d191f
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #4781 +/- ##
==========================================
- Coverage 74.68% 73.28% -1.40%
==========================================
Files 414 414
Lines 48657 48680 +23
==========================================
- Hits 36341 35677 -664
- Misses 9932 10663 +731
+ Partials 2384 2340 -44 ☔ View full report in Codecov by Sentry. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Outside diff range and nitpick comments (2)
warehouse/integrations/snowflake/snowflake_test.go (2)
48-59: Ensure the new fields intestCredentialsstruct are properly documented.Adding comments to explain the purpose of each new field, especially
UseKeyPairAuth,PrivateKey, andPrivateKeyPassphrase, would enhance maintainability and clarity for other developers.
1130-1133: Ensure configuration settings are validated.Consider adding validation for the configuration settings related to
appendOnlyTablesto ensure they meet expected formats or constraints.
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
Files selected for processing (11)
- go.mod (1 hunks)
- warehouse/integrations/bigquery/bigquery_test.go (2 hunks)
- warehouse/integrations/bigquery/middleware/middleware_test.go (2 hunks)
- warehouse/integrations/bigquery/testhelper/setup.go (1 hunks)
- warehouse/integrations/deltalake/deltalake_test.go (2 hunks)
- warehouse/integrations/manager/manager.go (3 hunks)
- warehouse/integrations/redshift/redshift_test.go (1 hunks)
- warehouse/integrations/snowflake/snowflake.go (9 hunks)
- warehouse/integrations/snowflake/snowflake_test.go (19 hunks)
- warehouse/integrations/snowflake/testdata/template.json (1 hunks)
- warehouse/utils/utils.go (2 hunks)
Files skipped from review due to trivial changes (2)
- go.mod
- warehouse/integrations/bigquery/testhelper/setup.go
Additional context used
Biome
warehouse/integrations/snowflake/testdata/template.json
[error] 28-28: Expected a property but instead found '{'. (parse)
Expected a property here.
[error] 28-28: Expected a property but instead found '{'. (parse)
Expected a property here.
[error] 28-28: unexpected character
.(parse)
[error] 29-29: expected
,but instead found"account"(parse)Remove "account"
[error] 153-153: Expected a property but instead found '{'. (parse)
Expected a property here.
[error] 153-153: Expected a property but instead found '{'. (parse)
Expected a property here.
[error] 153-153: unexpected character
.(parse)
[error] 154-154: expected
,but instead found"account"(parse)Remove "account"
[error] 278-278: Expected a property but instead found '{'. (parse)
Expected a property here.
[error] 278-278: Expected a property but instead found '{'. (parse)
Expected a property here.
[error] 278-278: unexpected character
.(parse)
[error] 279-279: expected
,but instead found"account"(parse)Remove "account"
[error] 405-405: Expected a property but instead found '{'. (parse)
Expected a property here.
[error] 405-405: Expected a property but instead found '{'. (parse)
Expected a property here.
[error] 405-405: unexpected character
.(parse)
[error] 406-406: expected
,but instead found"account"(parse)Remove "account"
[error] 531-531: Expected a property but instead found '{'. (parse)
Expected a property here.
[error] 531-531: Expected a property but instead found '{'. (parse)
Expected a property here.
[error] 531-531: unexpected character
.(parse)
[error] 532-532: expected
,but instead found"account"(parse)Remove "account"
Additional comments not posted (16)
warehouse/integrations/bigquery/middleware/middleware_test.go (1)
22-22: LGTM! Ensure the environment variable is correctly set in the test setup.warehouse/integrations/bigquery/bigquery_test.go (2)
Line range hint
1-1: The change integrates well with the existing test setup, enhancing simplicity and maintainability.
46-46: Simplified environment variable check usingos.LookupEnv.Verification successful
The usage of
bqHelper.TestKeyis consistent across the test suite, and the change to useos.LookupEnvis correctly implemented.
warehouse/integrations/bigquery/bigquery_test.gowarehouse/integrations/bigquery/middleware/middleware_test.goScripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that `bqHelper.TestKey` is consistently used across the test suite. # Test: Search for the usage of `bqHelper.TestKey`. Expect: Consistent usage across the test suite. rg --type go $'bqHelper.TestKey'Length of output: 511
warehouse/integrations/deltalake/deltalake_test.go (3)
74-74: Refactor to useos.LookupEnvfor environment variable checks.This change simplifies the check for environment variables by directly using
os.LookupEnv, which is a more idiomatic way in Go to check for the existence of an environment variable. Good job on making the test setup cleaner and more straightforward.
74-74: Utilize helper functions to reduce redundancy.The introduction of
mergeEventsMapandappendEventsMapfunctions is a good practice as it helps in reducing redundancy and makes the test cases cleaner and more maintainable. It's always beneficial to abstract repetitive data into utility functions.
74-74: Ensure comprehensive testing.This change appears to handle the test setup and execution well. However, ensure that all necessary scenarios are covered in the tests, especially considering the complexity and critical nature of the Delta Lake integration.
Verification successful
The output shows three test functions in the
deltalake_test.gofile:
TestIntegrationTestDeltalake_TrimErrorMessageTestDeltalake_ShouldMergeTo ensure comprehensive testing, we need to verify the specific scenarios covered within each of these test functions.
The extracted content of the test functions shows that the
TestIntegration,TestDeltalake_TrimErrorMessage, andTestDeltalake_ShouldMergefunctions cover a wide range of scenarios for the Delta Lake integration. These include various configurations, error handling, and specific functionalities like merging and appending data.The
TestIntegrationfunction, in particular, includes multiple sub-tests that cover different modes (e.g., Merge Mode, Append Mode, Parquet load files) and edge cases (e.g., schema mismatch, missing load files). This indicates a comprehensive approach to testing the integration.Given this detailed coverage, it appears that the necessary scenarios are well-covered in the tests.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that all necessary test cases are covered. # Test: Search for test cases in the file. Expect: Comprehensive coverage of scenarios. rg --type go 'func Test' warehouse/integrations/deltalake/deltalake_test.goLength of output: 213
Script:
#!/bin/bash # Description: Extract the content of the test functions to verify the scenarios covered. # Extract the content of the TestIntegration function ast-grep --lang go --pattern $'func TestIntegration($_) { $$$ }' warehouse/integrations/deltalake/deltalake_test.go # Extract the content of the TestDeltalake_TrimErrorMessage function ast-grep --lang go --pattern $'func TestDeltalake_TrimErrorMessage($_) { $$$ }' warehouse/integrations/deltalake/deltalake_test.go # Extract the content of the TestDeltalake_ShouldMerge function ast-grep --lang go --pattern $'func TestDeltalake_ShouldMerge($_) { $$$ }' warehouse/integrations/deltalake/deltalake_test.goLength of output: 96915
warehouse/integrations/redshift/redshift_test.go (2)
77-77: Simplify test credential checks using environment variables.Consider adding a comment explaining why this environment variable check is necessary, for clarity.
77-77: The test setup and execution are comprehensive and cover a wide range of scenarios.No further action needed.
warehouse/integrations/snowflake/snowflake.go (7)
21-21: Added dependency ongit.luolix.top/youmark/pkcs8for handling PKCS8 keys.This is consistent with the PR's objective to support key-pair authentication for Snowflake.
50-52: Introduced new constants for key-pair authentication.These constants are essential for the new authentication method's configuration.
136-146: Updated thecredentialsstruct to include fields for key-pair authentication.This change is necessary to support the new authentication method and is well-integrated with the existing structure.
Line range hint
191-211: Refactored theNewfunction to initialize new fields related to key-pair authentication.The function correctly initializes the new fields based on the configuration, aligning with the PR's objectives.
1078-1089: Implemented conditional logic for key-pair authentication in theconnectmethod.This implementation correctly handles both key-pair and traditional password authentication based on the configuration.
1130-1146: Added a new functionParsePrivateKeyto handle the parsing of private keys.This function is crucial for the key-pair authentication feature, handling both encrypted and unencrypted keys appropriately.
1376-1386: Enhanced thegetConnectionCredentialsmethod to support key-pair authentication.The method now fetches and sets all necessary parameters for both authentication methods, ensuring flexibility and security.
warehouse/integrations/snowflake/snowflake_test.go (1)
445-454: Validate the logic for setting authentication types based onUseKeyPairAuth.This script checks if the conditional logic for setting the
Authenticatoris consistently applied across the codebase.Verification successful
The search results indicate that the conditional logic for setting the
Authenticatorbased on theUseKeyPairAuthflag is present in thewarehouse/integrations/snowflake/snowflake_test.gofile. However, to ensure that this logic is correctly implemented and consistently applied, we need to verify the actual implementation details.Let's gather more information by examining the surrounding code to confirm that the
Authenticatoris set correctly based on theUseKeyPairAuthflag.
The extracted code confirms that the
Authenticatoris set correctly based on theUseKeyPairAuthflag. WhenUseKeyPairAuthis true, theAuthenticatoris set tosfdb.AuthTypeJwt, and when false, it is set tosfdb.AuthTypeSnowflake.
- The logic for setting the
Authenticatoris correctly implemented in thewarehouse/integrations/snowflake/snowflake_test.gofile.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that the authentication type is set correctly based on the `UseKeyPairAuth` flag. # Test: Search for the function usage. Expect: Correct setting of `Authenticator` based on the flag. rg --type go $'if tc.cred.UseKeyPairAuth {'Length of output: 162
Script:
#!/bin/bash # Description: Verify that the authentication type is set correctly based on the `UseKeyPairAuth` flag. # Test: Search for the function usage. Expect: Correct setting of `Authenticator` based on the flag. rg --type go 'if tc\.cred\.UseKeyPairAuth \{'Length of output: 130
Script:
#!/bin/bash # Description: Verify the implementation details of the conditional logic for setting the `Authenticator`. # Extract the surrounding code to inspect the implementation details. rg --context 10 'if tc\.cred\.UseKeyPairAuth \{' warehouse/integrations/snowflake/snowflake_test.goLength of output: 743
achettyiitr
force-pushed
the
feat.snowflake-key-pair
branch
from
25d191f to
ed719f3
Compare
We do have a simple test for validations in here, but I don't think we need to add these scenarios for validations since we are already capturing it in |
I was just checking if the code changes handle the validations that are triggered from UI. or do we need any additional changes in the grpc calls via cp router. |
The only change is in the integration config. No changes in web-app, config-backend, cp-router. |
Yes. Integration config changes need to go post stable rudder server release. |
Description
Linear Ticket
Security
Summary by CodeRabbit
Refactor
New Features
Chores
github.com/youmark/pkcs8.Tests