Re-do "fix: check status created a machine-id file (#3965)" (#4032)

- #3965 was reverted in #4018 due to a false positive test failure
  This PR is going to re-do the #3906 by reverting the #4018.
  Hopes all things could be self-linked in this way.

This reverts commit 39f95b1.

Signed-off-by: Xiangce Liu <[email protected]>
(cherry picked from commit b5a055d)

insights/client/connection.py

@@ -25,6 +25,7 @@
     from urllib.parse import quote
 from .utilities import (determine_hostname,
                         generate_machine_id,
+                        machine_id_exists,
                         write_unregistered_file,
                         write_registered_file,
                         os_release_info,
@@ -622,6 +623,8 @@ def _legacy_api_registration_check(self):
             string system is unregistered
         '''
         logger.debug('Checking registration status...')
+        if not machine_id_exists():
+            return None
         machine_id = generate_machine_id()
         try:
             url = self.api_url + '/v1/systems/' + machine_id
@@ -671,6 +674,9 @@ def _fetch_system_by_machine_id(self):
             False   system does not exist in inventory
             None    error connection or parsing response
         '''
+
+        if not machine_id_exists():
+            return False
         machine_id = generate_machine_id()
         try:
             # [circus music]

insights/client/utilities.py

@@ -127,7 +127,6 @@ def generate_machine_id(new=False,
         machine_id = str(uuid.uuid4())
         logger.debug("Creating %s", destination_file)
         write_to_disk(destination_file, content=machine_id)
-
     try:
         return str(uuid.UUID(str(machine_id).strip(), version=4))
     except ValueError as e:
@@ -137,6 +136,13 @@ def generate_machine_id(new=False,
         sys.exit(constants.sig_kill_bad)
 
 
+def machine_id_exists(destination_file=constants.machine_id_file):
+    """
+    Get the machine-id or None if /etc/insights-client/machine-id it does not exists
+    """
+    return os.path.isfile(destination_file)
+
+
 def _expand_paths(path):
     """
     Expand wildcarded paths

insights/tests/client/connection/test_LEGACY_reg_check.py

@@ -5,9 +5,10 @@
 
 
 @patch("insights.client.connection.generate_machine_id", return_value='xxxxxx')
+@patch("insights.client.connection.machine_id_exists", return_value=True)
 @patch("insights.client.connection.InsightsConnection._init_session")
 @patch("insights.client.connection.InsightsConnection.get_proxies")
-def test_registration_check_ok_reg(get_proxies, _init_session, _):
+def test_registration_check_ok_reg(get_proxies, _init_session, _machine_id_exists, _generate_machine_id):
     '''
     Request completed OK, registered
         Returns True
@@ -24,9 +25,10 @@ def test_registration_check_ok_reg(get_proxies, _init_session, _):
 
 
 @patch("insights.client.connection.generate_machine_id", return_value='xxxxxx')
+@patch("insights.client.connection.machine_id_exists", return_value=True)
 @patch("insights.client.connection.InsightsConnection._init_session")
 @patch("insights.client.connection.InsightsConnection.get_proxies")
-def test_registration_check_ok_reg_then_unreg(get_proxies, _init_session, _):
+def test_registration_check_ok_reg_then_unreg(get_proxies, _init_session, _machine_id_exists, _generate_machine_id):
     '''
     Request completed OK, was once registered but has been unregistered
         Returns the date it was unregistered
@@ -42,10 +44,10 @@ def test_registration_check_ok_reg_then_unreg(get_proxies, _init_session, _):
     assert conn.api_registration_check() == '2019-04-10'
 
 
-@patch("insights.client.connection.generate_machine_id", return_value='xxxxxx')
+@patch("insights.client.connection.machine_id_exists", return_value=True)
 @patch("insights.client.connection.InsightsConnection._init_session")
 @patch("insights.client.connection.InsightsConnection.get_proxies")
-def test_registration_check_ok_unreg(get_proxies, _init_session, _):
+def test_registration_check_ok_unreg(get_proxies, _init_session, _machine_id_exists):
     '''
     Request completed OK, has never been registered
         Returns None
@@ -62,9 +64,10 @@ def test_registration_check_ok_unreg(get_proxies, _init_session, _):
 
 
 @patch("insights.client.connection.generate_machine_id", return_value='xxxxxx')
+@patch("insights.client.connection.machine_id_exists", return_value=True)
 @patch("insights.client.connection.InsightsConnection._init_session")
 @patch("insights.client.connection.InsightsConnection.get_proxies")
-def test_registration_check_bad_res(get_proxies, _init_session, _):
+def test_registration_check_bad_res(get_proxies, _init_session, _machine_id_exists, _generate_machine_id):
     '''
     Can't parse response
         Returns False
@@ -81,10 +84,11 @@ def test_registration_check_bad_res(get_proxies, _init_session, _):
 
 
 @patch("insights.client.connection.generate_machine_id", return_value='xxxxxx')
+@patch("insights.client.connection.machine_id_exists", return_value=True)
 @patch("insights.client.connection.InsightsConnection._init_session")
 @patch("insights.client.connection.InsightsConnection.get_proxies")
 @patch("insights.client.connection.InsightsConnection.test_connection")
-def test_registration_check_conn_error(test_connection, get_proxies, _init_session, _):
+def test_registration_check_conn_error(test_connection, get_proxies, _init_session, _machine_id_exists, _generate_machine_id):
     '''
     Can't connect, run connection test
         Returns False

insights/tests/client/connection/test_reg_check.py

@@ -5,9 +5,10 @@
 
 
 @patch("insights.client.connection.generate_machine_id", return_value='xxxxxx')
+@patch("insights.client.connection.machine_id_exists", return_value=True)
 @patch("insights.client.connection.InsightsConnection._init_session")
 @patch("insights.client.connection.InsightsConnection.get_proxies")
-def test_registration_check_ok_reg(get_proxies, _init_session, _):
+def test_registration_check_ok_reg(get_proxies, _init_session, _machine_id_exists, _generate_machine_id):
     '''
     Request completed OK, registered
         Returns True
@@ -35,10 +36,10 @@ def test_registration_check_ok_reg(get_proxies, _init_session, _):
     assert conn.api_registration_check()
 
 
-@patch("insights.client.connection.generate_machine_id", return_value='xxxxxx')
+@patch("insights.client.connection.machine_id_exists", return_value=False)
 @patch("insights.client.connection.InsightsConnection._init_session")
 @patch("insights.client.connection.InsightsConnection.get_proxies")
-def test_registration_check_ok_unreg(get_proxies, _init_session, _):
+def test_registration_check_ok_unreg(get_proxies, _init_session, _machine_id_exists):
     '''
     Request completed OK, has not been registered
         Returns False
@@ -62,9 +63,10 @@ def test_registration_check_ok_unreg(get_proxies, _init_session, _):
 
 
 @patch("insights.client.connection.generate_machine_id", return_value='xxxxxx')
+@patch("insights.client.connection.machine_id_exists", return_value=True)
 @patch("insights.client.connection.InsightsConnection._init_session")
 @patch("insights.client.connection.InsightsConnection.get_proxies")
-def test_registration_check_parse_error(get_proxies, _init_session, _):
+def test_registration_check_parse_error(get_proxies, _init_session, _machine_id_exists, _generate_machine_id):
     '''
     Can't parse response
         Returns None
@@ -81,9 +83,10 @@ def test_registration_check_parse_error(get_proxies, _init_session, _):
 
 
 @patch("insights.client.connection.generate_machine_id", return_value='xxxxxx')
+@patch("insights.client.connection.machine_id_exists", return_value=True)
 @patch("insights.client.connection.InsightsConnection._init_session")
 @patch("insights.client.connection.InsightsConnection.get_proxies")
-def test_registration_check_bad_res(get_proxies, _init_session, _):
+def test_registration_check_bad_res(get_proxies, _init_session, _machine_id_exists, _generate_machine_id):
     '''
     Failure HTTP response
         Returns None
@@ -100,9 +103,10 @@ def test_registration_check_bad_res(get_proxies, _init_session, _):
 
 
 @patch("insights.client.connection.generate_machine_id", return_value='xxxxxx')
+@patch("insights.client.connection.machine_id_exists", return_value=True)
 @patch("insights.client.connection.InsightsConnection._init_session")
 @patch("insights.client.connection.InsightsConnection.get_proxies")
-def test_registration_check_conn_error(get_proxies, _init_session, _):
+def test_registration_check_conn_error(get_proxies, _init_session, _machine_id_exists, _generate_machine_id):
     '''
     Connection error
         Returns None

insights/tests/client/support/test_collect_support_info.py

@@ -112,15 +112,18 @@ def test_registration_check_registered_unreach(_, __):
     conn.api_registration_check.assert_called_once()
 
 
-@patch('insights.client.connection.generate_machine_id', return_value="xxxx-xxx-xxxx-xxx")
+@patch("insights.client.connection.generate_machine_id", return_value='xxxxxx')
+@patch("insights.client.connection.machine_id_exists", return_value=True)
 @patch("insights.client.connection.InsightsConnection._init_session")
 @patch("insights.client.connection.InsightsConnection.get_proxies")
 @patch("insights.client.connection.InsightsConnection.get", return_value=Mock(status_code=404, content='{"detail": "System with insights_id ID not found"}'))
 @patch('insights.client.support.write_registered_file')
 @patch('insights.client.support.write_unregistered_file')
 @patch('insights.client.support.write_to_disk')
-def test_registration_check_legacy_unregistered_good_json(write_to_disk, write_unregistered_file, write_registered_file, _geturl, __, ___, _generate_machine_id):
+def test_registration_check_legacy_unregistered_good_json(write_to_disk, write_unregistered_file, write_registered_file, _geturl, _proxies, _init_session, _machine_id_exists, _generate_machine_id):
     '''
+    In this test insights-client has a machine-id file but is not registered in server,
+    it checks if insights-client write the unregistered files when it notices is not registered.
     Ensure that connection function is called and data processed.
     When the system is not registered the server sends a 404 error with a json content.
     Check that when the client get this response the registered file and the machine-id files
@@ -136,14 +139,15 @@ def test_registration_check_legacy_unregistered_good_json(write_to_disk, write_u
     write_to_disk.assert_called_once()
 
 
-@patch('insights.client.connection.generate_machine_id', return_value="xxxx-xxx-xxxx-xxx")
+@patch("insights.client.connection.generate_machine_id", return_value='xxxxxx')
+@patch("insights.client.connection.machine_id_exists", return_value=True)
 @patch("insights.client.connection.InsightsConnection._init_session")
 @patch("insights.client.connection.InsightsConnection.get_proxies")
 @patch("insights.client.connection.InsightsConnection.get", return_value=Mock(status_code=404, content='{Page Not Found}'))
 @patch('insights.client.support.write_registered_file')
 @patch('insights.client.support.write_unregistered_file')
 @patch('insights.client.support.write_to_disk')
-def test_registration_check_legacy_unregistered_bad_json(write_to_disk, write_unregistered_file, write_registered_file, _geturl, __, ___, _generate_machine_id):
+def test_registration_check_legacy_unregistered_bad_json(write_to_disk, write_unregistered_file, write_registered_file, _geturl, _proxies, _init_session, _machine_id_exists, _generate_machine_id):
     '''
     Ensure that connection function is called and data processed.
     Check a 404 from a forward that doesn't contain a json a content.
@@ -159,15 +163,18 @@ def test_registration_check_legacy_unregistered_bad_json(write_to_disk, write_un
     write_to_disk.assert_not_called()
 
 
-@patch('insights.client.connection.generate_machine_id', return_value="xxxx-xxx-xxxx-xxx")
+@patch("insights.client.connection.generate_machine_id", return_value='xxxxxx')
+@patch("insights.client.connection.machine_id_exists", return_value=True)
 @patch("insights.client.connection.InsightsConnection._init_session")
 @patch("insights.client.connection.InsightsConnection.get_proxies")
 @patch("insights.client.connection.InsightsConnection.get", return_value=Mock(status_code=200, content='{"unregistered_at": "2019-04-10"}'))
 @patch('insights.client.support.write_registered_file')
 @patch('insights.client.support.write_unregistered_file')
 @patch('insights.client.support.write_to_disk')
-def test_registration_check_legacy_registered_then_unregistered(write_to_disk, write_unregistered_file, write_registered_file, _geturl, _, __, _generate_machine_id):
+def test_registration_check_legacy_registered_then_unregistered(write_to_disk, write_unregistered_file, write_registered_file, _geturl, _proxies, _init_session, _machine_id_exists, _generate_machine_id):
     '''
+    This test checks if insights-client removes the registration file when it notices it is not
+    registered through the API but has a machine-id file.
     Ensure that connection function is called and data processed.
     Legacy version responded with the unregistered_at as a json parameter.
     Check that when the client get this response the registered file and the machine-id files
@@ -182,13 +189,14 @@ def test_registration_check_legacy_registered_then_unregistered(write_to_disk, w
     write_unregistered_file.assert_called_once()
 
 
-@patch('insights.client.connection.generate_machine_id', return_value="xxxx-xxx-xxxx-xxx")
+@patch("insights.client.connection.generate_machine_id", return_value='xxxxxx')
+@patch("insights.client.connection.machine_id_exists", return_value=True)
 @patch("insights.client.connection.InsightsConnection._init_session")
 @patch("insights.client.connection.InsightsConnection.get_proxies")
 @patch("insights.client.connection.InsightsConnection.get", return_value=Mock(status_code=200, content='{"unregistered_at": null}'))
 @patch('insights.client.support.write_registered_file')
 @patch('insights.client.support.write_unregistered_file')
-def test_registration_check_legacy_registered(write_unregistered_file, write_registered_file, _geturl, _proxy, _session, _generate_machine_id):
+def test_registration_check_legacy_registered(write_unregistered_file, write_registered_file, _geturl, _proxy, _session, _machine_id_exists, _generate_machine_id):
     '''
     Ensure that connection function is called and data processed.
     When the systems is registered it get a 200 message with a json with the unregistered_at=null
@@ -204,14 +212,15 @@ def test_registration_check_legacy_registered(write_unregistered_file, write_reg
     write_unregistered_file.assert_not_called()
 
 
-@patch('insights.client.connection.generate_machine_id', return_value="xxxx-xxx-xxxx-xxx")
+@patch("insights.client.connection.generate_machine_id", return_value='xxxxxx')
+@patch("insights.client.connection.machine_id_exists", return_value=True)
 @patch("insights.client.connection.InsightsConnection._init_session")
 @patch("insights.client.connection.InsightsConnection.get_proxies")
 @patch("insights.client.connection.InsightsConnection.get", return_value=Mock(status_code=502, content='zSDFasfghsRGH'))
 @patch('insights.client.support.write_registered_file')
 @patch('insights.client.support.write_unregistered_file')
 @patch('insights.client.support.write_to_disk')
-def test_registration_check_legacy_bad_json(write_to_disk, write_unregistered_file, write_registered_file, _geturl, _, __, _generate_machine_id):
+def test_registration_check_legacy_bad_json(write_to_disk, write_unregistered_file, write_registered_file, _geturl, _proxy, _session, _machine_id_exists, _generate_machine_id):
     '''
     Ensure the function does not remove any file if a network error is encountered
     '''
@@ -226,14 +235,15 @@ def test_registration_check_legacy_bad_json(write_to_disk, write_unregistered_fi
     write_to_disk.assert_not_called()
 
 
-@patch('insights.client.connection.generate_machine_id', return_value="xxxx-xxx-xxxx-xxx")
+@patch("insights.client.connection.generate_machine_id", return_value='xxxxxx')
+@patch("insights.client.connection.machine_id_exists", return_value=True)
 @patch("insights.client.connection.InsightsConnection._init_session")
 @patch("insights.client.connection.InsightsConnection.get_proxies")
 @patch("insights.client.connection.InsightsConnection.get", return_value=Mock(status_code=502, content='{"details": "Bad gateway"}'))
 @patch('insights.client.support.write_registered_file')
 @patch('insights.client.support.write_unregistered_file')
 @patch('insights.client.support.write_to_disk')
-def test_registration_check_legacy_bad_connection(write_to_disk, write_unregistered_file, write_registered_file, _geturl, _, __, _generate_machine_id):
+def test_registration_check_legacy_bad_connection(write_to_disk, write_unregistered_file, write_registered_file, _geturl, _proxy, _session, _machine_id_exists, _generate_machine_id):
     '''
     Ensure the function does not remove any file if a network error is encountered
     '''

insights/tests/client/test_utilities.py

@@ -76,7 +76,10 @@ def test_write_to_disk_with_broken_path():
     os.remove(filename)
 
 
-def test_generate_machine_id():
+def test_get_machine_id():
+    """
+    Test get machine_id with generate_machine_id method, if the machine-id file exists
+    """
     machine_id_regex = re.match('\w{8}-\w{4}-\w{4}-\w{4}-\w{12}',
                                 util.generate_machine_id(destination_file='/tmp/testmachineid'))
     assert machine_id_regex.group(0) is not None
@@ -86,7 +89,7 @@ def test_generate_machine_id():
     os.remove('/tmp/testmachineid')
 
 
-def test_generate_machine_id_with_non_hyphen_id():
+def test_get_machineid_with_non_hyphen_id():
     content = '86f6f5fad8284730b708a2e44ba5c14a'
     filename = '/tmp/testmachineid'
     util.write_to_disk(filename, content=content)