feat(ec2): Add Transit Gateway settings to FlowLogResourceType #27262

dyoshikawa · 2023-09-23T02:28:11Z

Closes #27222.

AWS documents:

AWS::EC2::FlowLog - AWS CloudFormation

Other references:

non-97/transit-gateway-flow-logs

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

to FlowLogResourceType

aws-cdk-automation

The pull request linter has failed. See the aws-cdk-automation comment below for failure reasons. If you believe this pull request should receive an exemption, please comment and provide a justification.

A comment requesting an exemption should contain the text Exemption Request. Additionally, if clarification is needed add Clarification Request to a comment.

✅ Updated pull request passes all PRLinter validations. Dismissing previous PRLinter review.

aws-cdk-automation · 2023-10-02T00:08:41Z

This PR cannot be merged because it has conflicts. Please resolve them. The PR will be considered stale and closed if it remains in an unmergeable state.

scanlonp

The PR looks good! It just needs a couple more steps.

First, there are merge conflicts, please resolve these. They look relatively simple
Please link documentation you referenced in the process of finding the feature gap and adding the feature. Adding it to the PR description would be appreciated as well!
Address the comments on the integ test

I will be ready to approve when you get back with these asks!

scanlonp · 2023-10-02T20:05:54Z

packages/@aws-cdk-testing/framework-integ/test/aws-ec2/test/integ.vpc-flow-logs.ts

+      resourceType: FlowLogResourceType.fromTransitGatewayId(transitGateway.ref),
+      flowLogName: 'TransitGatewayFlowLogName',
+    });
+  }


Is there a reason you did not add a fromTransitGatewayAttachmentId resource type FlowLog into the test as well?

No reasons, so I worked on it!

scanlonp · 2023-10-02T20:06:16Z

packages/@aws-cdk-testing/framework-integ/test/aws-ec2/test/integ.vpc-flow-logs.ts

 const featureFlagTest = new FeatureFlagStack(app, 'FlowLogsFeatureFlag');

 const integ = new IntegTest(app, 'FlowLogs', {
  testCases: [
    new TestStack(app, 'FlowLogsTestStack'),
    featureFlagTest,
    new DependencyTestStack(app, 'DependencyTestStack'),
+    new TransitGatewayFlowLogStack(app, 'TransitGatewayFlowLogStack'),
  ],
  diffAssets: true,
 });


Are there assertions you could add here to confirm the FlowLog is created as you intend it to be?

I tried it. But, when I ran integration tests, the following error has occurred. The error message Response object is too is ambiguous, so I cannot proceed.

FlowLogsDefaultTestDeployAssertXXXXXX | 3/5 | 4:31:08 PM | CREATE_FAILED | Custom::DeployAssert@SdkCallEC2describeFlowLogs | FlowLogs/DefaultTest/DeployAssert/AwsApiCallEC2describeFlowLogsXXXXXXXXXXXXX/Default/Default (AwsApiCallEC2describeFlowLogsXXXXXXXXXXX) Response object is too long.

Could you help me?

Yes, this is a common problem and not well documented. One solution is to do something along the lines of:

const describe = integ.assertions.awsApiCall(...); describe.assertAtPath('path.key.in.object', ExpectedResult.stringLikeRegexp('string you expect'));

This will prevent the full response object coming back to cloudformation.

aws-cdk-automation · 2023-10-03T00:08:06Z

This PR cannot be merged because it has conflicts. Please resolve them. The PR will be considered stale and closed if it remains in an unmergeable state.

aws-cdk-automation · 2023-10-04T00:08:21Z

This PR cannot be merged because it has conflicts. Please resolve them. The PR will be considered stale and closed if it remains in an unmergeable state.

aws-cdk-automation · 2023-10-05T00:08:04Z

This PR cannot be merged because it has conflicts. Please resolve them. The PR will be considered stale and closed if it remains in an unmergeable state.

cm-dyoshikawa · 2023-10-05T02:26:29Z

@scanlonp Thank you for your review. I'll check those.

'origin' into transit-gateway-flow-log-resource-type

…ub.com/dyoshikawa/aws-cdk into transit-gateway-flow-log-resource-type

from a transit gateway attachment to docs

cm-dyoshikawa · 2023-10-17T03:51:54Z

@scanlonp Thank you. I worked on these again. So could you review more once?

scanlonp · 2023-10-18T22:35:49Z

packages/aws-cdk-lib/aws-ec2/README.md

+  this,
+  'TransitGatewayAttachment',
+  {
+    subnetIds: vpc.selectSubnets({


This is causing the build to fail since you have not created the vpc. Add the vpc declaration to the example:
declare const vpc: ec2.Vpc;

Thank you. I worked on it.

scanlonp · 2023-10-18T22:38:26Z

There is still the task of re-writing the user contract so that resourceType and trafficType are tightly coupled.

cm-dyoshikawa · 2023-11-01T00:57:22Z

@scanlonp I'm sorry to have kept you waiting. Could you review again?

There is still the task of re-writing the user contract so that resourceType and trafficType are tightly coupled.

I worked on it.

scanlonp

Couple more small comments. We should do some validation rather than coercion.

But also, I noticed that trafficType is used in the addFlowLogs() method. Deprecating it leaves no non-deprecated option for that function.

As an end result, we should have trafficType be deprecated when used in our from...() methods, and not deprecated when used in addFlowLogs(). This is a little tricky, but a possible solution is to have an interface extend FlowLogOptions (or have FlowLogOptions extend an interface), and have trafficType be deprecated in one and not in the other. Then give each method its proper interface.

This is a little convoluted, so let me put a little more thought into it. Ideas are welcome, but I will get back to you soon!

scanlonp · 2023-11-01T20:57:40Z

packages/aws-cdk-lib/aws-ec2/lib/vpc-flow-logs.ts

+    let trafficType: FlowLogTrafficType | undefined = undefined;
+    if (!['TransitGateway', 'TransitGatewayAttachment'].includes(props.resourceType.resourceType)) {


We should not need this now, right? Should be a validation rather than a coercion.

As to the code, because I had intended behavior that already exists will not changed.

OK, I will change as you say. But I don't know how should I write well so I asked you.

scanlonp · 2023-11-01T20:58:59Z

packages/aws-cdk-lib/aws-ec2/lib/vpc-flow-logs.ts

+      trafficType = props.trafficType ?? props.resourceType.trafficType ?? FlowLogTrafficType.ALL;
+    }


We should validate that both are not set.

Ok, but how should it failure? In that case, it should occur throw new Error() ?

scanlonp · 2023-11-01T21:06:22Z

packages/aws-cdk-lib/aws-ec2/lib/vpc-flow-logs.ts

@@ -616,6 +644,7 @@ export interface FlowLogOptions {
   * The type of traffic to log. You can log traffic that the resource accepts or rejects, or all traffic.
   *
   * @default ALL
+   * @deprecated Use `resourceType.trafficType` instead


This is used in the addFlowLogs method, which means we would be deprecating traffic type with no alternative there. Probably cannot deprecate this as-is. I overlooked this before.

Maybe instead of deprecating, we can give a warning if it is set here.

I see. In that case, can I use this.warn such like the code that already exists?

scanlonp

Ok, some new thoughts. Instead of deprecating, we give a warning if used in the from... context. This will have similar behavior to the deprecation, but leave it open for the add.. functions.

We can still error if the prop is provided twice.

scanlonp · 2023-11-08T22:59:42Z

packages/aws-cdk-lib/aws-ec2/lib/vpc-flow-logs.ts

@@ -616,6 +644,7 @@ export interface FlowLogOptions {
   * The type of traffic to log. You can log traffic that the resource accepts or rejects, or all traffic.
   *
   * @default ALL
+   * @deprecated Use `resourceType.trafficType` instead


Maybe instead of deprecating, we can give a warning if it is set here.

Pull request has been modified.

cm-dyoshikawa · 2023-11-22T08:07:40Z

@scanlonp
Thank you so much for your comments. I replied those. I'm considering about your instructions.

Also, I'm sorry for so late reaction. I've been busy these days.

aws-cdk-automation · 2023-11-22T08:08:17Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildv2Project1C6BFA3F-wQm2hXv2jqQv
Commit ID: e54a4ad
Result: FAILED
Build Logs (available for 30 days)

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

aws-cdk-automation · 2023-12-14T00:08:23Z

This PR has been in the BUILD FAILING state for 3 weeks, and looks abandoned. To keep this PR from being closed, please continue work on it. If not, it will automatically be closed in a week.

aws-cdk-automation · 2023-12-22T00:08:26Z

This PR has been deemed to be abandoned, and will be automatically closed. Please create a new PR for these changes if you think this decision has been made in error.

feat(ec2): Add Transit Gateway settings

5a0ccfa

to FlowLogResourceType

github-actions bot added the p2 label Sep 23, 2023

aws-cdk-automation requested a review from a team September 23, 2023 02:28

github-actions bot added the beginning-contributor [Pilot] contributed between 0-2 PRs to the CDK label Sep 23, 2023

aws-cdk-automation previously requested changes Sep 23, 2023

View reviewed changes

github-actions bot added effort/small Small work item – less than a day of effort p1 feature-request A feature should be added or improved. p2 and removed p2 p1 labels Sep 23, 2023

dyoshikawa changed the title ~~feat(ec2): Add Transit Gateway settings~~ feat(ec2): Add Transit Gateway settings to FlowLogResourceType Sep 23, 2023

dyoshikawa and others added 4 commits September 27, 2023 19:54

fix: eslint

c28a18e

Implement FlowLog for TransitGateway

a14d987

Merge branch 'main' into transit-gateway-flow-log-resource-type

502f636

Add Transit Gateway Flow Log to README

52f63cd

cm-dyoshikawa added 2 commits September 28, 2023 15:35

Revert integ-test-runner modifying

36f09a1

docs: typo

87c3018

dyoshikawa marked this pull request as ready for review September 29, 2023 05:04

github-actions bot mentioned this pull request Oct 1, 2023

Monthly PRs metrics report - Sep 2023 #27368

Closed

scanlonp self-assigned this Oct 2, 2023

scanlonp reviewed Oct 2, 2023

View reviewed changes

cm-dyoshikawa added 2 commits October 5, 2023 12:51

Merge remote-tracking branch

89b7add

'origin' into transit-gateway-flow-log-resource-type

exec integ test

8f704fa

scanlonp added pr/needs-maintainer-review This PR needs a review from a Core Team Member and removed pr/needs-community-review This PR needs a review from a Trusted Community Member or Core Team Member. labels Oct 16, 2023

aws-cdk-automation added pr/needs-community-review This PR needs a review from a Trusted Community Member or Core Team Member. and removed pr/needs-maintainer-review This PR needs a review from a Core Team Member labels Oct 16, 2023

cm-dyoshikawa and others added 4 commits October 17, 2023 12:33

import * as ec2

9169857

Merge branch 'transit-gateway-flow-log-resource-type' of https://gith…

04aa467

…ub.com/dyoshikawa/aws-cdk into transit-gateway-flow-log-resource-type

Merge branch 'main' into transit-gateway-flow-log-resource-type

0e9c940

add a flow log

c032d45

from a transit gateway attachment to docs

scanlonp reviewed Oct 18, 2023

View reviewed changes

cm-dyoshikawa and others added 5 commits October 26, 2023 15:32

fix readme

43a6848

Merge branch 'main' into transit-gateway-flow-log-resource-type

33b84c0

resourceType and trafficType are tightly coupled

0380ba7

Merge branch 'main' into transit-gateway-flow-log-resource-type

b110219

fix vpc-flow-logs

6a1f29c

cm-dyoshikawa added 2 commits November 1, 2023 09:57

Merge branch 'main' into transit-gateway-flow-log-resource-type

51f5c2a

Merge branch 'main' into transit-gateway-flow-log-resource-type

eaa40be

scanlonp previously requested changes Nov 7, 2023

View reviewed changes

aws-cdk-automation removed the pr/needs-community-review This PR needs a review from a Trusted Community Member or Core Team Member. label Nov 7, 2023

scanlonp reviewed Nov 18, 2023

View reviewed changes

Merge branch 'main' into transit-gateway-flow-log-resource-type

e54a4ad

aws-cdk-automation added the closed-for-staleness This issue was automatically closed because it hadn't received any attention in a while. label Dec 22, 2023

aws-cdk-automation closed this Dec 22, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(ec2): Add Transit Gateway settings to FlowLogResourceType #27262

feat(ec2): Add Transit Gateway settings to FlowLogResourceType #27262

dyoshikawa commented Sep 23, 2023 •

edited

Loading

aws-cdk-automation left a comment

aws-cdk-automation commented Oct 2, 2023

scanlonp left a comment

scanlonp Oct 2, 2023

cm-dyoshikawa Oct 6, 2023 •

edited

Loading

scanlonp Oct 2, 2023

dyoshikawa Oct 6, 2023

scanlonp Oct 18, 2023

aws-cdk-automation commented Oct 3, 2023

aws-cdk-automation commented Oct 4, 2023

aws-cdk-automation commented Oct 5, 2023

cm-dyoshikawa commented Oct 5, 2023

cm-dyoshikawa commented Oct 17, 2023

scanlonp Oct 18, 2023

cm-dyoshikawa Oct 26, 2023

scanlonp commented Oct 18, 2023

cm-dyoshikawa commented Nov 1, 2023 •

edited

Loading

scanlonp left a comment

scanlonp Nov 1, 2023

cm-dyoshikawa Nov 22, 2023

scanlonp Nov 1, 2023

cm-dyoshikawa Nov 22, 2023

scanlonp Nov 1, 2023

scanlonp Nov 8, 2023

cm-dyoshikawa Nov 22, 2023

scanlonp left a comment

scanlonp Nov 8, 2023

cm-dyoshikawa commented Nov 22, 2023 •

edited

Loading

aws-cdk-automation commented Nov 22, 2023

aws-cdk-automation commented Dec 14, 2023

aws-cdk-automation commented Dec 22, 2023

		let trafficType: FlowLogTrafficType \| undefined = undefined;
		if (!['TransitGateway', 'TransitGatewayAttachment'].includes(props.resourceType.resourceType)) {

		trafficType = props.trafficType ?? props.resourceType.trafficType ?? FlowLogTrafficType.ALL;
		}

feat(ec2): Add Transit Gateway settings to FlowLogResourceType #27262

feat(ec2): Add Transit Gateway settings to FlowLogResourceType #27262

Conversation

dyoshikawa commented Sep 23, 2023 • edited Loading

aws-cdk-automation left a comment

Choose a reason for hiding this comment

aws-cdk-automation commented Oct 2, 2023

scanlonp left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cm-dyoshikawa Oct 6, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aws-cdk-automation commented Oct 3, 2023

aws-cdk-automation commented Oct 4, 2023

aws-cdk-automation commented Oct 5, 2023

cm-dyoshikawa commented Oct 5, 2023

cm-dyoshikawa commented Oct 17, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

scanlonp commented Oct 18, 2023

cm-dyoshikawa commented Nov 1, 2023 • edited Loading

scanlonp left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

scanlonp left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cm-dyoshikawa commented Nov 22, 2023 • edited Loading

aws-cdk-automation commented Nov 22, 2023

AWS CodeBuild CI Report

aws-cdk-automation commented Dec 14, 2023

aws-cdk-automation commented Dec 22, 2023

dyoshikawa commented Sep 23, 2023 •

edited

Loading

cm-dyoshikawa Oct 6, 2023 •

edited

Loading

cm-dyoshikawa commented Nov 1, 2023 •

edited

Loading

cm-dyoshikawa commented Nov 22, 2023 •

edited

Loading