Details

      Description

      Issue:
      There are many similar routers in my network that have 10.100.NN.1 ip addresses:
      10.100.1.1, 10.100.2.1, etc

      Autodiscovery is not have possibility to add all 10.100.NN.1 addresses to discovery rule at that moment.

      Another example: https://www.zabbix.com/forum/showthread.php?t=6791

      I belive, cisco wildcard mask will be good solution for that.

        Activity

        Hide
        Andris Zeila added a comment -
        Show
        Andris Zeila added a comment - Specifications http://www.zabbix.org/wiki/Docs/specs/ZBXNEXT-2160
        Hide
        Andris Zeila added a comment - - edited

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

        Show
        Andris Zeila added a comment - - edited Server part done in development branch svn://svn.zabbix.com/branches/dev/ZBXNEXT-2160 .
        Hide
        Aleksandrs Saveljevs added a comment - - edited

        (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".

        Andris Zeila 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.

        Andris Zeila RESOLVED in r50633

        Aleksandrs Saveljevs 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.

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

        Aleksandrs Saveljevs 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.

        Andris Zeila CLOSED

        Show
        Aleksandrs Saveljevs added a comment - - edited (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". Andris Zeila 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. Andris Zeila RESOLVED in r50633 Aleksandrs Saveljevs 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. Andris Zeila mask range is now limited to 0-32 (IPv4) and 0-128 (IPv6). RESOLVED in r50668 Aleksandrs Saveljevs 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. Andris Zeila CLOSED
        Hide
        Aleksandrs Saveljevs added a comment - - edited

        (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])
        		...
        

        Andris Zeila RESOLVED in r50632

        Aleksandrs Saveljevs CLOSED

        Show
        Aleksandrs Saveljevs added a comment - - edited (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]) ... Andris Zeila RESOLVED in r50632 Aleksandrs Saveljevs CLOSED
        Hide
        Aleksandrs Saveljevs added a comment - - edited

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

        Andris Zeila Thanks, CLOSED

        Show
        Aleksandrs Saveljevs added a comment - - edited (3) [S] Please review r50624. It mostly suggests stylistic fixes and minor improvements. Andris Zeila Thanks, CLOSED
        Hide
        Aleksandrs Saveljevs added a comment - - edited

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

        Andris Zeila RESOLVED in r50636

        Aleksandrs Saveljevs CLOSED

        Show
        Aleksandrs Saveljevs added a comment - - edited (4) [S] How about we remove ip4_str2dig(), ip6_str2dig(), and ip6_dig2str() functions? Those are no longer used. Andris Zeila RESOLVED in r50636 Aleksandrs Saveljevs CLOSED
        Hide
        Aleksandrs Saveljevs added a comment - - edited

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

        Andris Zeila 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.

        Andris Zeila 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.

        Andris Zeila RESOLVED in r50653

        Aleksandrs Saveljevs 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.

        Andris Zeila RESOLVED in r50668

        Aleksandrs Saveljevs r50672 removes bytes completely from ZBX_DISCOVERER_IPRANGE_LIMIT. RESOLVED.

        Andris Zeila CLOSED

        Show
        Aleksandrs Saveljevs added a comment - - edited (5) [S] Server does not seem to impose the 64K limit on the number of addresses specified using ranges. Andris Zeila 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. Andris Zeila 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. Andris Zeila RESOLVED in r50653 Aleksandrs Saveljevs 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. Andris Zeila RESOLVED in r50668 Aleksandrs Saveljevs r50672 removes bytes completely from ZBX_DISCOVERER_IPRANGE_LIMIT. RESOLVED. Andris Zeila CLOSED
        Hide
        Aleksandrs Saveljevs added a comment - - edited

        (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.

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

        Aleksandrs Saveljevs 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.

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

        Andris Zeila CLOSED

        Show
        Aleksandrs Saveljevs added a comment - - edited (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. Andris Zeila reviewed, simpliefied is_uint_* defines in common.h RESOLVED in r50688 Aleksandrs Saveljevs 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. Aleksandrs Saveljevs Replaced "LL" suffix with __UINT64_C() macro in r50695, as per your suggestion. RESOLVED. Andris Zeila CLOSED
        Hide
        Aleksandrs Saveljevs added a comment - - edited

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

        Andris Zeila and r50710 (to get rid of compilation warning) CLOSED

        Show
        Aleksandrs Saveljevs added a comment - - edited (7) [S] There was an infinite loop in iprange_apply_mask() due to "bits" variable being unsigned. This is fixed in r50707 Andris Zeila and r50710 (to get rid of compilation warning) CLOSED
        Hide
        Andris Zeila added a comment - - edited

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

        Arvids Godjuks RESOLVED in r50776

        Ivo Kurzemnieks Provided better solution.

        RESOLVED in r51015, r51088, r51094

        Alexander Vladishev CLOSED

        Show
        Andris Zeila added a comment - - edited (8) [F] Frontend also must support the new IP range specifications Arvids Godjuks RESOLVED in r50776 Ivo Kurzemnieks Provided better solution. RESOLVED in r51015, r51088, r51094 Alexander Vladishev CLOSED
        Hide
        Alexander Vladishev added a comment - - edited

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

        Andris Zeila RESOLVED in r50727

        Aleksandrs Saveljevs Looks good, but please see r50744. RESOLVED.

        Andris Zeila thanks! CLOSED

        Show
        Alexander Vladishev added a comment - - edited (9) [PS] broadcast and network addresses should not be included in a range when we using IPv4 masks Andris Zeila RESOLVED in r50727 Aleksandrs Saveljevs Looks good, but please see r50744. RESOLVED. Andris Zeila thanks! CLOSED
        Hide
        Andris Zeila added a comment -

        Server side ready for retesting

        Show
        Andris Zeila added a comment - Server side ready for retesting
        Hide
        Arvids Godjuks (Inactive) added a comment - - edited

        (10) Changes in translation

        Alexander Vladishev 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.

        Ivo Kurzemnieks CLOSED.

        Show
        Arvids Godjuks (Inactive) added a comment - - edited (10) Changes in translation Alexander Vladishev 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. Ivo Kurzemnieks CLOSED.
        Hide
        Alexei Vladishev added a comment - - edited

        (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

        Ivo Kurzemnieks CLOSED.

        Show
        Alexei Vladishev added a comment - - edited (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 Ivo Kurzemnieks CLOSED.
        Hide
        Alexei Vladishev added a comment - - edited

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

        Arvids Godjuks RESOLVED in r50830

        Ivo Kurzemnieks CLOSED.

        Show
        Alexei Vladishev added a comment - - edited (12) Function isValidMask() always returns true, therefore it does not validate anything Arvids Godjuks RESOLVED in r50830 Ivo Kurzemnieks CLOSED.
        Hide
        Alexei Vladishev added a comment - - edited

        (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.

        Ivo Kurzemnieks CLOSED.

        Show
        Alexei Vladishev added a comment - - edited (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. Ivo Kurzemnieks CLOSED.
        Hide
        Ivo Kurzemnieks added a comment - - edited

        (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

        Alexander Vladishev CLOSED

        Show
        Ivo Kurzemnieks added a comment - - edited (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 Alexander Vladishev CLOSED
        Hide
        richlv added a comment - - edited

        subissues still open : 8, 10, 14

        Show
        richlv added a comment - - edited subissues still open : 8 , 10 , 14
        Hide
        Ivo Kurzemnieks added a comment - - edited

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

        RESOLVED in r51088, r51094

        Alexander Vladishev CLOSED

        Show
        Ivo Kurzemnieks added a comment - - edited (15) [F] Added frontend unit tests for IP and IP range validators. RESOLVED in r51088, r51094 Alexander Vladishev CLOSED
        Hide
        Alexander Vladishev added a comment - - edited

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

        Ivo Kurzemnieks RESOLVED in r51199

        Alexander Vladishev CLOSED

        Show
        Alexander Vladishev added a comment - - edited (16) CIPValidator class shouldn't support macros. It is not its task! Ivo Kurzemnieks RESOLVED in r51199 Alexander Vladishev CLOSED
        Hide
        Alexander Vladishev added a comment - - edited

        (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 ) {
        

        Ivo Kurzemnieks RESOLVED in r51199

        Alexander Vladishev CLOSED

        Show
        Alexander Vladishev added a comment - - edited (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 ) { Ivo Kurzemnieks RESOLVED in r51199 Alexander Vladishev CLOSED
        Hide
        Alexander Vladishev added a comment - - edited

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

        Ivo Kurzemnieks Will be removed in (21).
        CLOSED.

        Show
        Alexander Vladishev added a comment - - edited (18) CIPValidator.php , CIPRangeValidator.php - $empty variable should be renamed. May be $allowEmpty ? Ivo Kurzemnieks Will be removed in (21). CLOSED.
        Hide
        Alexander Vladishev added a comment - - edited

        (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.

        Ivo Kurzemnieks RESOLVED in r51199

        Alexander Vladishev CLOSED

        Show
        Alexander Vladishev added a comment - - edited (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. Ivo Kurzemnieks RESOLVED in r51199 Alexander Vladishev CLOSED
        Hide
        Alexander Vladishev added a comment - - edited

        (20) Please review my changes in r51128.

        Alexander Vladishev This changes will be removed under (21). CLOSED

        Show
        Alexander Vladishev added a comment - - edited (20) Please review my changes in r51128. Alexander Vladishev This changes will be removed under (21). CLOSED
        Hide
        Alexander Vladishev added a comment - - edited

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

        Ivo Kurzemnieks RESOLVED in r51199

        Alexander Vladishev CLOSED

        Show
        Alexander Vladishev added a comment - - edited (21) Unused property CIPRangeValidator.empty. CIPValidator.empty should be removed too. Ivo Kurzemnieks RESOLVED in r51199 Alexander Vladishev CLOSED
        Hide
        Alexander Vladishev added a comment - - edited

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

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

        Ivo Kurzemnieks RESOLVED in r51199

        Alexander Vladishev CLOSED

        Show
        Alexander Vladishev added a comment - - edited (22) All regular expressions should be unified. For example: '/^\d{2}$/' => '/^[0-9]{2}$/' Ivo Kurzemnieks RESOLVED in r51199 Alexander Vladishev CLOSED
        Hide
        Alexander Vladishev added a comment - - edited

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

        Ivo Kurzemnieks RESOLVED in r51199

        Alexander Vladishev CLOSED

        Show
        Alexander Vladishev added a comment - - edited (23) isValidRange() should be divided into three functions: isValidRange(), isValidRangeIPv4() and isValidRangeIPv6() Ivo Kurzemnieks RESOLVED in r51199 Alexander Vladishev CLOSED
        Hide
        Alexander Vladishev added a comment - - edited

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

        Ivo Kurzemnieks 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.

        Alexander Vladishev Thanks a lot! CLOSED

        Show
        Alexander Vladishev added a comment - - edited (24) Please review my changes in r51241, r51249. Ivo Kurzemnieks 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. Alexander Vladishev Thanks a lot! CLOSED
        Hide
        Ivo Kurzemnieks added a comment - - edited

        TESTED,

        but don't forget to close (24).

        Show
        Ivo Kurzemnieks added a comment - - edited TESTED, but don't forget to close (24) .
        Hide
        Alexander Vladishev added a comment -

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

        Show
        Alexander Vladishev added a comment - Available in pre-2.4.4 r51286 and pre-2.5.0 (trunk) r51287.
        Show
        Alexander Vladishev added a comment - - edited (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 Valkovskis 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 Alexander Vladishev 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 Valkovskis Looking good. I'll set this as CLOSED.
        Hide
        Aleksandrs Saveljevs added a comment - - edited

        (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
        

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

        Andris Zeila CLOSED

        Show
        Aleksandrs Saveljevs added a comment - - edited (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 Aleksandrs Saveljevs Fixed in development branch svn://svn.zabbix.com/branches/dev/ZBXNEXT-2160 in r51693. RESOLVED. Andris Zeila CLOSED
        Hide
        Aleksandrs Saveljevs added a comment -

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

        Show
        Aleksandrs Saveljevs added a comment - Fix of (26) available in pre-2.4.4 r51695 and pre-2.5.0 (trunk) r51696.

          People

          • Assignee:
            Unassigned
            Reporter:
            Andrey Protsenko
          • Votes:
            3 Vote for this issue
            Watchers:
            9 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved: