From 3e4d447f0e1453ef3c6fbfd121331ccb4d0a8c8f Mon Sep 17 00:00:00 2001 From: david-mclain Date: Wed, 27 Nov 2024 15:45:14 -0700 Subject: [PATCH] Fix: monitor-only not being set properly when false/no passed in config (#285) ## Manual Testing ### Verify Bug - Run new testcases provided (current branch) checking for when false parameter is sent in using old watchdog file - Test cases will fail as Manager daemon is not spawned ### Verify Fix - Run same testcases as above using new watchdog - See that Manager daemon is spawned in cases where false is sent as parameter --- landscape/client/deployment.py | 25 +++++++++++++++++-- landscape/client/tests/test_deployment.py | 30 ++++++++++++++++++++++- landscape/client/tests/test_watchdog.py | 21 +++++++++++++++- landscape/client/watchdog.py | 9 ++++--- 4 files changed, 78 insertions(+), 7 deletions(-) diff --git a/landscape/client/deployment.py b/landscape/client/deployment.py index 5a41b50e..e46d6f20 100644 --- a/landscape/client/deployment.py +++ b/landscape/client/deployment.py @@ -3,10 +3,11 @@ import subprocess import sys import time +from argparse import SUPPRESS from datetime import datetime from datetime import timezone from logging import debug -from argparse import SUPPRESS +from logging import info from typing import Sequence from twisted.logger import globalLogBeginner @@ -14,8 +15,8 @@ from landscape import VERSION from landscape.client import DEFAULT_CONFIG from landscape.client import GROUP -from landscape.client import USER from landscape.client import snap_http +from landscape.client import USER from landscape.client.snap_utils import get_snap_info from landscape.client.upgraders import UPGRADE_MANAGERS from landscape.lib import logging @@ -304,3 +305,23 @@ def generate_computer_title(auto_enroll_config): title = hostname return title + + +def convert_arg_to_bool(value: str) -> bool: + """ + Converts an argument provided that is in string format + to be a boolean value. + """ + TRUTHY_VALUES = {"true", "yes", "y", "1", "on"} + FALSY_VALUES = {"false", "no", "n", "0", "off"} + + if value.lower() in TRUTHY_VALUES: + return True + elif value.lower() in FALSY_VALUES: + return False + else: + info( + "Error. Invalid boolean provided in config or parameters. " + + "Defaulting to False.", + ) + return False diff --git a/landscape/client/tests/test_deployment.py b/landscape/client/tests/test_deployment.py index eca56a12..a73a0736 100644 --- a/landscape/client/tests/test_deployment.py +++ b/landscape/client/tests/test_deployment.py @@ -4,6 +4,7 @@ from landscape.client.deployment import BaseConfiguration from landscape.client.deployment import Configuration +from landscape.client.deployment import convert_arg_to_bool from landscape.client.deployment import generate_computer_title from landscape.client.deployment import get_versioned_persist from landscape.client.deployment import init_logging @@ -468,7 +469,7 @@ def test_generate_computer_title_wait_for_serial_no_serial_assertion( ) self.assertIsNone(title) mock_debug.assert_called_once_with( - "No serial assertion in snap info {}, waiting..." + "No serial assertion in snap info {}, waiting...", ) @mock.patch("landscape.client.deployment.debug") @@ -601,3 +602,30 @@ def test_generate_computer_title_with_date( }, ) self.assertEqual(title, "2024-machine") + + +class ArgConversionTest(LandscapeTest): + """Tests for `convert_arg_to_bool` function""" + + def test_true_values(self): + TRUTHY_VALUES = {"true", "yes", "y", "1", "on", "TRUE", "Yes"} + for t in TRUTHY_VALUES: + val = convert_arg_to_bool(t) + self.assertTrue(val) + + def test_false_values(self): + FALSY_VALUES = {"false", "no", "n", "0", "off", "FALSE", "No"} + for f in FALSY_VALUES: + val = convert_arg_to_bool(f) + self.assertFalse(val) + + @mock.patch("landscape.client.deployment.info") + def test_invalid_values(self, logging): + INVALID_VALUES = {"invalid", "truthy", "2", "exit"} + for i in INVALID_VALUES: + val = convert_arg_to_bool(i) + logging.assert_called_with( + "Error. Invalid boolean provided in config or parameters. " + + "Defaulting to False.", + ) + self.assertFalse(val) diff --git a/landscape/client/tests/test_watchdog.py b/landscape/client/tests/test_watchdog.py index 16c923fc..669bf4c9 100644 --- a/landscape/client/tests/test_watchdog.py +++ b/landscape/client/tests/test_watchdog.py @@ -12,8 +12,8 @@ from twisted.internet.utils import getProcessOutput from twisted.python.fakepwd import UserDatabase -from landscape.client import USER import landscape.client.watchdog +from landscape.client import USER from landscape.client.amp import ComponentConnector from landscape.client.broker.amp import RemoteBrokerConnector from landscape.client.reactor import LandscapeReactor @@ -1147,6 +1147,13 @@ def test_monitor_only(self): self.config.load(["--monitor-only"]) self.assertEqual(self.config.get_enabled_daemons(), [Broker, Monitor]) + def test_monitor_only_false(self): + self.config.load(["--monitor-only", "false"]) + self.assertEqual( + self.config.get_enabled_daemons(), + [Broker, Monitor, Manager], + ) + def test_default_daemons(self): self.config.load([]) self.assertEqual( @@ -1154,6 +1161,18 @@ def test_default_daemons(self): [Broker, Monitor, Manager], ) + @mock.patch("landscape.client.deployment.info") + def test_monitor_only_invalid_entry(self, logging): + self.config.load(["--monitor-only", "invalid-option"]) + logging.assert_called_once_with( + "Error. Invalid boolean provided in config or parameters. " + + "Defaulting to False.", + ) + self.assertEqual( + self.config.get_enabled_daemons(), + [Broker, Monitor, Manager], + ) + class WatchDogServiceTest(LandscapeTest): def setUp(self): diff --git a/landscape/client/watchdog.py b/landscape/client/watchdog.py index 322d732a..911ee0df 100644 --- a/landscape/client/watchdog.py +++ b/landscape/client/watchdog.py @@ -4,7 +4,6 @@ The main C{landscape-client} program uses this watchdog. """ - import errno import os import pwd @@ -26,13 +25,14 @@ from twisted.internet.error import ProcessExitedAlready from twisted.internet.protocol import ProcessProtocol -from landscape.client import IS_SNAP from landscape.client import GROUP +from landscape.client import IS_SNAP from landscape.client import USER from landscape.client.broker.amp import RemoteBrokerConnector from landscape.client.broker.amp import RemoteManagerConnector from landscape.client.broker.amp import RemoteMonitorConnector from landscape.client.deployment import Configuration +from landscape.client.deployment import convert_arg_to_bool from landscape.client.deployment import init_logging from landscape.client.reactor import LandscapeReactor from landscape.lib.bootstrap import BootstrapDirectory @@ -522,7 +522,10 @@ def make_parser(self): ) parser.add_argument( "--monitor-only", - action="store_true", + type=convert_arg_to_bool, + nargs="?", + const=True, + default=False, help="Don't enable management features. This is " "useful if you want to run the client as a non-root " "user.",