-
Notifications
You must be signed in to change notification settings - Fork 311
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
chore: data plane changes for credentials in transformation #4715
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 recent updates introduce a new Changes
Sequence Diagram(s) (Beta)sequenceDiagram
participant Client
participant BackendConfig
participant Processor
participant Transformer
Client->>BackendConfig: Request Configuration
BackendConfig-->>Client: Return Config with Credentials
BackendConfig->>Processor: Send Config with Credentials
Processor->>Processor: Initialize credentialsMap
Processor->>Transformer: Send TransformerEvent with Credentials
Transformer-->>Processor: Process and Return Results
Processor-->>Client: Return Processed Data
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 (
|
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.
Can you add a testcase to make sure the backend credentials will make it to the request to transformer in rudder-server ?!
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #4715 +/- ##
==========================================
+ Coverage 73.30% 73.40% +0.10%
==========================================
Files 416 416
Lines 48746 48763 +17
==========================================
+ Hits 35731 35796 +65
+ Misses 10676 10631 -45
+ Partials 2339 2336 -3 ☔ View full report in Codecov by Sentry. |
Can we add tests for the change ? |
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: 2
Outside diff range and nitpick comments (1)
processor/transformer/transformer.go (1)
99-106
: Add documentation for theCredential
struct and its fields.It's good practice to provide comments explaining the purpose and usage of each field in a struct, especially for public or exported types. This helps other developers understand the code more quickly and can be crucial for maintaining the code in the future.
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (6)
- backend-config/backend-config.go (1 hunks)
- backend-config/types.go (2 hunks)
- processor/processor.go (7 hunks)
- processor/transformer/transformer.go (1 hunks)
- processor/transformer/transformer_test.go (7 hunks)
- schema-forwarder/internal/testdata/configdata.go (1 hunks)
Additional context used
golangci-lint
backend-config/types.go
91-91: undefined: EventReplayConfig (typecheck)
Additional comments not posted (8)
backend-config/types.go (1)
77-81
: The addition of theCredential
struct is well-defined and aligns with the PR's objectives to handle credentials securely.schema-forwarder/internal/testdata/configdata.go (1)
192-203
: The addition of theCredentials
field inSampleBackendConfig
with appropriate test credentials is correctly implemented and aligns with the changes in the main configuration types.backend-config/backend-config.go (1)
145-145
: The addition of theCredentials
field in thefilterProcessorEnabledDestinations
function is correctly implemented, ensuring that credentials are appropriately passed through the system.processor/transformer/transformer_test.go (1)
206-213
: The addition of theCredentials
field in various test cases is correctly implemented, ensuring comprehensive testing of the new credentials handling functionality.Also applies to: 249-256, 366-373, 431-438, 564-571, 680-687, 763-770
processor/transformer/transformer.go (1)
99-100
: The addition of theCredentials
field to theTransformerEvent
struct aligns with the PR's objectives.This change enables the passing of multiple credentials with each event, enhancing the flexibility and security of the transformation process.
processor/processor.go (3)
850-861
: Consider simplifying the conversion ofcredentialsMap
to a list by using existing libraries or utility functions.
[REFACTOR_SUGGESTion]- var credentialsList []transformer.Credential - for id, credential := range credentialsMap { - credentialsList = append(credentialsList, transformer.Credential{ - ID: id, - Key: credential.Key, - Value: credential.Value, - IsSecret: credential.IsSecret, - }) - } - return credentialsList + return lo.MapToSlice(credentialsMap, func(id string, credential backendconfig.Credential) transformer.Credential { + return transformer.Credential{ + ID: id, + Key: credential.Key, + Value: credential.Value, + IsSecret: credential.IsSecret, + } + })
801-801
: Ensure proper initialization ofcredentialsMap
to avoid nil map assignment errors.- credentialsMap map[string][]transformer.Credential + credentialsMap map[string][]transformer.Credential = make(map[string][]transformer.Credential)Likely invalid or redundant comment.
1125-1125
: Ensure that thecredentialsMap
is properly accessed with a valid key to avoid potential runtime panics.
@@ -144,6 +144,7 @@ type Handle struct { | |||
eventSchemaV2Enabled bool | |||
archivalEnabled config.ValueLoader[bool] | |||
eventAuditEnabled map[string]bool | |||
credentialsMap map[string][]transformer.Credential |
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.
Ensure proper initialization of credentialsMap
to avoid nil map assignment errors.
- credentialsMap map[string][]transformer.Credential
+ credentialsMap map[string][]transformer.Credential = make(map[string][]transformer.Credential)
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.
credentialsMap map[string][]transformer.Credential | |
credentialsMap map[string][]transformer.Credential = make(map[string][]transformer.Credential) |
@@ -87,6 +93,7 @@ type ConfigT struct { | |||
ConnectionFlags ConnectionFlags `json:"flags"` | |||
Settings Settings `json:"settings"` | |||
UpdatedAt time.Time `json:"updatedAt"` | |||
Credentials map[string]Credential `json:"credentials"` |
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.
The addition of the Credentials
field in ConfigT
is appropriate for managing multiple credentials. However, there's an issue with an undefined EventReplayConfig
type used elsewhere in the file.
+ import "path/to/event_replay_config" // Ensure this path is correct
Committable suggestion was skipped due to low confidence.
4155fa2
to
b124ce4
Compare
We can write contract tests for the transformer. Something like this would be helpful? This way we make sure what metadata we expect in transformer. #4787 |
type Credential struct { | ||
Key string `json:"key"` | ||
Value string `json:"value"` | ||
IsSecret bool `json:"isSecret"` |
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.
do we need IsSecret
at server end? I suppose workspace config gives the raw Value
rt?
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.
Yes, we receive IsSecret from the workspace config, and we need to send it to the transformer
Description
Pass credentials along with events to Rudder Transformer, retrieved from the workspace configuration.
https://www.notion.so/rudderstacks/Transformation-secrets-5b234a813aa44e71948a68dffb0a66d2?pvs=4#89cddfd45a5343239e8d5398c7e61463
Linear Ticket
Fixes DAT-1204
https://linear.app/rudderstack/issue/DAT-1204/data-plane-flow
Security
Summary by CodeRabbit
New Features
Tests
Chores