Skip to content

Commit 5d393fe

Browse files
avirakavirak
committed
Fix for pre-mature deletion of veth pair for daemon-host network namespace.
1 parent f966279 commit 5d393fe

File tree

2 files changed

+228
-11
lines changed

2 files changed

+228
-11
lines changed

‎ecs-agent/netlib/platform/managed_linux.go‎

Lines changed: 10 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -470,16 +470,17 @@ func (m *managedLinux) configureDaemonNetNS(ctx context.Context, taskID string,
470470

471471
_, err = m.common.executeCNIPlugin(ctx, add, cniNetConf...)
472472
if err != nil {
473-
err = errors.Wrap(err, "failed to setup daemon network namespace bridge")
474-
return err
475-
}
476-
477-
// Add NAT masquerade rule for external connectivity
478-
err = m.addDaemonBridgeNATRule()
479-
if err != nil {
480-
logger.Warn("Failed to add NAT rule for daemon-bridge", logger.Fields{
473+
logger.Warn("CNI plugin execution failed for daemon namespace", logger.Fields{
481474
loggerfield.Error: err,
482475
})
476+
} else {
477+
// Add NAT masquerade rule for external connectivity
478+
err = m.addDaemonBridgeNATRule()
479+
if err != nil {
480+
logger.Warn("Failed to add NAT rule for daemon-bridge", logger.Fields{
481+
loggerfield.Error: err,
482+
})
483+
}
483484
}
484485
}
485486
return nil
@@ -512,7 +513,7 @@ func (m *managedLinux) StopDaemonNetNS(ctx context.Context, netNS *tasknetworkco
512513

513514
// Cleanup bridge config(veth pair).
514515
var cniNetConf []ecscni.PluginConfig
515-
cniNetConf = append(cniNetConf, createBridgePluginConfig(netNS.Path))
516+
cniNetConf = append(cniNetConf, createDaemonBridgePluginConfig(netNS.Path))
516517
add := false
517518

518519
_, err := m.common.executeCNIPlugin(ctx, add, cniNetConf...)

‎ecs-agent/netlib/platform/managed_linux_test.go‎

Lines changed: 218 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -27,6 +27,7 @@ import (
2727

2828
"github.com/aws/amazon-ecs-agent/ecs-agent/acs/model/ecsacs"
2929
mock_ec2 "github.com/aws/amazon-ecs-agent/ecs-agent/ec2/mocks"
30+
"github.com/aws/amazon-ecs-agent/ecs-agent/ipcompatibility"
3031
"github.com/aws/amazon-ecs-agent/ecs-agent/netlib/model/ecscni"
3132
mock_ecscni2 "github.com/aws/amazon-ecs-agent/ecs-agent/netlib/model/ecscni/mocks_ecscni"
3233
mock_ecscni "github.com/aws/amazon-ecs-agent/ecs-agent/netlib/model/ecscni/mocks_nsutil"
@@ -627,7 +628,7 @@ func TestConfigureDaemonNetNS(t *testing.T) {
627628
expectErr: true,
628629
},
629630
{
630-
name: "wrong state transition",
631+
name: "wrong state transition - no action needed",
631632
netNS: &tasknetworkconfig.NetworkNamespace{
632633
Path: "/var/run/netns/test-daemon",
633634
NetworkMode: "daemon-bridge",
@@ -647,7 +648,9 @@ func TestConfigureDaemonNetNS(t *testing.T) {
647648
err := ml.ConfigureDaemonNetNS(tt.netNS)
648649
if tt.expectErr {
649650
assert.Error(t, err)
650-
assert.Contains(t, err.Error(), "invalid transition state")
651+
if tt.name == "invalid transition state" {
652+
assert.Contains(t, err.Error(), "invalid transition state")
653+
}
651654
} else {
652655
assert.NoError(t, err)
653656
}
@@ -772,6 +775,219 @@ func TestBuildTaskNetworkConfiguration_DaemonBridge(t *testing.T) {
772775
assert.Equal(t, status.NetworkReadyPull, ns.DesiredState)
773776
}
774777

778+
func TestConfigureIPv4ForHostENI(t *testing.T) {
779+
tests := []struct {
780+
name string
781+
setupMocks func(*mock_ec2.MockEC2MetadataClient, *mock_netlinkwrapper.MockNetLink)
782+
ipComp ipcompatibility.IPCompatibility
783+
expectErr bool
784+
validate func(*testing.T, *ecsacs.ElasticNetworkInterface)
785+
}{
786+
{
787+
name: "successful IPv4 configuration",
788+
setupMocks: func(mockEC2Client *mock_ec2.MockEC2MetadataClient, mockNetLink *mock_netlinkwrapper.MockNetLink) {
789+
mockEC2Client.EXPECT().GetMetadata(PrivateIPv4Address).Return("10.194.20.1", nil).Times(1)
790+
mockEC2Client.EXPECT().GetMetadata(fmt.Sprintf(IPv4SubNetCidrBlock, macAddress)).Return("10.194.20.0/20", nil).Times(1)
791+
},
792+
ipComp: ipcompatibility.NewIPv4OnlyCompatibility(),
793+
expectErr: false,
794+
validate: func(t *testing.T, hostENI *ecsacs.ElasticNetworkInterface) {
795+
require.Len(t, hostENI.Ipv4Addresses, 1)
796+
assert.Equal(t, "10.194.20.1", *hostENI.Ipv4Addresses[0].PrivateAddress)
797+
assert.True(t, *hostENI.Ipv4Addresses[0].Primary)
798+
assert.Equal(t, "10.194.20.0/20", *hostENI.SubnetGatewayIpv4Address)
799+
},
800+
},
801+
{
802+
name: "IPv4 not compatible - no configuration",
803+
setupMocks: func(mockEC2Client *mock_ec2.MockEC2MetadataClient, mockNetLink *mock_netlinkwrapper.MockNetLink) {
804+
// No metadata calls expected
805+
},
806+
ipComp: ipcompatibility.NewIPv6OnlyCompatibility(),
807+
expectErr: false,
808+
validate: func(t *testing.T, hostENI *ecsacs.ElasticNetworkInterface) {
809+
assert.Nil(t, hostENI.Ipv4Addresses)
810+
assert.Nil(t, hostENI.SubnetGatewayIpv4Address)
811+
},
812+
},
813+
{
814+
name: "metadata error",
815+
setupMocks: func(mockEC2Client *mock_ec2.MockEC2MetadataClient, mockNetLink *mock_netlinkwrapper.MockNetLink) {
816+
mockEC2Client.EXPECT().GetMetadata(PrivateIPv4Address).Return("", fmt.Errorf("metadata unavailable")).Times(1)
817+
mockEC2Client.EXPECT().GetMetadata(fmt.Sprintf(IPv4SubNetCidrBlock, macAddress)).Return("", fmt.Errorf("metadata unavailable")).Times(1)
818+
},
819+
ipComp: ipcompatibility.NewIPv4OnlyCompatibility(),
820+
expectErr: true,
821+
validate: nil,
822+
},
823+
}
824+
825+
for _, tt := range tests {
826+
t.Run(tt.name, func(t *testing.T) {
827+
ctrl := gomock.NewController(t)
828+
defer ctrl.Finish()
829+
830+
mockMetadataClient := mock_ec2.NewMockEC2MetadataClient(ctrl)
831+
netLink := mock_netlinkwrapper.NewMockNetLink(ctrl)
832+
tt.setupMocks(mockMetadataClient, netLink)
833+
834+
ml := &managedLinux{
835+
client: mockMetadataClient,
836+
common: common{netlink: netLink},
837+
}
838+
839+
hostENI := &ecsacs.ElasticNetworkInterface{}
840+
err := ml.configureIPv4ForHostENI(hostENI, macAddress, tt.ipComp)
841+
842+
if tt.expectErr {
843+
assert.Error(t, err)
844+
} else {
845+
assert.NoError(t, err)
846+
if tt.validate != nil {
847+
tt.validate(t, hostENI)
848+
}
849+
}
850+
})
851+
}
852+
}
853+
854+
func TestConfigureIPv6ForHostENI(t *testing.T) {
855+
tests := []struct {
856+
name string
857+
setupMocks func(*mock_ec2.MockEC2MetadataClient, *mock_netlinkwrapper.MockNetLink)
858+
ipComp ipcompatibility.IPCompatibility
859+
expectErr bool
860+
validate func(*testing.T, *ecsacs.ElasticNetworkInterface)
861+
}{
862+
{
863+
name: "successful IPv6 configuration",
864+
setupMocks: func(mockEC2Client *mock_ec2.MockEC2MetadataClient, mockNetLink *mock_netlinkwrapper.MockNetLink) {
865+
mockEC2Client.EXPECT().GetMetadata(PrivateIPv6Address).Return("fe80::406:baff:fef9:4305", nil).Times(1)
866+
mockEC2Client.EXPECT().GetMetadata(fmt.Sprintf(IPv6SubNetCidrBlock, macAddress)).Return("fe80::406:baff:fef9:4305/60", nil).Times(1)
867+
},
868+
ipComp: ipcompatibility.NewIPv6OnlyCompatibility(),
869+
expectErr: false,
870+
validate: func(t *testing.T, hostENI *ecsacs.ElasticNetworkInterface) {
871+
require.Len(t, hostENI.Ipv6Addresses, 1)
872+
assert.Equal(t, "fe80::406:baff:fef9:4305", *hostENI.Ipv6Addresses[0].Address)
873+
assert.True(t, *hostENI.Ipv6Addresses[0].Primary)
874+
assert.Equal(t, "fe80::406:baff:fef9:4305/60", *hostENI.SubnetGatewayIpv6Address)
875+
},
876+
},
877+
{
878+
name: "IPv6 not compatible - no configuration",
879+
setupMocks: func(mockEC2Client *mock_ec2.MockEC2MetadataClient, mockNetLink *mock_netlinkwrapper.MockNetLink) {
880+
// No metadata calls expected
881+
},
882+
ipComp: ipcompatibility.NewIPv4OnlyCompatibility(),
883+
expectErr: false,
884+
validate: func(t *testing.T, hostENI *ecsacs.ElasticNetworkInterface) {
885+
assert.Nil(t, hostENI.Ipv6Addresses)
886+
assert.Nil(t, hostENI.SubnetGatewayIpv6Address)
887+
},
888+
},
889+
}
890+
891+
for _, tt := range tests {
892+
t.Run(tt.name, func(t *testing.T) {
893+
ctrl := gomock.NewController(t)
894+
defer ctrl.Finish()
895+
896+
mockMetadataClient := mock_ec2.NewMockEC2MetadataClient(ctrl)
897+
netLink := mock_netlinkwrapper.NewMockNetLink(ctrl)
898+
tt.setupMocks(mockMetadataClient, netLink)
899+
900+
ml := &managedLinux{
901+
client: mockMetadataClient,
902+
common: common{netlink: netLink},
903+
}
904+
905+
hostENI := &ecsacs.ElasticNetworkInterface{}
906+
err := ml.configureIPv6ForHostENI(hostENI, macAddress, tt.ipComp)
907+
908+
if tt.expectErr {
909+
assert.Error(t, err)
910+
} else {
911+
assert.NoError(t, err)
912+
if tt.validate != nil {
913+
tt.validate(t, hostENI)
914+
}
915+
}
916+
})
917+
}
918+
}
919+
920+
func TestCreateDaemonNetworkNamespace(t *testing.T) {
921+
tests := []struct {
922+
name string
923+
setupMocks func(*mock_ecscni.MockNetNSUtil)
924+
expectErr bool
925+
validate func(*testing.T, []*tasknetworkconfig.NetworkNamespace)
926+
}{
927+
{
928+
name: "successful namespace creation",
929+
setupMocks: func(mockNSUtil *mock_ecscni.MockNetNSUtil) {
930+
mockNSUtil.EXPECT().GetNetNSPath("host-daemon").Return("/var/run/netns/host-daemon").Times(1)
931+
},
932+
expectErr: false,
933+
validate: func(t *testing.T, namespaces []*tasknetworkconfig.NetworkNamespace) {
934+
require.Len(t, namespaces, 1)
935+
ns := namespaces[0]
936+
assert.Equal(t, "host-daemon", ns.Name)
937+
assert.Equal(t, "/var/run/netns/host-daemon", ns.Path)
938+
assert.Equal(t, "daemon-bridge", string(ns.NetworkMode))
939+
assert.Equal(t, status.NetworkNone, ns.KnownState)
940+
assert.Equal(t, status.NetworkReadyPull, ns.DesiredState)
941+
942+
require.Len(t, ns.NetworkInterfaces, 1)
943+
netInt := ns.NetworkInterfaces[0]
944+
assert.True(t, netInt.Default)
945+
assert.Equal(t, status.NetworkReadyPull, netInt.DesiredStatus)
946+
assert.Equal(t, status.NetworkNone, netInt.KnownStatus)
947+
},
948+
},
949+
}
950+
951+
for _, tt := range tests {
952+
t.Run(tt.name, func(t *testing.T) {
953+
ctrl := gomock.NewController(t)
954+
defer ctrl.Finish()
955+
956+
mockNSUtil := mock_ecscni.NewMockNetNSUtil(ctrl)
957+
tt.setupMocks(mockNSUtil)
958+
959+
ml := &managedLinux{
960+
common: common{nsUtil: mockNSUtil},
961+
}
962+
963+
// Create a properly configured ENI with IPv4 addresses
964+
hostENI := &ecsacs.ElasticNetworkInterface{
965+
Ec2Id: aws.String("i-1234567890abcdef0"),
966+
MacAddress: aws.String(macAddress),
967+
Ipv4Addresses: []*ecsacs.IPv4AddressAssignment{
968+
{
969+
Primary: aws.Bool(true),
970+
PrivateAddress: aws.String("10.194.20.1"),
971+
},
972+
},
973+
SubnetGatewayIpv4Address: aws.String("10.194.20.0/20"),
974+
}
975+
macToNames := map[string]string{macAddress: "eth0"}
976+
977+
namespaces, err := ml.createDaemonNetworkNamespace(hostENI, macToNames)
978+
979+
if tt.expectErr {
980+
assert.Error(t, err)
981+
} else {
982+
assert.NoError(t, err)
983+
if tt.validate != nil {
984+
tt.validate(t, namespaces)
985+
}
986+
}
987+
})
988+
}
989+
}
990+
775991
func TestAddDaemonBridgeNATRule(t *testing.T) {
776992
ml := &managedLinux{}
777993

0 commit comments

Comments
 (0)