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

merge #18

Merged
merged 76 commits into from
Aug 10, 2020
Merged

merge #18

merged 76 commits into from
Aug 10, 2020

Conversation

flemjame-at-amazon
Copy link
Owner


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

ddneilson and others added 30 commits July 29, 2020 21:19
Implements #9336 
----

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

Closes  #9228 

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
Collapse the PipelineAssetsRoleDefaultPolicy into a single up-front policy that
doesn't grow per-asset. This relaxes some of the permissions in exchange for
avoiding an O(N) policy size.

fixes #9316

_Testing notes:_ Successfully deployed a pipeline with 49 assets (25 file assets and 24 docker assets). 50 assets is the limit for a single stage of the pipeline. To scale out past 50 assets, we will need to segment the assets pipeline stage into multiple stages. I'm considering that out-of-scope for this bugfix.

----

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

If a certificate with automatic (Route53) DNS validation contains both a base
domain name and the wildcard for that domain (e.g., `example.com` and
`*.example.com`), the corresponding DNS validation records are identical.
This seems to have caused problems for the automated CloudFormation DNS
validation.

Solving the problem by removing the redundant wildcard entries from the
DomainValidationOption.

fixes #9248


----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
… examples in pipelines README (#9344)

Ran across some of these when preparing the How-To topic for the CDK Dev Guide; backporting to the README.

----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
name is the old name for actionName
…#8826)

**[ISSUE]**
Honestly, kind of miss being able to use `lerna` for any kind of npm test through parameters with the old `builddown`. Wanted to be able to run something like `builddown build+test` or `builddown test`.

**[APPROACH]**
Added a `-r | --reset` flag to allow the `foreach.sh` script to reset and run in one line like `builddown`/`buildup`.

**[NOTE]**
Won't change anyone's current workflow because `--reset` is still in codebase.

Also arguments for the script aren't restricted by order (i.e. `foreach.sh --reset --up yarn build ` and `builddown yarn build --up --reset` will produce the same results)!

----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
Making a small -- but useful -- README change as an excuse to capture the
breaking changes made in #9326, but that were missed in that commit message.

BREAKING CHANGE: Removed origin classes from the aws-cloudfront module.
* **aws-cloudfront:** Removed S3Origin and HttpOrigin from the aws-cloudfront module. Use the S3Origin and HttpOrigin classes in the aws-cloudfront-origins module instead.
* **aws-cloudfront:** Renamed Origin to OriginBase.


----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
**[ISSUE]**
schema definition mode is not strongly typed.

**[APPROACH]**
Make input prop `schemaDefinition` take a enum that allows users to choose between 2 modes: `CODE` and `FILE`. 

**[NOTE]**
This approach was taken in preparation for a code-first approach to schema generation with CDK. All of the functions tagged `@experimental` are subject to change.

BREAKING CHANGE: **appsync** prop `schemaDefinition` no longer takes string, instead it is required to configure schema definition mode.
- **appsync**: schemaDefinition takes param `SchemaDefinition.XXX` to declare how schema will be configured
  - **SchemaDefinition.CODE** allows schema definition through CDK
  - **SchemaDefinition.FILE** allows schema definition through schema.graphql file

Fixes #9301 

----

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

Closes #9368 

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

Expanding integ test coverage by ensuring there is one integ test per origin type.

----

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

----

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

----

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

Fixes #9374

----

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

----

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

The python 3.8 `bundlingDockerImage` (`amazon/aws-sam-cli-build-image-python3.8`) is based on `amazonlinux:2` and doesn't include `rsync`; use `cp` instead.


----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
This property is no longer used, and OriginBase is a publicly available class of the module.

BREAKING CHANGE: the property OriginBase.originId has been removed

----

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

The documentation at https://docs.aws.amazon.com/cdk/api/latest/docs/aws-s3-deployment-readme.html#prune currently mentions that you can create two deployments with different cache policies by using the `exclude` option in the `asset` function. The last deployment aims to only set the cache policy on `index.html` but ends up setting the policy on everything.

According to #9146 (comment), an exclude pattern should be preceded by a `'*'` glob pattern, for it to take the desired effect that is mentioned in the documentation (to only include the `index.html` file).

This PR adds the missing `'*'` glob pattern to the documentations example.


----

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

----

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

As per: https://json-schema.org/understanding-json-schema/reference/object.html, the additionalProperties can be boolean or JsonSchema.

JsonSchema was previously supported by APIGateway but It was removed for some reason here: 73a1de1.

fixes #8069

----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
When a stack is created as the root of the construct tree, we now implicitly create an `App` that serves as its parent scope.

The root stack is created with the ID `Default`, which ensures that `node.uniqueId` of constructs within that stack is preserved.

BREAKING CHANGE: in unit tests, the `node.path` of constructs within stacks created the root of the tree via `new Stack()` will now have a prefix `Default/` which represents an implicit `App` root.

Related: aws/aws-cdk-rfcs#192


----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
Since Athena does not have AWS constructs the tests are empty. What does
the team think about me adding one test to verify this patch is
correctly applied for the cfn generated constructs?

Can I also get feedback on the file name choice I made or a pointer to
conventions on the patch file names?

Fixes #6936

----

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

closes #9076


----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
chore: restore regular owners in the auto label action workflow


----

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

----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
Niranjan Jayakar and others added 29 commits August 6, 2020 13:53
The official docker images for lambda are not available yet for Go and
dotnet core runtimes. Switch back to using lambdaci in these cases.

fixes #9435


----

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

As part of Lambda proxy integration, a `AWS::Lambda::Permission`
resource is created to provide the HTTP service permission to invoke
the lambda function, creating the dependency, Permission → API.
The API, on the other hand, needs to refer to the Function's ARN and
hence creates the dependency, API → Function.

However, when the lambda function and the HTTP API are placed in
different stacks, this creates a cyclic dependency between these two
stacks.

A picture is worth a thousand words:

```
 +--------------------------------------------------------------+
 | Lambda stack                                                 |
 |                                                              |
 |    +-------------------+            +-----------------+      |
 |    |                   | via ARN    |                 |      |
 |    | Lambda Permission +----------->| Lambda Function |      |
 |    |                   |            |                 |      |
 |    +-------+-----------+            +-----------------+      |
 |            |                                ^                |
 +------------|--------------------------------|----------------+
              |via ARN                         |via ARN
              |                                |
 +------------|--------------------------------|-----------------+
 |            v                                |                 |
 |      +-----------+               +----------+---------+       |
 |      |           |    via ID     |                    |       |
 |      |  Http API |<--------------+  API Integration   |       |
 |      |           |               |                    |       |
 |      +-----------+               +--------------------+       |
 |                                                               |
 | API Gateway stack                                             |
 +---------------------------------------------------------------+
```

The fix here is to move the Lambda Permission resource into the same
stack as where the API integration is defined, thereby breaking the
dependency cycle. 
Now the 'API Gateway stack' will depend one way on the 'Lambda
stack'.

fixes #9075

BREAKING CHANGE: The parameter for the method `bind()` on
`IHttpRouteIntegration` has changed to accept one of type
`HttpRouteIntegrationBindOptions`. The previous parameter
`IHttpRoute` is now a property inside the new parameter under
the key `route`.

----

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

Fn.join() has some logic inside of it to simplify the expressions to concatenate the
array elements that do not contain any Tokens inside of them.
We don't want to do it in cfn-include though,
as that causes a diff from the original template.
So, wrap the array given as the second argument to Fn::Join into a Token,
to prevent the concatenation logic from triggering.

----

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

- [x] Adding a boolean prop for xrayEnabled
- [x]  Linking it to the xrayEnabled prop in class CfnGraphQLApi from appsync.generated.ts file that is generated on yarn build
- [x]  Writing a unit test to check whether that the boolean property is set in the CloudFormation Template
----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
We incorrectly handled union-types in the fromCloudFormation() generated code.
Also we merged the 'Transform' sections of the CloudFormation template incorrectly,
which has also been fixed in this change.

----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
This fixes #9492 by down-scoping some IAM permissions granted to the ASG that is created for an ECS cluster, and removing some unneccessary permissions.

### Testing

This was tested by deploying a simple app that was basically the sample from the ECS module readme, and verifying that: (a) the cluster is operational (i.e. tasks are running), and (b) those tasks are able to write to logs.

The essentials of the app are:
```ts
const app = new cdk.App();

const env = {
    account: process.env.CDK_DEFAULT_ACCOUNT,
    region: process.env.CDK_DEFAULT_REGION
}

const stack = new cdk.Stack(app, 'Testing', { env });
const vpc = new ec2.Vpc(stack, 'Vpc');

// Create an ECS cluster
const cluster = new ecs.Cluster(stack, 'Cluster', {
  vpc,
});

// Add capacity to it
cluster.addCapacity('DefaultAutoScalingGroupCapacity', {
  instanceType: new ec2.InstanceType("t2.xlarge"),
  desiredCapacity: 2,
});

const taskDefinition = new ecs.Ec2TaskDefinition(stack, 'TaskDef');

taskDefinition.addContainer('DefaultContainer', {
  image: ecs.ContainerImage.fromRegistry("amazon/amazon-ecs-sample"),
  memoryLimitMiB: 512,
  logging: ecs.LogDriver.awsLogs({
    logGroup: new logs.LogGroup(stack, 'LogGroup', {
      logGroupName: '/test-group/',
      removalPolicy: cdk.RemovalPolicy.DESTROY,
      retention: logs.RetentionDays.ONE_DAY,
    }),
    streamPrefix: 'testing-',
  }),
});

// Instantiate an Amazon ECS Service
const ecsService = new ecs.Ec2Service(stack, 'Service', {
  cluster,
  taskDefinition,
  desiredCount: 2,
});
```

----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
When we changed the merging behavior in #8251,
we forgot to account for the 'Rules' section.
To prevent that error from happening again,
let's default to merging objects without duplicates.

Fixes #9485

----

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

### Testing

This was tested by deploying a simple app that was basically the sample from the ECS module readme, and then manually killing off instances from the ECS cluster's ASG. When I killed off an instance I then verified, from the lambda logs, that the task-draining lambda was able to complete its work with no errors.

The essentials of the app are:
```ts
const app = new cdk.App();

const env = {
    account: process.env.CDK_DEFAULT_ACCOUNT,
    region: process.env.CDK_DEFAULT_REGION
}

const stack = new cdk.Stack(app, 'Testing', { env });
const vpc = new ec2.Vpc(stack, 'Vpc');

// Create an ECS cluster
const cluster = new ecs.Cluster(stack, 'Cluster', {
  vpc,
});

// Add capacity to it
cluster.addCapacity('DefaultAutoScalingGroupCapacity', {
  instanceType: new ec2.InstanceType("t2.xlarge"),
  desiredCapacity: 2,
});

const taskDefinition = new ecs.Ec2TaskDefinition(stack, 'TaskDef');

taskDefinition.addContainer('DefaultContainer', {
  image: ecs.ContainerImage.fromRegistry("amazon/amazon-ecs-sample"),
  memoryLimitMiB: 512,
  logging: ecs.LogDriver.awsLogs({
    logGroup: new logs.LogGroup(stack, 'LogGroup', {
      logGroupName: '/test-group/',
      removalPolicy: cdk.RemovalPolicy.DESTROY,
      retention: logs.RetentionDays.ONE_DAY,
    }),
    streamPrefix: 'testing-',
  }),
});

// Instantiate an Amazon ECS Service
const ecsService = new ecs.Ec2Service(stack, 'Service', {
  cluster,
  taskDefinition,
  desiredCount: 2,
});
```

----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
Some tests added later were not placed in the correct top-level describe
block.

No tests have been added, removed or modified in this change.

----

Before the change -

```
Test Suites: 7 passed, 7 total
Tests:       103 passed, 103 total
Snapshots:   0 total
Time:        17.307s, estimated 20s
```

After the change:

```
Test Suites: 7 passed, 7 total
Tests:       103 passed, 103 total
Snapshots:   0 total
Time:        17.49s
```

----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
`rsync` is no longer present in the latest
`amazon/aws-sam-cli-build-image-python3.6` image.

This is similar to #9355 


----

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

----

Closes #9531 

 This fix allows allowedValues in parameters to be used in cfn-include. 


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

Fixes #9175

----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
…by default as stated in the docstring (#9505)

Fixes #9494

----

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

Add a detailed description of this parameter as per the existing AWS API documentation to save confusion for users

Fixes #9393 

----

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

Fixes #9542 
Related #5383 

----

*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*
I designed & wrote the code for the L2 Volume construct. My design for the `grantAttachVolumeByResourceTag()` and `grantDetachVolumeByResourceTag()` is flawed. 

Those functions have three design requirements:
1) To allow an instance to attach a Volume to itself; 
2) To allow an instance to attach multiple, different, volumes; and
3) To allow the same volume to be attached to different instances via separate grants without clobbering other grants.

Since the implementation mechanism for this is a `ResourceTag` condition on the policy, the later two requirements mean that we must have both a unique tag key and a unique tag value for each separate permission grant (i.e. separate call to the volume's grant function).

The original design had the tag key being derived from only the volume, the tag value from only the instance(s), and allowed for overriding the tag key via a parameter to allow for the same volume to attach to multiple instances over separate grants. 

In hindsight, we should be deriving both the resource tag and value from the combination of the instance(s) and the volume's unique properties. This completely eliminates the need for the tag key override.


The current design results in code like:
```ts
    // To be able to mount the *same* volume to multiple instances we must provide a tag suffix to the permission grant
    // that is unique to this particular combination of volume + mount target.
    function hashUniqueIds(resources: IConstruct[]): string {
      const md5 = crypto.createHash('md5');
      resources.forEach(res => md5.update(res.node.uniqueId));
      return md5.digest('hex');
    }
    this.props.blockVolume.grantAttachVolumeByResourceTag(target.grantPrincipal, [target], hashUniqueIds([target, this.props.blockVolume]));
```

It is much more desirable for this to be simply:
```ts
    this.props.blockVolume.grantAttachVolumeByResourceTag(target.grantPrincipal, [target]);
```

Resolves: #9114

----

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

This is a prerequisite to offer a way out of Docker. We don't want to
keep references to bundling directories in the `package.json`
configuration because they have no meaning when running locally.


----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
In [CDK 2.0], the `applyAspect` API will be removed and instead will be accessible through a "trait" pattern:

    Aspects.of(scope).add(aspect)

Similarly, we are normalizing the tagging API to use the same pattern:

    Tags.of(scope).add(x, y)
    Tags.of(scope).remove(x)
    
The existing APIs are still supported but marked as @deprecated.

Related: aws/aws-cdk-rfcs#192, See [Design].

[CDK 2.0]: https://github.com/aws/aws-cdk-rfcs/blob/master/text/0192-remove-constructs-compat.md
[Design]: https://github.com/aws/aws-cdk-rfcs/blob/master/text/0192-remove-constructs-compat.md#02-aspects

----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
From `addApplication` or `addStackArtifactDeployment` method.

Currently, when calling `addStackArtifactDeployment`, you can pass in `AddStackOptions`, but the `executeRunOrder` property is not respected later in the process where `DeployCdkStackAction.fromStackArtifact` is called.
From addApplication, this property is used when manualApprovals has been opted in to, but as it is not respected, it results in bug #9101

Fixes #9101 (I think!)

----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
Closes #9474 (see for details).

```ts
const fileSystem = new efs.FileSystem(this, "EFS", { 
      removalPolicy: cdk.RemovalPolicy.DESTROY,
      lifecyclePolicy: efs.LifecyclePolicy.AFTER_7_DAYS,
      vpc,
});
```

Old behavior:
```bash
npm run build; cdk synth | grep -r -n1 "AFTER_"; cdk diff

> demo@0.1.0 build /Users/robertd/code/demo
> tsc

Stack EFSDemoTest
Resources
[~] AWS::EFS::FileSystem EFS EFSBAA8953A
 └─ [-] LifecyclePolicies
     └─ [{"TransitionToIA":"AFTER_14_DAYS"}]
```

**New (expected) behavior:** 🎆 🥳 🍾 
```bash
npm run build; cdk synth | grep -r -n1 "AFTER_"; cdk diff

> demo@0.1.0 build /Users/robertd/demo
> tsc

(standard input)-327-      LifecyclePolicies:
(standard input):328:        - TransitionToIA: AFTER_7_DAYS
(standard input)-329-    UpdateReplacePolicy: Delete

Stack EFSDemoTest
Resources
[~] AWS::EFS::FileSystem EFS EFSBAA8953A
 └─ [~] LifecyclePolicies
     └─ @@ -1,5 +1,5 @@
        [ ] [
        [ ]   {
        [-]     "TransitionToIA": "AFTER_14_DAYS"
        [+]     "TransitionToIA": "AFTER_7_DAYS"
        [ ]   }
        [ ] ]

```

----

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

This change moves CDK pipelines from standard build image 1.0 to 4.0


----

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

To reduce the chance for conflicts with generated domain-specific properties in subclasses of `Construct` (see [terraform-cdk issue]), we decided that we will rename `node` to `construct`, which is less prevalent.

We plan to remove the `node` API in the next major version of the AWS CDK, and therefore recommend users to migrate their code to use `construct` instead.

See discussion in the [RFC].

[terraform-cdk issue]: hashicorp/terraform-cdk#230
[RFC]: https://github.com/aws/aws-cdk-rfcs/blob/master/text/0192-remove-constructs-compat.md#10-construct-node

----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
In [CDK 2.0] the `node.addWarning()`, `node.addError()` and `node.addInfo()` APIs will be removed in favor of a "trait" pattern:

    Annotations.of(construct).addWarning(message)

The existing APIs are still supported but marked as `@deprecated`.

Related: aws/aws-cdk-rfcs#192, See [Design].

[CDK 2.0]: https://github.com/aws/aws-cdk-rfcs/blob/master/text/0192-remove-constructs-compat.md
[Design]: https://github.com/aws/aws-cdk-rfcs/blob/master/text/0192-remove-constructs-compat.md#09-logging

----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
@flemjame-at-amazon flemjame-at-amazon merged commit f73af4f into flemjame-at-amazon:master Aug 10, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.