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

Allow ICMP to have fromport other than -1 with to port -1 #3482

Merged
merged 1 commit into from
Jul 9, 2024

Conversation

kddejong
Copy link
Contributor

@kddejong kddejong commented Jul 9, 2024

Issue #, if available:
#2999

Description of changes:

  • Allow ICMP to have fromport other than -1 with to port -1

When picking a certain ICMP protocol the FromPort is that selection and the ToPort are -1.

from the API

"IpPermissions": [
                {
                    "FromPort": 9,
                    "IpProtocol": "icmp",
                    "IpRanges": [
                        {
                            "CidrIp": "0.0.0.0/0"
                        }
                    ],
                    "Ipv6Ranges": [],
                    "PrefixListIds": [],
                    "ToPort": -1,
                    "UserIdGroupPairs": []
                }
            ],

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

Copy link

codecov bot commented Jul 9, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 93.32%. Comparing base (19b13e2) to head (ce4e6dc).

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3482      +/-   ##
==========================================
- Coverage   93.33%   93.32%   -0.01%     
==========================================
  Files         325      325              
  Lines       11082    11082              
  Branches     2324     2324              
==========================================
- Hits        10343    10342       -1     
  Misses        414      414              
- Partials      325      326       +1     
Flag Coverage Δ
unittests ?

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@kddejong kddejong merged commit 3847b59 into aws-cloudformation:main Jul 9, 2024
22 checks passed
@kddejong kddejong deleted the fix/issue/2999-1 branch July 9, 2024 16:04
@vschurink
Copy link

hi @kddejong, it seems in version 1.7.1 this PR was unmerged again? I keep getting E3688 errors again when I have the ICMP configuration?

@kddejong
Copy link
Contributor Author

@vschurink I haven't been able to re-replicate this issue. All the following variations are fine for me.

Icmp1:
    Type: AWS::EC2::SecurityGroupIngress
    Properties:
      IpProtocol: 1
      CidrIp: 10.0.0.0/8
      FromPort: "9"
      ToPort: "-1"
  Icmp2:
    Type: AWS::EC2::SecurityGroupIngress
    Properties:
      IpProtocol: icmp
      CidrIp: 10.0.0.0/8
      FromPort: 5
      ToPort: -1
  Icmp3:
    Type: AWS::EC2::SecurityGroupIngress
    Properties:
      IpProtocol: 1
      CidrIp: 10.0.0.0/8
      FromPort: 5
      ToPort: "-1"

@vschurink
Copy link

@kddejong you are correct, neither can I now. Maybe i had an old version of cfn-lint in the cache. My bad. Thanks for your quick response.

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.

2 participants