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.

Sign up for GitHub

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

Fix tunnelDstIP field in traceflow on dual-stack #4529

Merged
merged 1 commit into from
Mar 3, 2023
Merged

Fix tunnelDstIP field in traceflow on dual-stack #4529

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion pkg/agent/controller/traceflow/packetin.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -237,7 +237,8 @@ func (c *Controller) parsePacketIn(pktIn *ofctrl.PacketIn) (*crdv1alpha1.Tracefl
if tableID == openflow.L2ForwardingOutTable.GetID() {
ob := new(crdv1alpha1.Observation)
tunnelDstIP := ""
isIPv6 := c.nodeConfig.NodeIPv6Addr != nil
// decide according to packet.
isIPv6 := etherData.Ethertype == protocol.IPv6_MSG
if match := getMatchTunnelDstField(matchers, isIPv6); match != nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe we'd better get both IPv4 tunnel dst and IPv6 tunnel dst, and set tunnelDstIP with valid one.

You can also add verification in traceflow_test.go:testTraceflowInterNode, to check if the traceflow observation has expected tunnelDstIP.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi,
Even if we get both IPv4 tunnel dst and IPv6 tunnel dst, tunnelDstIP value will be decided according to isIPv6. The current code sets the value of tunnelDstIP according to isIPv6 directly without getting both tunnel dst. So, why to get both tunnel dst explicitly?

testTraceflowInterNode currently runs in both encap mode and no encap mode. If tunnelDstIP is added then we have to make the test run only in encap mode. Is it ok to change this test to run only in encap mode?

Copy link
Contributor

Choose a reason for hiding this comment

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

whether it is dual stack / non dual stack, in both the cases handling can be done with isIPv6 flag with right packet type, With this logic isIPv6 flag remain false if packet is not of ipv6 type irrespective of stack and hence tunnelDstIP will be assigned with ipv4 and vice versa.

UT testing can be extended to see the right values in tunnelDstIP, if the mode is encap.

Copy link
Contributor

@gran-vmv gran-vmv Jan 4, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Hi, Even if we get both IPv4 tunnel dst and IPv6 tunnel dst, tunnelDstIP value will be decided according to isIPv6. The current code sets the value of tunnelDstIP according to isIPv6 directly without getting both tunnel dst. So, why to get both tunnel dst explicitly?

testTraceflowInterNode currently runs in both encap mode and no encap mode. If tunnelDstIP is added then we have to make the test run only in encap mode. Is it ok to change this test to run only in encap mode?

Because reading both ipv4/ipv6 tunnel dst is more simple to decide the tunnel dst, and it won't depend on the flow logic in OVS table.
Currently only IPv4 tunnel dst is used in DualStack env. Please check: https://github.com/antrea-io/antrea/blob/main/pkg/agent/openflow/client.go#L485

You can add the TunnelDstIP into testTraceflowInterNode like

node2TunnelDstIP = ""
switch encapMode {
case config.TrafficEncapModeEncap:
	node2TunnelDstIP = nodeIP(nodeIdx1)
case config.TrafficEncapModeHybrid:
	// check more
}
...
{
	Component:     v1alpha1.ComponentForwarding,
	ComponentInfo: "Output",
	Action:        v1alpha1.ActionForwarded,
	TunnelDstIP:   node2TunnelDstIP,
},

And add the comparison into compareObservations

Copy link
Contributor

Choose a reason for hiding this comment

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

@wenyingd
Do we use peer Node IPv4 as tunnelDst for IPv4 packet and peer Node IPv6 as tunnelDst for IPv6 packet?

Copy link
Contributor

Choose a reason for hiding this comment

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

We prefer to use tunnel format IPv4OverIPv4 and IPv6OverIPv6.

tunnelDstIP, err = getTunnelDstValue(match)
if err != nil {
Expand Down