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): support all Security Group protocols #13497

Closed
wants to merge 5 commits into from

Conversation

rectalogic
Copy link
Contributor

Protocol numbers must be used for all but a handful of protocols.

Issue #13403 was closed with an incorrect PR merge #13471. According to CFN docs only a handful of protocol names are supported, protocol numbers must be used for the rest.
https://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/aws-properties-ec2-security-group-rule-1.html#cfn-ec2-security-group-rule-ipprotocol

This adds all protocol numbers to the enum.


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

@gitpod-io
Copy link

gitpod-io bot commented Mar 9, 2021

@github-actions github-actions bot added the @aws-cdk/aws-ec2 Related to Amazon Elastic Compute Cloud label Mar 9, 2021
@mergify
Copy link
Contributor

mergify bot commented Mar 9, 2021

Title does not follow the guidelines of Conventional Commits. Please adjust title before merge.

@hollanddd
Copy link
Contributor

hollanddd commented Mar 9, 2021

Great. This is the info I'm looking for. I think that esp and ah functions need to be there in order to surface the underlying properties although i acknowledge that their implementation is incorrect.

@rix0rrr rix0rrr added pr-linter/exempt-readme The PR linter will not require README changes pr-linter/exempt-test The PR linter will not require test changes labels Mar 9, 2021
@rix0rrr rix0rrr changed the title Add support for all protocols to aws-ec2 Port feat(ec2): support all Security Group protocols Mar 9, 2021
@rectalogic
Copy link
Contributor Author

Great. This is the info I'm looking for. I think that esp and ah functions need to be there in order to surface the underlying properties although i acknowledge that their implementation is incorrect.

But are you going to make functions for every protocol? It seems unnecessary, users can just construct a Port as needed for all these obscure protocols:

new Port({
  protocol: Protocol.ESP,
  stringRepresentation: 'ESP',
});

@hollanddd
Copy link
Contributor

here are two commits to clean up the cruft from my original pr if you want to swipe them for your PR. First one removes the tests there were added and the second adds the protocols to the list of ignored docs in package.json

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.

Build fails

@rectalogic
Copy link
Contributor Author

@NetaNir
Copy link
Contributor

NetaNir commented Mar 16, 2021

@rectalogic the comment above includes the a link to the logs.

@rix0rrr
Copy link
Contributor

rix0rrr commented Mar 16, 2021

image

mergify bot pushed a commit that referenced this pull request Mar 16, 2021
Satisfies #13497 to close #13403

----

*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: f6e2794
  • Result: FAILED
  • Build Logs (available for 30 days)

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

@rectalogic
Copy link
Contributor Author

It's failing because I removed the APIs added by the previous botched merge

API elements with incompatible changes:
err  - METHOD @aws-cdk/aws-ec2.Port.ah: has been removed [removed:@aws-cdk/aws-ec2.Port.ah]
err  - METHOD @aws-cdk/aws-ec2.Port.esp: has been removed [removed:@aws-cdk/aws-ec2.Port.esp]

@hollanddd
Copy link
Contributor

It's addressed in #13593. Sorry for the confusion

hollanddd added a commit to hollanddd/aws-cdk that referenced this pull request Mar 17, 2021
Satisfies aws#13497 to close aws#13403

----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
hollanddd added a commit to hollanddd/aws-cdk that referenced this pull request Mar 18, 2021
Satisfies aws#13497 to close aws#13403

----

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

rix0rrr commented Mar 23, 2021

I think this PR is no longer necessary, right?

@rix0rrr rix0rrr added the response-requested Waiting on additional info and feedback. Will move to "closing-soon" in 7 days. label Mar 23, 2021
@rectalogic
Copy link
Contributor Author

right

@rectalogic rectalogic closed this Mar 23, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
@aws-cdk/aws-ec2 Related to Amazon Elastic Compute Cloud pr-linter/exempt-readme The PR linter will not require README changes pr-linter/exempt-test The PR linter will not require test changes response-requested Waiting on additional info and feedback. Will move to "closing-soon" in 7 days.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants