From f039f38395285a838dc4dfe89b4307d3130167a5 Mon Sep 17 00:00:00 2001 From: Xiangce Liu Date: Tue, 26 Mar 2024 10:16:23 +0800 Subject: [PATCH] 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 39f95b187330d80ad783e4e42f3f01f7edae47fb. Signed-off-by: Xiangce Liu (cherry picked from commit b5a055dfd32ecb054ea517210b5ae802bc2464a4) --- insights/client/connection.py | 6 ++++ insights/client/utilities.py | 8 ++++- .../connection/test_LEGACY_reg_check.py | 16 +++++---- .../tests/client/connection/test_reg_check.py | 16 +++++---- .../support/test_collect_support_info.py | 34 ++++++++++++------- insights/tests/client/test_utilities.py | 7 ++-- 6 files changed, 60 insertions(+), 27 deletions(-) diff --git a/insights/client/connection.py b/insights/client/connection.py index b3a67f56e8..05664cf02f 100644 --- a/insights/client/connection.py +++ b/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] diff --git a/insights/client/utilities.py b/insights/client/utilities.py index 0712313c9c..5f8cdb239a 100644 --- a/insights/client/utilities.py +++ b/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 diff --git a/insights/tests/client/connection/test_LEGACY_reg_check.py b/insights/tests/client/connection/test_LEGACY_reg_check.py index 6655fb980f..5e628bd675 100644 --- a/insights/tests/client/connection/test_LEGACY_reg_check.py +++ b/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 diff --git a/insights/tests/client/connection/test_reg_check.py b/insights/tests/client/connection/test_reg_check.py index c6dbca67e2..3b3b72df18 100644 --- a/insights/tests/client/connection/test_reg_check.py +++ b/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 diff --git a/insights/tests/client/support/test_collect_support_info.py b/insights/tests/client/support/test_collect_support_info.py index 96e43a72ba..c78466df0e 100644 --- a/insights/tests/client/support/test_collect_support_info.py +++ b/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 ''' diff --git a/insights/tests/client/test_utilities.py b/insights/tests/client/test_utilities.py index c824e22e2a..68203c132c 100644 --- a/insights/tests/client/test_utilities.py +++ b/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)