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(s3): auto-delete fails when bucket has been deleted manually #16645

Merged
merged 54 commits into from
Oct 8, 2021
Merged

fix(s3): auto-delete fails when bucket has been deleted manually #16645

merged 54 commits into from
Oct 8, 2021

Conversation

otaviomacedo
Copy link
Contributor

Even though buckets are not supposed to be deleted manually, this change makes the delete operation idempotent and thus more reliable.

Fixes #16619.


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

@otaviomacedo otaviomacedo requested a review from a team September 24, 2021 13:30
@gitpod-io
Copy link

gitpod-io bot commented Sep 24, 2021

@otaviomacedo otaviomacedo changed the title Auto delete ignore missing buckets fix(s3): auto-delete fails when bucket has been deleted manually Sep 24, 2021
@mergify mergify bot added the contribution/core This is a PR that came from AWS. label Sep 24, 2021
Copy link
Contributor

@LukvonStrom LukvonStrom left a comment

Choose a reason for hiding this comment

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

wouldn't using emptyBucket() in the try/catch alone suffice?

@otaviomacedo
Copy link
Contributor Author

wouldn't using emptyBucket() in the try/catch alone suffice?

Yes, thanks for catching this.

Co-authored-by: Jonathan Goldwasser <jogold@users.noreply.github.com>
@mergify
Copy link
Contributor

mergify bot commented Oct 8, 2021

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

njlynch and others added 10 commits October 8, 2021 12:05
This value was reduced as part of troubleshooting of various Node Worker memory
issues. These issues are theorized to have been mitigated by #16752. Our pack
time is currently over 2 hours, compared to 20-30 minutes prior to the set of
changes. By removing this worker count override, we should be able to get back
to normal pack times and speed up the pipeline.


----

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

This PR updates this repo's GitHub issue templates to v2. ([see prototype](https://github.com/ryparker/proto-github-issues-v2/issues/new/choose)) 

**Reviewers**: Please make sure that all the fields i've marked with `required: true` are necessary. A user will not be able to create an issue without these required fields being completed.

[GitHub issues v2 docs](https://docs.github.com/en/communities/using-templates-to-encourage-useful-issues-and-pull-requests/syntax-for-issue-forms)

<kbd>

<img width="1278" alt="CleanShot 2021-09-21 at 18 37 06@2x" src="https://user-images.githubusercontent.com/17558268/134269803-f5dda15c-6bdc-4c63-ac3e-65a3f1626246.png">

</kbd>

----

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

## Summary

This [commit](ceab036) broke EKS deployments. CloudFormation throws "Internal failure." when attempting to create an EKS cluster.

Full details : https://github.com/aws/aws-cdk/pull/16751/files#r720549975


This reverts commit ceab036.


----

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

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
…ler (#16771)

fixes :#16669

----

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


----

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

Fixes #16563.


----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
According to
[AWS Config best practices](https://docs.aws.amazon.com/config/latest/developerguide/evaluate-config_develop-rules_nodejs.html#restricted-lambda-policy),
we should add a `SourceAccount` condition to the Lambda Permission we create in `CustomRule`.

Note that we cannot add the `SourceArn` condition,
because that would cause a cyclic dependency between the `LambdaPermission` resource,
and the `Rule` resource
(as the `Rule` can only be created _after_ the `LambdaPermission` has been created -
this is validated by the AWS Config service -
and so needs a `DependOn` for the Lambda Permission).

----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
Currently, the `resourcesTotal` output is one short as it doesn't account for the `UPDATE_COMPLETE` event emitted when updating a stack. This PR increases the `resourcesTotal` variable depending on whether the stack is being updated or created.

Noticed this bug when using the CDK on private projects.

This has had a minor fix previously to address the `CREATE_COMPLETE` event emitted when creating a stack, however this did not address the `UPDATE_COMPLETE` event emitted when updating a stack. This caused updated events to produce the following output:

![image](https://user-images.githubusercontent.com/57939433/130373537-5dfacd3c-df7d-4272-abac-a4cf7c04cc47.png)

To address this issue, I:
- Added `+1` to the `resourcesTotal` prop in `packages/aws-cdk/lib/api/deploy-stack.ts` for the `StackActivityMonitor` class depending on whether the stack being deployed already exists using the `cloudFormationStack.exists` boolean.

I also addressed a spacing issue between the pipe (`|`) and the timestamp, as seen in the image above.

Collaborators:
- @JWK95: Provided code review & valid suggestions

----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
…th (#16269)

If a IAM user has a path, the ARN contains the path, e.g. `arn:aws:iam::account-id:user/path/MyUserName`.
Method `User.fromUserArn` parses this ARN to `userName`: `path/MyUserName`. The path is not removed correctly. The correct username would be `MyUserName`.

This PR changes the parsing of property `userName` to remove the path correctly. The logic is implemented according to [iam.Role](https://github.com/aws/aws-cdk/blob/d5ca419448e84f0cbb25dbd90d48fb4c407ede5c/packages/%40aws-cdk/aws-iam/lib/role.ts#L191-L194) where a similar conversion is necessary to support service roles.

Fixes #16256.


----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
Elad Ben-Israel and others added 25 commits October 8, 2021 12:06
Since 10.x is end of life we now build against 12.x

----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
Ran npm-check-updates and yarn upgrade to keep the `yarn.lock` file up-to-date.
* feat: cloudformation spec v43.0.0

* Add queuePolicyId

Co-authored-by: AWS CDK Team <aws-cdk@amazon.com>
Co-authored-by: Elad Ben-Israel <benisrae@amazon.com>
----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
Some AWS regions do not yet support the Node 14.x runtime for Lambda, which
makes it challenging to use anything that involves a vended custom resource that
uses it.

Downgrading all those resources to use Node 12.x instead, which is available in
all regions where Lambda is, and hence provides better coverage and usability.

Fixes cdklabs/cdk-ops#1686


----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
* feat: cloudformation spec v43.0.0

* feat: cloudformation spec v43.0.0

* patch lightsail

Co-authored-by: AWS CDK Team <aws-cdk@amazon.com>
Co-authored-by: Elad Ben-Israel <benisrae@amazon.com>
Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
Ran npm-check-updates and yarn upgrade to keep the `yarn.lock` file up-to-date.
Adds the relatively new [Lambda Authorizer for GraphqlApi](https://aws.amazon.com/blogs/mobile/appsync-lambda-auth/).

Closes: #16380. 

----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
…jects` to `false` (#16828)

## Summary

This PR:

- Updates the documentation of the `aws-s3/Bucket`'s `autoDeleteObjects` prop to clarify that a user must first deploy with the latest CDK package before updating `autoDeleteObjects`'s value to `false` (to prevent the bucket from being emptied). 

----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
…not` and `Match.absent` (#16678)

Fixes #16626.

This PR modifies `hasResourceProperties()` so that it accounts for the special case where `Properties` does not exist on the template. The following assertions previously behaved incorrectly when `Properties` were not in the template and are now fixed:

```ts
template.fromStack(stack); // some template with no `Properties`.

// assert that `Properties` does not exist in the template. Returns true.
template.hasResourceProperties('Foo::Bar', Match.absent());

// assert that `baz` is not a `Property` of 'Foo::Bar'. Returns true.
template.hasResourceProperties('Foo::Bar', {
   baz: Match.absent(),
};

// assert that `baz` does not have a value of `qux` in the `Properties` object. Returns true.
template.hasResourceProperties('Foo::Bar', Match.not({baz: 'qux'});
```

It also moves `AbsentMatch` into the `private` folder so that it can be exposed internally as a special case for `hasResourceProperties()`.

----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
Add support for X2gd instances.

Announcement: https://aws.amazon.com/about-aws/whats-new/2021/03/announcing-new-amazon-ec2-x2gd-instances-powered-by-aws-graviton2-processors/

Closes #16794.

----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
Docker build args are meant for build system specific configuration like http proxy or CodeArtifact tokens. Give the user an option to not hash build args so the DockerImageAsset hash can remain consistent even when using build args. An inconsistent hash means the asset is built every synth and that wastes time and space.

This change is backwards compatible as the default hashing behavior remains the same.

closes #15936


----

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

CDK was extracting the value of HTTPS?_PROXY and passing this to proxy-agent explicitly, which resulted in not honoring the NO_PROXY setting.

This removes that behavior and lets proxy-agent delegate to proxy-from-env, which will leverage values in HTTPS?_PROXY and NO_PROXY correctly.

Tested by deploying [this sample repo](https://github.com/ryparker/aws-cdk-sample-eks) and monitoring Squid proxy logs while triggering the "onEvent" Lambda.

Fixes #7121
Related PRs: #16751, #16751

----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
Co-authored-by: AWS CDK Team <aws-cdk@amazon.com>
Some service's resource policies require the specification of
`Principal: *`, and will not accept `Principal: { AWS: * }`.

Our code was making assumptions that the two were compatible, and could
be interchangeably used. Our modelling made it impossible to represent
`Principal: *`.

- Fix an issue in `PolicyStatement.fromJson()` which would incorrectly normalize
  away `Principal: *`.
- Add a new principal class, `StarPrincipal`, which represents the
  principal of that type in the CDK object model.


----

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

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
…ces (#16845)

The EdgeFunction uses a SSM string parameter under the hood to pass the Function
ARN between the different regions. The name of the parameter is derived from the
node path; this path may contain characters (e.g., spaces) that are invalid as
SSM parameter names.

Two fixes here: introduce new validation for SSM parameter names, and sanitize
the path prior to passing to SSM.

fixes #16832


----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
Ran npm-check-updates and yarn upgrade to keep the `yarn.lock` file up-to-date.
The cause for this is that the new 'Architecture' property added to the
CloudFormation specification is not classified as version locked or not.

Added a test to ensure that any missing properties are caught in the
future.

Further, deprecated the `architectures` property and replaced with a
singular `architecture` prop. Lambda Functions only support one
architecture.

Additionally, removed the CFN spec patch, now that the CloudFormation
resource specification includes the Architecture fields.

fixes #16814


----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
This PR allows NLB to have a single ALB as the target.

Fixes: #16679 

----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
…deploy" step. (#16857)

## Summary

This PR is a continuation of #16828 (review)

We want to make sure users know to deploy with the latest CDK version before toggling the `autoDeleteObjects` prop to `false`.

----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
This adds support for `StepFunctions::StateMachines` to be hotswapped. Only changes to the `DefinitionString` property will trigger hotswaps. Changes to other properties (or resources, except Lambda functions) will require full deployments.

----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: AutoBuildProject89A8053A-LhjRyN9kxr8o
  • Commit ID: 45def81
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

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

@mergify mergify bot merged commit 7b4fa72 into aws:master Oct 8, 2021
@mergify
Copy link
Contributor

mergify bot commented Oct 8, 2021

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

njlynch pushed a commit that referenced this pull request Oct 11, 2021
)

Even though buckets are not supposed to be deleted manually, this change makes the delete operation idempotent and thus more reliable.

Fixes #16619.

----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
TikiTDO pushed a commit to TikiTDO/aws-cdk that referenced this pull request Feb 21, 2022
…#16645)

Even though buckets are not supposed to be deleted manually, this change makes the delete operation idempotent and thus more reliable.

Fixes aws#16619.

----

*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
contribution/core This is a PR that came from AWS.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

aws-s3: deleting stack with already deleted S3 bucket with auto_delete_objects fails