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

extending CIDR validation to more types #1058

Merged
merged 13 commits into from
Jul 29, 2019
Merged

extending CIDR validation to more types #1058

merged 13 commits into from
Jul 29, 2019

Conversation

PatMyron
Copy link
Contributor

@PatMyron PatMyron commented Jul 28, 2019

#1000

grep'd the CloudFormationResourceSpecification to find unique CIDR strings to investigate adding

$ curl -s -N --compressed https://d1uauaxba7bl26.cloudfront.net/latest/gzip/CloudFormationResourceSpecification.json | grep -i "cidr" | awk '{$1=$1};1' | sort -u | grep -v '"Documentation":'
	"AWS::EC2::SubnetCidrBlock": {
	"AWS::EC2::VPCCidrBlock": {
	"AWS::MediaLive::InputSecurityGroup.InputWhitelistRuleCidr": {
	"AmazonProvidedIpv6CidrBlock": {
	"CIDRIP": {
	"Cidr": {
	"CidrBlock": {
	"CidrBlockAssociations": {
	"CidrIp": {
	"CidrIpv6": {
	"ClientCidrBlock": {
	"DestinationCidrBlock": {
	"DestinationIpv6CidrBlock": {
	"Ipv6CidrBlock": {
	"Ipv6CidrBlocks": {
	"ItemType": "InputWhitelistRuleCidr",
	"TargetNetworkCidr": {
	"TunnelInsideCidr": {

Added property type for existing CIDR string:

AWS::MediaLive::InputSecurityGroup.InputWhitelistRuleCidr

Added CIDR strings:

ClientCidrBlock: AWS::EC2::ClientVpnEndpoint

DestinationCidrBlock:
AWS::EC2::Route
AWS::EC2::VPNConnectionRoute
AWS::EC2::TransitGatewayRoute
AWS::EC2::ClientVpnRoute

TargetNetworkCidr: AWS::EC2::ClientVpnAuthorizationRule

TunnelInsideCidr: AWS::EC2::VPNConnection.VpnTunnelOptionsSpecification


I wasn't sure if REGEX_CIDR supported IPv6 CIDR blocks, so I refrained from adding any of those strings for now


Correcting tags in one file should be the only difference between the diffs of the two files:

https://github.com/aws-cloudformation/cfn-python-lint/blob/df74703d78c371d08010f2d73279b6e45ffde0e5/src/cfnlint/rules/parameters/Cidr.py#L30


AssertionError: Expected 27 failures, got 28 on test/fixtures/templates/quickstart/nist_vpc_management.yaml

https://github.com/aws-cloudformation/cfn-python-lint/blob/5fb676c6039e00bbe3e9096bbbf8dac62e6f64ba/test/fixtures/templates/quickstart/nist_vpc_management.yaml#L267-L270

https://github.com/aws-cloudformation/cfn-python-lint/blob/5fb676c6039e00bbe3e9096bbbf8dac62e6f64ba/test/fixtures/templates/quickstart/nist_vpc_management.yaml#L628-L629

AssertionError: Expected 31 failures, got 35 on test/fixtures/templates/quickstart/vpc-management.json

https://github.com/aws-cloudformation/cfn-python-lint/blob/5fb676c6039e00bbe3e9096bbbf8dac62e6f64ba/test/fixtures/templates/quickstart/vpc-management.json#L65-L69

https://github.com/aws-cloudformation/cfn-python-lint/blob/5fb676c6039e00bbe3e9096bbbf8dac62e6f64ba/test/fixtures/templates/quickstart/vpc-management.json#L865-L867


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

@codecov
Copy link

codecov bot commented Jul 28, 2019

Codecov Report

Merging #1058 into master will decrease coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1058      +/-   ##
==========================================
- Coverage   86.91%   86.91%   -0.01%     
==========================================
  Files         134      134              
  Lines        7934     7930       -4     
  Branches     1950     1952       +2     
==========================================
- Hits         6896     6892       -4     
  Misses        641      641              
  Partials      397      397
Impacted Files Coverage Δ
src/cfnlint/rules/parameters/Cidr.py 97.72% <100%> (-0.1%) ⬇️
src/cfnlint/rules/parameters/CidrAllowedValues.py 97.67% <100%> (-0.11%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5e5ef49...95d797c. Read the comment docs.

@PatMyron
Copy link
Contributor Author

@kddejong @fatbasstard should I raise the number of expected failures in the tests? The output difference from linting with these changes looks expected to me

$ diff <(cfn-lint test/fixtures/templates/quickstart/vpc-management.json) <(.tox/py37/bin/cfn-lint test/fixtures/templates/quickstart/vpc-management.json)
> W2509 AllowedPattern and/or AllowedValues for Parameter should be specified at Parameters/pProductionCIDR. Example for AllowedPattern: '^(([0-9]|[1-9][0-9]|1[0-9]{2}|2[0-4][0-9]|25[0-5])\.){3}([0-9]|[1-9][0-9]|1[0-9]{2}|2[0-4][0-9]|25[0-5])(\/([0-9]|[1-2][0-9]|3[0-2]))$'
> test/fixtures/templates/quickstart/vpc-management.json:65:4

$ diff <(cfn-lint test/fixtures/templates/quickstart/nist_vpc_management.yaml) <(.tox/py37/bin/cfn-lint test/fixtures/templates/quickstart/nist_vpc_management.yaml)
> W2509 AllowedPattern and/or AllowedValues for Parameter should be specified at Parameters/pProductionCIDR. Example for AllowedPattern: '^(([0-9]|[1-9][0-9]|1[0-9]{2}|2[0-4][0-9]|25[0-5])\.){3}([0-9]|[1-9][0-9]|1[0-9]{2}|2[0-4][0-9]|25[0-5])(\/([0-9]|[1-2][0-9]|3[0-2]))$'
> test/fixtures/templates/quickstart/nist_vpc_management.yaml:267:3

@kddejong
Copy link
Contributor

@PatMyron yea just increase the numbers if the errors are expected as a result of your changes

@PatMyron
Copy link
Contributor Author

PatMyron commented Jul 28, 2019

@kddejong I tested adding AllowedPattern to the pProductionCIDR parameter in both of those quickstart templates in another branch to verify that was the source of the increase in errors, but still see some of the errors, which was unexpected to me:

AssertionError: Expected 27 failures, got 28 on test/fixtures/templates/quickstart/nist_vpc_management.yaml

AssertionError: Expected 31 failures, got 34 on test/fixtures/templates/quickstart/vpc-management.json


I've forced the tests to pass because the output difference from linting those templates makes sense to me, but I am confused by the above and why the expected number of failures for vpc-management.json had to be increased by 4.

@PatMyron PatMyron changed the title CIDRs extending CIDR validation to more types Jul 28, 2019
@kddejong kddejong merged commit b31be25 into aws-cloudformation:master Jul 29, 2019
@PatMyron PatMyron deleted the cidr branch July 31, 2019 04:29
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