Skip to content

Commit

Permalink
Allow Node SNAT for Static Egress case
Browse files Browse the repository at this point in the history 
Implemented best effort scenario, where in case of
static Egress also, if there is no egress node then
the packets will be sent using normal Node SNAT, as
in case of dynamic Egress.

Signed-off-by: Pulkit Jain <[email protected]>
  • Loading branch information
@jainpulkit22
jainpulkit22 committed Nov 27, 2024
1 parent 18e9111 commit e64d3c5
Show file tree
Hide file tree
Showing 2 changed files with 30 additions and 75 deletions.
58 changes: 30 additions & 28 deletions pkg/agent/controller/egress/egress_controller.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1105,7 +1105,6 @@ func (c *EgressController) syncEgress(egressName string) error {
// Copy the previous ofPorts and Pods. They will be used to identify stale ofPorts and Pods.
staleOFPorts := eState.ofPorts.Union(nil)
stalePods := eState.pods.Union(nil)

// Get a copy of the desired Pods.
pods := func() sets.Set[string] {
c.egressGroupsMutex.RLock()
Expand All @@ -1118,39 +1117,42 @@ func (c *EgressController) syncEgress(egressName string) error {
}()

egressIP := net.ParseIP(eState.egressIP)
// Install SNAT flows for desired Pods.
for pod := range pods {
eState.pods.Insert(pod)
stalePods.Delete(pod)
egress, _ = c.egressLister.Get(egressName)
if egress.Status.EgressNode != "" {
// Install SNAT flows for desired Pods.
for pod := range pods {
eState.pods.Insert(pod)
stalePods.Delete(pod)

// If the Egress is not the effective one for the Pod, do nothing.
if !c.bindPodEgress(pod, egressName) {
continue
}

// If the Egress is not the effective one for the Pod, do nothing.
if !c.bindPodEgress(pod, egressName) {
continue
}
// Get the Pod's openflow port.
parts := strings.Split(pod, "/")
podNamespace, podName := parts[0], parts[1]
ifaces := c.ifaceStore.GetContainerInterfacesByPod(podName, podNamespace)
if len(ifaces) == 0 {
klog.Infof("Interfaces of Pod %s/%s not found", podNamespace, podName)
continue
}

// Get the Pod's openflow port.
parts := strings.Split(pod, "/")
podNamespace, podName := parts[0], parts[1]
ifaces := c.ifaceStore.GetContainerInterfacesByPod(podName, podNamespace)
if len(ifaces) == 0 {
klog.Infof("Interfaces of Pod %s/%s not found", podNamespace, podName)
continue
ofPort := ifaces[0].OFPort
if eState.ofPorts.Has(ofPort) {
staleOFPorts.Delete(ofPort)
continue
}
if err := c.ofClient.InstallPodSNATFlows(uint32(ofPort), egressIP, mark); err != nil {
return err
}
eState.ofPorts.Insert(ofPort)
}

ofPort := ifaces[0].OFPort
if eState.ofPorts.Has(ofPort) {
staleOFPorts.Delete(ofPort)
continue
}
if err := c.ofClient.InstallPodSNATFlows(uint32(ofPort), egressIP, mark); err != nil {
// Uninstall SNAT flows for stale Pods.
if err := c.uninstallPodFlows(egressName, eState, staleOFPorts, stalePods); err != nil {
return err
}
eState.ofPorts.Insert(ofPort)
}

// Uninstall SNAT flows for stale Pods.
if err := c.uninstallPodFlows(egressName, eState, staleOFPorts, stalePods); err != nil {
return err
}
return nil
}
Expand Down
47 changes: 0 additions & 47 deletions pkg/agent/controller/egress/egress_controller_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -271,20 +271,14 @@ func TestSyncEgress(t *testing.T) {
expectedCalls: func(mockOFClient *openflowtest.MockClient, mockRouteClient *routetest.MockInterface, mockIPAssigner *ipassignertest.MockIPAssigner) {
mockOFClient.EXPECT().InstallEgressQoS(uint32(1), uint32(500), uint32(500))
mockOFClient.EXPECT().InstallSNATMarkFlows(net.ParseIP(fakeLocalEgressIP1), uint32(1))
mockOFClient.EXPECT().InstallPodSNATFlows(uint32(1), net.ParseIP(fakeLocalEgressIP1), uint32(1))
mockOFClient.EXPECT().InstallPodSNATFlows(uint32(2), net.ParseIP(fakeLocalEgressIP1), uint32(1))
mockRouteClient.EXPECT().AddSNATRule(net.ParseIP(fakeLocalEgressIP1), uint32(1))
mockIPAssigner.EXPECT().UnassignIP(fakeLocalEgressIP1).Return(false, nil)

mockOFClient.EXPECT().UninstallSNATMarkFlows(uint32(1))
mockRouteClient.EXPECT().DeleteSNATRule(uint32(1))
mockOFClient.EXPECT().UninstallPodSNATFlows(uint32(1))
mockOFClient.EXPECT().UninstallPodSNATFlows(uint32(2))
mockIPAssigner.EXPECT().UnassignIP(fakeLocalEgressIP1).Return(false, nil)
mockOFClient.EXPECT().UninstallEgressQoS(uint32(1))

mockOFClient.EXPECT().InstallPodSNATFlows(uint32(1), net.ParseIP(fakeLocalEgressIP1), uint32(0))
mockOFClient.EXPECT().InstallPodSNATFlows(uint32(3), net.ParseIP(fakeLocalEgressIP1), uint32(0))
mockIPAssigner.EXPECT().UnassignIP(fakeLocalEgressIP1).Return(false, nil)
},
},
Expand Down Expand Up @@ -321,12 +315,8 @@ func TestSyncEgress(t *testing.T) {
},
},
expectedCalls: func(mockOFClient *openflowtest.MockClient, mockRouteClient *routetest.MockInterface, mockIPAssigner *ipassignertest.MockIPAssigner) {
mockOFClient.EXPECT().InstallPodSNATFlows(uint32(1), net.ParseIP(fakeRemoteEgressIP1), uint32(0))
mockOFClient.EXPECT().InstallPodSNATFlows(uint32(2), net.ParseIP(fakeRemoteEgressIP1), uint32(0))
mockIPAssigner.EXPECT().UnassignIP(fakeRemoteEgressIP1).Return(false, nil)

mockOFClient.EXPECT().UninstallPodSNATFlows(uint32(1))
mockOFClient.EXPECT().UninstallPodSNATFlows(uint32(2))
mockIPAssigner.EXPECT().UnassignIP(fakeRemoteEgressIP1).Return(false, nil)

mockOFClient.EXPECT().InstallEgressQoS(uint32(1), uint32(500), uint32(500))
Expand Down Expand Up @@ -371,16 +361,12 @@ func TestSyncEgress(t *testing.T) {
expectedCalls: func(mockOFClient *openflowtest.MockClient, mockRouteClient *routetest.MockInterface, mockIPAssigner *ipassignertest.MockIPAssigner) {
mockOFClient.EXPECT().InstallEgressQoS(uint32(1), uint32(500), uint32(500))
mockOFClient.EXPECT().InstallSNATMarkFlows(net.ParseIP(fakeLocalEgressIP1), uint32(1))
mockOFClient.EXPECT().InstallPodSNATFlows(uint32(1), net.ParseIP(fakeLocalEgressIP1), uint32(1))
mockOFClient.EXPECT().InstallPodSNATFlows(uint32(2), net.ParseIP(fakeLocalEgressIP1), uint32(1))
mockRouteClient.EXPECT().AddSNATRule(net.ParseIP(fakeLocalEgressIP1), uint32(1))
mockIPAssigner.EXPECT().UnassignIP(fakeLocalEgressIP1).Return(false, nil)

mockOFClient.EXPECT().UninstallEgressQoS(uint32(1))
mockOFClient.EXPECT().UninstallSNATMarkFlows(uint32(1))
mockRouteClient.EXPECT().DeleteSNATRule(uint32(1))
mockOFClient.EXPECT().UninstallPodSNATFlows(uint32(1))
mockOFClient.EXPECT().UninstallPodSNATFlows(uint32(2))
mockIPAssigner.EXPECT().UnassignIP(fakeLocalEgressIP1).Return(false, nil)
mockIPAssigner.EXPECT().UnassignIP(fakeLocalEgressIP2).Return(false, nil)

Expand Down Expand Up @@ -425,21 +411,15 @@ func TestSyncEgress(t *testing.T) {
expectedCalls: func(mockOFClient *openflowtest.MockClient, mockRouteClient *routetest.MockInterface, mockIPAssigner *ipassignertest.MockIPAssigner) {
mockOFClient.EXPECT().InstallEgressQoS(uint32(1), uint32(500), uint32(500))
mockOFClient.EXPECT().InstallSNATMarkFlows(net.ParseIP(fakeLocalEgressIP1), uint32(1))
mockOFClient.EXPECT().InstallPodSNATFlows(uint32(1), net.ParseIP(fakeLocalEgressIP1), uint32(1))
mockOFClient.EXPECT().InstallPodSNATFlows(uint32(2), net.ParseIP(fakeLocalEgressIP1), uint32(1))
mockRouteClient.EXPECT().AddSNATRule(net.ParseIP(fakeLocalEgressIP1), uint32(1))
mockIPAssigner.EXPECT().UnassignIP(fakeLocalEgressIP1).Return(false, nil)

mockOFClient.EXPECT().UninstallSNATMarkFlows(uint32(1))
mockRouteClient.EXPECT().DeleteSNATRule(uint32(1))
mockOFClient.EXPECT().UninstallPodSNATFlows(uint32(1))
mockOFClient.EXPECT().UninstallPodSNATFlows(uint32(2))
mockIPAssigner.EXPECT().UnassignIP(fakeLocalEgressIP1).Return(false, nil)
mockIPAssigner.EXPECT().UnassignIP(fakeRemoteEgressIP1).Return(false, nil)
mockOFClient.EXPECT().UninstallEgressQoS(uint32(1))

mockOFClient.EXPECT().InstallPodSNATFlows(uint32(1), net.ParseIP(fakeRemoteEgressIP1), uint32(0))
mockOFClient.EXPECT().InstallPodSNATFlows(uint32(3), net.ParseIP(fakeRemoteEgressIP1), uint32(0))
mockIPAssigner.EXPECT().UnassignIP(fakeRemoteEgressIP1).Return(false, nil)
},
},
Expand Down Expand Up @@ -475,12 +455,8 @@ func TestSyncEgress(t *testing.T) {
},
},
expectedCalls: func(mockOFClient *openflowtest.MockClient, mockRouteClient *routetest.MockInterface, mockIPAssigner *ipassignertest.MockIPAssigner) {
mockOFClient.EXPECT().InstallPodSNATFlows(uint32(1), net.ParseIP(fakeRemoteEgressIP1), uint32(0))
mockOFClient.EXPECT().InstallPodSNATFlows(uint32(2), net.ParseIP(fakeRemoteEgressIP1), uint32(0))
mockIPAssigner.EXPECT().UnassignIP(fakeRemoteEgressIP1).Return(false, nil)

mockOFClient.EXPECT().UninstallPodSNATFlows(uint32(1))
mockOFClient.EXPECT().UninstallPodSNATFlows(uint32(2))
mockIPAssigner.EXPECT().UnassignIP(fakeRemoteEgressIP1).Return(false, nil)
mockIPAssigner.EXPECT().UnassignIP(fakeLocalEgressIP1).Return(false, nil)

Expand Down Expand Up @@ -530,13 +506,10 @@ func TestSyncEgress(t *testing.T) {
},
expectedCalls: func(mockOFClient *openflowtest.MockClient, mockRouteClient *routetest.MockInterface, mockIPAssigner *ipassignertest.MockIPAssigner) {
mockOFClient.EXPECT().InstallSNATMarkFlows(net.ParseIP(fakeLocalEgressIP1), uint32(1))
mockOFClient.EXPECT().InstallPodSNATFlows(uint32(1), net.ParseIP(fakeLocalEgressIP1), uint32(1))
mockOFClient.EXPECT().InstallPodSNATFlows(uint32(2), net.ParseIP(fakeLocalEgressIP1), uint32(1))
mockRouteClient.EXPECT().AddSNATRule(net.ParseIP(fakeLocalEgressIP1), uint32(1))
mockIPAssigner.EXPECT().UnassignIP(fakeLocalEgressIP1).Return(false, nil)

mockOFClient.EXPECT().InstallSNATMarkFlows(net.ParseIP(fakeLocalEgressIP2), uint32(2))
mockOFClient.EXPECT().InstallPodSNATFlows(uint32(3), net.ParseIP(fakeLocalEgressIP2), uint32(2))
mockRouteClient.EXPECT().AddSNATRule(net.ParseIP(fakeLocalEgressIP2), uint32(2))
mockIPAssigner.EXPECT().UnassignIP(fakeLocalEgressIP2).Return(false, nil)

Expand Down Expand Up @@ -582,7 +555,6 @@ func TestSyncEgress(t *testing.T) {
expectedCalls: func(mockOFClient *openflowtest.MockClient, mockRouteClient *routetest.MockInterface, mockIPAssigner *ipassignertest.MockIPAssigner) {
mockOFClient.EXPECT().InstallSNATMarkFlows(net.ParseIP(fakeLocalEgressIP1), uint32(1))
mockOFClient.EXPECT().InstallPodSNATFlows(uint32(1), net.ParseIP(fakeLocalEgressIP1), uint32(1))
mockOFClient.EXPECT().InstallPodSNATFlows(uint32(2), net.ParseIP(fakeLocalEgressIP1), uint32(1))
mockRouteClient.EXPECT().AddSNATRule(net.ParseIP(fakeLocalEgressIP1), uint32(1))
mockOFClient.EXPECT().InstallPodSNATFlows(uint32(3), net.ParseIP(fakeLocalEgressIP1), uint32(1))
mockIPAssigner.EXPECT().UnassignIP(fakeLocalEgressIP1).Return(false, nil).Times(3)
Expand Down Expand Up @@ -630,14 +602,11 @@ func TestSyncEgress(t *testing.T) {
expectedCalls: func(mockOFClient *openflowtest.MockClient, mockRouteClient *routetest.MockInterface, mockIPAssigner *ipassignertest.MockIPAssigner) {
mockIPAssigner.EXPECT().UnassignIP(fakeLocalEgressIP1).Return(false, nil)
mockOFClient.EXPECT().InstallSNATMarkFlows(net.ParseIP(fakeLocalEgressIP1), uint32(1))
mockOFClient.EXPECT().InstallPodSNATFlows(uint32(1), net.ParseIP(fakeLocalEgressIP1), uint32(1))
mockOFClient.EXPECT().InstallPodSNATFlows(uint32(2), net.ParseIP(fakeLocalEgressIP1), uint32(1))
mockRouteClient.EXPECT().AddSNATRule(net.ParseIP(fakeLocalEgressIP1), uint32(1))
mockIPAssigner.EXPECT().AssignIP(fakeLocalEgressIP2, nil, true).Return(true, nil)
// forceAdvertise depends on how fast the Egress status update is reflected in the informer cache, which doesn't really matter.
mockIPAssigner.EXPECT().AssignIP(fakeLocalEgressIP2, nil, gomock.Any()).Return(false, nil)
mockOFClient.EXPECT().InstallSNATMarkFlows(net.ParseIP(fakeLocalEgressIP2), uint32(2))
mockOFClient.EXPECT().InstallPodSNATFlows(uint32(3), net.ParseIP(fakeLocalEgressIP2), uint32(2))
mockRouteClient.EXPECT().AddSNATRule(net.ParseIP(fakeLocalEgressIP2), uint32(2))
},
expectedEvents: []string{
Expand Down Expand Up @@ -730,14 +699,10 @@ func TestSyncEgress(t *testing.T) {
expectedCalls: func(mockOFClient *openflowtest.MockClient, mockRouteClient *routetest.MockInterface, mockIPAssigner *ipassignertest.MockIPAssigner) {
mockIPAssigner.EXPECT().AssignIP(fakeLocalEgressIP1, nil, true).Return(true, nil)
mockOFClient.EXPECT().InstallSNATMarkFlows(net.ParseIP(fakeLocalEgressIP1), uint32(1))
mockOFClient.EXPECT().InstallPodSNATFlows(uint32(1), net.ParseIP(fakeLocalEgressIP1), uint32(1))
mockOFClient.EXPECT().InstallPodSNATFlows(uint32(2), net.ParseIP(fakeLocalEgressIP1), uint32(1))
mockRouteClient.EXPECT().AddSNATRule(net.ParseIP(fakeLocalEgressIP1), uint32(1))

mockIPAssigner.EXPECT().UnassignIP(fakeLocalEgressIP1).Return(true, nil)
mockOFClient.EXPECT().UninstallSNATMarkFlows(uint32(1))
mockOFClient.EXPECT().UninstallPodSNATFlows(uint32(1))
mockOFClient.EXPECT().UninstallPodSNATFlows(uint32(2))
mockRouteClient.EXPECT().DeleteSNATRule(uint32(1))
},
expectedEvents: []string{
Expand Down Expand Up @@ -960,7 +925,6 @@ func TestSyncEgress(t *testing.T) {
expectedCalls: func(mockOFClient *openflowtest.MockClient, mockRouteClient *routetest.MockInterface, mockIPAssigner *ipassignertest.MockIPAssigner) {
mockIPAssigner.EXPECT().AssignIP(fakeLocalEgressIP1, &crdv1b1.SubnetInfo{Gateway: fakeGatewayIP, PrefixLength: 16, VLAN: 10}, true).Return(true, nil)
mockOFClient.EXPECT().InstallSNATMarkFlows(net.ParseIP(fakeLocalEgressIP1), uint32(1))
mockOFClient.EXPECT().InstallPodSNATFlows(uint32(1), net.ParseIP(fakeLocalEgressIP1), uint32(1))
mockRouteClient.EXPECT().AddSNATRule(net.ParseIP(fakeLocalEgressIP1), uint32(1))
mockIPAssigner.EXPECT().GetInterfaceID(&crdv1b1.SubnetInfo{Gateway: fakeGatewayIP, PrefixLength: 16, VLAN: 10}).Return(20, true)
mockRouteClient.EXPECT().AddEgressRoutes(uint32(101), 20, net.ParseIP(fakeGatewayIP), 16)
Expand Down Expand Up @@ -1029,7 +993,6 @@ func TestSyncEgress(t *testing.T) {
expectedCalls: func(mockOFClient *openflowtest.MockClient, mockRouteClient *routetest.MockInterface, mockIPAssigner *ipassignertest.MockIPAssigner) {
mockIPAssigner.EXPECT().AssignIP(fakeLocalEgressIP1, &crdv1b1.SubnetInfo{Gateway: fakeGatewayIP, PrefixLength: 16, VLAN: 10}, true).Return(true, nil)
mockOFClient.EXPECT().InstallSNATMarkFlows(net.ParseIP(fakeLocalEgressIP1), uint32(1))
mockOFClient.EXPECT().InstallPodSNATFlows(uint32(1), net.ParseIP(fakeLocalEgressIP1), uint32(1))
mockRouteClient.EXPECT().AddSNATRule(net.ParseIP(fakeLocalEgressIP1), uint32(1))
mockIPAssigner.EXPECT().GetInterfaceID(&crdv1b1.SubnetInfo{Gateway: fakeGatewayIP, PrefixLength: 16, VLAN: 10}).Return(20, true)
mockRouteClient.EXPECT().AddEgressRoutes(uint32(101), 20, net.ParseIP(fakeGatewayIP), 16)
Expand Down Expand Up @@ -1082,7 +1045,6 @@ func TestSyncEgress(t *testing.T) {
expectedCalls: func(mockOFClient *openflowtest.MockClient, mockRouteClient *routetest.MockInterface, mockIPAssigner *ipassignertest.MockIPAssigner) {
mockIPAssigner.EXPECT().AssignIP(fakeLocalEgressIP1, &crdv1b1.SubnetInfo{Gateway: fakeGatewayIP, PrefixLength: 16, VLAN: 10}, true).Return(true, nil)
mockOFClient.EXPECT().InstallSNATMarkFlows(net.ParseIP(fakeLocalEgressIP1), uint32(1))
mockOFClient.EXPECT().InstallPodSNATFlows(uint32(1), net.ParseIP(fakeLocalEgressIP1), uint32(1))
mockRouteClient.EXPECT().AddSNATRule(net.ParseIP(fakeLocalEgressIP1), uint32(1))
mockIPAssigner.EXPECT().GetInterfaceID(&crdv1b1.SubnetInfo{Gateway: fakeGatewayIP, PrefixLength: 16, VLAN: 10}).Return(20, true)
mockRouteClient.EXPECT().AddEgressRoutes(uint32(101), 20, net.ParseIP(fakeGatewayIP), 16)
Expand Down Expand Up @@ -1205,7 +1167,6 @@ func TestPodUpdateShouldSyncEgress(t *testing.T) {
c.informerFactory.WaitForCacheSync(stopCh)

c.mockOFClient.EXPECT().InstallSNATMarkFlows(net.ParseIP(fakeLocalEgressIP1), uint32(1))
c.mockOFClient.EXPECT().InstallPodSNATFlows(uint32(1), net.ParseIP(fakeLocalEgressIP1), uint32(1))
c.mockRouteClient.EXPECT().AddSNATRule(net.ParseIP(fakeLocalEgressIP1), uint32(1))
c.mockIPAssigner.EXPECT().UnassignIP(fakeLocalEgressIP1)
c.addEgressGroup(egressGroup)
Expand All @@ -1215,8 +1176,6 @@ func TestPodUpdateShouldSyncEgress(t *testing.T) {
require.NoError(t, c.syncEgress(item))
c.queue.Done(item)

c.mockOFClient.EXPECT().InstallPodSNATFlows(uint32(10), net.ParseIP(fakeLocalEgressIP1), uint32(1))
c.mockIPAssigner.EXPECT().UnassignIP(fakeLocalEgressIP1)
// Mock CNIServer
addPodInterface(c.ifaceStore, "ns1", "pendingPod", 10)
ev := types.PodUpdate{
Expand Down Expand Up @@ -1340,30 +1299,24 @@ func TestSyncOverlappingEgress(t *testing.T) {
checkQueueItemExistence(t, c.queue, egress1.Name, egress2.Name, egress3.Name)

c.mockOFClient.EXPECT().InstallSNATMarkFlows(net.ParseIP(fakeLocalEgressIP1), uint32(1))
c.mockOFClient.EXPECT().InstallPodSNATFlows(uint32(1), net.ParseIP(fakeLocalEgressIP1), uint32(1))
c.mockOFClient.EXPECT().InstallPodSNATFlows(uint32(2), net.ParseIP(fakeLocalEgressIP1), uint32(1))
c.mockRouteClient.EXPECT().AddSNATRule(net.ParseIP(fakeLocalEgressIP1), uint32(1))
c.mockIPAssigner.EXPECT().UnassignIP(fakeLocalEgressIP1)
err := c.syncEgress(egress1.Name)
assert.NoError(t, err)

// egress2's IP is not local and pod1 has enforced egress1, so only one Pod SNAT flow is expected.
c.mockOFClient.EXPECT().InstallPodSNATFlows(uint32(3), net.ParseIP(fakeRemoteEgressIP1), uint32(0))
c.mockIPAssigner.EXPECT().UnassignIP(fakeRemoteEgressIP1)
err = c.syncEgress(egress2.Name)
assert.NoError(t, err)

// egress3 shares the same IP as egress1 and pod2 has enforced egress1, so only one Pod SNAT flow is expected.
c.mockOFClient.EXPECT().InstallPodSNATFlows(uint32(4), net.ParseIP(fakeLocalEgressIP1), uint32(1))
c.mockIPAssigner.EXPECT().UnassignIP(fakeLocalEgressIP1)
err = c.syncEgress(egress3.Name)
assert.NoError(t, err)

// After deleting egress1, pod1 and pod2 no longer enforces egress1. The Egress IP shouldn't be released as egress3
// is still referring to it.
// egress2 and egress3 are expected to be triggered for resync.
c.mockOFClient.EXPECT().UninstallPodSNATFlows(uint32(1))
c.mockOFClient.EXPECT().UninstallPodSNATFlows(uint32(2))
c.crdClient.CrdV1beta1().Egresses().Delete(context.TODO(), egress1.Name, metav1.DeleteOptions{})
assert.Eventually(t, func() bool {
_, err := c.egressLister.Get(egress1.Name)
Expand Down

0 comments on commit e64d3c5

Please sign in to comment.