From 673fb279547f37e55dbad1468f2ab354b0ee4db5 Mon Sep 17 00:00:00 2001 From: Andreas Strikos Date: Fri, 11 Dec 2015 16:10:20 +0100 Subject: [PATCH 1/8] a bit of pep8 --- ripe/atlas/sagan/traceroute.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ripe/atlas/sagan/traceroute.py b/ripe/atlas/sagan/traceroute.py index ee47969..abd86e5 100644 --- a/ripe/atlas/sagan/traceroute.py +++ b/ripe/atlas/sagan/traceroute.py @@ -26,7 +26,7 @@ def __init__(self, data, **kwargs): self.version = self.ensure("version", int) self.rfc4884 = self.ensure("rfc4884", bool) - self.objects = self.ensure("obj", list) + self.objects = self.ensure("obj", list) class Packet(ParsingDict): From 239a51e66a31da6c1f08f6c4ef1593f94a5f2b27 Mon Sep 17 00:00:00 2001 From: Andreas Strikos Date: Fri, 11 Dec 2015 16:11:16 +0100 Subject: [PATCH 2/8] if any packet of last hop has error in it mark last_hop_responded as False --- ripe/atlas/sagan/traceroute.py | 2 +- tests/traceroute.py | 12 ++++++++++++ 2 files changed, 13 insertions(+), 1 deletion(-) diff --git a/ripe/atlas/sagan/traceroute.py b/ripe/atlas/sagan/traceroute.py index abd86e5..84ce53d 100644 --- a/ripe/atlas/sagan/traceroute.py +++ b/ripe/atlas/sagan/traceroute.py @@ -170,7 +170,7 @@ def last_hop_responded(self): self._last_hop_responded = False if self.hops and self.hops[-1].packets: for packet in self.hops[-1].packets: - if packet.origin: + if packet.origin and not packet.is_error: self._last_hop_responded = True break diff --git a/tests/traceroute.py b/tests/traceroute.py index 8be8649..af4d182 100644 --- a/tests/traceroute.py +++ b/tests/traceroute.py @@ -380,3 +380,15 @@ def test_traceroute_with_late_packets(): # Newer firmwares # No sample available yet + + +def test_last_responded_hop_with_error_packets(): + """" + Case where traceroute has a single hop and all of its packets have error + in them. + """ + + result = Result.get({'lts': 117, 'msm_id': 3059023, 'endtime': 1449152831, 'from': '2001:470:1883:1:6666:b3ff:feb0:dcda', 'dst_name': '2001:980:1284:10:beee:7bff:fe87:9eda', 'fw': 4720, 'timestamp': 1449152831, 'proto': 'ICMP', 'paris_id': 1, 'prb_id': 14468, 'af': 6, 'result': [{'result': [{'rtt': 0.714, 'size': 96, 'from': '2001:470:1883:1::1', 'err': 'N', 'ttl': 64}, {'rtt': 0.495, 'size': 96, 'from': '2001:470:1883:1::1', 'err': 'N', 'ttl': 64}, {'rtt': 0.48, 'size': 96, 'from': '2001:470:1883:1::1', 'err': 'N', 'ttl': 64}], 'hop': 1}], 'dst_addr': '2001:980:1284:10:beee:7bff:fe87:9eda', 'src_addr': '2001:470:1883:1:6666:b3ff:feb0:dcda', 'group_id': 3059023, 'type': 'traceroute', 'msm_name': 'Traceroute', 'size': 48}) + + assert(len(result.hops[0].packets) == 3) + assert(result.last_hop_responded is False) From a427bba54ac14d8d72ae2d11844f4a407bbab758 Mon Sep 17 00:00:00 2001 From: Andreas Strikos Date: Fri, 11 Dec 2015 16:53:55 +0100 Subject: [PATCH 3/8] change logic of when we consider last_hop_responded true --- ripe/atlas/sagan/traceroute.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ripe/atlas/sagan/traceroute.py b/ripe/atlas/sagan/traceroute.py index 84ce53d..aca2d4a 100644 --- a/ripe/atlas/sagan/traceroute.py +++ b/ripe/atlas/sagan/traceroute.py @@ -170,7 +170,7 @@ def last_hop_responded(self): self._last_hop_responded = False if self.hops and self.hops[-1].packets: for packet in self.hops[-1].packets: - if packet.origin and not packet.is_error: + if packet.rtt: self._last_hop_responded = True break From 8a593b69a81f2ab62748668504da9e7a2688a77e Mon Sep 17 00:00:00 2001 From: Andreas Strikos Date: Fri, 11 Dec 2015 16:54:41 +0100 Subject: [PATCH 4/8] added another property declaring if traceroute finished successfully --- ripe/atlas/sagan/traceroute.py | 16 ++++++++++++++++ tests/traceroute.py | 18 +++++++++++++++++- 2 files changed, 33 insertions(+), 1 deletion(-) diff --git a/ripe/atlas/sagan/traceroute.py b/ripe/atlas/sagan/traceroute.py index aca2d4a..4f7f8b0 100644 --- a/ripe/atlas/sagan/traceroute.py +++ b/ripe/atlas/sagan/traceroute.py @@ -129,6 +129,7 @@ def __init__(self, data, **kwargs): # Used by a few response tests below self._destination_ip_responded = None self._last_hop_responded = None + self._is_success = None @property def last_rtt(self): @@ -176,6 +177,21 @@ def last_hop_responded(self): return self.last_hop_responded + @property + def is_success(self): + + if self._is_success is not None: + return self._is_success + + self._is_success = False + if self.hops and self.hops[-1].packets: + for packet in self.hops[-1].packets: + if packet.rtt and not packet.is_error: + self._is_success = True + break + + return self.is_success + @property def end_time_timestamp(self): return timegm(self.end_time.timetuple()) diff --git a/tests/traceroute.py b/tests/traceroute.py index af4d182..0ae948b 100644 --- a/tests/traceroute.py +++ b/tests/traceroute.py @@ -265,6 +265,7 @@ def test_traceroute_4610(): assert(result.last_median_rtt == 273.494) assert(result.destination_ip_responded is False) assert(result.last_hop_responded is False) + assert(result.is_success is False) assert(result.ip_path[3][1] == "137.164.47.14") assert(result.hops[0].packets[0].destination_option_size is None) assert(result.hops[0].packets[0].hop_by_hop_option_size is None) @@ -298,6 +299,7 @@ def test_traceroute_responding_target(): assert(result.last_median_rtt == 217.103) assert(result.destination_ip_responded is True) assert(result.last_hop_responded is True) + assert(result.is_success is True) assert(result.ip_path[3][1] == "68.85.214.113") assert(result.hops[0].packets[0].destination_option_size is None) assert(result.hops[0].packets[0].hop_by_hop_option_size is None) @@ -377,6 +379,7 @@ def test_traceroute_with_late_packets(): assert(len(result.hops[1].packets) == 3) assert(result.hops[1].packets[0].origin is None) assert(result.hops[1].packets[0].rtt is None) + assert(result.is_success is False) # Newer firmwares # No sample available yet @@ -391,4 +394,17 @@ def test_last_responded_hop_with_error_packets(): result = Result.get({'lts': 117, 'msm_id': 3059023, 'endtime': 1449152831, 'from': '2001:470:1883:1:6666:b3ff:feb0:dcda', 'dst_name': '2001:980:1284:10:beee:7bff:fe87:9eda', 'fw': 4720, 'timestamp': 1449152831, 'proto': 'ICMP', 'paris_id': 1, 'prb_id': 14468, 'af': 6, 'result': [{'result': [{'rtt': 0.714, 'size': 96, 'from': '2001:470:1883:1::1', 'err': 'N', 'ttl': 64}, {'rtt': 0.495, 'size': 96, 'from': '2001:470:1883:1::1', 'err': 'N', 'ttl': 64}, {'rtt': 0.48, 'size': 96, 'from': '2001:470:1883:1::1', 'err': 'N', 'ttl': 64}], 'hop': 1}], 'dst_addr': '2001:980:1284:10:beee:7bff:fe87:9eda', 'src_addr': '2001:470:1883:1:6666:b3ff:feb0:dcda', 'group_id': 3059023, 'type': 'traceroute', 'msm_name': 'Traceroute', 'size': 48}) assert(len(result.hops[0].packets) == 3) - assert(result.last_hop_responded is False) + assert(result.last_hop_responded is True) + assert(result.is_success is False) + + +def test_is_success_with_error_packets(): + """" + Case where traceroute has a single hop and all of its packets have error + in them. + """ + + result = Result.get({'lts': 117, 'msm_id': 3059023, 'endtime': 1449152831, 'from': '2001:470:1883:1:6666:b3ff:feb0:dcda', 'dst_name': '2001:980:1284:10:beee:7bff:fe87:9eda', 'fw': 4720, 'timestamp': 1449152831, 'proto': 'ICMP', 'paris_id': 1, 'prb_id': 14468, 'af': 6, 'result': [{'result': [{'rtt': 0.714, 'size': 96, 'from': '2001:470:1883:1::1', 'err': 'N', 'ttl': 64}, {'rtt': 0.495, 'size': 96, 'from': '2001:470:1883:1::1', 'err': 'N', 'ttl': 64}, {'rtt': 0.48, 'size': 96, 'from': '2001:470:1883:1::1', 'err': 'N', 'ttl': 64}], 'hop': 1}], 'dst_addr': '2001:980:1284:10:beee:7bff:fe87:9eda', 'src_addr': '2001:470:1883:1:6666:b3ff:feb0:dcda', 'group_id': 3059023, 'type': 'traceroute', 'msm_name': 'Traceroute', 'size': 48}) + + assert(len(result.hops[0].packets) == 3) + assert(result.is_success is False) From b50927a14b101c962035fcd0eea0437ab0c93151 Mon Sep 17 00:00:00 2001 From: Andreas Strikos Date: Fri, 11 Dec 2015 18:07:37 +0100 Subject: [PATCH 5/8] refactor traceroute class to save some code duplications and be able to add more properties easily --- ripe/atlas/sagan/traceroute.py | 80 ++++++++++++++++------------------ 1 file changed, 37 insertions(+), 43 deletions(-) diff --git a/ripe/atlas/sagan/traceroute.py b/ripe/atlas/sagan/traceroute.py index 4f7f8b0..588d125 100644 --- a/ripe/atlas/sagan/traceroute.py +++ b/ripe/atlas/sagan/traceroute.py @@ -124,12 +124,14 @@ def __init__(self, data, **kwargs): self.hops = [] self.total_hops = 0 self.last_median_rtt = None - self._parse_hops(**kwargs) # Sets hops, last_median_rtt, and total_hops # Used by a few response tests below - self._destination_ip_responded = None - self._last_hop_responded = None - self._is_success = None + self.destination_ip_responded = False + self.last_hop_responded = False + self.is_success = False + self.last_hop_errors = [] + + self._parse_hops(**kwargs) # Sets hops, last_median_rtt, and total_hops @property def last_rtt(self): @@ -146,51 +148,35 @@ def target_responded(self): ) return self.destination_ip_responded - @property - def destination_ip_responded(self): - - if self._destination_ip_responded is not None: - return self._destination_ip_responded - - self._destination_ip_responded = False - if self.hops and self.hops[-1].packets: - destination_address = IP(self.destination_address) - for packet in self.hops[-1].packets: - if packet.origin and destination_address == IP(packet.origin): - self._destination_ip_responded = True - break - - return self.destination_ip_responded - - @property - def last_hop_responded(self): + def set_destination_ip_responded(self, last_hop): - if self._last_hop_responded is not None: - return self._last_hop_responded + destination_address = IP(self.destination_address) + for packet in last_hop.packets: + if packet.origin and destination_address == IP(packet.origin): + self.destination_ip_responded = True + break - self._last_hop_responded = False - if self.hops and self.hops[-1].packets: - for packet in self.hops[-1].packets: - if packet.rtt: - self._last_hop_responded = True - break + def set_last_hop_responded(self, last_hop): - return self.last_hop_responded + for packet in last_hop.packets: + if packet.rtt: + self.last_hop_responded = True + break - @property - def is_success(self): + def set_is_success(self, last_hop): - if self._is_success is not None: - return self._is_success + for packet in last_hop.packets: + if packet.rtt and not packet.is_error: + self.is_success = True + break - self._is_success = False - if self.hops and self.hops[-1].packets: - for packet in self.hops[-1].packets: - if packet.rtt and not packet.is_error: - self._is_success = True - break + def set_last_hop_errors(self, last_hop): - return self.is_success + for packet in last_hop.packets: + if packet.is_error: + print(dir(self)) + self.last_hop_errors.append(packet.error_message) + break @property def end_time_timestamp(self): @@ -215,7 +201,8 @@ def _parse_hops(self, **kwargs): self._handle_malformation("Legacy formats not supported") return - for hop in hops: + hops_number = len(hops) + for index, hop in enumerate(hops): hop = Hop(hop, **kwargs) @@ -225,6 +212,13 @@ def _parse_hops(self, **kwargs): self.hops.append(hop) self.total_hops += 1 + # If last hop set several usefull properties + if index + 1 == hops_number: + self.set_is_success(hop) + self.set_destination_ip_responded(hop) + self.set_last_hop_responded(hop) + self.set_last_hop_errors(hop) + __all__ = ( "TracerouteResult", From ed1bccf3995e990288b988497c6d39ee6069f528 Mon Sep 17 00:00:00 2001 From: Andreas Strikos Date: Mon, 14 Dec 2015 11:11:45 +0100 Subject: [PATCH 6/8] last tweeks;removing prints;add comments;add more tests --- ripe/atlas/sagan/traceroute.py | 15 +++++++-------- tests/traceroute.py | 1 + 2 files changed, 8 insertions(+), 8 deletions(-) diff --git a/ripe/atlas/sagan/traceroute.py b/ripe/atlas/sagan/traceroute.py index 588d125..d6bca96 100644 --- a/ripe/atlas/sagan/traceroute.py +++ b/ripe/atlas/sagan/traceroute.py @@ -149,7 +149,7 @@ def target_responded(self): return self.destination_ip_responded def set_destination_ip_responded(self, last_hop): - + """Sets the flag if destination IP responded.""" destination_address = IP(self.destination_address) for packet in last_hop.packets: if packet.origin and destination_address == IP(packet.origin): @@ -157,26 +157,26 @@ def set_destination_ip_responded(self, last_hop): break def set_last_hop_responded(self, last_hop): - + """Sets the flag if last hop responded.""" for packet in last_hop.packets: if packet.rtt: self.last_hop_responded = True break def set_is_success(self, last_hop): - + """Sets the flag if traceroute result is successfull or not.""" for packet in last_hop.packets: if packet.rtt and not packet.is_error: self.is_success = True break + else: + self.set_last_hop_errors(last_hop) def set_last_hop_errors(self, last_hop): - + """Sets the last hop's errors..""" for packet in last_hop.packets: if packet.is_error: - print(dir(self)) self.last_hop_errors.append(packet.error_message) - break @property def end_time_timestamp(self): @@ -214,10 +214,9 @@ def _parse_hops(self, **kwargs): # If last hop set several usefull properties if index + 1 == hops_number: - self.set_is_success(hop) self.set_destination_ip_responded(hop) self.set_last_hop_responded(hop) - self.set_last_hop_errors(hop) + self.set_is_success(hop) __all__ = ( diff --git a/tests/traceroute.py b/tests/traceroute.py index 0ae948b..348e014 100644 --- a/tests/traceroute.py +++ b/tests/traceroute.py @@ -408,3 +408,4 @@ def test_is_success_with_error_packets(): assert(len(result.hops[0].packets) == 3) assert(result.is_success is False) + assert(result.last_hop_errors == ['Network unreachable', 'Network unreachable', 'Network unreachable']) From 1ca24099bcd84899f011142b59f6d54618989a19 Mon Sep 17 00:00:00 2001 From: Andreas Strikos Date: Mon, 14 Dec 2015 11:16:18 +0100 Subject: [PATCH 7/8] bump version and add changes --- CHANGES.rst | 3 +++ ripe/atlas/sagan/version.py | 2 +- 2 files changed, 4 insertions(+), 1 deletion(-) diff --git a/CHANGES.rst b/CHANGES.rst index 16de957..3ae9aa2 100644 --- a/CHANGES.rst +++ b/CHANGES.rst @@ -1,6 +1,9 @@ Changelog ========= +* 1.1.7 + * Change condition of traceroute's ``last_hop_responded`` flag. + * Add couple of more traceroute's properties. ``is_success`` and ``last_hop_errors`` * 1.1.6 * Fix for `Issue #56`_ a case where the ``qbuf`` value wasn't being properly captured. diff --git a/ripe/atlas/sagan/version.py b/ripe/atlas/sagan/version.py index 1436d8f..bf78826 100644 --- a/ripe/atlas/sagan/version.py +++ b/ripe/atlas/sagan/version.py @@ -1 +1 @@ -__version__ = "1.1.6" +__version__ = "1.1.7" From 3e185daa3df3deb349f740ee5187d5ed620e1a31 Mon Sep 17 00:00:00 2001 From: Andreas Strikos Date: Mon, 14 Dec 2015 11:17:06 +0100 Subject: [PATCH 8/8] add a missing dot --- CHANGES.rst | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGES.rst b/CHANGES.rst index 3ae9aa2..3c309ad 100644 --- a/CHANGES.rst +++ b/CHANGES.rst @@ -3,7 +3,7 @@ Changelog * 1.1.7 * Change condition of traceroute's ``last_hop_responded`` flag. - * Add couple of more traceroute's properties. ``is_success`` and ``last_hop_errors`` + * Add couple of more traceroute's properties. ``is_success`` and ``last_hop_errors``. * 1.1.6 * Fix for `Issue #56`_ a case where the ``qbuf`` value wasn't being properly captured.