Skip to content

Commit

Permalink
Fix Traceflow implementation for external IPs and gateway IP (#1884)
Browse files Browse the repository at this point in the history 
PR #1883 fixes a panic in libOpenflow triggered when OVS receives reply
traffic for a Traceflow request with a valid dataplane tag as the ToS
field and the Linux packet mark set. However, it should be noted that
reply packets for Traceflow requests are generally meaningless and
should be ignored. In encapMode, The Traceflow implementation should
also not timeout when a Traceflow request leaves the overlay: as soon as
the request is forwarded through the gateway port, we should consider
the request complete, and ignore any potential reply packet. So we
include the following changes:

* add a new "ForwardedOutOfOverlay" Traceflow action when a request is
  forwarded out of the network managed by Antrea in encapMode. The
  Controller can then mark the request as "succeeded". In theory,
  something similar could be done for other traffic modes, but it would
  be much more complex.
* add support for Traceflow requests for which the destination is the
  gateway's IP, by reporting a "Delivered" action.
* add an OVS flow in charge of dropping reply traffic for Traceflow
  requests (using the conntrack state to match this traffic), thus
  ensuring it is not set to the Agent. In our testing, this is
  especially useful when the destination IP is the local Node's IP, as
  the IP ToS field seems to be preserved in that case, causing the reply
  packet to be treated as a Traceflow request.

We add end-to-end tests for both cases (external destination IP and
Antrea gateway destination IP).

See #1878
  • Loading branch information
@antoninbas
antoninbas committed Feb 26, 2021
1 parent 7122583 commit 8038935
Show file tree
Hide file tree
Showing 9 changed files with 406 additions and 151 deletions.
14 changes: 11 additions & 3 deletions pkg/agent/controller/traceflow/packetin.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -213,11 +213,19 @@ func (c *Controller) parsePacketIn(pktIn *ofctrl.PacketIn) (*opsv1alpha1.Tracefl
return nil, nil, err
}
}
if (c.networkConfig.TrafficEncapMode.SupportsEncap() && outputPort == config.DefaultTunOFPort) || outputPort == config.HostGatewayOFPort {
// Output port is Tunnel/Gateway port, packet is forwarded.
// tunnelDstIP is valid IP in encapMode, and empty string in other modes.
gatewayIP := c.nodeConfig.GatewayConfig.IPv4
if pktIn.Data.Ethertype == protocol.IPv6_MSG {
gatewayIP = c.nodeConfig.GatewayConfig.IPv6
}
if c.networkConfig.TrafficEncapMode.SupportsEncap() && outputPort == config.DefaultTunOFPort {
ob.TunnelDstIP = tunnelDstIP
ob.Action = opsv1alpha1.Forwarded
} else if ipDst == gatewayIP.String() && outputPort == config.HostGatewayOFPort {
ob.Action = opsv1alpha1.Delivered
} else if c.networkConfig.TrafficEncapMode.SupportsEncap() && outputPort == config.HostGatewayOFPort {
ob.Action = opsv1alpha1.ForwardedOutOfOverlay
} else if outputPort == config.HostGatewayOFPort { // noEncap
ob.Action = opsv1alpha1.Forwarded
} else {
// Output port is Pod port, packet is delivered.
ob.Action = opsv1alpha1.Delivered
Expand Down
61 changes: 4 additions & 57 deletions pkg/agent/openflow/client.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -842,63 +842,10 @@ func (c *client) SendTraceflowPacket(
}

func (c *client) InstallTraceflowFlows(dataplaneTag uint8) error {
flows := c.traceflowL2ForwardOutputFlows(dataplaneTag, cookie.Default)
if err := c.AddAll(flows); err != nil {
return err
}
flow := c.traceflowConnectionTrackFlows(dataplaneTag, cookie.Default)
if err := c.Add(flow); err != nil {
return err
}
flows = []binding.Flow{}
c.conjMatchFlowLock.Lock()
defer c.conjMatchFlowLock.Unlock()
// Copy default drop rules.
for _, ctx := range c.globalConjMatchFlowCache {
if ctx.dropFlow != nil {
copyFlowBuilder := ctx.dropFlow.CopyToBuilder(priorityNormal+2, false)
if ctx.dropFlow.FlowProtocol() == "" {
copyFlowBuilderIPv6 := ctx.dropFlow.CopyToBuilder(priorityNormal+2, false)
copyFlowBuilderIPv6 = copyFlowBuilderIPv6.MatchProtocol(binding.ProtocolIPv6)
flows = append(
flows, copyFlowBuilderIPv6.MatchIPDscp(dataplaneTag).
SetHardTimeout(300).
Action().SendToController(uint8(PacketInReasonTF)).
Done())
copyFlowBuilder = copyFlowBuilder.MatchProtocol(binding.ProtocolIP)
}
flows = append(
flows, copyFlowBuilder.MatchIPDscp(dataplaneTag).
SetHardTimeout(300).
Action().SendToController(uint8(PacketInReasonTF)).
Done())
}
}
// Copy Antrea NetworkPolicy drop rules.
for _, conj := range c.policyCache.List() {
for _, flow := range conj.(*policyRuleConjunction).metricFlows {
if flow.IsDropFlow() {
copyFlowBuilder := flow.CopyToBuilder(priorityNormal+2, false)
// Generate both IPv4 and IPv6 flows if the original drop flow doesn't match IP/IPv6.
// DSCP field is in IP/IPv6 headers so IP/IPv6 match is required in a flow.
if flow.FlowProtocol() == "" {
copyFlowBuilderIPv6 := flow.CopyToBuilder(priorityNormal+2, false)
copyFlowBuilderIPv6 = copyFlowBuilderIPv6.MatchProtocol(binding.ProtocolIPv6)
flows = append(
flows, copyFlowBuilderIPv6.MatchIPDscp(dataplaneTag).
SetHardTimeout(300).
Action().SendToController(uint8(PacketInReasonTF)).
Done())
copyFlowBuilder = copyFlowBuilder.MatchProtocol(binding.ProtocolIP)
}
flows = append(
flows, copyFlowBuilder.MatchIPDscp(dataplaneTag).
SetHardTimeout(300).
Action().SendToController(uint8(PacketInReasonTF)).
Done())
}
}
}
flows := []binding.Flow{}
flows = append(flows, c.traceflowL2ForwardOutputFlows(dataplaneTag, cookie.Default)...)
flows = append(flows, c.traceflowConnectionTrackFlows(dataplaneTag, cookie.Default)...)
flows = append(flows, c.traceflowNetworkPolicyFlows(dataplaneTag, cookie.Default)...)
return c.AddAll(flows)
}

Expand Down
41 changes: 19 additions & 22 deletions pkg/agent/openflow/client_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -38,11 +38,21 @@ import (

const bridgeName = "dummy-br"

var bridgeMgmtAddr = ofconfig.GetMgmtAddress(ovsconfig.DefaultOVSRunDir, bridgeName)
var (
bridgeMgmtAddr = ofconfig.GetMgmtAddress(ovsconfig.DefaultOVSRunDir, bridgeName)
gwMAC, _ = net.ParseMAC("AA:BB:CC:DD:EE:EE")
gwIP, ipNet, _ = net.ParseCIDR("10.0.1.1/24")
gwIPv6, _, _ = net.ParseCIDR("f00d::b00:0:0:0/80")
gatewayConfig = &config.GatewayConfig{
IPv4: gwIP,
IPv6: gwIPv6,
MAC: gwMAC,
}
nodeConfig = &config.NodeConfig{GatewayConfig: gatewayConfig}
)

func installNodeFlows(ofClient Client, cacheKey string) (int, error) {
hostName := cacheKey
gwIP, ipNet, _ := net.ParseCIDR("10.0.1.1/24")
peerNodeIP := net.ParseIP("192.168.1.1")
peerConfig := map[*net.IPNet]net.IP{
ipNet: gwIP,
Expand Down Expand Up @@ -95,10 +105,7 @@ func TestIdempotentFlowInstallation(t *testing.T) {
client := ofClient.(*client)
client.cookieAllocator = cookie.NewAllocator(0)
client.ofEntryOperations = m

gwMAC, _ := net.ParseMAC("AA:BB:CC:DD:EE:EE")
gatewayConfig := &config.GatewayConfig{MAC: gwMAC}
client.nodeConfig = &config.NodeConfig{GatewayConfig: gatewayConfig}
client.nodeConfig = nodeConfig

m.EXPECT().AddAll(gomock.Any()).Return(nil).Times(1)
// Installing the flows should succeed, and all the flows should be added into the cache.
Expand Down Expand Up @@ -126,10 +133,7 @@ func TestIdempotentFlowInstallation(t *testing.T) {
client := ofClient.(*client)
client.cookieAllocator = cookie.NewAllocator(0)
client.ofEntryOperations = m

gwMAC, _ := net.ParseMAC("AA:BB:CC:DD:EE:EE")
gatewayConfig := &config.GatewayConfig{MAC: gwMAC}
client.nodeConfig = &config.NodeConfig{GatewayConfig: gatewayConfig}
client.nodeConfig = nodeConfig

errorCall := m.EXPECT().AddAll(gomock.Any()).Return(errors.New("Bundle error")).Times(1)
m.EXPECT().AddAll(gomock.Any()).Return(nil).After(errorCall)
Expand Down Expand Up @@ -170,10 +174,7 @@ func TestFlowInstallationFailed(t *testing.T) {
client := ofClient.(*client)
client.cookieAllocator = cookie.NewAllocator(0)
client.ofEntryOperations = m

gwMAC, _ := net.ParseMAC("AA:BB:CC:DD:EE:EE")
gatewayConfig := &config.GatewayConfig{MAC: gwMAC}
client.nodeConfig = &config.NodeConfig{GatewayConfig: gatewayConfig}
client.nodeConfig = nodeConfig

// We generate an error for AddAll call.
m.EXPECT().AddAll(gomock.Any()).Return(errors.New("Bundle error"))
Expand Down Expand Up @@ -207,10 +208,7 @@ func TestConcurrentFlowInstallation(t *testing.T) {
client := ofClient.(*client)
client.cookieAllocator = cookie.NewAllocator(0)
client.ofEntryOperations = m

gwMAC, _ := net.ParseMAC("AA:BB:CC:DD:EE:EE")
gatewayConfig := &config.GatewayConfig{MAC: gwMAC}
client.nodeConfig = &config.NodeConfig{GatewayConfig: gatewayConfig}
client.nodeConfig = nodeConfig

var concurrentCalls atomic.Value // set to true if we observe concurrent calls
timeoutCh := make(chan struct{})
Expand Down Expand Up @@ -472,9 +470,9 @@ func prepareTraceflowFlow(ctrl *gomock.Controller) *client {
ofClient := NewClient(bridgeName, bridgeMgmtAddr, ovsconfig.OVSDatapathSystem, true, true)
c := ofClient.(*client)
c.cookieAllocator = cookie.NewAllocator(0)
c.nodeConfig = &config.NodeConfig{}
c.nodeConfig = nodeConfig
m := ovsoftest.NewMockBridge(ctrl)
m.EXPECT().AddFlowsInBundle(gomock.Any(), nil, nil).Return(nil).Times(3)
m.EXPECT().AddFlowsInBundle(gomock.Any(), nil, nil).Return(nil).Times(1)
c.bridge = m

mFlow := ovsoftest.NewMockFlow(ctrl)
Expand All @@ -489,8 +487,7 @@ func prepareTraceflowFlow(ctrl *gomock.Controller) *client {
func prepareSendTraceflowPacket(ctrl *gomock.Controller, success bool) *client {
ofClient := NewClient(bridgeName, bridgeMgmtAddr, ovsconfig.OVSDatapathSystem, true, true)
c := ofClient.(*client)
mac, _ := net.ParseMAC("aa:bb:cc:dd:ee:ff")
c.nodeConfig = &config.NodeConfig{GatewayConfig: &config.GatewayConfig{MAC: mac}}
c.nodeConfig = nodeConfig
m := ovsoftest.NewMockBridge(ctrl)
c.bridge = m
bridge := ofconfig.OFBridge{}
Expand Down
Loading

0 comments on commit 8038935

Please sign in to comment.