Conversation
New config param `instances.parameters.inboundIp`. Allows users to specify an inbound IP address or range, which is then used to whitelist all ports, including 22, in the instance's security group. By default this is set to 0.0.0.0/0 and therefore not whitelisted.
netaddr.IPNetwork used as part of the schema validation and type for inbound_ip param.
| }] | ||
| # add ports to the security group for the user-defined inbound IP | ||
| inbound_ip = instance_config.inbound_ip | ||
| cidr_ip = 'CidrIpv6' if inbound_ip == 6 else 'CidrIp' |
There was a problem hiding this comment.
It looks like you forgot to add a proper check for IPv6 addresses here
There was a problem hiding this comment.
woops! This is what happens when I don't do the unit tests - maybe the templates could to with some tests to ensure the args are at least building them correctly...
Forgot to call the .version on the IPNetwork object
This reverts commit 4fb6d74. This re-introduces spaces at the end of each markdown line
Single quotes for strings Trailing commas for multiline arrays
|
@apls777 Thank you for the feedback, I have made all the requested changes. |
| 'CidrIpv6': '::/0', | ||
| 'IpProtocol': 'tcp', | ||
| 'FromPort': port, | ||
| 'ToPort': port, |
There was a problem hiding this comment.
Just noticed that now if the inboundIp parameter is not specified, we're not opening ports for IPv6. So, I think we need to use None for the default value instead of 0.0.0.0/0 to know when the parameter wasn't specified and open ports for both protocols.
| ), | ||
| Optional('managedPolicyArns', default=[]): [str], | ||
| Optional('instanceProfileArn', default=None): str, | ||
| Optional('inboundIp', default='0.0.0.0/0'): Use(IPNetwork, error='"inboundIp" should be a valid IP ' |
There was a problem hiding this comment.
For the default value, the inbound_ip.version == 6 check will fail later in the code as the Use(IPNetwork, ... function is not applied. It can be fixed with the IPNetwork('0.0.0.0/0') value, but as I mentioned in another comment, we need to use None instead to know when the value wasn't set at all.
| 'spotInstance': False, | ||
| 'subnetId': '', | ||
| 'volumes': [], | ||
| 'inboundIp': '0.0.0.0/0', |
There was a problem hiding this comment.
Should be fixed as well when the default inboundIp value is changed.
2 participants
New config param
instances.parameters.inboundIp.Allows users to specify an inbound IP address or range, which is then
used to whitelist all ports, including 22, in the instance's security
group. By default this is set to 0.0.0.0/0 and therefore not
whitelisted.
Resolves #96