Skip to content

Commit

Permalink
Delete OVS port and flows before releasing Pod IP
Browse files Browse the repository at this point in the history 
Pod IP is recorded as OVS port attribute and used in Pod specific flows.
If releasing Pod IP before deleting OVS port and flows, it could happen
that multiple Pod ports and flows reference to the same Pod IP as the
same time in some corner cases and lead to corrupted network.

This commit ensures all resources that reference to Pod IP are deleted
before releasing the IP.

Signed-off-by: Quan Tian <qtian@vmware.com>
  • Loading branch information
@tnqn
tnqn committed Dec 12, 2023
1 parent ca1e85c commit 7c3818b
Show file tree
Hide file tree
Showing 6 changed files with 94 additions and 40 deletions.
5 changes: 5 additions & 0 deletions pkg/agent/cniserver/ipam/ipam_service.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@ import (
var ipamDrivers map[string][]IPAMDriver

// A cache of IPAM results.
// TODO: We should get rid of using global variables to store status, which makes testing complicated.
var ipamResults = sync.Map{}

type IPAMResult struct {
Expand All @@ -59,6 +60,10 @@ func ResetIPAMDrivers(ipamType string) {
}
}

func ResetIPAMResults() {
ipamResults = sync.Map{}
}

func argsFromEnv(cniArgs *cnipb.CniCmdArgs) *invoke.Args {
return &invoke.Args{
ContainerID: cniArgs.ContainerId,
Expand Down
17 changes: 10 additions & 7 deletions pkg/agent/cniserver/server.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -296,7 +296,7 @@ func (s *CNIServer) incompatibleCniVersionResponse(cniVersion string) *cnipb.Cni

func (s *CNIServer) unsupportedFieldResponse(key string, value interface{}) *cnipb.CniCmdResponse {
cniErrorCode := cnipb.ErrorCode_UNSUPPORTED_FIELD
cniErrorMsg := fmt.Sprintf("Network configuration does not support key %s and value %v", key, value)
cniErrorMsg := fmt.Sprintf("Network configuration does not support %s=%v", key, value)
return s.generateCNIErrorResponse(cniErrorCode, cniErrorMsg)
}

Expand Down Expand Up @@ -537,17 +537,20 @@ func (s *CNIServer) cmdDel(_ context.Context, cniConfig *CNIConfig) (*cnipb.CniC
if s.isChaining {
return s.interceptDel(cniConfig)
}
// Release IP to IPAM driver
if err := ipam.ExecIPAMDelete(cniConfig.CniCmdArgs, cniConfig.K8sArgs, cniConfig.IPAM.Type, infraContainer); err != nil {
klog.ErrorS(err, "Failed to delete IP addresses for container", "container", cniConfig.ContainerId)
return s.ipamFailureResponse(err), nil
}
klog.InfoS("Deleted IP addresses for container", "container", cniConfig.ContainerId)

// Remove host interface and OVS configuration
if err := s.podConfigurator.removeInterfaces(cniConfig.ContainerId); err != nil {
klog.ErrorS(err, "Failed to remove interfaces for container", "container", cniConfig.ContainerId)
return s.configInterfaceFailureResponse(err), nil
}
klog.InfoS("Deleted interfaces for container", "container", cniConfig.ContainerId)

// Release IP to IPAM driver
if err := ipam.ExecIPAMDelete(cniConfig.CniCmdArgs, cniConfig.K8sArgs, cniConfig.IPAM.Type, infraContainer); err != nil {
klog.ErrorS(err, "Failed to delete IP addresses for container", "container", cniConfig.ContainerId)
return s.ipamFailureResponse(err), nil
}

klog.InfoS("CmdDel for container succeeded", "container", cniConfig.ContainerId)

return &cnipb.CniCmdResponse{CniResult: []byte("")}, nil
Expand Down
64 changes: 43 additions & 21 deletions pkg/agent/cniserver/server_linux_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -223,16 +223,13 @@ func createCNIRequestAndInterfaceName(t *testing.T, name string, cniType string,
}

func TestCmdAdd(t *testing.T) {
controller := gomock.NewController(t)
ipamMock := ipamtest.NewMockIPAMDriver(controller)
ctx := context.TODO()

versionedIPAMResult, err := ipamResult.GetAsVersion(supportedCNIVersion)
assert.NoError(t, err)

for _, tc := range []struct {
name string
podName string
ipamType string
ipamAdd bool
ipamError error
Expand All @@ -248,7 +245,6 @@ func TestCmdAdd(t *testing.T) {
}{
{
name: "secondary-IPAM",
podName: "pod0",
ipamType: ipam.AntreaIPAMType,
cniType: "cniType",
enableSecondaryNetworkIPAM: true,
Expand All @@ -257,7 +253,6 @@ func TestCmdAdd(t *testing.T) {
response: resultToResponse(versionedIPAMResult),
}, {
name: "secondary-IPAM-failure",
podName: "pod1",
ipamType: ipam.AntreaIPAMType,
cniType: "cniType",
enableSecondaryNetworkIPAM: true,
Expand All @@ -272,7 +267,6 @@ func TestCmdAdd(t *testing.T) {
},
}, {
name: "chaining",
podName: "pod2",
ipamType: "test-cni-ipam",
enableSecondaryNetworkIPAM: false,
isChaining: true,
Expand All @@ -281,7 +275,6 @@ func TestCmdAdd(t *testing.T) {
containerIfaceExist: true,
}, {
name: "add-general-cni",
podName: "pod3",
ipamType: "test-cni-ipam",
ipamAdd: true,
enableSecondaryNetworkIPAM: false,
Expand All @@ -291,7 +284,6 @@ func TestCmdAdd(t *testing.T) {
containerIfaceExist: true,
}, {
name: "add-general-cni-failure",
podName: "pod3",
ipamType: "test-cni-ipam",
ipamAdd: true,
enableSecondaryNetworkIPAM: false,
Expand All @@ -304,9 +296,12 @@ func TestCmdAdd(t *testing.T) {
} {
t.Run(tc.name, func(t *testing.T) {
defer mockGetNSPath(nil)()
ipam.ResetIPAMResults()
controller := gomock.NewController(t)
ipamMock := ipamtest.NewMockIPAMDriver(controller)
cniserver := newMockCNIServer(t, controller, ipamMock, tc.ipamType, tc.enableSecondaryNetworkIPAM, tc.isChaining)
testIfaceConfigurator := newTestInterfaceConfigurator()
requestMsg, hostInterfaceName := createCNIRequestAndInterfaceName(t, tc.podName, tc.cniType, ipamResult, tc.ipamType, true)
requestMsg, hostInterfaceName := createCNIRequestAndInterfaceName(t, testPodNameA, tc.cniType, ipamResult, tc.ipamType, true)
testIfaceConfigurator.hostIfaceName = hostInterfaceName
cniserver.podConfigurator.ifConfigurator = testIfaceConfigurator
if tc.ipamAdd {
Expand Down Expand Up @@ -371,30 +366,27 @@ func TestCmdAdd(t *testing.T) {
}

func TestCmdDel(t *testing.T) {
controller := gomock.NewController(t)
ipamMock := ipamtest.NewMockIPAMDriver(controller)
ovsPortID := generateUUID(t)
ovsPort := int32(100)
ctx := context.TODO()

for _, tc := range []struct {
name string
podName string
ipamType string
ipamDel bool
ipamError error
cniType string
enableSecondaryNetworkIPAM bool
isChaining bool
disconnectOVS bool
disconnectOVSErr error
migrateRoute bool
delLocalIPAMRoute bool
delLocalIPAMRouteError error
response *cnipb.CniCmdResponse
}{
{
name: "secondary-IPAM",
podName: "pod1",
ipamType: ipam.AntreaIPAMType,
cniType: "cniType",
ipamDel: true,
Expand All @@ -404,7 +396,6 @@ func TestCmdDel(t *testing.T) {
},
{
name: "secondary-IPAM-failure",
podName: "pod1",
ipamType: ipam.AntreaIPAMType,
cniType: "cniType",
ipamDel: true,
Expand All @@ -418,9 +409,40 @@ func TestCmdDel(t *testing.T) {
},
},
},
{
name: "IPAM-failure",
ipamType: "host-local",
ipamDel: true,
ipamError: fmt.Errorf("failed to release IP"),
enableSecondaryNetworkIPAM: false,
isChaining: false,
disconnectOVS: true,
delLocalIPAMRoute: true,
response: &cnipb.CniCmdResponse{
Error: &cnipb.Error{
Code: cnipb.ErrorCode_IPAM_FAILURE,
Message: "failed to release IP",
},
},
},
{
name: "del-ovs-failure",
ipamType: "host-local",
enableSecondaryNetworkIPAM: false,
isChaining: false,
disconnectOVS: true,
disconnectOVSErr: ovsconfig.NewTransactionError(fmt.Errorf("failed to delete port"), true),
ipamDel: false,
delLocalIPAMRoute: false,
response: &cnipb.CniCmdResponse{
Error: &cnipb.Error{
Code: cnipb.ErrorCode_CONFIG_INTERFACE_FAILURE,
Message: fmt.Sprintf("failed to delete OVS port for container %s: failed to delete port", testPodInfraContainerID),
},
},
},
{
name: "chaining",
podName: "pod2",
ipamType: "test-delete",
enableSecondaryNetworkIPAM: false,
isChaining: true,
Expand All @@ -429,7 +451,6 @@ func TestCmdDel(t *testing.T) {
},
{
name: "del-general-cni",
podName: "pod3",
ipamType: "test-delete",
ipamDel: true,
enableSecondaryNetworkIPAM: false,
Expand All @@ -439,9 +460,8 @@ func TestCmdDel(t *testing.T) {
},
{
name: "del-general-cni-failure",
podName: "pod3",
ipamType: "test-delete",
ipamDel: true,
ipamDel: false,
enableSecondaryNetworkIPAM: false,
isChaining: false,
disconnectOVS: true,
Expand All @@ -450,10 +470,12 @@ func TestCmdDel(t *testing.T) {
},
} {
t.Run(tc.name, func(t *testing.T) {
controller := gomock.NewController(t)
ipamMock := ipamtest.NewMockIPAMDriver(controller)
cniserver := newMockCNIServer(t, controller, ipamMock, tc.ipamType, tc.enableSecondaryNetworkIPAM, tc.isChaining)
requestMsg, hostInterfaceName := createCNIRequestAndInterfaceName(t, tc.podName, tc.cniType, ipamResult, tc.ipamType, true)
requestMsg, hostInterfaceName := createCNIRequestAndInterfaceName(t, testPodNameA, tc.cniType, ipamResult, tc.ipamType, true)
containerID := requestMsg.CniArgs.ContainerId
containerIfaceConfig := interfacestore.NewContainerInterface(hostInterfaceName, containerID, tc.podName, testPodNamespace, containerVethMac, []net.IP{net.ParseIP("10.1.2.100")}, 0)
containerIfaceConfig := interfacestore.NewContainerInterface(hostInterfaceName, containerID, testPodNameA, testPodNamespace, containerVethMac, []net.IP{net.ParseIP("10.1.2.100")}, 0)
containerIfaceConfig.OVSPortConfig = &interfacestore.OVSPortConfig{PortUUID: ovsPortID, OFPort: ovsPort}
ifaceStore.AddInterface(containerIfaceConfig)
testIfaceConfigurator := newTestInterfaceConfigurator()
Expand All @@ -472,7 +494,7 @@ func TestCmdDel(t *testing.T) {
}
}
if tc.disconnectOVS {
mockOVSBridgeClient.EXPECT().DeletePort(ovsPortID).Return(nil).Times(1)
mockOVSBridgeClient.EXPECT().DeletePort(ovsPortID).Return(tc.disconnectOVSErr).Times(1)
mockOFClient.EXPECT().UninstallPodFlows(hostInterfaceName).Return(nil).Times(1)
}
if tc.migrateRoute {
Expand Down
4 changes: 2 additions & 2 deletions pkg/agent/cniserver/server_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -687,14 +687,14 @@ func generateNetworkConfiguration(name, cniVersion, cniType, ipamType string) *t
if cniType == "" {
netCfg.Type = AntreaCNIType
} else {
netCfg.Type = "cniType"
netCfg.Type = cniType
}
netCfg.IPAM = &types.IPAMConfig{Type: ipamType}
return netCfg
}

func newRequest(args string, netCfg *types.NetworkConfig, path string, t *testing.T) (*cnipb.CniCmdRequest, string) {
containerID := generateUUID(t)
_, _, containerID := cniservertest.ParseCNIArgs(args)
networkConfig, err := json.Marshal(netCfg)
if err != nil {
t.Error("Failed to generate Network configuration")
Expand Down
19 changes: 11 additions & 8 deletions pkg/agent/cniserver/server_windows_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -591,19 +591,22 @@ func TestCmdDel(t *testing.T) {
disconnectOVS: true,
endpointExists: true,
ifaceExists: true,
}, {
},
{
name: "interface-not-exist",
podName: "pod1",
containerID: containerID,
netns: "none",
ipamDel: true,
}, {
name: "ipam-delete-failure",
podName: "pod2",
containerID: containerID,
netns: "none",
ipamDel: true,
ipamError: fmt.Errorf("unable to delete IP"),
},
{
name: "ipam-delete-failure",
podName: "pod2",
containerID: containerID,
netns: "none",
ipamDel: true,
ipamError: fmt.Errorf("unable to delete IP"),
disconnectOVS: true,
errResponse: &cnipb.CniCmdResponse{
Error: &cnipb.Error{
Code: cnipb.ErrorCode_IPAM_FAILURE,
Expand Down
25 changes: 23 additions & 2 deletions pkg/agent/cniserver/testing/utils.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -14,10 +14,31 @@

package testing

import "fmt"
import (
"fmt"
"strings"
)

const argsFormat = "IgnoreUnknown=1;K8S_POD_NAMESPACE=%s;K8S_POD_NAME=%s;K8S_POD_INFRA_CONTAINER_ID=%s"

func GenerateCNIArgs(podName string, podNamespace string, podInfraContainerID string) string {
func GenerateCNIArgs(podName, podNamespace, podInfraContainerID string) string {
return fmt.Sprintf(argsFormat, podNamespace, podName, podInfraContainerID)
}

func ParseCNIArgs(args string) (podName, podNamespace, podInfraContainerID string) {
strs := strings.Split(args, ";")
for _, str := range strs {
fields := strings.Split(str, "=")
if len(fields) == 2 {
switch fields[0] {
case "K8S_POD_NAMESPACE":
podNamespace = fields[1]
case "K8S_POD_NAME":
podName = fields[1]
case "K8S_POD_INFRA_CONTAINER_ID":
podInfraContainerID = fields[1]
}
}
}
return
}

0 comments on commit 7c3818b

Please sign in to comment.