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

Add srcPodIP field in Traceflow observations #6247

Merged
merged 1 commit into from
May 9, 2024

Conversation

Atish-iaf
Copy link
Contributor

@Atish-iaf Atish-iaf commented Apr 22, 2024

@Atish-iaf Atish-iaf force-pushed the add-srcPodIP-traceflow branch from 54003b1 to f6336a8 Compare April 22, 2024 05:40
@Atish-iaf Atish-iaf marked this pull request as ready for review April 22, 2024 06:53
@rajnkamr rajnkamr changed the title Add "srcPodIP" field in Traceflow observations Add srcPodIP field in Traceflow observations Apr 22, 2024
@Atish-iaf Atish-iaf marked this pull request as draft April 22, 2024 09:28
@Atish-iaf Atish-iaf force-pushed the add-srcPodIP-traceflow branch from f6336a8 to 3b6a6ba Compare April 22, 2024 12:30
@Atish-iaf Atish-iaf marked this pull request as ready for review April 22, 2024 13:36
@Atish-iaf
Copy link
Contributor Author

/test-ipv6-e2e
/test-ipv6-only-e2e

1 similar comment
@Atish-iaf
Copy link
Contributor Author

/test-ipv6-e2e
/test-ipv6-only-e2e

@Atish-iaf
Copy link
Contributor Author

/test-kind-ipv6-only-e2e
/test-kind-ipv6-e2e

@Atish-iaf Atish-iaf marked this pull request as draft April 23, 2024 12:40
@Atish-iaf Atish-iaf force-pushed the add-srcPodIP-traceflow branch from 3b6a6ba to 8449791 Compare April 25, 2024 06:52
@Atish-iaf
Copy link
Contributor Author

/test-kind-ipv6-only-e2e
/test-kind-ipv6-e2e

@Atish-iaf
Copy link
Contributor Author

/test-ipv6-e2e

@Atish-iaf
Copy link
Contributor Author

/test-ipv6-only-e2e

@Atish-iaf
Copy link
Contributor Author

/test-kind-ipv6-only-e2e
/test-kind-ipv6-e2e

1 similar comment
@Atish-iaf
Copy link
Contributor Author

/test-kind-ipv6-only-e2e
/test-kind-ipv6-e2e

@Atish-iaf Atish-iaf marked this pull request as ready for review April 25, 2024 14:10
@Atish-iaf Atish-iaf force-pushed the add-srcPodIP-traceflow branch from 8449791 to 175105d Compare April 30, 2024 06:08
@Atish-iaf Atish-iaf requested review from antoninbas and tnqn April 30, 2024 06:11
@Atish-iaf
Copy link
Contributor Author

/test-kind-ipv6-only-e2e
/test-kind-ipv6-e2e

build/charts/antrea/crds/traceflow.yaml Show resolved Hide resolved
@@ -173,6 +173,12 @@ func (c *Controller) parsePacketIn(pktIn *ofctrl.PacketIn) (*crdv1beta1.Traceflo
ob := new(crdv1beta1.Observation)
ob.Component = crdv1beta1.ComponentSpoofGuard
ob.Action = crdv1beta1.ActionForwarded
// ctNwSrc is invalid incase of ICMP6.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't understand what this comment is trying to say here, it really needs to include more information.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ctNwSrc doesn't contain original src ip incase of icmp6 packet. It should contain but don't know why it always have :: incase of icmp6. It contains original src ip in all other cases(tcp/udp/ipv4/ipv6). Maybe some limitation from OVS ?
We don't use ipSrc directly here because ipSrc and ctNwSrc are different incase of hairpin.
Incase of icmp6, even if ctNwSrc is invalid, we can use ipSrc as hairpin is not applicable in icmp6, so ipSrc contains src pod IP.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the information.

ctNwSrc doesn't contain original src ip incase of icmp6 packet

What kind of ICMPv6 packet specifically are you referring to, and how did you observe this condition? Was it because of an e2e test?
ICMPv6 is very complex, and some packets use multicast, and it's possible that conntrack handles different ICMPv6 packets differently.

I'm tagging @wenyingd as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

how did you observe this condition? Was it because of an e2e test?

Yes, all traceflow ICMPv6 testcases(using icmpv6 protocol 58) failed in dual-stack and ipv6-only e2e kind jobs.

What kind of ICMPv6 packet specifically are you referring to

Traceflow IPv6 packets with source as pod and destination an IPv6 address using ICMPv6 protocol(58). I modified the code to read related fields from traceflow packet and logged it.

For ICMP in IPv6 antctl tf -S pod2 -D 2001:4860:4860::8888 -f ipv6 following is the log :

I0502 05:44:19.714801       1 packetin.go:133] "IPv6 Packet" ipSrc="fd00:10:244:1::4"
I0502 05:44:19.714853       1 packetin.go:134] "IPv6 Packet" ipDst="2001:4860:4860::8888"
I0502 05:44:19.714884       1 packetin.go:135] "IPv6 Packet" ctNwSrc="::"
I0502 05:44:19.714959       1 packetin.go:136] "IPv6 Packet" ctNwDst="::8888"
I0502 05:44:19.715019       1 packetin.go:137] "IPv6 Packet" nextHeader(proto)=58
I0502 05:44:19.715042       1 packetin.go:457] "Packet-In" NXM_NX_CT_STATE value={"Data":33} mask={"Data":255}
I0502 05:44:19.715066       1 packetin.go:467] "Packet-In" NXM_NX_CT_ZONE=65510
I0502 05:44:19.715104       1 packetin.go:477] "Packet-In" NXM_NX_CT_MARK=3
I0502 05:44:19.715146       1 packetin.go:487] "Packet-In" NXM_NX_CT_NW_PROTO=58

For TCP in IPv6 antctl tf -S pod2 -D 2001:4860:4860::8888 -f ipv6,tcp,tcp_dst=80 following is the log :

I0502 05:44:59.722052       1 packetin.go:133] "IPv6 Packet" ipSrc="fd00:10:244:1::4"
I0502 05:44:59.722158       1 packetin.go:134] "IPv6 Packet" ipDst="2001:4860:4860::8888"
I0502 05:44:59.722247       1 packetin.go:135] "IPv6 Packet" ctNwSrc="fd00:10:244:1::4"
I0502 05:44:59.722396       1 packetin.go:136] "IPv6 Packet" ctNwDst="2001:4860:4860::8888"
I0502 05:44:59.722478       1 packetin.go:137] "IPv6 Packet" nextHeader(proto)=6
I0502 05:44:59.722529       1 packetin.go:457] "Packet-In" NXM_NX_CT_STATE value={"Data":33} mask={"Data":255}
I0502 05:44:59.722562       1 packetin.go:467] "Packet-In" NXM_NX_CT_ZONE=65510
I0502 05:44:59.722650       1 packetin.go:477] "Packet-In" NXM_NX_CT_MARK=3
I0502 05:44:59.722692       1 packetin.go:487] "Packet-In" NXM_NX_CT_NW_PROTO=6

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Atish-iaf I was able to reproduce, thanks for the detailed information.

I do think this is an OVS-specific issue. I was able to reproduce with a "standalone" OVS bridge (i.e., without Antrea) and I reported the bug to the OVS community: openvswitch/ovs-issues#327

I suggest that you add a comment here explaining the situation, with a link to the OVS bug, and then we can proceed with this PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @antoninbas

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was indeed a bug in the OVS kernel module and has been patched in Linux: https://git.kernel.org/pub/scm/linux/kernel/git/netdev/net.git/commit/?id=7c988176b6c1. I don't know if it will be backported.

@Atish-iaf Atish-iaf added the area/ops/traceflow Issues or PRs related to the Traceflow feature label May 2, 2024
@rajnkamr rajnkamr added this to the Antrea v2.1 release milestone May 3, 2024
@Atish-iaf Atish-iaf requested a review from wenyingd May 6, 2024 04:23
@Atish-iaf Atish-iaf force-pushed the add-srcPodIP-traceflow branch from 175105d to f85a26e Compare May 7, 2024 05:11
@Atish-iaf Atish-iaf requested a review from antoninbas May 7, 2024 05:14
@Atish-iaf
Copy link
Contributor Author

/test-kind-ipv6-only-e2e
/test-kind-ipv6-e2e

@Atish-iaf
Copy link
Contributor Author

/test-kind-ipv6-only-e2e

1 similar comment
@Atish-iaf
Copy link
Contributor Author

/test-kind-ipv6-only-e2e

@Atish-iaf Atish-iaf force-pushed the add-srcPodIP-traceflow branch from f85a26e to 94c63ae Compare May 7, 2024 19:44
Copy link
Contributor

@antoninbas antoninbas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

one more nit, otherwise LGTM

pkg/agent/controller/traceflow/packetin.go Outdated Show resolved Hide resolved
Signed-off-by: Kumar Atish <kumar.atish@broadcom.com>
@Atish-iaf Atish-iaf force-pushed the add-srcPodIP-traceflow branch from 94c63ae to 006297f Compare May 8, 2024 04:47
@antoninbas
Copy link
Contributor

@tnqn @wenyingd do you want to do a quick review, looks good on my side

@antoninbas
Copy link
Contributor

/test-all

@antoninbas
Copy link
Contributor

/test-kind-ipv6-e2e
/test-kind-ipv6-only-e2e

@Atish-iaf
Copy link
Contributor Author

/test-kind-ipv6-only-e2e

1 similar comment
@Atish-iaf
Copy link
Contributor Author

/test-kind-ipv6-only-e2e

@antoninbas antoninbas merged commit 43eb612 into antrea-io:main May 9, 2024
57 of 60 checks passed
@antoninbas antoninbas added the action/release-note Indicates a PR that should be included in release notes. label May 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
action/release-note Indicates a PR that should be included in release notes. area/ops/traceflow Issues or PRs related to the Traceflow feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants