Set default flag to 2 for TCP traceflow #4948

luolanzone · 2023-05-08T06:58:46Z

Resolves #4897

antoninbas

I am fine with merging this, but maybe we should revisit this old issue (#1852) and instead make Traceflow more user-friendly by automatically setting the SYN bit. Users would still be able to clear the TCP flags by providing tcp_flags=0.

luolanzone · 2023-05-09T01:41:57Z

@antoninbas thanks for the input, I do feel we'd better to keep the same behaviors for both cases when Pod or Service as the destination. I noticed that the flag is set as 2 when the destination is Service by default in traceflow controller, not sure why it's not set for Pod case. Let's see what's @tnqn opinion.

tnqn · 2023-05-11T14:29:15Z

@antoninbas thanks for the input, I do feel we'd better to keep the same behaviors for both cases when Pod or Service as the destination. I noticed that the flag is set as 2 when the destination is Service by default in traceflow controller, not sure why it's not set for Pod case. Let's see what's @tnqn opinion.

I don't know the history regarding this. But defaulting tcp flags works for me. Where is the flag set for Service?

luolanzone · 2023-05-12T01:57:42Z

@tnqn here is the code https://github.com/antrea-io/antrea/blob/main/pkg/agent/controller/traceflow/traceflow_controller.go#L500-L504, when it's not live traffic traceflow, it will set default flag as 2.

luolanzone · 2023-05-15T02:31:23Z

@tnqn @antoninbas could you review again? I updated the code to set a TCP flag to 2 by default.

antoninbas · 2023-05-15T23:12:11Z

pkg/agent/controller/traceflow/traceflow_controller.go

@@ -536,6 +536,9 @@ func (c *Controller) preparePacket(tf *crdv1alpha1.Traceflow, intf *interfacesto
 		if tf.Spec.Packet.TransportHeader.TCP.Flags != 0 {
 			packet.TCPFlags = uint8(tf.Spec.Packet.TransportHeader.TCP.Flags)
 		}
+		if !liveTraffic && tf.Spec.Packet.TransportHeader.TCP.Flags == 0 {


this code needs to be revisited for this PR:

antrea/pkg/agent/controller/traceflow/traceflow_controller.go

Lines 500 to 504 in 01817a5

if !liveTraffic {

// Set the SYN flag. In encap mode, the SYN flag is only required for

// Service traffic, but probably we should always set it.

packet.TCPFlags = 2

}

The comment is no longer relevant and it is confusing to set the TCP flags in 2 places.

Remove this part since we always set it as 2 by default.

So I guess by setting the TCP flags here, it is not possible for the users to force the flags to be 0. I don't know if this is an issue at all though, since Traceflow is unlikely to be useful when the SYN flag is not set.

@tnqn what do you think? When we promote the API to beta, should be make the Flags field a pointer instead (*int32)?

tcp_flags 0 seems only useful when we want to verify the pipeline can drop TCP NULL scan. I'm fine with changing the field to pointer to make the use case supported.

antoninbas · 2023-05-15T23:13:40Z

docs/antctl.md

+# Start a Traceflow from pod1 to pod2, with a TCP SYN packet to destination port 1234
+# tcp_flags will be 2 by default when it's not provided.
+$ antctl traceflow -S pod1 -D pod2 -f tcp,tcp_dst=1234,tcp_flags=2


Not sure why we are adding this example at all, since the flags are now being set by default. Explicitly using tcp_flags=2 is a bit misleading?

antoninbas · 2023-05-15T23:15:32Z

docs/traceflow-guide.md

@@ -89,6 +89,7 @@ spec:
      tcp:
        srcPort: 10000 # Source port needs to be set when Protocol is TCP/UDP.
        dstPort: 80 # Destination port needs to be set when Protocol is TCP/UDP.
+        flags: 2 # Construct a SYN packet, when Protocol is TCP, flags will be set to 2 by default when it's not provided.


Suggested change

flags: 2 # Construct a SYN packet, when Protocol is TCP, flags will be set to 2 by default when it's not provided.

flags: 2 # Construct a SYN packet: 2 is also the default value when the flags field is omitted.

Signed-off-by: Lan Luo <luola@vmware.com>

antoninbas

LGTM
Waiting for @tnqn to review

antoninbas · 2023-05-16T19:16:55Z

pkg/agent/controller/traceflow/traceflow_controller.go

@@ -536,6 +536,9 @@ func (c *Controller) preparePacket(tf *crdv1alpha1.Traceflow, intf *interfacesto
 		if tf.Spec.Packet.TransportHeader.TCP.Flags != 0 {
 			packet.TCPFlags = uint8(tf.Spec.Packet.TransportHeader.TCP.Flags)
 		}
+		if !liveTraffic && tf.Spec.Packet.TransportHeader.TCP.Flags == 0 {


So I guess by setting the TCP flags here, it is not possible for the users to force the flags to be 0. I don't know if this is an issue at all though, since Traceflow is unlikely to be useful when the SYN flag is not set.

@tnqn what do you think? When we promote the API to beta, should be make the Flags field a pointer instead (*int32)?

tnqn

LGTM

antoninbas · 2023-05-17T17:18:56Z

/test-all

tnqn · 2023-05-22T04:15:30Z

/test-e2e

Signed-off-by: Lan Luo <luola@vmware.com>

luolanzone requested a review from tnqn May 8, 2023 06:58

antoninbas previously approved these changes May 8, 2023

View reviewed changes

luolanzone added the kind/documentation Categorizes issue or PR as related to a documentation. label May 10, 2023

luolanzone dismissed antoninbas’s stale review via 4e9a1a1 May 15, 2023 02:30

luolanzone force-pushed the update-example branch from a5f7e2c to 4e9a1a1 Compare May 15, 2023 02:30

luolanzone changed the title ~~Update traceflow samples~~ Set default flag to 2 for TCP traceflow May 15, 2023

luolanzone force-pushed the update-example branch from 4e9a1a1 to 6aa14ab Compare May 15, 2023 02:33

antoninbas reviewed May 15, 2023

View reviewed changes

Set default flag to 2 for TCP traceflow

be4460c

Signed-off-by: Lan Luo <luola@vmware.com>

luolanzone force-pushed the update-example branch from 6aa14ab to be4460c Compare May 16, 2023 01:50

antoninbas removed the kind/documentation Categorizes issue or PR as related to a documentation. label May 16, 2023

antoninbas reviewed May 16, 2023

View reviewed changes

tnqn approved these changes May 17, 2023

View reviewed changes

tnqn added the action/release-note Indicates a PR that should be included in release notes. label May 22, 2023

tnqn added this to the Antrea v1.12 release milestone May 22, 2023

tnqn merged commit e10ec6a into antrea-io:main May 22, 2023

tnqn mentioned this pull request May 31, 2023

Antrea v2.0 #4832

Closed

9 tasks

luolanzone mentioned this pull request Jun 1, 2023

[Antrea 2.0] Promote APIs to appropriate versions and feature gates to appropriate stages #5068

Closed

27 tasks

ceclinux pushed a commit to ceclinux/antrea that referenced this pull request Jun 5, 2023

Set default flag to 2 for TCP traceflow (antrea-io#4948)

56512c2

Signed-off-by: Lan Luo <luola@vmware.com>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Set default flag to 2 for TCP traceflow #4948

Set default flag to 2 for TCP traceflow #4948

luolanzone commented May 8, 2023

antoninbas left a comment

luolanzone commented May 9, 2023

tnqn commented May 11, 2023

luolanzone commented May 12, 2023

luolanzone commented May 15, 2023

antoninbas May 15, 2023

luolanzone May 16, 2023

antoninbas May 16, 2023

tnqn May 17, 2023

antoninbas May 15, 2023

luolanzone May 16, 2023

antoninbas May 15, 2023

luolanzone May 16, 2023

antoninbas left a comment

antoninbas May 16, 2023

tnqn left a comment

antoninbas commented May 17, 2023

tnqn commented May 22, 2023

	if !liveTraffic {
	// Set the SYN flag. In encap mode, the SYN flag is only required for
	// Service traffic, but probably we should always set it.
	packet.TCPFlags = 2
	}

	flags: 2 # Construct a SYN packet, when Protocol is TCP, flags will be set to 2 by default when it's not provided.
	flags: 2 # Construct a SYN packet: 2 is also the default value when the flags field is omitted.

Set default flag to 2 for TCP traceflow #4948

Set default flag to 2 for TCP traceflow #4948

Conversation

luolanzone commented May 8, 2023

antoninbas left a comment

Choose a reason for hiding this comment

luolanzone commented May 9, 2023

tnqn commented May 11, 2023

luolanzone commented May 12, 2023

luolanzone commented May 15, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

antoninbas left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tnqn left a comment

Choose a reason for hiding this comment

antoninbas commented May 17, 2023

tnqn commented May 22, 2023