Skip to content
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

pgsql: track 'progress' in tx per direction - v0 #11540

Closed
wants to merge 2 commits into from

Conversation

jufajardini
Copy link
Contributor

Sharing to check if this is the right direction (heh)

Link to ticket: https://redmine.openinfosecfoundation.org/issues/
https://redmine.openinfosecfoundation.org/issues/7113

Describe changes:

  • add a tx state for when the pgsql request is done
  • track tx_completion per tx direction
  • bring the trigger raw stream reassembly call to the moment when tx is completed

Provide values to any of the below to override the defaults.

  • To use an LibHTP, Suricata-Verify or Suricata-Update pull request,
    link to the pull request in the respective _BRANCH variable.
  • Leave unused overrides blank or remove.

SV_BRANCH=OISF/suricata-verify#1990

Copy link

codecov bot commented Jul 22, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 82.50%. Comparing base (7f6c963) to head (7f03644).

Additional details and impacted files
@@            Coverage Diff             @@
##           master   #11540      +/-   ##
==========================================
- Coverage   82.51%   82.50%   -0.01%     
==========================================
  Files         923      923              
  Lines      248732   248749      +17     
==========================================
+ Hits       205232   205238       +6     
- Misses      43500    43511      +11     
Flag Coverage Δ
fuzzcorpus 60.41% <100.00%> (+0.01%) ⬆️
livemode 18.62% <3.12%> (-0.01%) ⬇️
pcap 44.08% <3.12%> (-0.01%) ⬇️
suricata-verify 61.74% <96.87%> (-0.02%) ⬇️
unittests 59.06% <56.25%> (-0.01%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

@suricata-qa
Copy link

Information: QA ran without warnings.

Pipeline 21679

@suricata-qa
Copy link

Information: QA ran without warnings.

Pipeline 21683

@suricata-qa
Copy link

Information: QA ran without warnings.

Pipeline 21689

While PGSQL can have multiple responses within a single transaction, the
current implementation can only have one request. It makes sense then to
track the transaction progress per flow direction.

This will help when triggering raw stream reassembly or for
unidirectional transactions, and may be useful when we implement sub-
protocols that can have multiple requests per transaction, as well.

CancelRequests and TerminationRequests are examples of unidirectional
transactions. While we know this, we can't garantee that either messages
would result in early termination of existing transactions, so we skip
finishing existing transactions - which shouldn't be common, regardless,
as PGSQL is mostly a sequential protocol.

Bug OISF#7113
Once we are tracking tx progress per-direction for PGSQL, we can trigger
the raw stream reassembly for detection purposes once the transactions
are completed.

Task OISF#7000
@jufajardini jufajardini marked this pull request as ready for review July 23, 2024 22:11
@jufajardini
Copy link
Contributor Author

I think this is now ready for a review.

@suricata-qa
Copy link

Information: QA ran without warnings.

Pipeline 21690

@victorjulien victorjulien marked this pull request as draft July 24, 2024 07:47
@victorjulien
Copy link
Member

Lets keep a PR that has seen these force pushes as draft.

@jufajardini
Copy link
Contributor Author

Lets keep a PR that has seen these force pushes as draft.

oh, okie, sorry.

@jufajardini
Copy link
Contributor Author

Non-draft version: #11547

@jufajardini jufajardini deleted the pgsql-7113/v0 branch September 20, 2024 22:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants