Error handling fixes #203
Error handling fixes #203
Conversation
| @@ -268,7 +268,8 @@ func (fr *FileReceiver) processMessage(msg *pubsub.Message) error { | |||
| // Optionally decode and decrypt message | |||
| data, err = compliance.Reveal(fr.transformConfig, data) | |||
| if err != nil { | |||
| return logger.LogErrorf("unable to reveal event: %v", err).Err() | |||
| logger.LogErrorf("unable to reveal event: %v", err) | |||
There was a problem hiding this comment.
I don't know if this is going to be a workaround or a hack to get past where the event used in cancelling a transfer comes in without needing to be decrypted, or if this is going to be a permanent solution.
There was a problem hiding this comment.
Looks like there are different event types for ACHFile and CancelACHFile. Wouldn't it be better to check the type, and not trying to decrypt CancelACHFile events. That way true decryption errors could fail as they should. You could just move the decryption down to line 307 or 315.
There was a problem hiding this comment.
we could try to check the event type. it would require reading first (to get the event type), possibly failing because it needs to be revealed, an(d then reading again). but it shouldn't depend on the event type. it should just be that what we got was either encrypted or it wasn't, and we have to either reveal it or read it first to find out.
There was a problem hiding this comment.
I see, I did not notice that. If we need to get something working soon, I am ok with this approach. However, I think that we should create a ticket to fix this in a better way. It looks to me like the achgateway can receive events over http and over kafka. The kafka events come from ach-orchestrator and are encrypted, the http events are in the clear. After receiving the event, they are passed to code that processes both types and trys to decrypt them. The decryption always fails for the events received over http.
The decryption should be moved into the kafka specific components (configurable) so that decrypted events get delivered to the common code in all cases. This would fix the achgateway correctly in all cases.
There was a problem hiding this comment.
I see, I did not notice that. If we need to get something working soon, I am ok with this approach. However, I think that we should create a ticket to fix this in a better way. It looks to me like the achgateway can receive events over http and over kafka. The kafka events come from ach-orchestrator and are encrypted, the http events are in the clear. After receiving the event, they are passed to code that processes both types and trys to decrypt them. The decryption always fails for the events received over http.
The decryption should be moved into the kafka specific components (configurable) so that decrypted events get delivered to the common code in all cases. This would fix the achgateway correctly in all cases.
There was a problem hiding this comment.
that's a good point. Can you raise a Linear card for that change?
There was a problem hiding this comment.
|
I am confused as to: What were you trying to do? What where you expecting and what did you get? Where is the new test case for the new behavior? This looks like a hack. |
It is a bit of a hack - at least to prompt discussion. Test case is still in progress. I probably need to update the description a bit more. |
| @@ -268,7 +268,8 @@ func (fr *FileReceiver) processMessage(msg *pubsub.Message) error { | |||
| // Optionally decode and decrypt message | |||
| data, err = compliance.Reveal(fr.transformConfig, data) | |||
| if err != nil { | |||
| return logger.LogErrorf("unable to reveal event: %v", err).Err() | |||
| logger.LogErrorf("unable to reveal event: %v", err) | |||
There was a problem hiding this comment.
Looks like there are different event types for ACHFile and CancelACHFile. Wouldn't it be better to check the type, and not trying to decrypt CancelACHFile events. That way true decryption errors could fail as they should. You could just move the decryption down to line 307 or 315.
Changes
Fix to error handling for events
Why Are Changes Being Made
Better logging for error handling.
Better support for events written without encryption to be decrypted.
The specific flow used to see this is the
http://localhost:8080/shards/{shardName}/files/{fileID}endpoint, which produces an unencrypted event to pass internally for achgateway to consume, where the consumer expects the event to be encrypted.