conntrack: handle TCP flags #391

ronensc · 2023-02-16T14:06:52Z

This PR adds the ability to read the TCP flags field and perform the following actions:

Ending a connection when the FIN_ACK flag is set and avoid waiting the EndConnectionTimeout.
Swapping source and destination when SYN_ACK is set on the first flowlog.

Each action can be enabled/disabled by a feature flag in the conntrack config:

conntrack:
  # ...
  tcpFlags:
    fieldName: "TCPFlags"
    detectEndConnection: true
    swapAB: true

An operational metric was added to track how many of these actions were performed:

conntrack_tcp_flags{action="detectEndConnection"} 1
conntrack_tcp_flags{action="swapAB"} 1

Additional changes:

Updated go.mod to 1.18 to be able to update netobserv-ebpf-agent
Updated netobserv-ebpf-agent dependency to get the Flags field in the flow protobuf
Added json tag to the conntrack API

Solves #299

go get github.com/netobserv/netobserv-ebpf-agent@latest go get github.com/netobserv/flowlogs-pipeline/pkg/pipeline go mod vendor

Subjects() has been deprecated in Go 1.18: golang/go#46287

jpinsonneau · 2023-02-20T12:49:19Z

pkg/config/generic_map.go

@@ -34,7 +34,7 @@ func (m GenericMap) Copy() GenericMap {

 func (m GenericMap) IsDuplicate() bool {
 	if duplicate, hasKey := m["Duplicate"]; hasKey {
-		if isDuplicate, err := utils.ConvertToBool(duplicate); err != nil {
+		if isDuplicate, err := utils.ConvertToBool(duplicate); err == nil {


While testing this PR I noticed I've made a mistake here 👼
Sorry about that

Thanks for the fix! I missed that too...

jpinsonneau · 2023-02-20T13:43:12Z

docs/api.md

+             detectEndConnection: detect end connections by FIN_ACK flag
+             swapAB: swap source and destination when the first flowlog contains the SYN_ACK flag


I wonder if these two should be the default behavior as it's more reliable & convenient than the timeouts

The swapAB feature isn't related to the timeouts.
The detectEndConnection feature is in addition to the timeouts. It can't replace the timeouts because it's not guaranteed that will receive a flowlog with FIN_ACK flag for every TCP connection (either because of sampling or because of SYN attack). But, it may allow us to increase the endConnectionTimeout for TCP.

stleerh · 2023-02-21T18:31:35Z

pkg/api/conntrack.go

+	OutputFields          []OutputField              `yaml:"outputFields,omitempty" json:"outputFields,omitempty" doc:"list of output fields"`
+	Scheduling            []ConnTrackSchedulingGroup `yaml:"scheduling,omitempty" json:"scheduling,omitempty" doc:"list of timeouts and intervals to apply per selector"`
+	MaxConnectionsTracked int                        `yaml:"maxConnectionsTracked,omitempty" json:"maxConnectionsTracked,omitempty" doc:"maximum number of connections we keep in our cache (0 means no limit)"`
+	TCPFlags              ConnTrackTCPFlags          `yaml:"tcpFlags,omitempty" json:"tcpFlags" doc:"settings for handling TCP flags"`


Missing ",omitempty" for json:

TCPFlags ConnTrackTCPFlags `yaml:"tcpFlags,omitempty" json:"tcpFlags,omitempty" doc:"settings for handling TCP flags"`

Thanks! good catch

openshift-ci · 2023-02-22T10:45:23Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please ask for approval from kalmanmeth. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

codecov-commenter · 2023-02-22T12:37:12Z

Codecov Report

Merging #391 (3b48145) into main (8d5b93c) will increase coverage by 0.53%.
The diff coverage is 88.34%.

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

@@            Coverage Diff             @@
##             main     #391      +/-   ##
==========================================
+ Coverage   60.98%   61.51%   +0.53%     
==========================================
  Files          91       91              
  Lines        6297     6390      +93     
==========================================
+ Hits         3840     3931      +91     
  Misses       2223     2223              
- Partials      234      236       +2

Flag	Coverage Δ
unittests	`61.51% <88.34%> (+0.53%)`	⬆️

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

Impacted Files	Coverage Δ
pkg/pipeline/pipeline.go	`91.42% <ø> (ø)`
pkg/pipeline/extract/conntrack/store.go	`80.64% <50.00%> (-3.53%)`	⬇️
pkg/api/conntrack.go	`88.81% <84.61%> (-0.87%)`	⬇️
pkg/config/generic_map.go	`100.00% <100.00%> (+23.07%)`	⬆️
pkg/pipeline/decode/decode_protobuf.go	`77.55% <100.00%> (+0.46%)`	⬆️
pkg/pipeline/extract/conntrack/conn.go	`78.15% <100.00%> (+2.45%)`	⬆️
pkg/pipeline/extract/conntrack/conntrack.go	`93.37% <100.00%> (+1.11%)`	⬆️
pkg/pipeline/extract/conntrack/metrics.go	`100.00% <100.00%> (ø)`
pkg/pipeline/pipeline_builder.go	`71.06% <100.00%> (ø)`
pkg/pipeline/utils/multiorderedmap.go	`100.00% <100.00%> (ø)`
... and 2 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

openshift-ci bot added the do-not-merge/work-in-progress label Feb 16, 2023

ronensc force-pushed the conntrack-tcp-flags branch from 0585d4c to 1a0fa34 Compare February 16, 2023 14:09

ronensc requested review from KalmanMeth, eranra, jotak and jpinsonneau February 16, 2023 15:52

ronensc marked this pull request as ready for review February 16, 2023 15:52

openshift-ci bot removed the do-not-merge/work-in-progress label Feb 16, 2023

ronensc added the enhancement New feature or request label Feb 16, 2023

ronensc added 20 commits February 20, 2023 10:39

Update ebpf-agent dependency

ccae28f

go get github.com/netobserv/netobserv-ebpf-agent@latest go get github.com/netobserv/flowlogs-pipeline/pkg/pipeline go mod vendor

Fix Generic related build issues

0c72d5c

Add Flags fields to decode_protobuf

0f1a439

Rename test funcions

00311a8

Handle FIN_ACK

457f32b

Add a test for MoveToFront

55f5835

Validate TCPFlags field name is not empty

797908e

Add correct direction

1db84db

Add test case for mismatch of AB field count

0be8d9f

Add operational metric for tcp flags

8dce385

Rename CorrectDirection -> SwapAB

1c5242b

Change test

2ebbe10

Update README

55d5599

Add json tag to conntrack api

a636ea7

Update docs

e861beb

Rename variable

a7c41c4

Make linter happy

af8ec46

Make linter happy

6c0470e

Subjects() has been deprecated in Go 1.18: golang/go#46287

Enable SwapAB only when the feature flag is set

5b5748e

Fix rebase errors

60245a3

ronensc force-pushed the conntrack-tcp-flags branch from 4e2d06f to 60245a3 Compare February 20, 2023 08:46

NETOBSERV-838 fix IsDuplicate

89a3971

jpinsonneau reviewed Feb 20, 2023

View reviewed changes

KalmanMeth previously approved these changes Feb 21, 2023

View reviewed changes

openshift-ci bot assigned KalmanMeth Feb 21, 2023

openshift-ci bot added the lgtm label Feb 21, 2023

stleerh reviewed Feb 21, 2023

View reviewed changes

ronensc added 3 commits February 22, 2023 12:27

Add missing 'omitempty'

833136a

Add parenthesis for clarity

72847f7

Add tests for IsDuplicate()

3b48145

ronensc dismissed KalmanMeth’s stale review via 3b48145 February 22, 2023 10:45

openshift-ci bot removed the lgtm label Feb 22, 2023

KalmanMeth approved these changes Feb 27, 2023

View reviewed changes

openshift-ci bot added the lgtm label Feb 27, 2023

ronensc merged commit 9740aa3 into netobserv:main Mar 1, 2023

ronensc deleted the conntrack-tcp-flags branch March 1, 2023 12:44

ronensc mentioned this pull request Mar 22, 2023

Enable TCP flags handling in FLP netobserv/network-observability-operator#305

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

conntrack: handle TCP flags #391

conntrack: handle TCP flags #391

ronensc commented Feb 16, 2023 •

edited

Loading

jpinsonneau Feb 20, 2023

ronensc Feb 22, 2023

jpinsonneau Feb 20, 2023

ronensc Feb 22, 2023

stleerh Feb 21, 2023

ronensc Feb 22, 2023

openshift-ci bot commented Feb 22, 2023

codecov-commenter commented Feb 22, 2023

		detectEndConnection: detect end connections by FIN_ACK flag
		swapAB: swap source and destination when the first flowlog contains the SYN_ACK flag

conntrack: handle TCP flags #391

conntrack: handle TCP flags #391

Conversation

ronensc commented Feb 16, 2023 • edited Loading

jpinsonneau Feb 20, 2023

Choose a reason for hiding this comment

ronensc Feb 22, 2023

Choose a reason for hiding this comment

jpinsonneau Feb 20, 2023

Choose a reason for hiding this comment

ronensc Feb 22, 2023

Choose a reason for hiding this comment

stleerh Feb 21, 2023

Choose a reason for hiding this comment

ronensc Feb 22, 2023

Choose a reason for hiding this comment

openshift-ci bot commented Feb 22, 2023

codecov-commenter commented Feb 22, 2023

Codecov Report

ronensc commented Feb 16, 2023 •

edited

Loading