Skip to content

Conversation

@eladb
Copy link
Contributor

@eladb eladb commented Jan 17, 2021

Manually resolve conflicts resulting from #12504


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

briete and others added 30 commits January 11, 2021 10:37
fixes #11970 

---
_By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license_
In the event that assets are not actually available at synthesis
time, we still want to support JIT (just-in-time) asset
generation via external tooling. This would, for instance, enable
a third party tool to fetch additional resources prior to
bundling/building and subsequent uploading.

This adds a new interface for both File and Docker asset types
that allows users to specify an executable. The executable,
depending on the asset type, must then reply with a specific key
on stdout, which will then get picked up and used by CDK Assets.

This also updates the default stack synthesizer to support adding
external sources directly. This is technically a breaking change
for anyone who currently extends the class.


----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
Import IAM users by ARN or attributes.

Closes #12340
----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
Right now, Configuration.load() completely ignores context structure in ~/.cdk.json.
Documentation says it will merge context of ~/.cdk.json.

Fixes #10823, fixes #4802

Change that behavior to merge context of userConfig in Configuration.load()
Add tests for this behavior and first test for ~/.cdk.json at all

----

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

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

Although the constructor of the AssetCode doesn't imply it's real
dependencies, an AssetCode currently can only be associated to consumers
within a single stack.

Attempting to use them across stacks results in the conspicuous error -
"Cannot reference across apps. Consuming and producing stacks must be
defined within the same CDK app."

The correct solution here would have been to model this such that the
stack dependency is made explicit on the customer facing API, perhaps by
taking a 'scope' parameter.

For the moment, throw a more relevant error message so that it's obvious to
the user on what is wrong.

----

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


----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
…that is not compatible to the default instance type (#12441)

> Note: both issues here were introduced in #11962

## Problem 1

When creating a `Nodegroup` without passing instance types, we currently default to use `t3.medium`:

https://github.com/aws/aws-cdk/blob/da1ed08a6a2de584f5ddf43dab4efbb530541419/packages/%40aws-cdk/aws-eks/lib/managed-nodegroup.ts#L294

This default is then used to calculate the expected AMI type, and assert that the configured AMI type is indeed as expected:

https://github.com/aws/aws-cdk/blob/da1ed08a6a2de584f5ddf43dab4efbb530541419/packages/%40aws-cdk/aws-eks/lib/managed-nodegroup.ts#L302-L304

However, a user might configure instance types on the launch template, and an AMI type on the nodegroup. In this scenario, we still use the default instance type to perform the validation, which will fail if the ami type is not compatible with it.

To make things worse, we don't actually use the default instance type at all, apart from the validation:

https://github.com/aws/aws-cdk/blob/da1ed08a6a2de584f5ddf43dab4efbb530541419/packages/%40aws-cdk/aws-eks/lib/managed-nodegroup.ts#L329-L330

And in-fact, this default was only introduced in this [PR](#11962), which also added the problematic validation. 

### Solution

Drop the default instance type altogether, like it was before. The new validation will only take place if the user explicitly configured both `instanceTypes` and `amiType` on the nodegroup. Since the default value was never actually used, this doesn't incur any behavior change.

## Problem 2

When a launch template is used, we currently ignore the value of `amiType` explicitly passed by the user:

https://github.com/aws/aws-cdk/blob/da1ed08a6a2de584f5ddf43dab4efbb530541419/packages/%40aws-cdk/aws-eks/lib/managed-nodegroup.ts#L324-L325

This behavior means that users who configured a launch template without a custom ami, and passing an `amiType` to the nodegroup, would now result in no ami specification at all, defaulting to whatever EKS does, which might not be what the user had in mind.

There's no good reason to do this, we should either throw a validation error if both are used, or pass the explicit value nevertheless, even though it might cause problems.

### Solution

When a user explicitly passes an AMI type, just use it and assume the user knows what he/she is doing. When a user does not explicitly pass it, only apply the default if a launch template is not used. 

> If we apply the default in the presence of a launch template, a user would not be able to escape if they also have a custom AMI in the launch template.

This change means that users who previously "relied" on this override, might now experience a deployment failure if they are using a custom AMI in the launch template, those users can resolve the problem by removing the `amiType` property from the nodegroup (since it wasn't used, its not needed). I don't imagine many such users exist since this behavior is new and it doesn't make much sense to configure both a custom AMI and an `amiType`.

--------------------

Fixes #12389

BREAKING CHANGE: Explicitly passing `amiType` to nodegroups will now take affect even if a launch template is configured as well. If your launch template contains a custom AMI, this will cause a deployment failure, to resolve, remove the explicit `amiType` from the nodegroup configuration.

----

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

Don't include the logging keys for disabled logging options as some cluster versions reject unknown keys.

Fixes #11223

----

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

Added `spotInterruptHandler` to `addAutoScalingGroupCapacity` and `connectAutoScalingGroupCapacity` that allows disabling the installation of the spot-interrupt-handler helm chart.

Closes #12451 

----

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

----

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

This change allows customers to include an ACM certificiate or specify a file certificate for their listeners to use to terminate TLS. #10051 

```typescript
const cert = new Certificate(stack, 'cert', {
  domainName: '',
});

new appmesh.VirtualNode(stack, 'test-node', {
  mesh,
  dnsHostName: 'test',
  listeners: [appmesh.VirtualNodeListener.grpc({
    port: 80,
    tlsCertificate: appmesh.TlsCertificate.acm({
      acmCertificate: cert,
      tlsMode: TlsMode.STRICT,
    }),
  },
  )],
});
```
----
*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
…12365)

Fixes #4295
Closes #5033

----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
Bumps [decamelize](https://github.com/sindresorhus/decamelize) from 4.0.0 to 5.0.0.
- [Release notes](https://github.com/sindresorhus/decamelize/releases)
- [Commits](sindresorhus/decamelize@v4.0.0...v5.0.0)

Signed-off-by: dependabot-preview[bot] <[email protected]>

Co-authored-by: dependabot-preview[bot] <27856297+dependabot-preview[bot]@users.noreply.github.com>
Require topicName property for FIFO SNS topics as a workaround to [issue 681](aws-cloudformation/cloudformation-coverage-roadmap#681) reported in the CloudFormation coverage roadmap. 

Also adding additional logic to append '.fifo' to FIFO topic name if not provided explicitly by the user.

Fixes #12386
----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
Lazily add a formatted cluster arn to the RdsHttpEndpointConfig. Before we were just adding the cluster id, but the [docs](https://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/aws-properties-appsync-datasource-rdshttpendpointconfig.html) call for the Cluster Arn instead. 

Fixes: #11536

----

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

Right now, Bucket.grantWrite() adds an 's3:PutObject*' permission to the passed principal.
That means it grants the 's3:PubObjectAcl' permission,
which allows changing the ACL of an individual object written to the Bucket,
potentially giving it more visibility than other objects in the Bucket.

Change that behavior to grant 's3:PutObject' instead.
Since customers might be relying on the old overly-broad permissions,
gate that change behind a feature flag,
which means only newly created CDK projects will get this behavior.

----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
For v2, we will be releasing automatically once per week. The 'bump'
job will create the PR with the necessary changes. Auto-approve
the bump PR.

ref: aws/cdk-ops#1132
Bringing the two branches back into sync.

----

*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*
minor typo

----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
The default is currently not clearly documented.

Fixes #12082.


----

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

`MappingTemplate` is not a global value and it has to either be explicitly imported:

```
import { MappingTemplate } from '@aws-cdk/aws-appsync';
```

or accessed via `appsync` field (since there is an `import * as appsync from '@aws-cdk/aws-appsync';` on line 55).

It's a small thing but now the whole example code is more or less copy&paste-able 👍 (same goes for `PrimaryKey` and `Values`)


----

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

Includes removing a bunch of patches that no longer apply, as many custom Tags property types have been removed in this version of the spec in favor of using the common Tag type.

----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
Bumps [aws-sdk](https://github.com/aws/aws-sdk-js) from 2.824.0 to 2.827.0.
- [Release notes](https://github.com/aws/aws-sdk-js/releases)
- [Changelog](https://github.com/aws/aws-sdk-js/blob/master/CHANGELOG.md)
- [Commits](aws/aws-sdk-js@v2.824.0...v2.827.0)

Signed-off-by: dependabot-preview[bot] <[email protected]>

Co-authored-by: dependabot-preview[bot] <27856297+dependabot-preview[bot]@users.noreply.github.com>
dirknilius and others added 4 commits January 15, 2021 16:08
…#12272)

Closes #12256

Clarified the usage of custom user data with default or custom AMI by adding another example and pointing out the differences.

Changed the launch template reference in the examples to use the latest version rather than the default. If you don't know the difference it would be unexpected for the user to change the user data without having it applied to the nodes after deploy.

Removed unnecessary wrapping of instance type string with typed version and calling toString.

Make the sentence about defining instance type either in launch template or node group more understandable.


----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
We were missing a few cases where we did not validate that a package had the correct `maturity` set:

- A package with L2s could still have a `maturity` of 'cfn-only'
- A package with only L1s could still have a non-'cfn-only' `maturity`

Fix that in the linter, along with the violations discovered.

----

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

Relates to #12478


----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
@eladb eladb requested a review from nija-at January 17, 2021 08:45
@eladb eladb added pr/forward-merge PR label to indicate forward merges of master to v2-main pr/no-squash This PR should be merged instead of squash-merging it labels Jan 17, 2021
@mergify mergify bot added the contribution/core This is a PR that came from AWS. label Jan 17, 2021
@eladb eladb self-assigned this Jan 17, 2021
@gitpod-io
Copy link

gitpod-io bot commented Jan 17, 2021

@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: AutoBuildProject6AEA49D1-qxepHUsryhcu
  • Commit ID: ac0908c
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

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

@eladb eladb added the pr/auto-approve Notifies the GH action to auto-approve this PR label Jan 17, 2021
@eladb eladb requested a review from iliapolo January 17, 2021 14:30
@mergify
Copy link
Contributor

mergify bot commented Jan 17, 2021

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

@eladb eladb merged commit fe1da26 into v2-main Jan 17, 2021
@eladb eladb deleted the benisrae/merge/master-v2 branch January 17, 2021 14:43
@njlynch njlynch restored the benisrae/merge/master-v2 branch January 18, 2021 11:47
njlynch added a commit that referenced this pull request Jan 18, 2021
mergify bot pushed a commit that referenced this pull request Jan 18, 2021
#12552 manually fixed some issues with the forward merge. However, the end result was squashed, which is breaking the new forward merges. This change reverts the squashed merge, and then brings in the merged commits again.

Verified I can run `git merge origin/master --no-edit` cleanly with this (rather than `v2-main` HEAD) as my HEAD branch.

----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
njlynch added a commit that referenced this pull request Jan 18, 2021
@njlynch njlynch deleted the benisrae/merge/master-v2 branch January 18, 2021 13:47
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. pr/auto-approve Notifies the GH action to auto-approve this PR pr/forward-merge PR label to indicate forward merges of master to v2-main pr/no-squash This PR should be merged instead of squash-merging it

Projects

None yet

Development

Successfully merging this pull request may close these issues.