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

Process attachment payload message for EBS attach #3911

Merged
merged 5 commits into from
Sep 19, 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

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

19 changes: 17 additions & 2 deletions ecs-agent/acs/session/attach_resource_responder.go
Original file line number Diff line number Diff line change
Expand Up @@ -91,6 +91,7 @@ func (r *attachResourceResponder) handleAttachMessage(message *ecsacs.ConfirmAtt
Status: status.AttachmentNone,
},
AttachmentProperties: attachmentProperties,
AttachmentType: aws.StringValue(message.Attachment.AttachmentType),
})

// Send ACK.
Expand Down Expand Up @@ -180,9 +181,23 @@ func validateAttachmentAndReturnProperties(message *ecsacs.ConfirmAttachmentMess
attachmentProperties[name] = value
}

err = resource.ValidateResource(attachmentProperties)
// For "AmazonElasticBlockStorage" used by the EBS attach, ACS is using attachmentType to indicate its attachment type.
attachmentType := aws.StringValue(message.Attachment.AttachmentType)
if attachmentType == resource.AmazonElasticBlockStorage {
err = resource.ValidateRequiredProperties(
attachmentProperties,
resource.GetVolumeSpecificPropertiesForEBSAttach(),
)
if err != nil {
return nil, errors.Wrap(err, "resource attachment validation by attachment type failed")
}
return attachmentProperties, nil
}

// For "EphemeralStorage" and "ElasticBlockStorage", ACS is using resourceType to indicate its attachment type.
xxx0624 marked this conversation as resolved.
Show resolved Hide resolved
err = resource.ValidateResourceByResourceType(attachmentProperties)
if err != nil {
return nil, errors.Wrap(err, "resource attachment validation error")
return nil, errors.Wrap(err, "resource attachment validation by resource type failed ")
}

return attachmentProperties, nil
Expand Down
111 changes: 100 additions & 11 deletions ecs-agent/acs/session/attach_resource_responder_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,29 @@ const (
)

var (
testAttachmentPropertiesForEBSAttach = []*ecsacs.AttachmentProperty{
{
Name: aws.String(resource.SourceVolumeHostPathKey),
Value: aws.String("taskarn-vol-id"),
},
{
Name: aws.String(resource.VolumeIdKey),
Value: aws.String("id1"),
},
{
Name: aws.String(resource.VolumeSizeGibKey),
Value: aws.String("size1"),
},
{
Name: aws.String(resource.VolumeNameKey),
Value: aws.String("name"),
},
{
Name: aws.String(resource.DeviceNameKey),
Value: aws.String("device1"),
},
}

testAttachmentProperties = []*ecsacs.AttachmentProperty{
{
Name: aws.String(resource.FargateResourceIdName),
Expand Down Expand Up @@ -83,46 +106,47 @@ func TestValidateAttachResourceMessage(t *testing.T) {
defer ctrl.Finish()

_, err := validateAttachResourceMessage(nil)

require.Error(t, err)

// Verify the Attachment is required.
confirmAttachmentMessageCopy := *testConfirmAttachmentMessage
confirmAttachmentMessageCopy.Attachment = nil

_, err = validateAttachResourceMessage(&confirmAttachmentMessageCopy)

require.Error(t, err)

// Verify the MessageId is required.
confirmAttachmentMessageCopy = *testConfirmAttachmentMessage
confirmAttachmentMessageCopy.MessageId = aws.String("")

_, err = validateAttachResourceMessage(&confirmAttachmentMessageCopy)

require.Error(t, err)

// Verify the ClusterArn is required.
confirmAttachmentMessageCopy = *testConfirmAttachmentMessage
confirmAttachmentMessageCopy.ClusterArn = aws.String("")

_, err = validateAttachResourceMessage(&confirmAttachmentMessageCopy)

require.Error(t, err)

// Verify the ContainerInstanceArn is required.
confirmAttachmentMessageCopy = *testConfirmAttachmentMessage
confirmAttachmentMessageCopy.ContainerInstanceArn = aws.String("")

_, err = validateAttachResourceMessage(&confirmAttachmentMessageCopy)

require.Error(t, err)

// Verify the AttachmentArn is required and uses correct format.
confirmAttachmentMessageCopy = *testConfirmAttachmentMessage
confirmAttachmentMessageCopy.Attachment.AttachmentArn = aws.String("incorrectArn")

_, err = validateAttachResourceMessage(&confirmAttachmentMessageCopy)

require.Error(t, err)
}

func TestValidateAttachmentAndReturnProperties(t *testing.T) {
t.Run("with no attachment type", testValidateAttachmentAndReturnPropertiesWithoutAttachmentType)
t.Run("with attachment type", testValidateAttachmentAndReturnPropertiesWithAttachmentType)
}

// testValidateAttachmentAndReturnPropertiesWithoutAttachmentType verifies all required properties for either
// resource type: "EphemeralStorage" or "ElasticBlockStorage".
func testValidateAttachmentAndReturnPropertiesWithoutAttachmentType(t *testing.T) {
ctrl := gomock.NewController(t)
defer ctrl.Finish()

Expand Down Expand Up @@ -157,6 +181,71 @@ func TestValidateAttachmentAndReturnProperties(t *testing.T) {
}
}

// testValidateAttachmentAndReturnPropertiesWithAttachmentType verifies all required properties for attachment
// type: "AmazonElasticBlockStorage".
func testValidateAttachmentAndReturnPropertiesWithAttachmentType(t *testing.T) {
ctrl := gomock.NewController(t)
defer ctrl.Finish()

// Verify the AttachmentArn is required and uses the correct format.
confirmAttachmentMessageCopy := *testConfirmAttachmentMessage
confirmAttachmentMessageCopy.Attachment.AttachmentArn = aws.String("incorrectArn")
_, err := validateAttachmentAndReturnProperties(&confirmAttachmentMessageCopy)
require.Error(t, err)
confirmAttachmentMessageCopy.Attachment.AttachmentArn = aws.String(testAttachmentArn)

// Verify the property name & property value must be non-empty.
for _, property := range confirmAttachmentMessageCopy.Attachment.AttachmentProperties {
t.Run(property.String(), func(t *testing.T) {
originalPropertyName := property.Name
property.Name = aws.String("")
_, err := validateAttachmentAndReturnProperties(&confirmAttachmentMessageCopy)
require.Error(t, err)
property.Name = originalPropertyName

originalPropertyValue := property.Value
property.Value = aws.String("")
_, err = validateAttachmentAndReturnProperties(&confirmAttachmentMessageCopy)
require.Error(t, err)
property.Value = originalPropertyValue
})
}

// Reset attachment to be a good one with invalid attachment type.
confirmAttachmentMessageCopy.Attachment.AttachmentType = aws.String("invalid-type")
_, err = validateAttachmentAndReturnProperties(&confirmAttachmentMessageCopy)
require.NoError(t, err)

// Reset attachment to be a good one with valid attachment type.
confirmAttachmentMessageCopy.Attachment.AttachmentType = aws.String("AmazonElasticBlockStorage")
confirmAttachmentMessageCopy.Attachment.AttachmentProperties = testAttachmentPropertiesForEBSAttach

// Verify all required properties for the attachment for EBS attach are present.
requiredProperties := []string{
"volumeId",
"volumeSizeGib",
"deviceName",
"sourceVolumeHostPath",
"volumeName",
}
for _, requiredProperty := range requiredProperties {
verified := false
for _, property := range confirmAttachmentMessageCopy.Attachment.AttachmentProperties {
if requiredProperty == aws.StringValue(property.Name) {
originalPropertyName := property.Name
property.Name = aws.String("")
_, err := validateAttachmentAndReturnProperties(&confirmAttachmentMessageCopy)
require.Error(t, err)
property.Name = originalPropertyName

verified = true
break
}
}
require.True(t, verified, "Missing required property: %s", requiredProperty)
}
}

// TestResourceAckHappyPath tests the happy path for a typical ConfirmAttachmentMessage and confirms expected
// ACK request is made
func TestResourceAckHappyPath(t *testing.T) {
Expand Down
2 changes: 2 additions & 0 deletions ecs-agent/api/resource/ebs_discovery.go
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,8 @@ const (
)

var (
// When confirming an EBS volume is attached to a host, if the expected volume ID does not
// match the volume ID found on the host, this error is returned.
ErrInvalidVolumeID = errors.New("EBS volume IDs do not match")
)

Expand Down
Loading