Fix /proc/net/route requirement that causes errors on FreeBSD #1638

kawaiivoldemort · 2019-09-18T15:57:29Z

Description

This is a fix for Issue #1479).

What it does is:

Instead of searching the linux specific /proc/net/route in wireserver_route_exists in dhcp.py, it calls the osutil.get_list_of_routes API and checks if the route destination matches the KNOWN_WIRESERVER_IP
It then defines various route related functions in osutil/freebsd.py so that this works on FreeBSD
One of these functions parses netstat -rn and creates RouteEntry objects that look similar enough to the Linux ones for the rest of the code to work

Issue #1479

PR information

The title of the PR is clear and informative.
There are a small number of commits, each of which has an informative message. This means that previously merged commits do not appear in the history of the PR. For information on cleaning up the commits in your pull request, see this page.
Except for special cases involving multiple contributors, the PR is started from a fork of the main repository, not a branch.
If applicable, the PR references the bug/issue that it fixes in the description.
New Unit tests were added for the changes made and Travis.CI is passing.

Quality of Code and Contribution Guidelines

I have read the contribution guidelines.

This change is

msftclas · 2019-09-18T15:57:44Z

All CLA requirements met.

to build_route_list

codecov · 2019-09-19T11:04:36Z

Codecov Report

❗ No coverage uploaded for pull request base (develop@0105f5f). Click here to learn what that means.
The diff coverage is 65.78%.

@@            Coverage Diff             @@
##             develop    #1638   +/-   ##
==========================================
  Coverage           ?   67.19%           
==========================================
  Files              ?       80           
  Lines              ?    11440           
  Branches           ?     1608           
==========================================
  Hits               ?     7687           
  Misses             ?     3420           
  Partials           ?      333

Impacted Files	Coverage Δ
azurelinuxagent/common/dhcp.py	`30.37% <100%> (ø)`
azurelinuxagent/common/osutil/freebsd.py	`42.67% <65.33%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0105f5f...08ba686. Read the comment docs.

- Moved route conversion code from read_route_table to build_route_list - Fixed some typos - Fixed some routes being ignored

…to develop

kawaiivoldemort · 2019-09-19T12:44:03Z

1ee6e01 is just a squish of all the other commits. It has all the changes.

larohra

Thank you for your contribution @onenachi. Could you please add some test cases for the changes in freebsd.py file? We have a test file \tests\common\osutil\test_freebsd.py where you can add the specific tests for your changes to improve the code coverage.

I've also initiated a run from our automated end-to-end testing just to ensure the changes are not breaking anything.

Apart from that the changes look good to me.

larohra · 2019-10-04T17:48:30Z

azurelinuxagent/common/osutil/freebsd.py

+            # Add the route
+            linux_style_route_file.append("{0}\t{1}\t{2}\t{3}\t{4}\t{5}\t{6}\t{7}\t{8}\t{9}\t{10}".format(
+                    netif, dest, gw, flags, refcount, use, metric, mask, mtu, window, irtt))
+        return linux_style_route_file


The original implementation of this function handles all errors in function and doesn't propagate any exception further.

try: with open('/proc/net/route') as routing_table: return list(map(str.strip, routing_table.readlines())) except Exception as e: logger.error("Cannot read route table [{0}]", ustr(e)) return []

After you refactor this function, could you please ensure that the read_route_table function doesn't raise any exception so as to not break the agent

done with 515b672

larohra · 2019-10-04T17:49:15Z

tests/common/osutil/test_freebsd.py

@@ -29,6 +31,103 @@ def tearDown(self):
    def test_get_dhcp_pid_should_return_a_list_of_pids(self):
        osutil_get_dhcp_pid_should_return_a_list_of_pids(self, FreeBSDOSUtil())

+    def test_empty_proc_net_route(self):


Thank you for adding the tests 👍

larohra

Left some comments around refactoring and exception handling.

larohra · 2019-10-04T19:23:59Z

azurelinuxagent/common/osutil/freebsd.py

+            return linux_style_route_file
+        netstat_output = netstat_output[3:]
+        # Parse the Netstat -RN header line
+        n_columns = 0


Could you add a comment with a sample output of netstat -rn is to make it easier for the reader to understand the parsing logic

Exmples added as requested in 10819af

larohra · 2019-10-04T19:27:48Z

azurelinuxagent/common/osutil/freebsd.py

+
+        :return: Entries in the route priority table from `netstat -rn`
+        :rtype: list(str)
+        """


Could you please add an example of how the netstat -rn output differs with the linux based proc/net/route header just to make sure what this function is parsing

Exmples added as requested in 10819af

- refactored it into meaningful subfunctions - modified the function documentation to make it more understandable - added examples to the function documentation

larohra

@narrieta this LGTM, can you please take a look

narrieta · 2019-10-08T20:17:27Z

azurelinuxagent/common/dhcp.py

-            if os.path.exists(route_file) and \
-                    KNOWN_WIRESERVER_IP_ENTRY in open(route_file).read():
+            route_table = self.osutil.read_route_table()
+            if any([(KNOWN_WIRESERVER_IP_ENTRY in route) for route in route_table]):


In the original code there was a check for exists(route_file); if it was false, we would output a warning.
In the new code this would be reported as an error.

I'm not sure how to work around this. read_route_table will log an error by default and then return an empty list if /proc/net/route doesn't exist.

The only thing it guarantees is not to raise an exception.

Im not sure changing that function would be appropriate. I do think using that function would be appropriate since checking route is clearly a platform specific task.

I also do think that this is an edge case and not really an issue as it will only log an error on systems without /proc/net/route (non Linux systems) which have not overridden that function with their own osutil (so now, neither Linux nor FreeBSD).

narrieta

requestion 1 small change, otherwise looks good

narrieta · 2019-10-10T16:25:34Z

azurelinuxagent/common/osutil/freebsd.py

+            :rtype: list(dict)
+            """
+            cmd = "netstat -rn -f inet"
+            ret, output = shellutil.run_get_output(cmd)


we are deprecating shellutil.run_get_output in favor of shellutil.run_command, could you change this?

Done with a0112e2 and 72bddb7

narrieta · 2019-10-16T16:52:11Z

azurelinuxagent/common/osutil/freebsd.py

+            :rtype: list(dict)
+            """
+            cmd = "netstat -rn -f inet"
+            output = shellutil.run_command(cmd, log_error=True)


thank you for the update.

run_command sets shell=False when invoking subprocess.Popen, so the command needs to be passed as an array of strings; see, for example https://github.com/Azure/WALinuxAgent/blob/develop/azurelinuxagent/common/utils/cryptutil.py#L55

Also, please be sure to tests this end-to-end on your side. Our test infrastructure does not validate FreeBSD.

Thanks

Fixed in 08ba686

Tested in FreeBSD 11.2 on Azure:

Route table is read successfully.

Sorry about that.

narrieta · 2019-10-21T18:19:50Z

LGTM; running automation - will approve and merge after that

narrieta · 2019-10-23T15:27:30Z

Automation OK

narrieta

Thank you!

Azure/WALinuxAgent#1638 - Update WWW to where the code exists - Switch to use Python 3 to match the ports' default to avoid some subprocess invoking issues Approved by: Wei Hu <weh@microsoft.com> (maintainer, implicitly) Sponsored by: The FreeBSD Foundation git-svn-id: svn+ssh://svn.freebsd.org/ports/head@517374 35697150-7ecd-e111-bb59-0022644237b5

Azure/WALinuxAgent#1638 - Update WWW to where the code exists - Switch to use Python 3 to match the ports' default to avoid some subprocess invoking issues Approved by: Wei Hu <weh@microsoft.com> (maintainer, implicitly) Sponsored by: The FreeBSD Foundation

Azure/WALinuxAgent#1638 - Update WWW to where the code exists - Switch to use Python 3 to match the ports' default to avoid some subprocess invoking issues Approved by: Wei Hu <weh@microsoft.com> (maintainer, implicitly) Sponsored by: The FreeBSD Foundation git-svn-id: svn+ssh://svn.freebsd.org/ports/head@517374 35697150-7ecd-e111-bb59-0022644237b5

Azure/WALinuxAgent#1638 - Update WWW to where the code exists - Switch to use Python 3 to match the ports' default to avoid some subprocess invoking issues Approved by: Wei Hu <weh@microsoft.com> (maintainer, implicitly) Sponsored by: The FreeBSD Foundation

- Update to 2.2.45 (pre-release) to pull in the requried fix Azure/WALinuxAgent#1638 - Update WWW to where the code exists - Switch to use Python 3 to match the ports' default to avoid some subprocess invoking issues Approved by: Wei Hu <weh@microsoft.com> (maintainer, implicitly) Sponsored by: The FreeBSD Foundation Approved by: ports-secteam (blanket(s): bugfix release)

- Fix /proc/net/route requirement on FreeBSD

db21ecf

kawaiivoldemort requested review from larohra, narrieta, pgombar and vrdmr as code owners September 18, 2019 15:57

kawaiivoldemort changed the title ~~Fix /proc/net/route requirement on FreeBSD (to fix https://github.com/Azure/WALinuxAgent/issues/1479)~~ Fix /proc/net/route requirement that causes errors on FreeBSD Sep 18, 2019

Moved route conversion code from read_route_table

c98c917

to build_route_list

Nachiketa Ramesh added 4 commits September 19, 2019 17:20

- Fixed some typos

ceeb1b1

- Fixed some routes being ignored

3b7b987

- Fix /proc/net/route requirement on FreeBSD

1ee6e01

- Moved route conversion code from read_route_table to build_route_list - Fixed some typos - Fixed some routes being ignored

Merge branch 'develop' of https://github.com/onenachi/WALinuxAgent in…

3d17625

…to develop

vrdmr assigned larohra Sep 26, 2019

larohra suggested changes Oct 2, 2019

View reviewed changes

kawaiivoldemort added 14 commits October 3, 2019 17:44

- Added Unit Tests

8a84b14

- Fixed the patch

a245b2d

- Fixed using osutil instead of FreeBSD osutil

12aaffb

- Patched the right function

9e24775

- Fixed the route table

2e5eee2

- fixed zero length field name in python 2.6

92a1fd7

- Fixed the routes in the valid test

8f221c4

- More fixes for valid routes

9e4b64a

- Fixed some IP parsing

6ae71f7

- Fixed the parsing of the ipv4 addresses

b086c46

- Fixed some typos (comma placement and params)

766815b

- FIxed a typo

f79cf03

- Ordering of netmask check

db17060

- Changed subnet in test

c62829a

larohra reviewed Oct 4, 2019

View reviewed changes

larohra suggested changes Oct 4, 2019

View reviewed changes

- added exception handling to read_route_table in freebsd.py

515b672

larohra reviewed Oct 4, 2019

View reviewed changes

Nachiketa Ramesh added 2 commits October 7, 2019 16:58

- maked netstat parsing code more readable

10819af

- refactored it into meaningful subfunctions - modified the function documentation to make it more understandable - added examples to the function documentation

- Fixed the FreeBSD tests

e9d7a64

kawaiivoldemort requested a review from larohra October 7, 2019 11:37

larohra reviewed Oct 7, 2019

View reviewed changes

narrieta reviewed Oct 8, 2019

View reviewed changes

narrieta requested changes Oct 10, 2019

View reviewed changes

Nachiketa Ramesh added 2 commits October 15, 2019 14:47

moved from run_get_output to run_command in _get_netstat_rn_ipv4_routes

a0112e2

- Fixed the freebsd tests to patch run_command

72bddb7

kawaiivoldemort requested a review from narrieta October 16, 2019 09:55

narrieta reviewed Oct 16, 2019

View reviewed changes

- Fixed run_command in freebsd.py

08ba686

kawaiivoldemort requested a review from narrieta October 21, 2019 03:29

narrieta approved these changes Oct 23, 2019

View reviewed changes

larohra approved these changes Oct 23, 2019

View reviewed changes

narrieta merged commit f2af566 into Azure:develop Oct 23, 2019

SystemsUK mentioned this pull request Jan 22, 2020

Deprovision RHEL 7.7 using 2.2.45 loops with ERROR Daemon /proc/net/route contains no routes #1764

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix /proc/net/route requirement that causes errors on FreeBSD #1638

Fix /proc/net/route requirement that causes errors on FreeBSD #1638

kawaiivoldemort commented Sep 18, 2019 •

edited

Loading

msftclas commented Sep 18, 2019 •

edited

Loading

codecov bot commented Sep 19, 2019 •

edited

Loading

kawaiivoldemort commented Sep 19, 2019 •

edited

Loading

larohra left a comment

larohra Oct 4, 2019

kawaiivoldemort Oct 4, 2019 •

edited

Loading

larohra Oct 4, 2019

larohra left a comment

larohra Oct 4, 2019

kawaiivoldemort Oct 7, 2019 •

edited

Loading

larohra Oct 4, 2019

kawaiivoldemort Oct 7, 2019 •

edited

Loading

larohra left a comment

narrieta Oct 8, 2019

kawaiivoldemort Oct 9, 2019

kawaiivoldemort Oct 9, 2019 •

edited

Loading

narrieta Oct 10, 2019

narrieta left a comment

narrieta Oct 10, 2019

kawaiivoldemort Oct 15, 2019

narrieta Oct 16, 2019 •

edited

Loading

kawaiivoldemort Oct 21, 2019 •

edited

Loading

narrieta commented Oct 21, 2019

narrieta commented Oct 23, 2019

narrieta left a comment

Fix /proc/net/route requirement that causes errors on FreeBSD #1638

Fix /proc/net/route requirement that causes errors on FreeBSD #1638

Conversation

kawaiivoldemort commented Sep 18, 2019 • edited Loading

Description

PR information

Quality of Code and Contribution Guidelines

msftclas commented Sep 18, 2019 • edited Loading

codecov bot commented Sep 19, 2019 • edited Loading

Codecov Report

kawaiivoldemort commented Sep 19, 2019 • edited Loading

larohra left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kawaiivoldemort Oct 4, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

larohra left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kawaiivoldemort Oct 7, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kawaiivoldemort Oct 7, 2019 • edited Loading

Choose a reason for hiding this comment

larohra left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kawaiivoldemort Oct 9, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

narrieta left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

narrieta Oct 16, 2019 • edited Loading

Choose a reason for hiding this comment

kawaiivoldemort Oct 21, 2019 • edited Loading

Choose a reason for hiding this comment

narrieta commented Oct 21, 2019

narrieta commented Oct 23, 2019

narrieta left a comment

Choose a reason for hiding this comment

kawaiivoldemort commented Sep 18, 2019 •

edited

Loading

msftclas commented Sep 18, 2019 •

edited

Loading

codecov bot commented Sep 19, 2019 •

edited

Loading

kawaiivoldemort commented Sep 19, 2019 •

edited

Loading

kawaiivoldemort Oct 4, 2019 •

edited

Loading

kawaiivoldemort Oct 7, 2019 •

edited

Loading

kawaiivoldemort Oct 7, 2019 •

edited

Loading

kawaiivoldemort Oct 9, 2019 •

edited

Loading

narrieta Oct 16, 2019 •

edited

Loading

kawaiivoldemort Oct 21, 2019 •

edited

Loading