Open atnoslen opened 8 years ago
Well, yes or no. The ipaddr('address')
filter is used for extracting the IP address from a CIDR, but it can also be used to filter out host IP addresses from a list, therefore I wouldn't change it.
At the same time ipaddr('network')
would give you the network IP address without the CIDR, if that's the behaviour you want. Of course it will then not pass the second host IP address.
I guess that a filter that basically strips the CIDR prefix from a string could be added, any suggestion for a name?
However, are you aware that the first address you gave is a network address, which can't really be used in a host context? It only changes into a host address in /15
CIDR subnets and bigger.
I would propose that 'address' and/or 'ip' return the address supplied. 'host' should respond with the host address iff that is a host address given the subnet mask (or alone). 'network' should provide the address iff the address is a network given the subnet mask. Similar, 'broadcast' iff the address is a broadcast address given the subnet mask.
The reason follows. An IP address alone is just an address. It does not gain any special meaning unless supplied with a subnet mask, in which it can become a host, network, and/or broadcast address. Take the following cases.
/32 - Only a host address, though it can also be considered a network address as it can be placed in routing tables. /31 - The first IP is both host and network address. The second IP is only a host address, but never a broadcast address. /30 to /0 - Follows what is considered normal addressing.
A more concrete example. 192.168.1.4/32 is a host (and possibly considered a network by some). 192.168.1.4/31 is both a host address and network address. 192.168.1.4/30 is only a network address. The only information that changed was the bit mask and thereby the type of address it is. However, the IP address itself has not changed.
Further, in RFC 791 they mention both network addresses and host addresses (note both referred to as an address). In RFC 919, the RFC to include broadcast addresses, refers to itself as a broadcast address. Though in RFC 1519 they use network number more often without any mention of host or broadcasts.
Nomenclature in IT is very important. It also makes perfect sense why this module was developed as is considering Ansible's roots. I would argue, however, that generic modules utilize prescribed nomenclature without assumptions. Extended functionality should absolutely be included (I like that we can tell if something is a host or not), but should be wrapped in functions that do not conflict with the prescribed meaning.
I realize changing functionality in modules such as this are not easy and can damage many customers' deployments. Thus I would recommend adding 'ip_address' to always return the IP address supplied. Then provide warning that 'address' and 'ip' are changing functionality in some future version to remove confusion on the most generic meaning. Further, I would recommend refactoring to include the /31 edge case as the current code only accounts for /32 as host and /31 as a network without any hosts. 'broadcast' will supply /31 networks with the second IP, which it most certainly is not.
If you are looking for use cases, I will be happy to supply ample amounts from network engineering discipline.
I agree that an IP address without and with a CIDR prefix are different things, however I disagree with your proposed solution. Let me bring back your example:
networks: [
'10.1.0.0/24',
'10.2.3.4/32'
]
{{ networks[0] | ipaddr('address') }} --> None
{{ networks[1] | ipaddr('address') }} --> '10.2.3.4'
With this code, you are not adding any information to the given IP addresses; in fact you are stripping them from essential information, ie. CIDR prefix. This way, from meaningful network and host addresses they become simple strings of numbers and punctuation, useless without nomenclature which as you said, is important.
NB, the issue you mention about /31
prefix not working correctly should be now fixed in the ipaddr()
filter included in the Ansible devel
branch, and will mot likely be available with the next Ansible release, ie. 2.2.
I'm not very experienced with networking infrastructure, I mostly work on the server side. I suppose that there could be cases in network engineering where you can configure IP addresses of networks and hosts at the same time without specifying CIDR prefixes for each, however that case sounds weird to me. But all right then, I'm willing to accept it. :-) However, I don't agree with the suggestion of changing the meaning of address
and ip
queries in the ipaddr()
filter. In my view, what you are trying to do with the above example is to strip the CIDR prefix from the given IP addresses, no matter what they are. This should be a valid filter since that way you could strip the prefixes from multiple addresses in a list at once. Therefore, instead of chaning the function of existing address
and ip
queries, I would propose to add a new query, stripcidr
which would remove any CIDR prefixes from given IP addresses if they are present. What do you think about this solution?
CIDR notation was defined in RFC 4632 sec 3.1 that describes concatenating two pieces of information. "In CIDR notation, a prefix is shown as a 4-octet quantity, just like a traditional IPv4 address or network number, followed by the "/" (slash) character, followed by a decimal value between 0 and 32 that describes the number of significant bits." Emphasis mine. Sometimes you need to work on the more atomic portions of information, as shown below.
Examine the following for differing pieces of network equipment:
nx_os
ip access-list 1
permit ip 192.168.4.0/24 any
permit ip 192.168.1.1/32 any
ios_xe
access-list 1 permit 192.168.4.0 0.0.0.255
access-list 1 permit 192.168.1.1
ios
access-list 1 permit 192.168.4.0 0.0.0.255
access-list 1 permit host 192.168.1.1
asa
access-list foo permit 192.168.4.0 255.255.255.0 any
access-list foo permit host 192.168.1.1 any
F5
192.168.4.0/255.255.255.0
192.168.1.1
These are all the same ACL, but can be described simply as
{snmp_readonly_networks: [{action:'permit', source:'192.168.4.0/24'},
{action:'permit', source:'192.168.1.1/32'}]}
That source information (or added destination information in extended ACL's) must be represented in many ways as shown above, but is fundamentally the same information. Sometimes in pure CIDR format (nx_os). Sometimes as [address] [subnetmask]
(asa). Sometimes as [address] [hostmask]
where hostmask
is the inverse of subnetmask
(ios flavored). Sometimes as [address]/[subnetmask]
(F5). I could take the source
and split the string. Get the bitmask
and then have a lookup table for the required information, but that kind of defeats the purpose of taking a CIDR address and getting the same information represented differently out of it, which already exist, mind. The key here is that the address
and the bitmask
are two separate pieces of information that combine to provide more context. But sometimes you need to work with one of the other individually.
So you are correct that 192.168.4.128
means something semantically different than 192.168.4.128/25
. 192.168.4.128
is just an address with a numeric value of 3232236672
and a bitstream of 11000000101010000000010010000000
. It has no further information or value than that. It can be placed in an IPv4 header and sent on the network just as is. The bitmask
/25
is a separate piece of information that when combined with the address
provide further pieces of information. However, the bitmask/subnetmask
is not included in the IPv4 header. That is configured on hosts and routers to describe broadcast domains. Network address and broadcast address can then be derived from that.
Now I can fully understand not wanting to completely change the functionality of already coded functions. We've all coded ourselves into corners (I know I have). I was only making the suggestion that as more network engineers start to use Ansible to manage their inventory that the names of functions more accurately represent the de facto and published nomenclature. An address
is nothing more than just an address
. While CIDR represents two pieces of information, address/bitmask
. I would accept stripcidr
as adequate given the context, but would highly recommend standardizing on industry standard nomenclature for less confusion in the future.
Well... Why didn't you tell me that in the first place! :-)
One of the more important reasons to write ipaddr()
filter was so that I could take a piece of information, like a host/prefix string, and output it in any format that was required by the application. You can see that in this repository README file, where the same list of different IP addresses and subnets are rendered again and again in different ways.
It seems to me that what you really need, is some additional ipaddr()
queries that output the information from a given CIDR in a format accepted by ios, F5, etc. as needed. That way we can agree on a common input format (host/prefix) and have output formatted as needed.
So, I would make a set of additional functions that output the data in the formats that are currently missing:
[address] [subnetmask]
[address] [hostmask]
[address]/[subnetmask]
Pick a good names for them and submit that. I imagine that there would be some clarification needed about some things - for example, in the format expected by F5, is the single host always without a mask, and are networks always with masks? This could be coded in the finished function for that particular format.
As I said before, I'm not a networking guy, but a server guy. Therefore, ipaddr()
was written from a server perspective. If there are other formats that are missing for networking equipment, I don't see a reason not to add them, especially that Ansible is focused more on networking recently.
I would also suggest that the Python library ipaddr
is an interface for, netaddr, makes a similar distinctions.
By the API, class netaddr.IPAddress
is just an address without a bitmask. class netaddr.IPNetwork
, is what you are using, but includes both an address and a subnet/bit mask. Notice that ip
in the API for netaddr.IPNetwork
is "The IP address of this IPNetwork object" and makes note of "This is may or may not be the same as the network IP address which varies according to the value of the CIDR subnet prefix.". So when ipaddr
is a wrapper/interface for netaddr
I am expecting that same functionality for the same named parameter.
Roger that. Possibly an extension of the typical format(value, *args, **kwargs)
so that delimeters can vary the output.
Something like (but I'm not advocating for these specifically)
%ip - IP address %sm - subnet mask %hm - host mask (also called a wild card mask) %bm - bit mask
Thus you could possibly have ipaddr('format', '%ip/%bm')
or ipaddr('format', '%ip %hm')
. Then just do a regex replace of %ip
with v.ip
.
Sure, a format
query with custom string sounds good. :+1: Any idea how to implement that in Python?
I'm aware about the distinction between netaddr.IPAddress
and netaddr.IPNetwork
. The ipaddr()
filter is not 1 to 1 implementation of the netaddr
Python library because I had to work with constrained nature of Ansible filter system. I tried to keep things simple to use and consistent; I imagine that 1 to 1 mapping would require two filters, one for hosts and another for networks. I'm not sure if that would be better in the end.
I 100% agree simplicity rules the day and that there may not be a necessity to expose every class
from netaddr
, though it also may just be a very simple wrapper to create and leave all "logic" to the base library.
I would recommend following https://docs.python.org/3/library/string.html#formatstrings. Another frame of reference is the Jinja2 format function (no direct link, but it's on that page). All that would be needed are the delimiters.
I also looked at the code base and don't see variable number of arguments in the class definition. Much like jquery's functionality where you can give the first parameter of what sub-function you're calling and then provide arguments against that sub-function. For reference: http://stackoverflow.com/questions/919680/can-a-variable-number-of-arguments-be-passed-to-a-function .
I'm just now starting with python after years in other languages, otherwise I'd pull request immediately to help. I'm going to play on my own to try to get that functionality in without any core changes (you're using named parameters) so I can become better at python anyway. Then when I have minimum changes necessary, I'll pull request and submit. Hopefully it's not too far off your dev branch.
I think I may have a generic solution to this.
First, I modified the ipaddr
definition to be
def ipaddr(value, query = '', *args, **kwargs):
''' Check if string is an IP address or network and filter it '''
version = False
alias = 'ipaddr'
if kwargs.has_key('version'):
version = kwargs['version']
if kwargs.has_key('alias'):
alias = kwargs['alias']
This left the version
and alias
as default values while still allowing them to be set. The reasoning behind this was leaving them in as default parameters would conflict with ordering with dynamic parameters. That is, if version
was left as parameter 3 and a query parameter was added in that position, that would overwrite version
and get blown out.
Added function format to query_func_extra_args
and query_func_map
.
query_func_extra_args = {
'format':('args',),
query_func_map = {
'format': _format_query,
Then defined _format_query
.
def _format_query(v, args):
return args[0].replace('%ip', str(v.ip)) \
.replace('%bm', str(v.prefixlen)) \
.replace('%sm', str(v.netmask)) \
.replace('%hm', str(v.hostmask))
This can obviously be cleaned up for ensuring minimum number of arguments. It can be used as such:
ipaddr('format', '%ip %hm')
or ipaddr('format', '%ip/%bm')
Further, this allows additional N number of arguments with queries, while retaining the named defaults for existing functionality.
I've attached the modified ipaddr.py.zip file for your consideration and testing.
@atnoslen Interesting. Did you try it with list of IP addresses? How about mixed list of IP addresses and subnets, how does the filter behave then?
Excellent question. I'm not sure of all the tests required to support a change of this magnitude. I'll see if i can run some additional tests. Standby for results.
@atnoslen BTW, if you haven't noticed, ipaddr()
is included in Ansible Core; I haven't updated this repository to keep the original version, but you probably should work with the new one. You can find it here.
I was modifying the one that came with Ansible (after backing my version up, of course). I noticed this version was quite a bit different and didn't know where it stood in the dev pipeline.
Ran most of the list tests on your main page. You can see it came back exactly as expected.
Here are the results:
TASK [Test {{ test_list | ipaddr }}] *******************************************
ok: [localhost] => {
"msg": [
"192.24.2.1",
"::1",
"192.168.32.0/24",
"fe80::100/10",
"2001:db8:32c:faad::/64"
]
}
TASK [Test {{ test_list | ipv4 }}] *********************************************
ok: [localhost] => {
"msg": [
"192.24.2.1",
"192.168.32.0/24"
]
}
TASK [Test {{ test_list | ipv6 }}] *********************************************
ok: [localhost] => {
"msg": [
"::1",
"fe80::100/10",
"2001:db8:32c:faad::/64"
]
}
TASK [Test {{ test_list | ipaddr('address') }}] ********************************
ok: [localhost] => {
"msg": [
"192.24.2.1",
"::1",
"fe80::100"
]
}
TASK [Test {{ test_list | ipaddr('host') }}] ***********************************
ok: [localhost] => {
"msg": [
"192.24.2.1/32",
"::1/128",
"fe80::100/10"
]
}
TASK [Test {{ test_list | ipv4('host') }}] *************************************
ok: [localhost] => {
"msg": [
"192.24.2.1/32"
]
}
TASK [Test {{ test_list | ipv6('host') }}] *************************************
ok: [localhost] => {
"msg": [
"::1/128",
"fe80::100/10"
]
}
TASK [Test {{ test_list | ipaddr('public') }}] *********************************
ok: [localhost] => {
"msg": [
"192.24.2.1",
"2001:db8:32c:faad::/64"
]
}
TASK [Test {{ test_list | ipaddr('private') }}] ********************************
ok: [localhost] => {
"msg": [
"192.168.32.0/24",
"fe80::100/10"
]
}
TASK [Test {{ test_list | ipaddr('net') }}] ************************************
ok: [localhost] => {
"msg": [
"192.168.32.0/24",
"2001:db8:32c:faad::/64"
]
}
TASK [Test {{ test_list | ipaddr('net') | ipaddr('size') }}] *******************
ok: [localhost] => {
"msg": [
256,
18446744073709551616
]
}
TASK [Test {{ test_list | ipaddr('192.168.0.0/8') }}] **************************
ok: [localhost] => {
"msg": [
"192.24.2.1",
"192.168.32.0/24"
]
}
TASK [Test {{ test_list | ipaddr('net') | ipaddr('0') }}] **********************
ok: [localhost] => {
"msg": [
"192.168.32.0/24",
"2001:db8:32c:faad::/64"
]
}
TASK [Test {{ test_list | ipaddr('net') | ipaddr('1) }}] ***********************
ok: [localhost] => {
"msg": [
"192.168.32.1/24",
"2001:db8:32c:faad::1/64"
]
}
TASK [Test {{ test_list | ipaddr('net') | ipaddr('-1') }}] *********************
ok: [localhost] => {
"msg": [
"192.168.32.255/24",
"2001:db8:32c:faad:ffff:ffff:ffff:ffff/64"
]
}
TASK [Test {{ test_list | ipaddr('net') | ipaddr('200') }}] ********************
ok: [localhost] => {
"msg": [
"192.168.32.200/24",
"2001:db8:32c:faad::c8/64"
]
}
TASK [Test {{ test_list | ipaddr('net') | ipaddr('-200') }}] *******************
ok: [localhost] => {
"msg": [
"192.168.32.56/24",
"2001:db8:32c:faad:ffff:ffff:ffff:ff38/64"
]
}
TASK [Test {{ test_list | ipaddr('net') | ipaddr('400') }}] ********************
ok: [localhost] => {
"msg": [
"2001:db8:32c:faad::190/64"
]
}
TASK [Test {{ host_prefix | ipaddr('host/prefix') }}] **************************
ok: [localhost] => {
"msg": [
"2001:db8:deaf:be11::ef3/64",
"192.0.2.48/24"
]
}
Test with the format query as well. 30 sie 2016 00:10 "atnoslen" notifications@github.com napisał(a):
Ran most of the list tests on your main page. You can see it came back exactly as expected.
Here are the results:
TASK [Test {{ test_list | ipaddr }}] *** ok: [localhost] => { "msg": [ "192.24.2.1", "::1", "192.168.32.0/24", "fe80::100/10", "2001:db8:32c:faad::/64" ] }
TASK [Test {{ test_list | ipv4 }}] ***** ok: [localhost] => { "msg": [ "192.24.2.1", "192.168.32.0/24" ] }
TASK [Test {{ test_list | ipv6 }}] ***** ok: [localhost] => { "msg": [ "::1", "fe80::100/10", "2001:db8:32c:faad::/64" ] }
TASK [Test {{ test_list | ipaddr('address') }}] **** ok: [localhost] => { "msg": [ "192.24.2.1", "::1", "fe80::100" ] }
TASK [Test {{ test_list | ipaddr('host') }}] *** ok: [localhost] => { "msg": [ "192.24.2.1/32", "::1/128", "fe80::100/10" ] }
TASK [Test {{ test_list | ipv4('host') }}] ***** ok: [localhost] => { "msg": [ "192.24.2.1/32" ] }
TASK [Test {{ test_list | ipv6('host') }}] ***** ok: [localhost] => { "msg": [ "::1/128", "fe80::100/10" ] }
TASK [Test {{ test_list | ipaddr('public') }}] ***** ok: [localhost] => { "msg": [ "192.24.2.1", "2001:db8:32c:faad::/64" ] }
TASK [Test {{ test_list | ipaddr('private') }}] **** ok: [localhost] => { "msg": [ "192.168.32.0/24", "fe80::100/10" ] }
TASK [Test {{ test_list | ipaddr('net') }}] **** ok: [localhost] => { "msg": [ "192.168.32.0/24", "2001:db8:32c:faad::/64" ] }
TASK [Test {{ test_list | ipaddr('net') | ipaddr('size') }}] *** ok: [localhost] => { "msg": [ 256, 18446744073709551616 ] }
TASK [Test {{ test_list | ipaddr('192.168.0.0/8') }}] ** ok: [localhost] => { "msg": [ "192.24.2.1", "192.168.32.0/24" ] }
TASK [Test {{ test_list | ipaddr('net') | ipaddr('0') }}] ** ok: [localhost] => { "msg": [ "192.168.32.0/24", "2001:db8:32c:faad::/64" ] }
TASK [Test {{ test_list | ipaddr('net') | ipaddr('1) }}] *** ok: [localhost] => { "msg": [ "192.168.32.1/24", "2001:db8:32c:faad::1/64" ] }
TASK [Test {{ test_list | ipaddr('net') | ipaddr('-1') }}] ***** ok: [localhost] => { "msg": [ "192.168.32.255/24", "2001:db8:32c:faad:ffff:ffff:ffff:ffff/64" ] }
TASK [Test {{ test_list | ipaddr('net') | ipaddr('200') }}] **** ok: [localhost] => { "msg": [ "192.168.32.200/24", "2001:db8:32c:faad::c8/64" ] }
TASK [Test {{ test_list | ipaddr('net') | ipaddr('-200') }}] *** ok: [localhost] => { "msg": [ "192.168.32.56/24", "2001:db8:32c:faad:ffff:ffff:ffff:ff38/64" ] }
TASK [Test {{ test_list | ipaddr('net') | ipaddr('400') }}] **** ok: [localhost] => { "msg": [ "2001:db8:32c:faad::190/64" ] }
TASK [Test {{ host_prefix | ipaddr('host/prefix') }}] ** ok: [localhost] => { "msg": [ "2001:db8:deaf:be11::ef3/64", "192.0.2.48/24" ] }
— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/drybjed/ansible-ipaddr-filter/issues/2#issuecomment-243273464, or mute the thread https://github.com/notifications/unsubscribe-auth/AA_t9nuP5H-iPcuEXDjqpzMj6RVugeIeks5qk1jbgaJpZM4JsfvG .
Modified:
# Check if value is a list and parse each element
elif isinstance(value, (list, tuple, types.GeneratorType)):
_ret = []
for element in value:
if ipaddr(element, str(query), *args, **kwargs):
_ret.append(ipaddr(element, str(query), *args, **kwargs))
def ipv4(value, query = '', *args, **kwargs):
kwargs['version'] = 4
kwargs['alias'] = 'ipv4'
return ipaddr(value, query, *args, **kwargs)
def ipv6(value, query = '', *args, **kwargs):
kwargs['version'] = 6
kwargs['alias'] = 'ipv6'
return ipaddr(value, query, *args, **kwargs)
Produces
TASK [Test {{ test_list | ipaddr }}] *******************************************
ok: [localhost] => {
"msg": [
"192.24.2.1",
"::1",
"192.168.32.0/24",
"fe80::100/10",
"2001:db8:32c:faad::/64"
]
}
TASK [Test {{ test_list | ipaddr('format', '%ip %sm') }}] **********************
ok: [localhost] => {
"msg": [
"192.24.2.1 255.255.255.255",
"::1 ffff:ffff:ffff:ffff:ffff:ffff:ffff:ffff",
"192.168.32.0 255.255.255.0",
"fe80::100 ffc0::",
"2001:db8:32c:faad:: ffff:ffff:ffff:ffff::"
]
}
TASK [Test {{ test_list | ipv4 }}] *********************************************
ok: [localhost] => {
"msg": [
"192.24.2.1",
"192.168.32.0/24"
]
}
TASK [Test {{ test_list | ipv4('format', '%ip %sm') }}] ************************
ok: [localhost] => {
"msg": [
"192.24.2.1 255.255.255.255",
"192.168.32.0 255.255.255.0"
]
}
TASK [Test {{ test_list | ipv6 }}] *********************************************
ok: [localhost] => {
"msg": [
"::1",
"fe80::100/10",
"2001:db8:32c:faad::/64"
]
}
TASK [Test {{ test_list | ipv6('format', '%ip %sm') }}] ************************
ok: [localhost] => {
"msg": [
"::1 ffff:ffff:ffff:ffff:ffff:ffff:ffff:ffff",
"fe80::100 ffc0::",
"2001:db8:32c:faad:: ffff:ffff:ffff:ffff::"
]
}
With this functionality, you could take some of the multi-variable helpers and put them into a query now. Then have the helper class just call that query for legacy. I'll do this with ipsubnet
and nthhost
with those query names and report back.
@atnoslen Excellent. So the question now, in your examples single hosts are present without corresponding hostmasks/netmasks. Is that required in the applications you are going to use that in, or can you provide equivalent of 255.255.255.255 in these cases instead? Now that I think about it, format
query could accept 1 or 2 parameters, and use different one for single hosts vs subnets. What do you think about this idea? I suppose that the order should be something like, ipaddr('format', subnet_string, host_string)
, so that the parameters are somewhat consistent.
Technically, there are some legacy ACL's where host is a special case, such that they read the following way:
! Subnet/network example
access-list 5 permit 10.0.0.0 0.255.255.255
! Host example
access-list 5 permit host 10.1.1.1
This would require special handling, as you mention. I'm considering writing an ACL helper/filter as ACL's have some special formatting in some spaces and conditions. Further, ACL's can get even crazier once you get to extended and/or named.
! ACL Header
ip access-list extended FOO
! Two different IP subnet objects, source and destination
! Permitting 10.0.0.0/24 to 192.168.0.0/16
permit ip 10.0.0.0 0.0.0.255 192.168.0.0 0.0.255.255
! "host" and "any" represent a /32 and /0 respectively.
! Also shows they can be reversed.
permit ip host 10.1.1.1 any
permit ip any host 10.1.1.1
! TCP/UDP port numbers can be interspersed on source and/or destination
permit tcp any any eq telnet
permit udp any eq domain any gt 1023
! Or more parameters
permit tcp any any gt 1023 established
The syntax on each OS, even with the same vendor, can be very different making this even more complicated.
So, although the changes I was talking about help for the simple case, especially standard ACL's for SNMP and the like, for the larger case, I need to develop a library to parse these out properly.
I think that some of these could be handled by Jinja in a template. Anyway, how much do you think the format
query helps with at the moment, 90%? I suppose that this warrants for inclusion in Ansible. You should perpare a Pull Request in https://github.com/ansible/ansible once you are confident that the filter works as expected. Don't forget to update the ipaddr documentation to include some example usage patters of the new query.
Is there a dev version of this filter that I can use?
@amsinha2, the ipaddr()
filter is now in the Ansible core therefore you should do any development of it in a fork of the Ansible repository.
Take a list of networks and hosts,
Due to this in ipaddr.py
As a result, with a mixed list of networks and hosts, you can't iterate over the list and get the address supplied.
This becomes imperative for multi-os support of access lists where a line may be a network or a host. I see no reason to not return the IP already supplied, regardless if it was a network or not.