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

feat(ec2): L2 Construct - VpcPeeringConnection #9339

Closed
wants to merge 1 commit into from
Closed

feat(ec2): L2 Construct - VpcPeeringConnection #9339

wants to merge 1 commit into from

Conversation

civilizeddev
Copy link
Contributor

fixes #9338


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: AutoBuildProject6AEA49D1-qxepHUsryhcu
  • Commit ID: c9c3808
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

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

Copy link
Contributor

@rix0rrr rix0rrr left a comment

Choose a reason for hiding this comment

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

I think this PR would benefit from waiting for a different one for a bit.

/**
* VPC with which you are creating the VPC peering connection.
*/
export class PeerVpc {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think that if you wait for #8280 to be merged, this can be replaced with an IVpc instead which seems nicer.

Copy link
Contributor Author

@civilizeddev civilizeddev Aug 12, 2020

Choose a reason for hiding this comment

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

Well, the peer vpc is different from a regular vpc. (that is, the peer vpc is not a vpc.)

  • You cannot get its details. only account, region, vpc-id, and cidr are provided.
  • You have no control over it. You cannot do anything with it. So IVpc might be false implication.

*
* @default - conditional
*/
readonly role?: iam.IRole
Copy link
Contributor

Choose a reason for hiding this comment

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

I guess we could probably create this role? What are the permissions on it that are necessary? Please give links to documentation?

The current documentation is not really enough for people to make use of this feature, and we should always be looking if we can simplify it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The peer(acceptor)'s acceptance is required to establish vpc peering connection.

This is usually done by hand on the console.

image

Maybe an IAM Role with this policy is needed:

"Effect": "Allow",
"Action": [
  "ec2:AcceptVpcPeeringConnection"
],
"Resource": [
  "arn:aws:ec2:(requestor-region):(requestor-account):vpc-peering-connection/(vpc-peering-connection-id)",
  "arn:aws:ec2:(acceptor-region):(acceptor-account):vpc/(vpc-id)",
]

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure it's correct. I'll look into it a little bit more.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

/**
* Props for constructing VpcPeeringConnection
*/
export interface VpcPeeringConnectionProps extends VpcPeeringConnectionOptions {
Copy link
Contributor

Choose a reason for hiding this comment

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

You probably only want to make a distinction between VpcPeeringConnectionProps and VpcPeeringConnectionOptions if you're going to add a vpc.addPeeringConnection() method to Vpc itself (or maybe even to IVpc?)

Copy link
Contributor Author

@civilizeddev civilizeddev Aug 12, 2020

Choose a reason for hiding this comment

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

I intended that.

@civilizeddev civilizeddev marked this pull request as draft August 13, 2020 14:17
@civilizeddev
Copy link
Contributor Author

@Comradin
Copy link

Any news or progress for this pr? I am badly missing the possibility to peer vpc's with the cdk and not by hand

@rix0rrr rix0rrr removed their assignment Jun 22, 2021
@rix0rrr rix0rrr added feature-request A feature should be added or improved. p2 labels Mar 4, 2022
@TheRealAmazonKendra
Copy link
Contributor

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature-request A feature should be added or improved. p2
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[aws-ec2] L2 Construct - VpcPeeringConnection
6 participants