Specifications http://www.zabbix.org/wiki/Docs/specs/ZBXNEXT-2160

Server part done in development branch svn://svn.zabbix.com/branches/dev/ZBXNEXT-2160 .

(1) [S] Functions iprangev4_parse() and iprangev6_parse() do not seem to validate the mask integer, so it is possible to write masks like "192.168.0.0/asdf".

wiper while it does not have explicit check if the mask is given correctly, it would still fail 192.168.0.0/asdf as the mask would be 0. Though it would accept 192.168.0.0/16asdf range.

wiper RESOLVED in r50633

asaveljevs I wonder whether is_uint31() function should be used instead of is_uint32(), considering that bits variable is of type "int", rather than "unsigned int".

Also, while according to (5) we do not validate that the size of the range is 64K, we should probably validate that mask is within reasonable bounds (1 to 32 for IPv4, 1 to 128 for IPv6). If we validate that, then the added "0 <= i" check in iprange_apply_mask() does not seem to be necessary. REOPENED.

wiper mask range is now limited to 0-32 (IPv4) and 0-128 (IPv6).
RESOLVED in r50668

asaveljevs There was a bug in a call to is_uint_n_range(), where a mask like "192.168.0.0/16999999" would be accepted. This is fixed in r50672. RESOLVED.

wiper CLOSED

(2) [S] Function iprangev6_parse() accesses a byte beyond the terminating NUL in the following conditional:

check_fill:
	/* check if the next group is empty */
	if (':' == ptr[1])
		...

wiper RESOLVED in r50632

asaveljevs CLOSED

(3) [S] Please review r50624. It mostly suggests stylistic fixes and minor improvements.

wiper Thanks, CLOSED

(4) [S] How about we remove ip4_str2dig(), ip6_str2dig(), and ip6_dig2str() functions? Those are no longer used.

wiper RESOLVED in r50636

asaveljevs CLOSED

(5) [S] Server does not seem to impose the 64K limit on the number of addresses specified using ranges.

wiper yes, I understand that it should be done by frontend. But server does limit the bitmask to 16+ (112+) and I'm now wondering if it's right.

wiper Iprange parsing functions should not check the range for any limits. However a new function to check the range size should be added and used in discoverer to validate the range before trying to iterate through it.

wiper RESOLVED in r50653

asaveljevs A new constant was introduced:

#define ZBX_DISCOVERER_IPRANGE_LIMIT	(64 * ZBX_KIBIBYTE)

It is then used like so:

zabbix_log(LOG_LEVEL_WARNING, "discovery rule \"%s\": IP range exceeds %d KB address limit",
		drule->name, ZBX_DISCOVERER_IPRANGE_LIMIT / ZBX_KIBIBYTE);

However, IP ranges are not exactly measured in kibibytes.

There is also a suggestion to rename iprange_get_address_count() to iprange_volume().

Finally, iprange_volume() can overflow a 64-bit integer. Its return value should probably be limited by ZBX_MAX_UINT64 (a new constant).

Whie at it, please review r50664. REOPENED.

wiper RESOLVED in r50668

asaveljevs r50672 removes bytes completely from ZBX_DISCOVERER_IPRANGE_LIMIT. RESOLVED.

wiper CLOSED

(6) [S] Please review r50681 - it simplifies calls to is_uint_n_range() in IP range parse functions. If this looks good, we might also wish to simplify calls to is_uint_n_range() in include/common.h.

wiper reviewed, simpliefied is_uint_* defines in common.h
RESOLVED in r50688

asaveljevs Removing "LL" from long constants gave warnings on 32-bit FreeBSD:

integer constant is too large for 'long' type

This is fixed in r50694. Please review. RESOLVED.

asaveljevs Replaced "LL" suffix with __UINT64_C() macro in r50695, as per your suggestion. RESOLVED.

wiper CLOSED

(7) [S] There was an infinite loop in iprange_apply_mask() due to "bits" variable being unsigned. This is fixed in r50707

wiper and r50710 (to get rid of compilation warning) CLOSED

(8) [F] Frontend also must support the new IP range specifications

arvids.godjuks RESOLVED in r50776

iivs Provided better solution.

RESOLVED in r51015, r51088, r51094

sasha CLOSED

(9) [PS] broadcast and network addresses should not be included in a range when we using IPv4 masks

wiper RESOLVED in r50727

asaveljevs Looks good, but please see r50744. RESOLVED.

wiper thanks! CLOSED

Server side ready for retesting

(10) Changes in translation

sasha Translation string changes:

Removed:

• Field "%1$s" is not IP range.
• Field "%1$s" is not IP.
• Incorrect IP range "%s".
• Incorrect action condition ip "%1$s".
• Incorrect interface IP parameter "%s" provided.
• Field "%1$s" is not integer list or range.

Added:

• IP address cannot be empty.
• IP address range cannot be empty.
• Invalid IP address "%1$s".
• Invalid IP address "%1$s": must be a string.
• Invalid IP address range "%1$s".
• Invalid IP address range "%1$s": must be a string.
• IP range "%1$s" exceeds "%2$s" address limit.

RESOLVED.

iivs CLOSED.

(11) IPs like '/192.168.2.33', '192.168.2/10000.33', '192.168.2/.33' must not be valid

arvids.godjuks RESOLVED in r50830

iivs CLOSED.

(12) Function isValidMask() always returns true, therefore it does not validate anything

arvids.godjuks RESOLVED in r50830

iivs CLOSED.

(13) Function hasError() is (wrongly) used in isValidMask() but not used at all in isValidRange(). The function must be removed.

arvids.godjuks RESOLVED in r50830
Re-wrote the mask validation to be the same way as range validation - now both are consistent.

iivs CLOSED.

(14) [F] Part of IP validation was in validate.inc.php, so I moved that functionality to CIPValidator class. (Notice IP is in upper case, since it is an abbrevation). Then I extended CIPRangeValidator class from CIPValidator to additionally handle IP ranges. I also fixed several problems, like in case when IP comma-separated list skipped validating IP addresses after fist comma, and possibility to pass other type of variables in CIPValidator and CIPRangeValidator validate methods. Before that even array could be passed causing SQL errors. And I also fixed several other coding style issues.

RESOLVED in r51015, r51088

sasha CLOSED

subissues still open : 8, 10, 14

(15) [F] Added frontend unit tests for IP and IP range validators.

RESOLVED in r51088, r51094

sasha CLOSED

(16) CIPValidator class shouldn't support macros. It is not its task!

iivs RESOLVED in r51199

sasha CLOSED

(17) frontends/php/include/classes/validators/CIPValidator.php:101 - here is unnecessary with is_numeric() function call

if (!is_numeric($matches[$i]) || $matches[$i] > 255 || $matches[$i] < 0 ) {

iivs RESOLVED in r51199

sasha CLOSED

(18) CIPValidator.php, CIPRangeValidator.php - $empty variable should be renamed. May be $allowEmpty?

iivs Will be removed in (21).
CLOSED.

(19) isIPv4() and isIPv6() functions should be removed. If we will use these functions any string with dot or colon will be a valid IP address.

iivs RESOLVED in r51199

sasha CLOSED

(20) Please review my changes in r51128.

sasha This changes will be removed under (21). CLOSED

(21) Unused property CIPRangeValidator.empty. CIPValidator.empty should be removed too.

iivs RESOLVED in r51199

sasha CLOSED

(22) All regular expressions should be unified. For example:

'/^\d{2}$/' => '/^[0-9]{2}$/'

iivs RESOLVED in r51199

sasha CLOSED

(23) isValidRange() should be divided into three functions: isValidRange(), isValidRangeIPv4() and isValidRangeIPv6()

iivs RESOLVED in r51199

sasha CLOSED

(24) Please review my changes in r51241, r51249.

iivs Looks good. Thanks for improving the code!
I corrected action condition unit test, removed unused constant T_ZBX_INT_RANGE and updated IP range validator test with few more variations. See changes I've made in r51275.

sasha Thanks a lot! CLOSED

TESTED,

but don't forget to close (24).

Available in pre-2.4.4 r51286 and pre-2.5.0 (trunk) r51287.

(25) Documentation changes:

• Zabbix API changes in 2.4
• What's new in Zabbix 2.4.4
• Configuring a network discovery rule 2.4 and 3.0
• Action conditions 2.4 and 3.0

martins-v Updated:

• https://www.zabbix.com/documentation/2.4/manual/introduction/whatsnew244
• https://www.zabbix.com/documentation/2.4/manual/discovery/network_discovery/rule#rule_attributes
• https://www.zabbix.com/documentation/3.0/manual/discovery/network_discovery/rule#rule_attributes
• https://www.zabbix.com/documentation/2.4/manual/config/notifications/action/conditions#configuration
• https://www.zabbix.com/documentation/3.0/manual/config/notifications/action/conditions#configuration

sasha Great!

Also updated:

• https://www.zabbix.com/documentation/2.4/manual/api/changes_2.4

Removed outdated note from:

• https://www.zabbix.com/documentation/2.4/manual/discovery/network_discovery/rule#rule_attributes
• https://www.zabbix.com/documentation/3.0/manual/discovery/network_discovery/rule#rule_attributes

martins-v Looking good. I'll set this as CLOSED.

(26) Windows agent does not compile:

misc.o : error LNK2019: unresolved external symbol _iprange_validate referenced in function _ip_in_list
misc.o : error LNK2019: unresolved external symbol _iprange_first referenced in function _ip_in_list
misc.o : error LNK2019: unresolved external symbol _iprange_parse referenced in function _ip_in_list

asaveljevs Fixed in development branch svn://svn.zabbix.com/branches/dev/ZBXNEXT-2160 in r51693. RESOLVED.

wiper CLOSED

Fix of (26) available in pre-2.4.4 r51695 and pre-2.5.0 (trunk) r51696.