Handle sequence of query packets #637
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
get ecnryption_setting from appropriate query packet
allow to run external acra with pre-generated config
actualize unit tests for searchable filter
Zhaars
reviewed
Feb 15, 2023
decryptor/base/proxy.go
Outdated
| return &proxySetting{ | ||
| keystore: keystore, parser: parser, tableSchemaStore: tableSchema, censor: censor, | ||
| connectionWrapper: wrapper, poisonRecordCallbackStorage: callbackStorage, | ||
| }, nil |
There was a problem hiding this comment.
Do we need to return err in this constructor? I see no error-returning stuff here, or its a stub for future things?
There was a problem hiding this comment.
no, we don't need. will return back. one of the approach was to initialize extractor in the setting but after that took cycle imports which hard to fix. so moved it to the proxy but forgot back function signature back
decryptor/postgresql/pg_decryptor.go
Outdated
| func NewEncryptionSettingExtractor(ctx context.Context, schema config.TableSchemaStore, parser *sqlparser.Parser) (EncryptionSettingExtractor, error) { | ||
| queryEncryptor, err := encryptor.NewPostgresqlQueryEncryptor(schema, parser, nil) | ||
| if err != nil { | ||
| return EncryptionSettingExtractor{}, nil |
There was a problem hiding this comment.
Should we return the error here?
…zation even if all flows don't return any error
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Main goal of this PR to support cases with several Parse packets in the one TCP packet.
Now, if Acra receive Parse_1 + Bind_1 + Parse_2 + Bind_2 + Execute_1, it will collect encryption settings from the Parse_1, then overwrite it from Parse_2. And when process data rows after the execute it will apply encryption settings for Parse_1 matched to the Parse_2. Because it stores only last one.
But there is not only that problem. DB driver can register prepared statements in the one packet and execute in the next one in the another order. And there is no way to pull correct settings for incoming DataRow.
In this PR acra remember only packets that request DataRow from the db. It is SimpleQuery and Execute. Only these two type of packets request rows from the database. So now, we remember query or set of packets from the extended protocol with Parse + Bind + Execute packets.
But we should know when we should remove saved packet and stop apply these settings for the next packets. Database separates results for requests by CommandComplete (one for every Execute and set of them each query in SimpleQueryPacket that may contain several SQL splited by ';'), ErrorResponse, EmptyResultResponse (only for SimpleQuery with sql='') and PortalSuspended (only for Execute).
So, there is added tracking incoming commands for data rows and parsing queries for settings only before data decryption stage.
Also removed tracking redundant packets like Bind/Parse, because doesn't need anymore.
It is draft because remaining integration test with simulation custom flow with several parse/execute pairs. But main logic is ready for review
Checklist
with new changes