Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Decouple ENI in AttachmentStateChange #3969

Merged
merged 1 commit into from
Oct 21, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion agent/acs/session/attach_eni_common.go
Original file line number Diff line number Diff line change
Expand Up @@ -127,7 +127,7 @@ func (eniHandler *eniHandler) removeENIAttachmentData(mac string) {
seelog.Errorf("Unable to retrieve ENI Attachment for mac address %s: ", mac)
return
}
attachmentId, err := utils.GetENIAttachmentId(attachmentToRemove.AttachmentARN)
attachmentId, err := utils.GetAttachmentId(attachmentToRemove.AttachmentARN)
if err != nil {
seelog.Errorf("Failed to get attachment id for %s: %v", attachmentToRemove.AttachmentARN, err)
} else {
Expand Down
10 changes: 5 additions & 5 deletions agent/acs/session/attach_eni_common_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ import (
"github.com/aws/amazon-ecs-agent/agent/data"
"github.com/aws/amazon-ecs-agent/agent/engine/dockerstate"
"github.com/aws/amazon-ecs-agent/ecs-agent/acs/session/testconst"
"github.com/aws/amazon-ecs-agent/ecs-agent/api/attachmentinfo"
"github.com/aws/amazon-ecs-agent/ecs-agent/api/attachment"
ni "github.com/aws/amazon-ecs-agent/ecs-agent/netlib/model/networkinterface"
)

Expand All @@ -53,7 +53,7 @@ func testENIAckTimeout(t *testing.T, attachmentType string) {

expiresAt := time.Now().Add(time.Millisecond * testconst.WaitTimeoutMillis)
eniAttachment := &ni.ENIAttachment{
AttachmentInfo: attachmentinfo.AttachmentInfo{
AttachmentInfo: attachment.AttachmentInfo{
TaskARN: testconst.TaskARN,
AttachmentARN: attachmentArn,
ExpiresAt: expiresAt,
Expand Down Expand Up @@ -99,7 +99,7 @@ func testENIAckWithinTimeout(t *testing.T, attachmentType string) {
dataClient := data.NewNoopClient()
expiresAt := time.Now().Add(time.Millisecond * testconst.WaitTimeoutMillis)
eniAttachment := &ni.ENIAttachment{
AttachmentInfo: attachmentinfo.AttachmentInfo{
AttachmentInfo: attachment.AttachmentInfo{
TaskARN: testconst.TaskARN,
AttachmentARN: attachmentArn,
ExpiresAt: expiresAt,
Expand Down Expand Up @@ -141,7 +141,7 @@ func testHandleENIAttachment(t *testing.T, attachmentType, taskArn string) {
taskEngineState := dockerstate.NewTaskEngineState()
expiresAt := time.Now().Add(time.Millisecond * testconst.WaitTimeoutMillis)
eniAttachment := &ni.ENIAttachment{
AttachmentInfo: attachmentinfo.AttachmentInfo{
AttachmentInfo: attachment.AttachmentInfo{
TaskARN: taskArn,
AttachmentARN: attachmentArn,
ExpiresAt: expiresAt,
Expand Down Expand Up @@ -188,7 +188,7 @@ func testHandleExpiredENIAttachment(t *testing.T, attachmentType, taskArn string
dataClient := data.NewNoopClient()

eniAttachment := &ni.ENIAttachment{
AttachmentInfo: attachmentinfo.AttachmentInfo{
AttachmentInfo: attachment.AttachmentInfo{
TaskARN: taskArn,
AttachmentARN: attachmentArn,
ExpiresAt: expiresAt,
Expand Down
4 changes: 2 additions & 2 deletions agent/acs/session/attach_instance_eni_responder_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ import (
"github.com/aws/amazon-ecs-agent/ecs-agent/acs/model/ecsacs"
acssession "github.com/aws/amazon-ecs-agent/ecs-agent/acs/session"
"github.com/aws/amazon-ecs-agent/ecs-agent/acs/session/testconst"
"github.com/aws/amazon-ecs-agent/ecs-agent/api/attachmentinfo"
"github.com/aws/amazon-ecs-agent/ecs-agent/api/attachment"
ni "github.com/aws/amazon-ecs-agent/ecs-agent/netlib/model/networkinterface"
)

Expand Down Expand Up @@ -113,7 +113,7 @@ func TestInstanceENIAckSingleMessageWithDuplicateENIAttachment(t *testing.T) {
mockState.EXPECT().
ENIByMac(testconst.RandomMAC).
Return(&ni.ENIAttachment{
AttachmentInfo: attachmentinfo.AttachmentInfo{
AttachmentInfo: attachment.AttachmentInfo{
ExpiresAt: expiresAt,
},
}, true).
Expand Down
4 changes: 2 additions & 2 deletions agent/acs/session/attach_task_eni_responder_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ import (
"github.com/aws/amazon-ecs-agent/ecs-agent/acs/model/ecsacs"
acssession "github.com/aws/amazon-ecs-agent/ecs-agent/acs/session"
"github.com/aws/amazon-ecs-agent/ecs-agent/acs/session/testconst"
"github.com/aws/amazon-ecs-agent/ecs-agent/api/attachmentinfo"
"github.com/aws/amazon-ecs-agent/ecs-agent/api/attachment"
ni "github.com/aws/amazon-ecs-agent/ecs-agent/netlib/model/networkinterface"
)

Expand Down Expand Up @@ -113,7 +113,7 @@ func TestTaskENIAckSingleMessageWithDuplicateENIAttachment(t *testing.T) {
mockState.EXPECT().
ENIByMac(testconst.RandomMAC).
Return(&ni.ENIAttachment{
AttachmentInfo: attachmentinfo.AttachmentInfo{
AttachmentInfo: attachment.AttachmentInfo{
ExpiresAt: expiresAt,
},
}, true).
Expand Down
8 changes: 4 additions & 4 deletions agent/acs/session/payload_responder_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -34,10 +34,10 @@ import (
"github.com/aws/amazon-ecs-agent/ecs-agent/acs/model/ecsacs"
acssession "github.com/aws/amazon-ecs-agent/ecs-agent/acs/session"
"github.com/aws/amazon-ecs-agent/ecs-agent/acs/session/testconst"
"github.com/aws/amazon-ecs-agent/ecs-agent/api/eni"
apiresource "github.com/aws/amazon-ecs-agent/ecs-agent/api/resource"
apiresource "github.com/aws/amazon-ecs-agent/ecs-agent/api/attachment/resource"
apitaskstatus "github.com/aws/amazon-ecs-agent/ecs-agent/api/task/status"
"github.com/aws/amazon-ecs-agent/ecs-agent/credentials"
ni "github.com/aws/amazon-ecs-agent/ecs-agent/netlib/model/networkinterface"
"github.com/aws/amazon-ecs-agent/ecs-agent/wsclient"
"github.com/aws/aws-sdk-go/aws"
"github.com/golang/mock/gomock"
Expand Down Expand Up @@ -858,7 +858,7 @@ func TestHandlePayloadMessageAddedENITrunkToTask(t *testing.T) {
Arn: aws.String(testconst.TaskARN),
ElasticNetworkInterfaces: []*ecsacs.ElasticNetworkInterface{
{
InterfaceAssociationProtocol: aws.String(eni.VLANInterfaceAssociationProtocol),
InterfaceAssociationProtocol: aws.String(ni.VLANInterfaceAssociationProtocol),
AttachmentArn: aws.String(attachmentARN),
Ec2Id: aws.String(ec2ID),
Ipv4Addresses: []*ecsacs.IPv4AddressAssignment{
Expand Down Expand Up @@ -890,7 +890,7 @@ func TestHandlePayloadMessageAddedENITrunkToTask(t *testing.T) {

// Validate the added task has the ENI trunk information as expected.
taskeni := addedTask.GetPrimaryENI()
assert.Equal(t, eni.VLANInterfaceAssociationProtocol, taskeni.InterfaceAssociationProtocol)
assert.Equal(t, ni.VLANInterfaceAssociationProtocol, taskeni.InterfaceAssociationProtocol)
assert.Equal(t, testconst.RandomMAC, taskeni.InterfaceVlanProperties.TrunkInterfaceMacAddress)
assert.Equal(t, vlanID, taskeni.InterfaceVlanProperties.VlanID)
}
Expand Down
6 changes: 3 additions & 3 deletions agent/api/ecsclient/client.go
Original file line number Diff line number Diff line change
Expand Up @@ -657,14 +657,14 @@ func (client *APIECSClient) SubmitContainerStateChange(change api.ContainerState
}

func (client *APIECSClient) SubmitAttachmentStateChange(change api.AttachmentStateChange) error {
attachmentStatus := change.Attachment.Status.String()
attachmentStatus := change.Attachment.GetAttachmentStatus()

req := ecs.SubmitAttachmentStateChangesInput{
Cluster: &client.config.Cluster,
Attachments: []*ecs.AttachmentStateChange{
{
AttachmentArn: aws.String(change.Attachment.AttachmentARN),
Status: aws.String(attachmentStatus),
AttachmentArn: aws.String(change.Attachment.GetAttachmentARN()),
Status: aws.String(attachmentStatus.String()),
},
},
}
Expand Down
12 changes: 5 additions & 7 deletions agent/api/ecsclient/client_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -24,9 +24,6 @@ import (
"testing"
"time"

"github.com/aws/amazon-ecs-agent/ecs-agent/api/attachmentinfo"
"github.com/aws/amazon-ecs-agent/ecs-agent/api/status"

"github.com/golang/mock/gomock"
"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"
Expand All @@ -37,6 +34,7 @@ import (
"github.com/aws/amazon-ecs-agent/agent/config"
"github.com/aws/amazon-ecs-agent/agent/ec2"
mock_ec2 "github.com/aws/amazon-ecs-agent/agent/ec2/mocks"
"github.com/aws/amazon-ecs-agent/ecs-agent/api/attachment"
apicontainerstatus "github.com/aws/amazon-ecs-agent/ecs-agent/api/container/status"
apitaskstatus "github.com/aws/amazon-ecs-agent/ecs-agent/api/task/status"
"github.com/aws/amazon-ecs-agent/ecs-agent/async"
Expand Down Expand Up @@ -1063,9 +1061,9 @@ func TestDiscoverTelemetryEndpointAfterPollEndpointCacheHit(t *testing.T) {
}
}

// TestSubmitTaskStateChangeWithAttachments tests the SubmitTaskStateChange API
// TestSubmitTaskStateChangeWithENIAttachments tests the SubmitTaskStateChange API
// also send the Attachment Status
func TestSubmitTaskStateChangeWithAttachments(t *testing.T) {
func TestSubmitTaskStateChangeWithENIAttachments(t *testing.T) {
mockCtrl := gomock.NewController(t)
defer mockCtrl.Finish()

Expand All @@ -1086,9 +1084,9 @@ func TestSubmitTaskStateChangeWithAttachments(t *testing.T) {
err := client.SubmitTaskStateChange(api.TaskStateChange{
TaskARN: "task_arn",
Attachment: &ni.ENIAttachment{
AttachmentInfo: attachmentinfo.AttachmentInfo{
AttachmentInfo: attachment.AttachmentInfo{
AttachmentARN: "eni_arn",
Status: status.AttachmentAttached,
Status: attachment.AttachmentAttached,
},
},
})
Expand Down
6 changes: 3 additions & 3 deletions agent/api/statechange.go
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ import (
apicontainer "github.com/aws/amazon-ecs-agent/agent/api/container"
apitask "github.com/aws/amazon-ecs-agent/agent/api/task"
"github.com/aws/amazon-ecs-agent/agent/statechange"
"github.com/aws/amazon-ecs-agent/ecs-agent/api/attachment"
apicontainerstatus "github.com/aws/amazon-ecs-agent/ecs-agent/api/container/status"
apitaskstatus "github.com/aws/amazon-ecs-agent/ecs-agent/api/task/status"
"github.com/aws/amazon-ecs-agent/ecs-agent/logger"
Expand Down Expand Up @@ -98,8 +99,7 @@ type TaskStateChange struct {
// AttachmentStateChange represents a state change that needs to be sent to the
// SubmitAttachmentStateChanges API
type AttachmentStateChange struct {
// Attachment is the eni attachment object to send
Attachment *ni.ENIAttachment
Attachment attachment.Attachment
}

type ErrShouldNotSendEvent struct {
Expand Down Expand Up @@ -352,7 +352,7 @@ func (change *TaskStateChange) String() string {
// String returns a human readable string representation of this object
func (change *AttachmentStateChange) String() string {
if change.Attachment != nil {
return fmt.Sprintf("%s -> %s, %s", change.Attachment.AttachmentARN, change.Attachment.Status.String(),
return fmt.Sprintf("%s -> %v, %s", change.Attachment.GetAttachmentARN(), change.Attachment.GetAttachmentStatus(),
change.Attachment.String())
}

Expand Down
2 changes: 1 addition & 1 deletion agent/api/task/task.go
Original file line number Diff line number Diff line change
Expand Up @@ -41,9 +41,9 @@ import (
taskresourcevolume "github.com/aws/amazon-ecs-agent/agent/taskresource/volume"
"github.com/aws/amazon-ecs-agent/agent/utils"
"github.com/aws/amazon-ecs-agent/ecs-agent/acs/model/ecsacs"
apiresource "github.com/aws/amazon-ecs-agent/ecs-agent/api/attachment/resource"
apicontainerstatus "github.com/aws/amazon-ecs-agent/ecs-agent/api/container/status"
apierrors "github.com/aws/amazon-ecs-agent/ecs-agent/api/errors"
apiresource "github.com/aws/amazon-ecs-agent/ecs-agent/api/resource"
apitaskstatus "github.com/aws/amazon-ecs-agent/ecs-agent/api/task/status"
"github.com/aws/amazon-ecs-agent/ecs-agent/credentials"
"github.com/aws/amazon-ecs-agent/ecs-agent/ecs_client/model/ecs"
Expand Down
4 changes: 2 additions & 2 deletions agent/api/task/task_attachment_handler.go
Original file line number Diff line number Diff line change
Expand Up @@ -19,12 +19,12 @@ import (
"github.com/aws/amazon-ecs-agent/agent/api/serviceconnect"
taskresourcevolume "github.com/aws/amazon-ecs-agent/agent/taskresource/volume"
"github.com/aws/amazon-ecs-agent/ecs-agent/acs/model/ecsacs"
apiresource "github.com/aws/amazon-ecs-agent/ecs-agent/api/resource"
apiresource "github.com/aws/amazon-ecs-agent/ecs-agent/api/attachment/resource"
"github.com/aws/amazon-ecs-agent/ecs-agent/logger"
"github.com/aws/aws-sdk-go/aws"
)

// AttachmentHandler defines an interface to handel attachment received from ACS.
// AttachmentHandler defines an interface to handle attachment received from ACS.
type AttachmentHandler interface {
parseAttachment(acsAttachment *ecsacs.Attachment) error
validateAttachment(acsTask *ecsacs.Task, task *Task) error
Expand Down
2 changes: 1 addition & 1 deletion agent/api/task/task_attachment_handler_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ import (
"github.com/aws/amazon-ecs-agent/agent/api/serviceconnect"
taskresourcevolume "github.com/aws/amazon-ecs-agent/agent/taskresource/volume"
"github.com/aws/amazon-ecs-agent/ecs-agent/acs/model/ecsacs"
apiresource "github.com/aws/amazon-ecs-agent/ecs-agent/api/resource"
apiresource "github.com/aws/amazon-ecs-agent/ecs-agent/api/attachment/resource"
"github.com/aws/aws-sdk-go/aws"
"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"
Expand Down
2 changes: 1 addition & 1 deletion agent/api/task/task_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -48,8 +48,8 @@ import (
taskresourcevolume "github.com/aws/amazon-ecs-agent/agent/taskresource/volume"
"github.com/aws/amazon-ecs-agent/agent/utils"
"github.com/aws/amazon-ecs-agent/ecs-agent/acs/model/ecsacs"
apiresource "github.com/aws/amazon-ecs-agent/ecs-agent/api/attachment/resource"
apicontainerstatus "github.com/aws/amazon-ecs-agent/ecs-agent/api/container/status"
apiresource "github.com/aws/amazon-ecs-agent/ecs-agent/api/resource"
apitaskstatus "github.com/aws/amazon-ecs-agent/ecs-agent/api/task/status"
"github.com/aws/amazon-ecs-agent/ecs-agent/credentials"
mock_credentials "github.com/aws/amazon-ecs-agent/ecs-agent/credentials/mocks"
Expand Down
2 changes: 1 addition & 1 deletion agent/api/task/taskvolume.go
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ import (
"github.com/aws/amazon-ecs-agent/agent/taskresource/fsxwindowsfileserver"
taskresourcetypes "github.com/aws/amazon-ecs-agent/agent/taskresource/types"
taskresourcevolume "github.com/aws/amazon-ecs-agent/agent/taskresource/volume"
apiresource "github.com/aws/amazon-ecs-agent/ecs-agent/api/resource"
apiresource "github.com/aws/amazon-ecs-agent/ecs-agent/api/attachment/resource"

"github.com/cihub/seelog"
"github.com/pkg/errors"
Expand Down
2 changes: 1 addition & 1 deletion agent/api/task/taskvolume_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -28,8 +28,8 @@ import (
mock_dockerapi "github.com/aws/amazon-ecs-agent/agent/dockerclient/dockerapi/mocks"
"github.com/aws/amazon-ecs-agent/agent/taskresource"
taskresourcevolume "github.com/aws/amazon-ecs-agent/agent/taskresource/volume"
apiresource "github.com/aws/amazon-ecs-agent/ecs-agent/api/attachment/resource"
apicontainerstatus "github.com/aws/amazon-ecs-agent/ecs-agent/api/container/status"
apiresource "github.com/aws/amazon-ecs-agent/ecs-agent/api/resource"

"github.com/docker/docker/api/types/volume"
"github.com/golang/mock/gomock"
Expand Down
4 changes: 2 additions & 2 deletions agent/app/data_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ import (
"github.com/aws/amazon-ecs-agent/agent/engine/dockerstate"
"github.com/aws/amazon-ecs-agent/agent/engine/image"
"github.com/aws/amazon-ecs-agent/agent/statemanager"
"github.com/aws/amazon-ecs-agent/ecs-agent/api/attachmentinfo"
"github.com/aws/amazon-ecs-agent/ecs-agent/api/attachment"
"github.com/aws/amazon-ecs-agent/ecs-agent/eventstream"
ni "github.com/aws/amazon-ecs-agent/ecs-agent/netlib/model/networkinterface"
"github.com/golang/mock/gomock"
Expand Down Expand Up @@ -76,7 +76,7 @@ var (
}

testENIAttachment = &ni.ENIAttachment{
AttachmentInfo: attachmentinfo.AttachmentInfo{
AttachmentInfo: attachment.AttachmentInfo{
AttachmentARN: testAttachmentArn,
AttachStatusSent: false,
},
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -19,21 +19,24 @@ package data
import (
"testing"

"github.com/aws/amazon-ecs-agent/ecs-agent/api/attachmentinfo"
"github.com/aws/amazon-ecs-agent/ecs-agent/api/attachment"
"github.com/aws/amazon-ecs-agent/ecs-agent/api/attachment/resource"
ni "github.com/aws/amazon-ecs-agent/ecs-agent/netlib/model/networkinterface"
"github.com/stretchr/testify/assert"
)

const (
testAttachmentArn = "arn:aws:ecs:us-west-2:167933679560:attachment/test-arn"
testAttachmentArn2 = "arn:aws:ecs:us-west-2:167933679560:attachment/test-arn2"
testAttachmentArn3 = "arn:aws:ecs:us-west-2:123456789012:volume/test-arn3"
testAttachmentArn4 = "arn:aws:ecs:us-west-2:123456789012:volume/test-arn4"
)

func TestManageENIAttachments(t *testing.T) {
testClient := newTestClient(t)

testEniAttachment := &ni.ENIAttachment{
AttachmentInfo: attachmentinfo.AttachmentInfo{
AttachmentInfo: attachment.AttachmentInfo{
AttachmentARN: testAttachmentArn,
AttachStatusSent: false,
},
Expand All @@ -49,7 +52,7 @@ func TestManageENIAttachments(t *testing.T) {
assert.Equal(t, testAttachmentArn, res[0].AttachmentARN)

testEniAttachment2 := &ni.ENIAttachment{
AttachmentInfo: attachmentinfo.AttachmentInfo{
AttachmentInfo: attachment.AttachmentInfo{
AttachmentARN: testAttachmentArn2,
AttachStatusSent: true,
},
Expand All @@ -67,15 +70,66 @@ func TestManageENIAttachments(t *testing.T) {
assert.Len(t, res, 0)
}

func TestManageResourceAttachments(t *testing.T) {
testClient := newTestClient(t)

testResAttachment := &resource.ResourceAttachment{
AttachmentInfo: attachment.AttachmentInfo{
AttachmentARN: testAttachmentArn3,
AttachStatusSent: false,
},
}

assert.NoError(t, testClient.SaveResourceAttachment(testResAttachment))
testResAttachment.SetSentStatus()
assert.NoError(t, testClient.SaveResourceAttachment(testResAttachment))
res, err := testClient.GetResourceAttachments()
assert.NoError(t, err)
assert.Len(t, res, 1)
assert.Equal(t, true, res[0].AttachStatusSent)
assert.Equal(t, testAttachmentArn3, res[0].AttachmentARN)

testResAttachment2 := &resource.ResourceAttachment{
AttachmentInfo: attachment.AttachmentInfo{
AttachmentARN: testAttachmentArn4,
AttachStatusSent: true,
},
}

assert.NoError(t, testClient.SaveResourceAttachment(testResAttachment2))
res, err = testClient.GetResourceAttachments()
assert.NoError(t, err)
assert.Len(t, res, 2)

assert.NoError(t, testClient.DeleteResourceAttachment("test-arn3"))
assert.NoError(t, testClient.DeleteResourceAttachment("test-arn4"))
res, err = testClient.GetResourceAttachments()
assert.NoError(t, err)
assert.Len(t, res, 0)
}

func TestSaveENIAttachmentInvalidID(t *testing.T) {
testClient := newTestClient(t)

testEniAttachment := &ni.ENIAttachment{
AttachmentInfo: attachmentinfo.AttachmentInfo{
AttachmentInfo: attachment.AttachmentInfo{
AttachmentARN: "invalid-arn",
AttachStatusSent: false,
},
}

assert.Error(t, testClient.SaveENIAttachment(testEniAttachment))
}

func TestSaveResAttachmentInvalidID(t *testing.T) {
testClient := newTestClient(t)

testResAttachment := &resource.ResourceAttachment{
AttachmentInfo: attachment.AttachmentInfo{
AttachmentARN: "invalid-arn",
AttachStatusSent: false,
},
}

assert.Error(t, testClient.SaveResourceAttachment(testResAttachment))
}
Loading