feat: ssh config for RS and POSTGRES destinations #1751
Conversation
vamsikrishnakandi
requested review from
lvrach,
cisse21,
debanjan97,
am6010,
ssbeefeater,
AchuthaSourabhC,
ruchiramoitra,
ItsSudip,
shrouti1507,
sandeepdsvs,
krishna2020 and
1abhishekpandey
as code owners
WalkthroughThe changes introduced in this pull request enhance PostgreSQL and RS destination configurations by adding a new boolean property Changes
Suggested reviewers
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 using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #1751 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 2 2
Lines 53 53
Branches 7 7
=========================================
Hits 53 53 ☔ 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 (7)
src/configurations/destinations/rs/ui-config.json (1)
177-190: LGTM: SSH Host field added correctly with a minor suggestionThe SSH Host field is well-implemented with appropriate prerequisites and feature flag control. However, consider updating the regex pattern to enforce a more strict hostname format.
Consider updating the regex pattern to enforce a stricter hostname format:
- "regex": "^(.{0,100})$", + "regex": "^(([a-zA-Z0-9]|[a-zA-Z0-9][a-zA-Z0-9\\-]*[a-zA-Z0-9])\\.)*([A-Za-z0-9]|[A-Za-z0-9][A-Za-z0-9\\-]*[A-Za-z0-9])$",This pattern ensures a valid hostname format while still allowing IP addresses.
test/data/validation/destinations/postgres.json (3)
Line range hint
443-448: LGTM! SSH configuration added successfully.The new SSH configuration has been implemented correctly, aligning with the PR objectives. This addition allows for SSH tunneling in PostgreSQL destinations.
Consider adding a comment explaining the purpose of this test case to improve readability and maintainability.
Line range hint
449-1000: LGTM! Comprehensive consent management test cases added.The new test cases for consent management are well-structured and cover various scenarios, including:
- Multiple consent management providers
- Custom provider with valid and invalid configurations
- OneTrust and Ketch specific configurations
- Various platform-specific settings (android, ios, web, etc.)
These additions enhance the test coverage and align with the PR objectives.
Consider adding a test case for a scenario where multiple consent management providers are configured with conflicting settings to ensure the system handles such cases gracefully.
Line range hint
1-1000: Overall, excellent additions to the test suite.The changes in this file significantly enhance the test coverage for PostgreSQL and other destination configurations. Key improvements include:
- Implementation of SSH configuration tests, allowing for validation of SSH tunneling functionality.
- Comprehensive test cases for consent management, covering multiple providers and scenarios.
These additions align well with the PR objectives and will help ensure the robustness of the SSH and consent management features.
As the test suite grows, consider organizing the test cases into separate files or sections for better maintainability. For example, you could have separate files for SSH configuration tests and consent management tests.
src/configurations/destinations/postgres/ui-config.json (1)
70-83: Consider using a more specific regex for SSH Host validation.The SSH Host field is well-implemented, but the current regex
^(.{0,100})$allows any string up to 100 characters. Consider using a more specific regex to validate hostnames, such as:^(([a-zA-Z0-9]|[a-zA-Z0-9][a-zA-Z0-9\-]*[a-zA-Z0-9])\.)*([A-Za-z0-9]|[A-Za-z0-9][A-Za-z0-9\-]*[A-Za-z0-9])$This regex will ensure that only valid hostnames or IP addresses are entered.
src/configurations/destinations/rs/schema.json (1)
1070-1113: LGTM: SSH configuration conditional logicThe addition of conditional logic for SSH configuration is well-implemented. It correctly requires the necessary SSH-related properties only when
useSSHis set to true. The patterns forsshHost,sshPort, andsshUserare appropriate.One minor suggestion:
Consider adding a pattern for thesshPublicKeyto ensure it matches the expected format of an SSH public key. For example:"sshPublicKey": { "type": "string", "pattern": "^(ssh-rsa|ssh-dss|ecdsa-[a-zA-Z0-9-]+|ssh-ed25519)\\s+[A-Za-z0-9+/]+[=]{0,3}(\\s+.+)?$" }This pattern would validate the general structure of SSH public keys.
src/configurations/destinations/postgres/schema.json (1)
1160-1189: Approve SSH configuration block with a suggestion for improvementThe new conditional block for SSH configuration is well-structured and aligns with the PR objective. It correctly defines the additional required properties when
useSSHis true. However, there's a minor issue that could be improved:The patterns for
sshHost,sshPort, andsshUser(^(.{0,100})$) allow for empty strings. Since these are required fields when SSH is enabled, it might be better to ensure they contain at least one character.Consider updating the patterns to
^(.{1,100})$forsshHost,sshPort, andsshUserto ensure they are not empty when required. Here's the suggested change:"sshHost": { "type": "string", - "pattern": "^(.{0,100})$" + "pattern": "^(.{1,100})$" }, "sshPort": { "type": "string", - "pattern": "^(.{0,100})$" + "pattern": "^(.{1,100})$" }, "sshUser": { "type": "string", - "pattern": "^(.{0,100})$" + "pattern": "^(.{1,100})$" },This change ensures that these fields contain at least one character when SSH is enabled.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
- src/configurations/destinations/postgres/schema.json (2 hunks)
- src/configurations/destinations/postgres/ui-config.json (1 hunks)
- src/configurations/destinations/rs/schema.json (2 hunks)
- src/configurations/destinations/rs/ui-config.json (1 hunks)
- test/data/validation/destinations/postgres.json (1 hunks)
🧰 Additional context used
🔇 Additional comments (8)
src/configurations/destinations/rs/ui-config.json (2)
169-176: LGTM: SSH Connection checkbox added correctlyThe new SSH Connection checkbox is well-implemented with appropriate default value and feature flag control.
169-231: Summary: SSH configuration fields added successfully with minor improvements neededThe new SSH configuration fields for the RS destination have been successfully implemented. They align well with the PR objectives and are appropriately controlled by the "WAREHOUSE_SSH_TUNNELLING" feature flag. A few minor improvements have been suggested:
- Consider using a stricter regex pattern for the SSH Host field.
- Update the regex pattern for the SSH Port field to ensure valid port numbers.
- Fix the error message for the SSH User field.
- Clarify the process for populating the SSH Public Key field.
Overall, these changes enhance the functionality of the RS destination configuration without introducing breaking changes.
src/configurations/destinations/postgres/ui-config.json (2)
62-69: LGTM: SSH Connection checkbox field is well-implemented.The new checkbox field for enabling SSH connection is correctly implemented with appropriate labeling, default value, and feature flag control. This aligns well with the PR objectives of implementing SSH configuration rendering for PostgreSQL destinations.
112-124: LGTM: SSH Public Key field is correctly implemented.The SSH Public Key field is well-implemented as a read-only textarea. This is appropriate for a public key that should be generated and not manually entered by the user. The field is correctly tied to the SSH connection checkbox and feature flag.
src/configurations/destinations/rs/schema.json (2)
27-30: LGTM: Addition ofuseSSHpropertyThe addition of the
useSSHproperty as a boolean with a default value of false is appropriate. This change aligns with the PR objective of implementing SSH configuration for RS destinations while maintaining backward compatibility.
Line range hint
27-1113: Summary: SSH configuration for RS destination successfully implementedThe changes in this file successfully implement SSH configuration options for the RS (Redshift) destination. Key points:
- The
useSSHproperty allows users to opt-in to SSH tunneling.- Conditional logic ensures that SSH-related properties are only required when SSH is enabled.
- The implementation maintains backward compatibility with existing configurations.
- The changes align well with the PR objectives and enhance the flexibility of the RS destination.
These modifications appear to be well-structured and do not introduce any breaking changes or security issues.
src/configurations/destinations/postgres/schema.json (2)
48-51: LGTM: Addition ofuseSSHpropertyThe addition of the
useSSHproperty is well-implemented. It's a boolean type with a default value of false, which ensures backward compatibility with existing configurations. This change aligns with the PR objective of implementing SSH configuration for PostgreSQL destinations.
Line range hint
1-1189: Summary: Well-implemented SSH configuration supportThe changes to the PostgreSQL configuration schema successfully implement SSH support as per the PR objectives. The addition of the
useSSHproperty and the conditional block for SSH-related properties are well-structured and maintain backward compatibility with existing configurations.Key points:
- The
useSSHproperty provides a clear way to enable/disable SSH functionality.- SSH-related properties are only required when
useSSHis true, ensuring flexibility in configuration.- The implementation doesn't affect other parts of the schema, maintaining compatibility with existing setups.
The suggested improvement for the patterns of
sshHost,sshPort, andsshUseris minor and can be addressed to further enhance the robustness of the schema.Overall, these changes effectively add SSH configuration capabilities to the PostgreSQL destination while maintaining the integrity of the existing schema structure.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
src/configurations/destinations/rs/ui-config.json (1)
177-190: Consider improving the regex pattern for SSH HostThe SSH Host field is well-implemented with appropriate prerequisites and feature flag control. However, the current regex pattern is too permissive for a hostname.
Consider updating the regex pattern to enforce a more specific hostname format:
- "regex": "^(.{0,100})$", + "regex": "^(([a-zA-Z0-9]|[a-zA-Z0-9][a-zA-Z0-9\\-]*[a-zA-Z0-9])\\.)*([A-Za-z0-9]|[A-Za-z0-9][A-Za-z0-9\\-]*[A-Za-z0-9])$",This pattern ensures a valid hostname format while still allowing IP addresses.
src/configurations/destinations/postgres/ui-config.json (2)
70-83: Consider using a more specific regex for SSH Host validation.The SSH Host field is well-implemented, but the current regex
^(.{0,100})$allows any string up to 100 characters. Consider using a more specific regex for hostname validation:-"regex": "^(.{0,100})$", +"regex": "^(([a-zA-Z0-9]|[a-zA-Z0-9][a-zA-Z0-9\-]*[a-zA-Z0-9])\.)*([A-Za-z0-9]|[A-Za-z0-9][A-Za-z0-9\-]*[A-Za-z0-9])$",This regex will ensure that only valid hostnames are entered.
112-124: Consider adding regex validation for SSH Public Key.The SSH Public Key field is well-implemented, but it lacks a regex validation for the public key format. Consider adding a regex to ensure that only valid SSH public keys are entered:
+"regex": "^(ssh-rsa AAAAB3NzaC1yc2|ecdsa-sha2-nistp256 AAAAE2VjZHNhLXNoYTItbmlzdHAyNT|ecdsa-sha2-nistp384 AAAAE2VjZHNhLXNoYTItbmlzdHAzOD|ecdsa-sha2-nistp521 AAAAE2VjZHNhLXNoYTItbmlzdHA1Mj|ssh-ed25519 AAAAC3NzaC1lZDI1NTE5|ssh-dss AAAAB3NzaC1kc3)[0-9A-Za-z+/]+[=]{0,3}( .*)?$", +"regexErrorMessage": "Invalid SSH Public Key format",This regex will ensure that only valid SSH public key formats are accepted.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- src/configurations/destinations/postgres/ui-config.json (1 hunks)
- src/configurations/destinations/rs/ui-config.json (1 hunks)
🧰 Additional context used
🔇 Additional comments (9)
src/configurations/destinations/rs/ui-config.json (5)
169-176: LGTM: SSH Connection checkbox implemented correctlyThe SSH Connection checkbox is well-implemented with appropriate labeling, default value, and feature flag control.
191-204: Update regex pattern for SSH Port as previously suggestedThe SSH Port field is well-implemented with appropriate prerequisites and feature flag control. As mentioned in a previous comment, the regex pattern should be updated to enforce a valid port number range.
Please apply the following change:
- "regex": "^(.{0,100})$", + "regex": "^([1-9][0-9]{0,4})$",This pattern ensures the port number is between 1 and 65535.
205-218: Fix regexErrorMessage for SSH User as previously suggestedThe SSH User field is well-implemented with appropriate prerequisites and feature flag control. As mentioned in a previous comment, there's a minor error in the regexErrorMessage that needs to be fixed.
Please apply the following change:
- "regexErrorMessage": "Invalid Port", + "regexErrorMessage": "Invalid User",
219-231: LGTM: SSH Public Key field implemented correctlyThe SSH Public Key field is well-implemented with appropriate prerequisites, feature flag control, and read-only setting. The developer has clarified that the web application handles populating this field, which explains why it's both required and read-only.
169-231: Overall: SSH connection configuration well-implementedThe addition of SSH connection fields to the Redshift destination configuration is well-implemented and follows existing patterns. The new fields are appropriately controlled by the
WAREHOUSE_SSH_TUNNELLINGfeature flag and have correct prerequisites.A few minor improvements were suggested:
- More specific regex pattern for the SSH Host field.
- Updated regex pattern for the SSH Port field to enforce valid port numbers.
- Fixed regexErrorMessage for the SSH User field.
These changes enhance the robustness of the configuration and improve user experience by providing more accurate validation and error messages.
src/configurations/destinations/postgres/ui-config.json (4)
62-69: LGTM: SSH Connection checkbox implemented correctly.The SSH Connection checkbox is well-implemented with appropriate labeling, default value, and feature flag control.
84-97: Skipping comment on SSH Port field.A previous review comment has already addressed the need to update the regex for SSH Port validation. That comment is still valid and applicable.
98-111: Skipping comment on SSH User field.A previous review comment has already addressed the need to fix the regexErrorMessage for the SSH User field. That comment is still valid and applicable.
62-124: Summary: SSH configuration for PostgreSQL implemented successfully.The implementation of SSH configuration rendering for PostgreSQL destinations has been completed successfully. The changes align well with the PR objectives:
- New fields for SSH connection settings have been added.
- The fields are controlled by the
WAREHOUSE_SSH_TUNNELLINGfeature flag.- Proper prerequisites ensure the SSH fields are only shown when SSH is enabled.
Minor improvements have been suggested for regex validations to enhance input validation and user experience. Overall, the changes effectively eliminate the need for UI configuration in the web application for PostgreSQL destinations with SSH tunneling enabled.
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Outside diff range and nitpick comments (1)
src/configurations/destinations/postgres/ui-config.json (1)
70-82: Consider using a more specific regex for SSH Host validation.While the current implementation is functional, using a more specific regex for hostname validation could improve data integrity. Consider using a regex that validates hostnames more strictly, such as:
^(([a-zA-Z0-9]|[a-zA-Z0-9][a-zA-Z0-9\-]*[a-zA-Z0-9])\.)*([A-Za-z0-9]|[A-Za-z0-9][A-Za-z0-9\-]*[A-Za-z0-9])$This regex will ensure that only valid hostnames are entered.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- src/configurations/destinations/postgres/ui-config.json (1 hunks)
- src/configurations/destinations/rs/ui-config.json (1 hunks)
🧰 Additional context used
🔇 Additional comments (4)
src/configurations/destinations/rs/ui-config.json (2)
169-176: LGTM: SSH Connection checkbox implemented correctlyThe SSH Connection checkbox is well-implemented with appropriate feature flag control and a safe default value.
216-227: LGTM: SSH Public Key field implemented correctlyThe SSH Public Key field is well-implemented as a read-only, required field. This is appropriate for a public key that should be generated and managed by the system.
src/configurations/destinations/postgres/ui-config.json (2)
62-69: LGTM: SSH Connection checkbox implemented correctly.The SSH Connection checkbox is well-implemented with the correct use of
preRequisiteFeatureFlagas suggested in the previous review.
109-120: LGTM: SSH Public Key field implemented correctly.The SSH Public Key field is well-implemented as a read-only textarea input. This is appropriate for displaying public keys that are typically generated elsewhere.
What are the changes introduced in this PR?
Rendering SSH config for RS and POSTGRES destinations from UI config from integrations. This is controlled by
WAREHOUSE_SSH_TUNNELLINGfeature flag which is validated by web-app to render the fields.What is the related Linear task?
Part of PIPE-1611
Please explain the objectives of your changes below
We can to remove UI config in web-app for SSH Tunnelling enabled destinations (RS and POSTGRES)
Any changes to existing capabilities/behaviour, mention the reason & what are the changes ?
N/A
Any new dependencies introduced with this change?
N/A
Any new checks got introduced or modified in test suites. Please explain the changes.
N/A
Developer checklist
My code follows the style guidelines of this project
No breaking changes are being introduced.
All related docs linked with the PR?
All changes manually tested?
Any documentation changes needed with this change?
I have executed schemaGenerator tests and updated schema if needed
Are sensitive fields marked as secret in definition config?
My test cases and placeholders use only masked/sample values for sensitive fields
Is the PR limited to 10 file changes & one task?
Reviewer checklist
Is the type of change in the PR title appropriate as per the changes?
Verified that there are no credentials or confidential data exposed with the changes.
Summary by CodeRabbit
New Features
Bug Fixes
Tests