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

aws ecs register-task-definition fails in the new upgrade(2.1.20) #38

Closed
2 tasks done
abirdatta opened this issue Jan 20, 2021 · 17 comments
Closed
2 tasks done

aws ecs register-task-definition fails in the new upgrade(2.1.20) #38

abirdatta opened this issue Jan 20, 2021 · 17 comments
Assignees
Labels
bug Something isn't working ecs

Comments

@abirdatta
Copy link

Confirm by changing [2.1.20] to [2.1.19] below to ensure that it's a bug:

Describe the bug
aws ecs register-task-definition fails when the cli-input-json of task definition has registeredBy and registeredAt parameters.
actual error -

Parameter validation failed:
Unknown parameter in input: "registeredAt", must be one of: family, taskRoleArn, executionRoleArn, networkMode, containerDefinitions, volumes, placementConstraints, requiresCompatibilities, cpu, memory, tags, pidMode, ipcMode, proxyConfiguration, inferenceAccelerators
Unknown parameter in input: "registeredBy", must be one of: family, taskRoleArn, executionRoleArn, networkMode, containerDefinitions, volumes, placementConstraints, requiresCompatibilities, cpu, memory, tags, pidMode, ipcMode, proxyConfiguration, inferenceAccelerators

Whereas the aws ecs describe-task-definition returns the task definition json with the registeredBy and registeredAt parameters

SDK version number

Platform/OS/Hardware/Device
Linux
MacOS

To Reproduce (observed behavior)
TASK_DEFINITION=$(aws ecs describe-task-definition --task-definition "xyz:123" --region ap-southeast-1)
NEW_TASK_DEFINTIION=$(echo $TASK_DEFINITION | jq --arg IMAGE "image:tag" '.taskDefinition | .containerDefinitions[0].image = $IMAGE | del(.taskDefinitionArn) | del(.revision) | del(.status) | del(.requiresAttributes) | del(.compatibilities)')
NEW_TASK=$(aws ecs register-task-definition --cli-input-json "$NEW_TASK_DEFINTIION" --region ap-southeast-1)

Expected behavior
aws ecs register-task-definition --cli-input-json "$NEW_TASK_DEFINTIION" should work when the task definition json has registeredBy and registeredAt parameters.

Logs/output

2021-01-20 13:39:53,987 - MainThread - awscli.clidriver - DEBUG - Exception caught in main()
Traceback (most recent call last):
  File "awscli/clidriver.py", line 457, in main
  File "awscli/clidriver.py", line 586, in __call__
  File "awscli/clidriver.py", line 765, in __call__
  File "awscli/clidriver.py", line 894, in invoke
  File "awscli/clidriver.py", line 906, in _make_client_call
  File "botocore/client.py", line 249, in _api_call
  File "botocore/client.py", line 541, in _make_api_call
  File "botocore/client.py", line 589, in _convert_to_request_dict
  File "botocore/validate.py", line 297, in serialize_to_request
botocore.exceptions.ParamValidationError: Parameter validation failed:
Unknown parameter in input: "registeredAt", must be one of: family, taskRoleArn, executionRoleArn, networkMode, containerDefinitions, volumes, placementConstraints, requiresCompatibilities, cpu, memory, tags, pidMode, ipcMode, proxyConfiguration, inferenceAccelerators
Unknown parameter in input: "registeredBy", must be one of: family, taskRoleArn, executionRoleArn, networkMode, containerDefinitions, volumes, placementConstraints, requiresCompatibilities, cpu, memory, tags, pidMode, ipcMode, proxyConfiguration, inferenceAccelerators

Parameter validation failed:
Unknown parameter in input: "registeredAt", must be one of: family, taskRoleArn, executionRoleArn, networkMode, containerDefinitions, volumes, placementConstraints, requiresCompatibilities, cpu, memory, tags, pidMode, ipcMode, proxyConfiguration, inferenceAccelerators
Unknown parameter in input: "registeredBy", must be one of: family, taskRoleArn, executionRoleArn, networkMode, containerDefinitions, volumes, placementConstraints, requiresCompatibilities, cpu, memory, tags, pidMode, ipcMode, proxyConfiguration, inferenceAccelerators

Additional context

@abirdatta abirdatta changed the title aws-cli ecs register-task-definition fails in the new upgrade aws ecs register-task-definition fails in the new upgrade(2.1.20) Jan 20, 2021
@tanmaycolaco
Copy link

tanmaycolaco commented Jan 20, 2021

Do check out this thread onces fabfuel/ecs-deploy#154. The botocore version may be causing this issue

@fdaugan
Copy link

fdaugan commented Jan 20, 2021

Workaround:
NEW_TASK_DEFINTIION=$(echo $TASK_DEFINITION | jq --arg IMAGE "image:tag" '.taskDefinition | .containerDefinitions[0].image = $IMAGE | del(.taskDefinitionArn) | del(.revision) | del(.status) | del(.requiresAttributes) | del(.compatibilities) | del(.registeredAt) | del(.registeredBy)')

The same way we manually deleted statusattribute (and many others), we have to handle registeredAt and registeredBy cases.
Sounds legitimate.

@abirdatta
Copy link
Author

Workaround:
NEW_TASK_DEFINTIION=$(echo $TASK_DEFINITION | jq --arg IMAGE "image:tag" '.taskDefinition | .containerDefinitions[0].image = $IMAGE | del(.taskDefinitionArn) | del(.revision) | del(.status) | del(.requiresAttributes) | del(.compatibilities) | del(.registeredAt) | del(.registeredBy)')

The same way we manually deleted statusattribute (and many others), we have to handle registeredAt and registeredBy cases.
Sounds legitimate.

Thanks, we already are doing this in our deployment scripts, but we have many to change, so would like to know if we have to do this workaround or a fix is expected for this. Also requested this as the register-task-definition api documentation page has these attributes in the task definition json input details.

@kdaily kdaily self-assigned this Jan 20, 2021
@kdaily
Copy link
Member

kdaily commented Jan 20, 2021

Hi @abirdatta et al., looking into this.

These fields were added in botocore yesterday, commit boto/botocore@e66253e.

@kdaily
Copy link
Member

kdaily commented Jan 20, 2021

I can see that the definition for DescribeTaskDefinition and RegisterTaskDefinition are not the same, so it's expected that this will not work. However, I don't know if that was intentional or not. I will check in with ECS to find out.

@dev-head
Copy link

Perhaps this should be moved into a feature request to allow for parity between the describe and register json? it is far from ideal to have to manually delete attributes in order to register it again.

This release highlights the burden this places on our CI/CD pipelines, where if we do need to delete newly added fields, we may be forced to go and update hundreds of repositories across different code bases and across different git vendors and across different CI/CD vendors...then test each one by forcing a deployment, costing time and money...where, if instead the api can just account for it on it's end and remove the fields it doesn't need for registering a task...boom everyone wins.

/shrug my 2 cents

@kdaily
Copy link
Member

kdaily commented Jan 21, 2021

@dev-head it's a fair suggestion! I'm not sure why the RegisterTaskDefinition doesn't use the same shape for the request as the DescribeTaskDefinition uses for the response:

https://github.com/boto/botocore/blob/develop/botocore/data/ecs/2014-11-13/service-2.json#L4841-L4934

In this case, we wouldn't want users to try and change the registeredAt value, but the API could handle that for you to ignore non-editable fields.

@dtserekhman-starz
Copy link

I agree, there must be parity between the "get" and "set" type methods. For example, we initially call DescribeTaskDefinition method, dynamically clone the resulted structure and only change one attribute on the clone. Then, we turn around and call RegisterTaskDefinition by passing the cloned taskDefinition structure, and we are now getting the error reported on this thread because RegisterTaskDefinition does not take some of the arguments returned by the DescribeTaskDefinition.

On the other hand, I also agree that registeredAt and registeredBy represent system timestamps/footprints which probably should not be preserved on the new TaskDefinitions. And I don't like the idea of introducing extra arguments to the RegisterTaskDefinition which simply will be ignored or reset to some other values. It's bad programming.

How about we introduce another DescribeTaskDefinition method (can't think of a good name for it yet) which will always guarantee parity of the returned arguments to the arguments taken by the RegisterTaskDefinition? This method will help out a lot for dynamic cloning and creation of the new TaskDefinition.

Or better yet, have a RegisterTaskDefinition() factory method which takes another TaskDefinition as an argument!

@dtserekhman-starz
Copy link

dtserekhman-starz commented Jan 22, 2021

On the other note, getting down to the root cause of the issue here, it is my understanding this was a long-standing bug which just now got fixed and caused this "side effect"?

The reason why I say it was a long-standing bug is because in our code we lock-in the version of the ECS client upon its instantiation like so:

client = session.client('ecs', api_version = '2014-11-13')

So, I was really surprised when all of the sudden our code broke with a newer version of boto3. Should not the specification of specific api_version in the constructor above guarantee a certain API contract, which should not change?

I went ahead and found the API doc which states that very same API version on the cover page (2014-11-13):
https://docs.aws.amazon.com/AmazonECS/latest/APIReference/ecs-api.pdf

And sure enough, this "2014-11-13" versioned doc already lists registeredAt and registeredBy attributes on the DescribeTaskDefinition method.

So, these attributes were a part of "2014-11-13" API contract and should have been returned by boto library all along but they were not. And only now this "bug" got fixed, but obviously started causing this other side effect problem where users "were trained" not to expect to see these attributes :)

@aloysiustany
Copy link

Starting to see the same problem now reporting for deregisteredAt

lokst referenced this issue in CircleCI-Public/aws-ecs-orb Jan 26, 2021
@kdaily
Copy link
Member

kdaily commented Jan 26, 2021

@dtserekhman-starz - the API version you're seeing there is not in use. The AWS CLI v2 (and botocore version 2) do not have this attribute required anymore. The SDKs are updated when the API models change, and the AWS CLI updates when the Python SDK updates.

@kdaily
Copy link
Member

kdaily commented Jan 26, 2021

The service team is aware of the issue, and I have raised the point of improving the developer experience. I will update once I have an update from them.

@kdaily
Copy link
Member

kdaily commented Feb 1, 2021

P43625635

@kdaily
Copy link
Member

kdaily commented Feb 1, 2021

I'm moving this to our central aws-sdk repository for further tracking as this issue would affect other SDKs as well.

@kdaily kdaily transferred this issue from aws/aws-cli Feb 1, 2021
@kdaily kdaily added the ecs label Feb 1, 2021
@motawy
Copy link

motawy commented Feb 2, 2021

From what is written on the docs only "family" was required but now it asks for family, networkMode, containerName, containerImage, memory, cpu, requiresCompatibilities and now also executionRoleArn. So far I still get error messages for more requirements.

Is this an expected behaviour?

@kdaily
Copy link
Member

kdaily commented Aug 11, 2021

Thank you everyone for your patience and feedkback. Unfortunately the ECS team does not have any plans to make any changes to this behavior, so I'm going to close this issue.

@motawy - I think your question is separate from this. If you're still having trouble, can you open up a separate issue with your debug logs at the AWS CLI GitHub repository?

@kdaily kdaily closed this as completed Aug 11, 2021
@github-actions
Copy link

This issue is now closed.

Comments on closed issues are hard for our team to see.
If you need more assistance, please either tag a team member or open a new issue that references this one.
If you wish to keep having a conversation with other community members under this issue feel free to do so.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working ecs
Projects
None yet
Development

No branches or pull requests

9 participants