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

fix(pipelines): 'ConfirmPermissionsBroadening' incorrectly invokes lambda for AWS CLI v2 #21462

Merged
merged 7 commits into from Aug 6, 2022

Conversation

trevorharwell
Copy link
Contributor

@trevorharwell trevorharwell commented Aug 4, 2022

Whenever our pipelines use the construct we get the following error:

An error occurred (InvalidRequestContentException) when calling the Invoke operation: Could not parse request body into json: Could not parse payload into json: Unexpected character ('>' (code 62)): expected a valid value (JSON String, Number, Array, Object or token 'null', 'true' or 'false')

Turns out for AWS CLI v2 you need to specify a flag to send in raw text input.

Documentation here
https://docs.aws.amazon.com/lambda/latest/dg/invocation-async.html

Explained here
https://medium.com/cloud-recipes/use-cli-binary-format-flag-with-aws-cli-version-2-34d590479280

All Submissions:

Adding new Unconventional Dependencies:

  • This PR adds new unconventional dependencies following the process described here

New Features

  • Have you added the new feature to an integration test?
    • Did you use yarn integ to deploy the infrastructure and generate the snapshot (i.e. yarn integ without --dry-run)?

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license

@gitpod-io
Copy link

gitpod-io bot commented Aug 4, 2022

@github-actions github-actions bot added the p2 label Aug 4, 2022
@aws-cdk-automation aws-cdk-automation requested a review from a team August 4, 2022 15:36
@mrgrain
Copy link
Contributor

mrgrain commented Aug 4, 2022

Hi @trevorharwell Could you elaborate on the issue and your motivation to fix? Specifically I'm wondering about the difference between AWS CLI v1 vs v2. Is v2 the default on these pipeline or something you added in yourself?

@trevorharwell
Copy link
Contributor Author

@mrgrain My primary motivation is that this construct has been broken for my organization since May 2022. Every build gets the error I describe above. To my knowledge, I am not (nor am I able to without some seriously messy escape hatches) making any changes to the codebuild machine. I believe aws/codebuild/standard:5.0 comes standard with AWS CLI v2. As such, the invoke lambda command is not correct per the AWS CLI v2 documentation.

Let me know if I am missing something.

Also, struggling to run the integration tests. Keep getting an empty zip file error which prevents deployment.

@trevorharwell
Copy link
Contributor Author

@mrgrain Confirmed that aws/codebuild/standard:5.0 uses AWS CLI v2

https://github.com/aws/aws-codebuild-docker-images/blob/master/ubuntu/standard/5.0/Dockerfile#L126

@mdownhower
Copy link

@mrgrain, I am one of Trevor's co-workers, this error only occurs when there are no changes to the permissions. When there are changes to permissions we do not get this error and the output does display the differences. Unfortunately this has created a manual approval step regardless of whether permissions have broadened or not.

@mrgrain
Copy link
Contributor

mrgrain commented Aug 4, 2022

Thanks @trevorharwell That's already helpful. Do you know what might have changed in May 2022 for it to suddenly break?

@mdownhower
Copy link

[> Thanks @trevorharwell That's already helpful. Do you know what might have changed in May 2022 for it to suddenly break?

If I remember correctly we began noticing this error after #20739 ](#20739)

@trevorharwell
Copy link
Contributor Author

@mdownhower Correct. It was then subsequently fixed by #20861

Which is what I think might have caused the codebuild box to start using AWS CLI v2.

@mrgrain
Copy link
Contributor

mrgrain commented Aug 4, 2022

I'll need some time to investigate this a bit further. My concern is that ApplicationSecurityCheck is agnostic to the build image, i.e. it can be run with any build image and while we are now defaulting to an image with AWS CLI v2, not everyone will.

I definitely would like to see integration tests for this, so we don't run into the same issue again.

Copy link
Contributor

@TheRealAmazonKendra TheRealAmazonKendra left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1 on everything @mrgrain said. We definitely need thorough testing for this. It seems that this change breaks the build so I'm concerned that this wasn't actually tested or built at all before this PR was submitted. It couldn't have passed locally.

@mrgrain
Copy link
Contributor

mrgrain commented Aug 5, 2022

Confirmed. This change updated the image. The modern image includes AWS CLI v2.

@mrgrain mrgrain added p1 @aws-cdk/pipelines CDK Pipelines library and removed p2 labels Aug 5, 2022
@mrgrain mrgrain self-assigned this Aug 5, 2022
@mergify mergify bot dismissed TheRealAmazonKendra’s stale review August 5, 2022 14:40

Pull request has been modified.

@trevorharwell
Copy link
Contributor Author

@mrgrain I'm happy to work on integration tests. As I stated above, I could never actually run the integration tests locally because I get the following error on deployment:

Failed resources:
PipelineSecurityStack | 10:43:59 AM | CREATE_FAILED        | AWS::Lambda::Function       | Custom::S3AutoDeleteObjectsCustomResourceProvider/Handler (CustomS3AutoDeleteObjectsCustomResourceProviderHandler9D90184F) Resource handler returned message: "Uploaded file must be a non-empty zip (Service: Lambda, Status Code: 400, Request ID: 327c83c1-c094-416c-8497-889ae48b3f22)" (RequestToken: 21970a97-cb76-2c13-dce3-cd3245c16f84, HandlerErrorCode: InvalidRequest)

This is the command I ran: yarn integ test/integ.pipeline-security.js --profiles sandbox --update-on-failed --parallel-regions us-east-1

@mrgrain
Copy link
Contributor

mrgrain commented Aug 5, 2022

@trevorharwell Just letting you know it fails for me as well. I'm looking into it.

@mrgrain
Copy link
Contributor

mrgrain commented Aug 5, 2022

@trevorharwell This commit fixes the broken test and updates the snapshot for your change: mrgrain@492c6c3
Feel free to rebase/cherry-pick/whatever or let me know if you'd like me to push this to your branch.

Furhtermore @TheRealAmazonKendra and I have discussed this and unfortunately we don't really have the means at the moment to test this in an automated way. So with my change this is as good as it gets.

Thanks again for this contribution. I'm sorry this has been broken for you for such a long time. 😬

@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: AutoBuildv2Project1C6BFA3F-wQm2hXv2jqQv
  • Commit ID: b0465e3
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

@mergify
Copy link
Contributor

mergify bot commented Aug 6, 2022

Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

@mergify mergify bot merged commit a913d60 into aws:main Aug 6, 2022
@trevorharwell trevorharwell deleted the pipelines-cli-binary-format branch August 6, 2022 15:46
josephedward pushed a commit to josephedward/aws-cdk that referenced this pull request Aug 30, 2022
…mbda for AWS CLI v2 (aws#21462)

Whenever our pipelines use the construct we get the following error:
```
An error occurred (InvalidRequestContentException) when calling the Invoke operation: Could not parse request body into json: Could not parse payload into json: Unexpected character ('>' (code 62)): expected a valid value (JSON String, Number, Array, Object or token 'null', 'true' or 'false')
```

Turns out for AWS CLI v2 you need to specify a flag to send in raw text input.

Documentation here
https://docs.aws.amazon.com/lambda/latest/dg/invocation-async.html

Explained here
https://medium.com/cloud-recipes/use-cli-binary-format-flag-with-aws-cli-version-2-34d590479280

### All Submissions:

* [x] Have you followed the guidelines in our [Contributing guide?](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md)

### Adding new Unconventional Dependencies:

* [ ] This PR adds new unconventional dependencies following the process described [here](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md/#adding-new-unconventional-dependencies)

### New Features

* [ ] Have you added the new feature to an [integration test](https://github.com/aws/aws-cdk/blob/main/INTEGRATION_TESTS.md)?
	* [ ] Did you use `yarn integ` to deploy the infrastructure and generate the snapshot (i.e. `yarn integ` without `--dry-run`)?

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
@aws-cdk/pipelines CDK Pipelines library p1
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants