feat: add destinationID to rETL endpoint #4234
Conversation
|
Important Auto Review SkippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the To trigger a single review, invoke the 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 (
|
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## master #4234 +/- ##
==========================================
+ Coverage 73.98% 74.01% +0.03%
==========================================
Files 388 389 +1
Lines 55127 55179 +52
==========================================
+ Hits 40784 40843 +59
+ Misses 12017 12013 -4
+ Partials 2326 2323 -3 ☔ View full report in Codecov by Sentry. |
mihir20
requested review from
lvrach,
fracasula,
atzoum,
cisse21,
BonapartePC,
achettyiitr and
Sidddddarth
| } | ||
| isValidDestination, isDestinationEnabled := gw.validateDestinationID(destinationID, arctx) | ||
| if !isValidDestination { | ||
| errorMessage = response.InvalidDestinationID |
There was a problem hiding this comment.
Shouldn't this be DestinationNotFound instead of InvalidDestinationID?
| errorMessage = response.InvalidDestinationID | |
| errorMessage = response.DestinationNotFound |
There was a problem hiding this comment.
can destination not found can be considered as a case of Invalid destination, similar to what we are doing for Source ID not found?
| func GetRequestTypeFromCtx(ctx context.Context) (string, bool) { | ||
| reqType, ok := ctx.Value(gwtypes.CtxParamCallType).(string) | ||
| return reqType, ok | ||
| } | ||
|
|
||
| // GetAuthRequestFromCtx : get auth request from context | ||
| func GetAuthRequestFromCtx(ctx context.Context) (*gwtypes.AuthRequestContext, bool) { | ||
| authReqCtx, ok := ctx.Value(gwtypes.CtxParamAuthRequestContext).(*gwtypes.AuthRequestContext) | ||
| return authReqCtx, ok | ||
| } |
There was a problem hiding this comment.
Shouldn't these be used everywhere now that they exist? What's the point of returning the second value (bool) if nobody cares about it in practice? Would it be more sincere to rename these functions to MustGetXxx and panic if casting fails?
There was a problem hiding this comment.
added handling for bool, we will fail request with ISE if we are not able to get these from context, As we don't want our application to panic in such cases.
| @@ -27,6 +27,8 @@ type AuthRequestContext struct { | |||
| SourceJobRunID string | |||
| SourceTaskRunID string | |||
| Source backendconfig.SourceT | |||
| // DestinationID is optional param, destination id will be present for rETL Request | |||
There was a problem hiding this comment.
Not sure if we need this comment. SourceJobRunID & SourceTaskRunID are both optional as well & we don't mention when they are going to be present. One reason why we would choose not to include such comments would be change, i.e. at some point we will decide to change (when these will be present) and almost certainly we'll forget to update the relevant comments! So we end up with invalid comments!
There was a problem hiding this comment.
IMO its better to keep this comment, and if we start using this field for cases we should update the comment, this way we can keep more readable and intuitive. Or we can just add a comment that this field is optional. That way reading through the code might give us a better understanding that these fields may or many not necessarily
always be available.
| gw.handleFailureStats(errorMessage, reqType, arctx) | ||
| }() | ||
| reqType, ok := gwCtx.GetRequestTypeFromCtx(r.Context()) | ||
| if !ok { |
There was a problem hiding this comment.
Now we need test scenarios for the !ok conditions :)
Another reason why I was proposing to not provide the option to use this method separately without embedding sourceIDAuth in it :)
There was a problem hiding this comment.
Adding tests should be fine this way we will be able to prevent panics in the server code.
ref: #4234 (comment)
| @@ -119,6 +123,57 @@ func (gw *Handle) sourceIDAuth(delegate http.HandlerFunc) http.HandlerFunc { | |||
| } | |||
| } | |||
|
|
|||
| // authDestIDForSource middleware to authenticate destinationId in the X-Rudder-Destination-Id header. | |||
| // If the destinationId is invalid, the request is rejected. | |||
There was a problem hiding this comment.
Do we really need to validate destinationId at the gateway level? The requests will anyways be filtered out in processor and since it is only an internal endpoint don't think sources would send an invalid destinationId. We could just accept whatever we get in the gateway level and move on
There was a problem hiding this comment.
IMO we should not trust what's coming in requests, we should always validate critical inputs.
mihir20
force-pushed
the
mihir/pipe-619
branch
from
5f3facd to
c32f8fb
Compare
Description
Adding Destination ID to rETL endpoint to provide a way for rETL to send an event from a source to a destination. If there are multiple sources rETL will call
/retlendpoint multiple times to deliver to multiple destinations.Linear Ticket
Pipe-619
Security