Validate file system type for EBS-backed task attachment payload #3954
Conversation
mye956
changed the title
| @@ -202,6 +202,10 @@ func validateAttachmentAndReturnProperties(message *ecsacs.ConfirmAttachmentMess | |||
| if err != nil { | |||
| return nil, errors.Wrap(err, "resource attachment validation by attachment type failed") | |||
| } | |||
| err = resource.ValidateFileSystemType(attachmentProperties[resource.FileSystemKey]) | |||
There was a problem hiding this comment.
nit - can we have some unit tests to enforce/make sure
- all allowed file system are allowed as expected
- others will be disallowed/failed as expected
There was a problem hiding this comment.
yep, changes were made to the attach resource responder unit test to check for invalid values for file system types. I'll add some changes for happy cases as well
| for _, property := range confirmAttachmentMessageCopy.Attachment.AttachmentProperties { | ||
| if aws.StringValue(property.Name) == resource.FileSystemKey { | ||
| originalPropertyValue := property.Value | ||
| property.Value = aws.String("SomeFilesystemType") | ||
| _, err = validateAttachmentAndReturnProperties(&confirmAttachmentMessageCopy) | ||
| require.Error(t, err) | ||
| property.Value = originalPropertyValue | ||
|
|
||
| originalPropertyValue = property.Value | ||
| property.Value = aws.String("") | ||
| _, err = validateAttachmentAndReturnProperties(&confirmAttachmentMessageCopy) | ||
| require.Error(t, err) | ||
| property.Value = originalPropertyValue | ||
| } | ||
| } |
There was a problem hiding this comment.
[non-blocking] In future I recommend separating out test cases from test runner code to better readability.
There was a problem hiding this comment.
yep I agree, the unit test in our attach resource responder is a bit messy to read. I'll add a todo
There was a problem hiding this comment.
actually, I like the idea of separating this out of the runner code. I made it's own test
| @@ -74,6 +74,16 @@ const ( | |||
| FileSystemKey = "fileSystem" | |||
| ) | |||
|
|
|||
| var ( | |||
| AllowedFSTypes = map[string]bool{ | |||
There was a problem hiding this comment.
Why do we want to export this map?
There was a problem hiding this comment.
good point! I'll restrict this to be private instead of global
mye956
force-pushed
the
ebsFSValidation
branch
2 times, most recently
from
9422ad4 to
396f5f8
Compare
mye956
force-pushed
the
ebsFSValidation
branch
from
396f5f8 to
937b8e7
Compare
Summary
This PR will introduce a new validation check for the attachment payload of EBS-backed task. Specifically if the file system type is included within the attachment payload then we want to ensure that they are of the following types:
Implementation details
Implemented a new functionality called
ValidateFileSystemTypethat checks if the filesystem value is one of the specified types listed above. These types are essentially stored as a set calledAllowedFSTypes.Testing
Modified the existing unit tests within the attachment resource responder to now also check if we receive any errors if the file system type is some invalid value as well as empty.
New tests cover the changes: Yes
Description for the changelog
Validate file system type within attachment payload of EBS-backed tasks
Does this PR include breaking model changes? If so, Have you added transformation functions?
Licensing
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.