Skip to content

Commit

Permalink
node: udn: Ensure UDN traffic doesn't leak into default network servi…
Browse files Browse the repository at this point in the history 
…ces.

In local gateway mode all UDN traffic that needs to exit the node
was redirected to the host (via the mp-X port).  There however it was
injected into the br-ex/breth0 bridge where it got sent into the default
network patch port (because the destination IP is that of a service
CIDR).  That traffic eventually reached the default network endpoint and
the connection was successfully set up.

The mode above is not the correct mode of operation for UDN.  UDN pods
should not be allowed to access any default network services (except
select ones like kapi).

To achieve that we change the br-ex/breth0 flows and add new per-UDN
flows that detect traffic originated by a given network and only direct
it back to that network's patch port.

This commit updates the e2e test to cover this scenario.  This change
also indirectly fixes the fact that node port default services should
not be accessible from UDN when the target IP is the local node IP (that
traffic is now sent back to the UDN GR where no LB is configured for the
default service).

New flow specific unit tests are also added to make sure these flows are
not accidentally removed in the future.

Signed-off-by: Dumitru Ceara <[email protected]>
  • Loading branch information
@dceara
dceara committed Sep 19, 2024
1 parent 6201f3f commit 44ff253
Show file tree
Hide file tree
Showing 5 changed files with 183 additions and 47 deletions.
73 changes: 59 additions & 14 deletions go-controller/pkg/node/gateway_shared_intf.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -280,9 +280,9 @@ func (npw *nodePortWatcher) updateServiceFlowCache(service *kapi.Service, netInf

// Add flows for default network services that are accessible from UDN networks
if util.IsNetworkSegmentationSupportEnabled() {
// The flow added below has a higher priority than the default network service flow:
// priority=500,ip,in_port=LOCAL,nw_dst=10.96.0.0/16 actions=ct(commit,table=2,zone=64001,nat(src=169.254.0.2))
// This ordering ensures that there is no SNAT for UDN originated traffic.
// The flow added below has a higher priority than the per UDN service flow:
// priority=200, table=2, ip, ip_src=169.254.0.<UDN>, actions=set_field:<bridge-mac>->eth_dst,output:<UDN-patch-port>
// This ordering ensures that traffic to UDN allowed default services goes to the the default patch port.

if util.IsUDNEnabledService(ktypes.NamespacedName{Namespace: service.Namespace, Name: service.Name}.String()) {
key = strings.Join([]string{"UDNAllowedSVC", service.Namespace, service.Name}, "_")
Expand All @@ -296,10 +296,13 @@ func (npw *nodePortWatcher) updateServiceFlowCache(service *kapi.Service, netInf
ipPrefix = "ipv6"
masqueradeSubnet = config.Gateway.V6MasqueradeSubnet
}
// table 0, user-defined network host -> OVN towards default cluster network services
npw.ofm.updateFlowCacheEntry(key, []string{fmt.Sprintf("cookie=%s, priority=600, in_port=%s, %s, %s_src=%s, %s_dst=%s,"+
"actions=ct(commit,zone=%d,table=2)",
defaultOpenFlowCookie, npw.ofm.defaultBridge.ofPortHost, ipPrefix, ipPrefix, masqueradeSubnet, ipPrefix, service.Spec.ClusterIP, config.Default.HostMasqConntrackZone)})
// table 2, user-defined network host -> OVN towards default cluster network services
defaultNetConfig := npw.ofm.defaultBridge.netConfig[types.DefaultNetworkName]

npw.ofm.updateFlowCacheEntry(key, []string{fmt.Sprintf("cookie=%s, priority=300, table=2, %s, %s_src=%s, %s_dst=%s, "+
"actions=set_field:%s->eth_dst,output:%s",
defaultOpenFlowCookie, ipPrefix, ipPrefix, masqueradeSubnet, ipPrefix, service.Spec.ClusterIP,
npw.ofm.getDefaultBridgeMAC().String(), defaultNetConfig.ofPortPatch)})
}
}
return utilerrors.Join(errors...)
Expand Down Expand Up @@ -1317,11 +1320,23 @@ func flowsForDefaultBridge(bridge *bridgeConfiguration, extraIPs []net.IP) ([]st
masqSubnet = config.Gateway.V6MasqueradeSubnet
}

// table 0, Host -> OVN towards SVC, SNAT to special IP
// table 0, Host (default network) -> OVN towards SVC, SNAT to special IP.
dftFlows = append(dftFlows,
fmt.Sprintf("cookie=%s, priority=500, in_port=%s, %s, %s_dst=%s,"+
fmt.Sprintf("cookie=%s, priority=500, in_port=%s, %s, %s_dst=%s, "+
"actions=ct(commit,zone=%d,nat(src=%s),table=2)",
defaultOpenFlowCookie, ofPortHost, protoPrefix, protoPrefix, svcCIDR, config.Default.HostMasqConntrackZone, masqIP))
defaultOpenFlowCookie, ofPortHost, protoPrefix, protoPrefix,
svcCIDR, config.Default.HostMasqConntrackZone, masqIP))

if util.IsNetworkSegmentationSupportEnabled() {
// table 0, Host (UDNs) -> OVN towards SVC, SNAT to special IP.
// For packets originating from UDN, commit without NATing, those
// have already been SNATed to the masq IP of the UDN.
dftFlows = append(dftFlows,
fmt.Sprintf("cookie=%s, priority=550, in_port=%s, %s, %s_src=%s, %s_dst=%s, "+
"actions=ct(commit,zone=%d,table=2)",
defaultOpenFlowCookie, ofPortHost, protoPrefix, protoPrefix,
masqSubnet, protoPrefix, svcCIDR, config.Default.HostMasqConntrackZone))
}

masqDst := masqIP
if util.IsNetworkSegmentationSupportEnabled() {
Expand Down Expand Up @@ -1421,8 +1436,38 @@ func flowsForDefaultBridge(bridge *bridgeConfiguration, extraIPs []net.IP) ([]st

// table 2, dispatch from Host -> OVN
dftFlows = append(dftFlows,
fmt.Sprintf("cookie=%s, table=2, "+
"actions=set_field:%s->eth_dst,output:%s", defaultOpenFlowCookie, bridgeMacAddress, defaultNetConfig.ofPortPatch))
fmt.Sprintf("cookie=%s, priority=100, table=2, "+
"actions=set_field:%s->eth_dst,output:%s", defaultOpenFlowCookie,
bridgeMacAddress, defaultNetConfig.ofPortPatch))

// table 2, priority 200, dispatch from UDN -> Host -> OVN. These packets have
// already been SNATed to the UDN's masq IP.
if config.IPv4Mode {
for _, netConfig := range bridge.patchedNetConfigs() {
if netConfig.masqCTMark == ctMarkOVN {
continue
}
dftFlows = append(dftFlows,
fmt.Sprintf("cookie=%s, priority=200, table=2, ip, ip_src=%s, "+
"actions=set_field:%s->eth_dst,output:%s",
defaultOpenFlowCookie, netConfig.v4MasqIPs.ManagementPort.IP,
bridgeMacAddress, netConfig.ofPortPatch))
}
}

if config.IPv6Mode {
for _, netConfig := range bridge.patchedNetConfigs() {
if netConfig.masqCTMark == ctMarkOVN {
continue
}

dftFlows = append(dftFlows,
fmt.Sprintf("cookie=%s, priority=200, table=2, ip6, ipv6_src=%s, "+
"actions=set_field:%s->eth_dst,output:%s",
defaultOpenFlowCookie, netConfig.v6MasqIPs.ManagementPort.IP,
bridgeMacAddress, netConfig.ofPortPatch))
}
}

// table 3, dispatch from OVN -> Host
dftFlows = append(dftFlows,
Expand Down Expand Up @@ -1523,7 +1568,7 @@ func commonFlows(subnets []*net.IPNet, bridge *bridgeConfiguration) ([]string, e
dftFlows = append(dftFlows,
fmt.Sprintf("cookie=%s, priority=100, in_port=%s, dl_src=%s, ip, ip_src=%s, "+
"actions=ct(commit, zone=%d, nat(src=%s), exec(set_field:%s->ct_mark)), output:%s",
defaultOpenFlowCookie, netConfig.ofPortPatch, bridgeMacAddress, netConfig.v4MasqIP.IP, config.Default.ConntrackZone,
defaultOpenFlowCookie, netConfig.ofPortPatch, bridgeMacAddress, netConfig.v4MasqIPs.GatewayRouter.IP, config.Default.ConntrackZone,
physicalIP.IP, netConfig.masqCTMark, ofPortPhys))
}
}
Expand Down Expand Up @@ -1598,7 +1643,7 @@ func commonFlows(subnets []*net.IPNet, bridge *bridgeConfiguration) ([]string, e
dftFlows = append(dftFlows,
fmt.Sprintf("cookie=%s, priority=100, in_port=%s, dl_src=%s, ipv6, ipv6_src=%s, "+
"actions=ct(commit, zone=%d, nat(src=%s), exec(set_field:%s->ct_mark)), output:%s",
defaultOpenFlowCookie, netConfig.ofPortPatch, bridgeMacAddress, netConfig.v6MasqIP.IP, config.Default.ConntrackZone,
defaultOpenFlowCookie, netConfig.ofPortPatch, bridgeMacAddress, netConfig.v6MasqIPs.GatewayRouter.IP, config.Default.ConntrackZone,
physicalIP.IP, netConfig.masqCTMark, ofPortPhys))
}
}
Expand Down
35 changes: 17 additions & 18 deletions go-controller/pkg/node/gateway_udn.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -52,10 +52,10 @@ type UserDefinedNetworkGateway struct {
// masqCTMark holds the mark value for this network
// which is used for egress traffic in shared gateway mode
masqCTMark uint
// v4MasqIP holds the IPv4 masquerade IP for this network
v4MasqIP *net.IPNet
// v6MasqIP holds the IPv6 masquerade IP for this network
v6MasqIP *net.IPNet
// v4MasqIPs holds the IPv4 masquerade IPs for this network
v4MasqIPs *udn.MasqueradeIPs
// v6MasqIPs holds the IPv6 masquerade IPs for this network
v6MasqIPs *udn.MasqueradeIPs
// stores the pointer to default network's gateway so that
// we can leverage it from here to program UDN flows on breth0
// Currently we use the openflowmanager and nodeIPManager from
Expand Down Expand Up @@ -83,7 +83,7 @@ func (b *bridgeConfiguration) getBridgePortConfigurations() ([]bridgeUDNConfigur
}

// addNetworkBridgeConfig adds the patchport and ctMark value for the provided netInfo into the bridge configuration cache
func (b *bridgeConfiguration) addNetworkBridgeConfig(nInfo util.NetInfo, masqCTMark uint, v4MasqIP, v6MasqIP *net.IPNet) {
func (b *bridgeConfiguration) addNetworkBridgeConfig(nInfo util.NetInfo, masqCTMark uint, v4MasqIPs, v6MasqIPs *udn.MasqueradeIPs) {
b.Lock()
defer b.Unlock()

Expand All @@ -95,8 +95,8 @@ func (b *bridgeConfiguration) addNetworkBridgeConfig(nInfo util.NetInfo, masqCTM
netConfig := &bridgeUDNConfiguration{
patchPort: patchPort,
masqCTMark: fmt.Sprintf("0x%x", masqCTMark),
v4MasqIP: v4MasqIP,
v6MasqIP: v6MasqIP,
v4MasqIPs: v4MasqIPs,
v6MasqIPs: v6MasqIPs,
}

b.netConfig[netName] = netConfig
Expand Down Expand Up @@ -149,8 +149,8 @@ type bridgeUDNConfiguration struct {
patchPort string
ofPortPatch string
masqCTMark string
v4MasqIP *net.IPNet
v6MasqIP *net.IPNet
v4MasqIPs *udn.MasqueradeIPs
v6MasqIPs *udn.MasqueradeIPs
}

func (netConfig *bridgeUDNConfiguration) setBridgeNetworkOfPortsInternal() error {
Expand Down Expand Up @@ -179,23 +179,22 @@ func NewUserDefinedNetworkGateway(netInfo util.NetInfo, networkID int, node *v1.
defaultNetworkGateway Gateway) (*UserDefinedNetworkGateway, error) {
// Generate a per network conntrack mark and masquerade IPs to be used for egress traffic.
var (
v4MasqIP *net.IPNet
v6MasqIP *net.IPNet
v4MasqIPs *udn.MasqueradeIPs
v6MasqIPs *udn.MasqueradeIPs
err error
)
masqCTMark := ctMarkUDNBase + uint(networkID)
if config.IPv4Mode {
v4MasqIPs, err := udn.AllocateV4MasqueradeIPs(networkID)
v4MasqIPs, err = udn.AllocateV4MasqueradeIPs(networkID)
if err != nil {
return nil, fmt.Errorf("failed to get v4 masquerade IP, network %s (%d): %v", netInfo.GetNetworkName(), networkID, err)
}
v4MasqIP = v4MasqIPs.GatewayRouter
}
if config.IPv6Mode {
v6MasqIPs, err := udn.AllocateV6MasqueradeIPs(networkID)
v6MasqIPs, err = udn.AllocateV6MasqueradeIPs(networkID)
if err != nil {
return nil, fmt.Errorf("failed to get v6 masquerade IP, network %s (%d): %v", netInfo.GetNetworkName(), networkID, err)
}
v6MasqIP = v6MasqIPs.GatewayRouter
}

gw, ok := defaultNetworkGateway.(*gateway)
Expand All @@ -211,8 +210,8 @@ func NewUserDefinedNetworkGateway(netInfo util.NetInfo, networkID int, node *v1.
kubeInterface: kubeInterface,
vrfManager: vrfManager,
masqCTMark: masqCTMark,
v4MasqIP: v4MasqIP,
v6MasqIP: v6MasqIP,
v4MasqIPs: v4MasqIPs,
v6MasqIPs: v6MasqIPs,
gateway: gw,
ruleManager: ruleManager,
}, nil
Expand Down Expand Up @@ -258,7 +257,7 @@ func (udng *UserDefinedNetworkGateway) AddNetwork() error {
return fmt.Errorf("could not set loose mode for reverse path filtering on management port %s: %v", mgmtPortName, err)
}
if udng.openflowManager != nil {
udng.openflowManager.addNetwork(udng.NetInfo, udng.masqCTMark, udng.v4MasqIP, udng.v6MasqIP)
udng.openflowManager.addNetwork(udng.NetInfo, udng.masqCTMark, udng.v4MasqIPs, udng.v6MasqIPs)

waiter := newStartupWaiterWithTimeout(waitForPatchPortTimeout)
readyFunc := func() (bool, error) {
Expand Down
105 changes: 101 additions & 4 deletions go-controller/pkg/node/gateway_udn_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ import (
corev1 "k8s.io/api/core/v1"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/client-go/kubernetes/fake"
utilnet "k8s.io/utils/net"
"k8s.io/utils/ptr"

"github.com/ovn-org/ovn-kubernetes/go-controller/pkg/config"
Expand Down Expand Up @@ -176,6 +177,61 @@ func setUpUDNOpenflowManagerFakeOVSCommands(fexec *ovntest.FakeExec) {
})
}

func checkDefaultSvcIsolationOVSFlows(flows []string, defaultConfig *bridgeUDNConfiguration, ofPortHost, bridgeMAC string, svcCIDR *net.IPNet) {
By(fmt.Sprintf("Checking default service isolation flows for %s", svcCIDR.String()))

var masqIP string
var protoPrefix string
if utilnet.IsIPv4CIDR(svcCIDR) {
protoPrefix = "ip"
masqIP = config.Gateway.MasqueradeIPs.V4HostMasqueradeIP.String()
} else {
protoPrefix = "ip6"
masqIP = config.Gateway.MasqueradeIPs.V6HostMasqueradeIP.String()
}

var nTable0Flows int
var nTable2Flows int
for _, flow := range flows {
if strings.Contains(flow, fmt.Sprintf("priority=500, in_port=%s, %s, %s_dst=%s, actions=ct(commit,zone=%d,nat(src=%s),table=2)",
ofPortHost, protoPrefix, protoPrefix, svcCIDR, config.Default.HostMasqConntrackZone,
masqIP)) {
nTable0Flows++
} else if strings.Contains(flow, fmt.Sprintf("priority=100, table=2, actions=set_field:%s->eth_dst,output:%s",
bridgeMAC, defaultConfig.ofPortPatch)) {
nTable2Flows++
}
}

Expect(nTable0Flows).To(Equal(1))
Expect(nTable2Flows).To(Equal(1))
}

func checkUDNSvcIsolationOVSFlows(flows []string, netConfig *bridgeUDNConfiguration, netName, ofPortHost, bridgeMAC string, svcCIDR *net.IPNet, expectedNFlows int) {
By(fmt.Sprintf("Checking UDN %s service isolation flows for %s; expected %d flows",
netName, svcCIDR.String(), expectedNFlows))

var mgmtMasqIP string
var protoPrefix string
if utilnet.IsIPv4CIDR(svcCIDR) {
mgmtMasqIP = netConfig.v4MasqIPs.ManagementPort.IP.String()
protoPrefix = "ip"
} else {
mgmtMasqIP = netConfig.v6MasqIPs.ManagementPort.IP.String()
protoPrefix = "ip6"
}

var nFlows int
for _, flow := range flows {
if strings.Contains(flow, fmt.Sprintf("priority=200, table=2, %s, %s_src=%s, actions=set_field:%s->eth_dst,output:%s",
protoPrefix, protoPrefix, mgmtMasqIP, bridgeMAC, netConfig.ofPortPatch)) {
nFlows++
}
}

Expect(nFlows).To(Equal(expectedNFlows))
}

var _ = Describe("UserDefinedNetworkGateway", func() {
var (
netName = "bluenet"
Expand Down Expand Up @@ -522,6 +578,7 @@ var _ = Describe("UserDefinedNetworkGateway", func() {
// FIXME: extract openflow manager func from the spawning of a go routine so it can be called directly below.
localGw.openflowManager.syncFlows()
flowMap := udnGateway.gateway.openflowManager.flowCache

Expect(len(flowMap["DEFAULT"])).To(Equal(45))
Expect(udnGateway.masqCTMark).To(Equal(udnGateway.masqCTMark))
var udnFlows int
Expand All @@ -539,20 +596,32 @@ var _ = Describe("UserDefinedNetworkGateway", func() {

Expect(udnGateway.AddNetwork()).To(Succeed())
flowMap = udnGateway.gateway.openflowManager.flowCache
Expect(len(flowMap["DEFAULT"])).To(Equal(59)) // 14 UDN Flows are added by default
Expect(len(flowMap["DEFAULT"])).To(Equal(61)) // 16 UDN Flows are added by default
Expect(len(udnGateway.openflowManager.defaultBridge.netConfig)).To(Equal(2)) // default network + UDN network
defaultUdnConfig := udnGateway.openflowManager.defaultBridge.netConfig["default"]
bridgeUdnConfig := udnGateway.openflowManager.defaultBridge.netConfig["bluenet"]
bridgeMAC := udnGateway.openflowManager.defaultBridge.macAddress.String()
ofPortHost := udnGateway.openflowManager.defaultBridge.ofPortHost
for _, flows := range flowMap {
for _, flow := range flows {
if strings.Contains(flow, fmt.Sprintf("0x%x", udnGateway.masqCTMark)) {
// UDN Flow
udnFlows++
} else if strings.Contains(flow, fmt.Sprintf("in_port=%s", udnGateway.openflowManager.defaultBridge.netConfig["bluenet"].ofPortPatch)) {
} else if strings.Contains(flow, fmt.Sprintf("in_port=%s", bridgeUdnConfig.ofPortPatch)) {
udnFlows++
}
}
}
Expect(udnFlows).To(Equal(14))

for _, svcCIDR := range config.Kubernetes.ServiceCIDRs {
// Check flows for default network service CIDR.
checkDefaultSvcIsolationOVSFlows(flowMap["DEFAULT"], defaultUdnConfig, ofPortHost, bridgeMAC, svcCIDR)

// Expect exactly one flow per UDN for table 2 for service isolation.
checkUDNSvcIsolationOVSFlows(flowMap["DEFAULT"], bridgeUdnConfig, "bluenet", ofPortHost, bridgeMAC, svcCIDR, 1)
}

cnode := node.DeepCopy()
kubeMock.On("UpdateNodeStatus", cnode).Return(nil) // check if network key gets deleted from annotation
Expect(udnGateway.DelNetwork()).To(Succeed())
Expand All @@ -569,6 +638,14 @@ var _ = Describe("UserDefinedNetworkGateway", func() {
}
}
Expect(udnFlows).To(Equal(0))

for _, svcCIDR := range config.Kubernetes.ServiceCIDRs {
// Check flows for default network service CIDR.
checkDefaultSvcIsolationOVSFlows(flowMap["DEFAULT"], defaultUdnConfig, ofPortHost, bridgeMAC, svcCIDR)

// Expect no more flows per UDN for table 2 for service isolation.
checkUDNSvcIsolationOVSFlows(flowMap["DEFAULT"], bridgeUdnConfig, "bluenet", ofPortHost, bridgeMAC, svcCIDR, 0)
}
return nil
})
Expect(err).NotTo(HaveOccurred())
Expand Down Expand Up @@ -711,20 +788,32 @@ var _ = Describe("UserDefinedNetworkGateway", func() {

Expect(udnGateway.AddNetwork()).To(Succeed())
flowMap = udnGateway.gateway.openflowManager.flowCache
Expect(len(flowMap["DEFAULT"])).To(Equal(59)) // 14 UDN Flows are added by default
Expect(len(flowMap["DEFAULT"])).To(Equal(61)) // 16 UDN Flows are added by default
Expect(len(udnGateway.openflowManager.defaultBridge.netConfig)).To(Equal(2)) // default network + UDN network
defaultUdnConfig := udnGateway.openflowManager.defaultBridge.netConfig["default"]
bridgeUdnConfig := udnGateway.openflowManager.defaultBridge.netConfig["bluenet"]
bridgeMAC := udnGateway.openflowManager.defaultBridge.macAddress.String()
ofPortHost := udnGateway.openflowManager.defaultBridge.ofPortHost
for _, flows := range flowMap {
for _, flow := range flows {
if strings.Contains(flow, fmt.Sprintf("0x%x", udnGateway.masqCTMark)) {
// UDN Flow
udnFlows++
} else if strings.Contains(flow, fmt.Sprintf("in_port=%s", udnGateway.openflowManager.defaultBridge.netConfig["bluenet"].ofPortPatch)) {
} else if strings.Contains(flow, fmt.Sprintf("in_port=%s", bridgeUdnConfig.ofPortPatch)) {
udnFlows++
}
}
}
Expect(udnFlows).To(Equal(14))

for _, svcCIDR := range config.Kubernetes.ServiceCIDRs {
// Check flows for default network service CIDR.
checkDefaultSvcIsolationOVSFlows(flowMap["DEFAULT"], defaultUdnConfig, ofPortHost, bridgeMAC, svcCIDR)

// Expect exactly one flow per UDN for tables 0 and 2 for service isolation.
checkUDNSvcIsolationOVSFlows(flowMap["DEFAULT"], bridgeUdnConfig, "bluenet", ofPortHost, bridgeMAC, svcCIDR, 1)
}

cnode := node.DeepCopy()
kubeMock.On("UpdateNodeStatus", cnode).Return(nil) // check if network key gets deleted from annotation
Expect(udnGateway.DelNetwork()).To(Succeed())
Expand All @@ -741,6 +830,14 @@ var _ = Describe("UserDefinedNetworkGateway", func() {
}
}
Expect(udnFlows).To(Equal(0))

for _, svcCIDR := range config.Kubernetes.ServiceCIDRs {
// Check flows for default network service CIDR.
checkDefaultSvcIsolationOVSFlows(flowMap["DEFAULT"], defaultUdnConfig, ofPortHost, bridgeMAC, svcCIDR)

// Expect no more flows per UDN for tables 0 and 2 for service isolation.
checkUDNSvcIsolationOVSFlows(flowMap["DEFAULT"], bridgeUdnConfig, "bluenet", ofPortHost, bridgeMAC, svcCIDR, 0)
}
return nil
})
Expect(err).NotTo(HaveOccurred())
Expand Down
Loading

0 comments on commit 44ff253

Please sign in to comment.