From 3a99b095279e3bee2f1433c129206910e973d134 Mon Sep 17 00:00:00 2001 From: Himawan Winarto Date: Fri, 8 May 2026 09:10:04 -0400 Subject: [PATCH 1/4] feat: implement JujuLoginCheck to check for Juju authentication Signed-off-by: Himawan Winarto --- sunbeam-python/sunbeam/core/checks.py | 86 +++++++++++++++---- .../sunbeam/features/maintenance/checks.py | 29 ++++--- sunbeam-python/sunbeam/provider/maas/steps.py | 5 +- .../tests/unit/sunbeam/core/test_checks.py | 50 +++++++++++ 4 files changed, 136 insertions(+), 34 deletions(-) diff --git a/sunbeam-python/sunbeam/core/checks.py b/sunbeam-python/sunbeam/core/checks.py index caa63288a..5c21027e1 100644 --- a/sunbeam-python/sunbeam/core/checks.py +++ b/sunbeam-python/sunbeam/core/checks.py @@ -13,16 +13,25 @@ import click from rich.console import Console +from rich.status import Status from snaphelpers import Snap, SnapCtl from sunbeam.clusterd.client import Client from sunbeam.clusterd.service import ClusterServiceUnavailableException from sunbeam.core.common import ( RAM_16_GB_IN_KB, + ResultType, + StepContext, get_host_total_cores, get_host_total_ram, ) -from sunbeam.core.juju import JujuStepHelper +from sunbeam.core.juju import JujuAccount, JujuStepHelper +from sunbeam.core.progress import ( + CompositeProgressReporter, + LoggingProgressReporter, + RichProgressReporter, +) +from sunbeam.steps.juju import JujuLoginStep LOG = logging.getLogger(__name__) @@ -38,10 +47,10 @@ def run_preflight_checks(checks: Sequence["Check"], console: Console): Raise ClickException in case of Result Failures. """ for check in checks: - LOG.debug(f"Starting pre-flight check {check.name}") + LOG.debug("Starting pre-flight check %s", check.name) message = f"{check.description} ... " - with console.status(message): - if not check.run(): + with console.status(message) as check_status: + if not check.run(check_status=check_status): raise click.ClickException(check.message) @@ -65,7 +74,7 @@ def __init__(self, name: str, description: str = ""): self.description = description self.message = "Check successful" - def run(self) -> bool: + def run(self, check_status: Status | None = None) -> bool: """Run the check logic here. Return True if check is Ok. @@ -74,6 +83,47 @@ def run(self) -> bool: return True +class JujuLoginCheck(Check): + """Authenticate with the Juju controller.""" + + def __init__(self, juju_account: JujuAccount | None, controller: str | None = None): + """Initialize the JujuLoginCheck. + + :param juju_account: Juju account to use for authentication. + :param controller: Juju controller to authenticate with. + """ + self.step = JujuLoginStep(juju_account, controller) + super().__init__( + "Check Juju controller login", + f"Authenticating with Juju controller: {controller or 'current'}", + ) + + def run(self, check_status: Status | None = None) -> bool: + """Authenticate with Juju using the preflight check status.""" + if check_status is None: + raise ValueError("JujuLoginCheck requires an active status.") + + reporter = CompositeProgressReporter( + RichProgressReporter(check_status, self.step.status), + LoggingProgressReporter(), + ) + context = StepContext(status=check_status, reporter=reporter) + + skip_result = self.step.is_skip(context) + if skip_result.result_type == ResultType.SKIPPED: + return True + if skip_result.result_type == ResultType.FAILED: + self.message = skip_result.message + return False + + result = self.step.run(context) + if result.result_type == ResultType.FAILED: + self.message = result.message + return False + + return True + + class DiagnosticResultType(enum.Enum): """Enum for diagnostic result types.""" @@ -181,7 +231,7 @@ def __init__(self): "Checking for presence of Juju", ) - def run(self) -> bool: + def run(self, check_status: Status | None = None) -> bool: """Check for juju-bin content.""" snap = Snap() juju_content = snap.paths.snap / "juju" @@ -202,7 +252,7 @@ def __init__(self): "Checking for presence of ssh-keys interface", ) - def run(self) -> bool: + def run(self, check_status: Status | None = None) -> bool: """Check for ssh-keys interface.""" snap = Snap() snap_ctl = SnapCtl() @@ -236,7 +286,7 @@ def __init__(self): f"Checking if user {self.user} is member of group {self.group}", ) - def run(self) -> bool: + def run(self, check_status: Status | None = None) -> bool: """Return false if user is not member of group. Checks: @@ -286,7 +336,7 @@ def __init__(self): "Checking for ~/.local/share directory", ) - def run(self) -> bool: + def run(self, check_status: Status | None = None) -> bool: """Check for ~./local/share.""" snap = Snap() @@ -314,7 +364,7 @@ def __init__(self, fqdn: str): ) self.fqdn = fqdn - def run(self) -> bool: + def run(self, check_status: Status | None = None) -> bool: """Return false if FQDN is not valid. Checks: @@ -383,7 +433,7 @@ def __init__(self, fqdn, hypervisor_hostname): self.fqdn = fqdn self.hypervisor_hostname = hypervisor_hostname - def run(self) -> bool: + def run(self, check_status: Status | None = None) -> bool: """Verify if Hypervisor Hostname is same as FQDN.""" if self.fqdn == self.hypervisor_hostname: return True @@ -404,7 +454,7 @@ def __init__(self): "Checking for host configuration of minimum 4 core and 16G RAM", ) - def run(self) -> bool: + def run(self, check_status: Status | None = None) -> bool: """Validate system requirements. Checks: @@ -432,7 +482,7 @@ def __init__(self, client: Client): ) self.client = client - def run(self) -> bool: + def run(self, check_status: Status | None = None) -> bool: """Validate deployment has been bootstrapped. Checks: @@ -459,7 +509,7 @@ def __init__(self): ) self.client = Client.from_socket() - def run(self) -> bool: + def run(self, check_status: Status | None = None) -> bool: """Verify local instance of clusterd is not bootstrapped. Checks: @@ -489,7 +539,7 @@ def __init__(self, token: str): ) self.token = token - def run(self) -> bool: + def run(self, check_status: Status | None = None) -> bool: """Return false if the token provided is not valid. Checks: @@ -551,7 +601,7 @@ def __init__(self, controller: str, data_location: Path): self.controller = controller self.data_location = data_location - def run(self) -> bool: + def run(self, check_status: Status | None = None) -> bool: """Validate registration of juju controller. Checks: @@ -577,7 +627,7 @@ def __init__(self): f"Checking if user {self.user} is member of group {self.group}", ) - def run(self) -> bool: + def run(self, check_status: Status | None = None) -> bool: """Return false if user is not member of group. Checks: @@ -609,7 +659,7 @@ def __init__(self): "Checking if lxd juju controller exists", ) - def run(self) -> bool: + def run(self, check_status: Status | None = None) -> bool: """Check if lxd juju controller exists.""" controllers = JujuStepHelper().get_controllers(clouds=["localhost"]) if len(controllers) == 0: diff --git a/sunbeam-python/sunbeam/features/maintenance/checks.py b/sunbeam-python/sunbeam/features/maintenance/checks.py index 34c757564..13568d742 100644 --- a/sunbeam-python/sunbeam/features/maintenance/checks.py +++ b/sunbeam-python/sunbeam/features/maintenance/checks.py @@ -6,6 +6,7 @@ from collections.abc import Mapping from rich.console import Console +from rich.status import Status from sunbeam.clusterd.client import Client from sunbeam.core.checks import Check @@ -77,7 +78,7 @@ def __init__( self.node = node self.force = force - def run(self) -> bool: + def run(self, check_status: Status | None = None) -> bool: """Run the check logic here. Return True if check is Ok. @@ -118,7 +119,7 @@ def __init__( self.node = node self.force = force - def run(self) -> bool: + def run(self, check_status: Status | None = None) -> bool: """Run the check logic here. Return True if check is Ok. @@ -158,7 +159,7 @@ def __init__( self.node = node self.force = force - def run(self) -> bool: + def run(self, check_status: Status | None = None) -> bool: """Run the check logic here. Return True if check is Ok. @@ -192,7 +193,7 @@ def __init__( self.node = node self.force = force - def run(self) -> bool: + def run(self, check_status: Status | None = None) -> bool: """Run the check logic here. Return True if check is Ok. @@ -239,7 +240,7 @@ def __init__( self.action_params["check-only"] = True self.force = force - def run(self) -> bool: + def run(self, check_status: Status | None = None) -> bool: """Run the check logic here. Return True if check is Ok. @@ -285,7 +286,7 @@ def __init__( ) self.jhelper = jhelper - def run(self) -> bool: + def run(self, check_status: Status | None = None) -> bool: """Run the check logic here. Return True if check is Ok. @@ -316,7 +317,7 @@ def __init__(self, node: str, cluster_status: dict[str, typing.Any]): self.node = node self.cluster_status = cluster_status - def run(self) -> bool: + def run(self, check_status: Status | None = None) -> bool: """Check if the node is in the cluster.""" if not self.cluster_status.get(self.node): self.message = f"'{self.node}' does not exist in cluster." @@ -335,7 +336,7 @@ def __init__(self, cluster_status: dict[str, typing.Any], force: bool = False): self.force = force self.cluster_status = cluster_status - def run(self) -> bool: + def run(self, check_status: Status | None = None) -> bool: """Check if the cluster has more than one node.""" if len(self.cluster_status) > 1: return True @@ -368,7 +369,7 @@ def __init__( self.deployment = deployment self.cluster_status = cluster_status - def run(self) -> bool: + def run(self, check_status: Status | None = None) -> bool: """Check if the cluster has only one active control role.""" try: kube_client = get_kube_client(self.deployment.get_client()) @@ -436,7 +437,7 @@ def __init__( self.jhelper = jhelper self.deployment = deployment - def run(self) -> bool: + def run(self, check_status: Status | None = None) -> bool: """Check if the k8s dqlite has enough redundancy.""" datastore = self._get_k8s_configs().get(K8S_DATASTORE_CONFIG, "") if datastore != "dqlite": @@ -549,7 +550,7 @@ def __init__( self.force = force self.deployment = deployment - def run(self) -> bool: + def run(self, check_status: Status | None = None) -> bool: """Check if the k8s resource has enough replicas.""" try: kube_client = get_kube_client(self.deployment.get_client()) @@ -618,7 +619,7 @@ def __init__(self, node: str, deployment: Deployment): self.node = node self.deployment = deployment - def run(self) -> bool: + def run(self, check_status: Status | None = None) -> bool: """Check if the node has juju controller pods.""" if is_maas_deployment(self.deployment): LOG.debug( @@ -687,7 +688,7 @@ def __init__(self, node: str, deployment: Deployment, force: bool = False): self.force = force self.deployment = deployment - def run(self) -> bool: + def run(self, check_status: Status | None = None) -> bool: """Check if the node is cordoned.""" try: kube_client = get_kube_client(self.deployment.get_client()) @@ -725,7 +726,7 @@ def __init__(self, node: str, deployment: Deployment, force: bool = False): self.force = force self.deployment = deployment - def run(self) -> bool: + def run(self, check_status: Status | None = None) -> bool: """Check if the node is uncordoned.""" try: kube_client = get_kube_client(self.deployment.get_client()) diff --git a/sunbeam-python/sunbeam/provider/maas/steps.py b/sunbeam-python/sunbeam/provider/maas/steps.py index 8a3f09acb..09bc79a35 100644 --- a/sunbeam-python/sunbeam/provider/maas/steps.py +++ b/sunbeam-python/sunbeam/provider/maas/steps.py @@ -18,6 +18,7 @@ import click import tenacity from rich.console import Console +from rich.status import Status from snaphelpers import Snap import sunbeam.core.questions @@ -1042,7 +1043,7 @@ def __init__(self, deployment: maas_deployment.MaasDeployment): ) self.deployment = deployment - def run(self) -> bool: + def run(self, check_status: Status | None = None) -> bool: """Check network mapping is complete.""" network_to_space_mapping = self.deployment.network_mapping spaces = network_to_space_mapping.values() @@ -1066,7 +1067,7 @@ def __init__(self, deployment: maas_deployment.MaasDeployment, juju_controller): self.controller = juju_controller self.maas_client = maas_client.MaasClient.from_deployment(deployment) - def run(self) -> bool: + def run(self, check_status: Status | None = None) -> bool: """Check if juju controller is required.""" machines = maas_client.list_machines( self.maas_client, tags=maas_deployment.RoleTags.JUJU_CONTROLLER.value diff --git a/sunbeam-python/tests/unit/sunbeam/core/test_checks.py b/sunbeam-python/tests/unit/sunbeam/core/test_checks.py index 7ca9e4a6e..b4aac7486 100644 --- a/sunbeam-python/tests/unit/sunbeam/core/test_checks.py +++ b/sunbeam-python/tests/unit/sunbeam/core/test_checks.py @@ -8,6 +8,7 @@ from unittest.mock import Mock from sunbeam.core import checks +from sunbeam.core.common import Result, ResultType class TestSshKeysConnectedCheck: @@ -71,6 +72,55 @@ def test_run_daemon_not_active(self, mocker, snap): assert "Clusterd service is not active" in check.message +class TestJujuLoginCheck: + def test_run_skipped(self, mocker): + step = Mock(status="Authenticating with Juju controller: current ... ") + step.is_skip.return_value = Result(ResultType.SKIPPED) + mocker.patch.object(checks, "JujuLoginStep", return_value=step) + + check = checks.JujuLoginCheck(Mock()) + result = check.run(check_status=Mock()) + + assert result is True + step.run.assert_not_called() + + def test_run_completed(self, mocker): + step = Mock(status="Authenticating with Juju controller: current ... ") + step.is_skip.return_value = Result(ResultType.COMPLETED) + step.run.return_value = Result(ResultType.COMPLETED) + mocker.patch.object(checks, "JujuLoginStep", return_value=step) + + check = checks.JujuLoginCheck(Mock()) + result = check.run(check_status=Mock()) + + assert result is True + step.run.assert_called_once() + + def test_run_skip_failed(self, mocker): + step = Mock(status="Authenticating with Juju controller: current ... ") + step.is_skip.return_value = Result(ResultType.FAILED, "failed to check login") + mocker.patch.object(checks, "JujuLoginStep", return_value=step) + + check = checks.JujuLoginCheck(Mock()) + result = check.run(check_status=Mock()) + + assert result is False + assert check.message == "failed to check login" + step.run.assert_not_called() + + def test_run_failed(self, mocker): + step = Mock(status="Authenticating with Juju controller: current ... ") + step.is_skip.return_value = Result(ResultType.COMPLETED) + step.run.return_value = Result(ResultType.FAILED, "failed to login") + mocker.patch.object(checks, "JujuLoginStep", return_value=step) + + check = checks.JujuLoginCheck(Mock()) + result = check.run(check_status=Mock()) + + assert result is False + assert check.message == "failed to login" + + class TestLocalShareCheck: def test_run(self, mocker, snap): mocker.patch.object(checks, "Snap", return_value=snap) From f765032eb41df819482faabb747444cd8af7084a Mon Sep 17 00:00:00 2001 From: Himawan Winarto Date: Thu, 30 Apr 2026 23:54:04 -0400 Subject: [PATCH 2/4] feat: add JujuLoginCheck before any Juju action Signed-off-by: Himawan Winarto --- .../sunbeam/commands/dashboard_url.py | 13 +++-- .../sunbeam/commands/generate_cloud_config.py | 13 +++-- sunbeam-python/sunbeam/commands/launch.py | 4 ++ sunbeam-python/sunbeam/commands/openrc.py | 12 +++-- sunbeam-python/sunbeam/commands/plans.py | 5 ++ sunbeam-python/sunbeam/commands/proxy.py | 11 +++- sunbeam-python/sunbeam/commands/refresh.py | 15 ++++++ sunbeam-python/sunbeam/commands/resize.py | 9 +++- sunbeam-python/sunbeam/commands/sso.py | 51 +++++++++++++++---- .../sunbeam/features/baremetal/feature.py | 7 +++ .../sunbeam/features/interface/v1/base.py | 7 +++ .../sunbeam/features/ldap/feature.py | 15 ++++++ .../sunbeam/features/loadbalancer/feature.py | 10 ++++ .../sunbeam/features/maintenance/commands.py | 8 ++- .../sunbeam/features/observability/feature.py | 4 ++ sunbeam-python/sunbeam/features/tls/common.py | 5 ++ .../sunbeam/features/validation/feature.py | 6 +-- .../sunbeam/features/vault/feature.py | 17 +++++++ sunbeam-python/sunbeam/provider/commands.py | 7 ++- .../sunbeam/provider/local/commands.py | 21 +++++--- .../sunbeam/provider/maas/commands.py | 23 ++++++++- .../unit/sunbeam/commands/test_refresh.py | 1 + .../baremetal/test_baremetal_feature.py | 2 + .../features/maintenance/test_commands.py | 2 + .../tests/unit/sunbeam/features/test_base.py | 38 ++++++++++++++ .../sunbeam/features/test_loadbalancer.py | 8 +++ .../unit/sunbeam/provider/test_commands.py | 3 +- 27 files changed, 281 insertions(+), 36 deletions(-) diff --git a/sunbeam-python/sunbeam/commands/dashboard_url.py b/sunbeam-python/sunbeam/commands/dashboard_url.py index 1da7e3edb..d08b259b6 100644 --- a/sunbeam-python/sunbeam/commands/dashboard_url.py +++ b/sunbeam-python/sunbeam/commands/dashboard_url.py @@ -7,7 +7,11 @@ from rich.console import Console from sunbeam.core import juju -from sunbeam.core.checks import VerifyBootstrappedCheck, run_preflight_checks +from sunbeam.core.checks import ( + JujuLoginCheck, + VerifyBootstrappedCheck, + run_preflight_checks, +) from sunbeam.core.deployment import Deployment from sunbeam.core.openstack import OPENSTACK_MODEL @@ -37,9 +41,12 @@ def retrieve_dashboard_url(jhelper: juju.JujuHelper) -> str: def dashboard_url(ctx: click.Context) -> None: """Retrieve OpenStack Dashboard URL.""" deployment: Deployment = ctx.obj - preflight_checks = [] - preflight_checks.append(VerifyBootstrappedCheck(deployment.get_client())) + preflight_checks = [ + VerifyBootstrappedCheck(deployment.get_client()), + JujuLoginCheck(deployment.juju_account), + ] run_preflight_checks(preflight_checks, console) + jhelper = juju.JujuHelper(deployment.juju_controller) with console.status("Retrieving dashboard URL from Horizon service ... "): diff --git a/sunbeam-python/sunbeam/commands/generate_cloud_config.py b/sunbeam-python/sunbeam/commands/generate_cloud_config.py index fa6778171..1cd1b4687 100644 --- a/sunbeam-python/sunbeam/commands/generate_cloud_config.py +++ b/sunbeam-python/sunbeam/commands/generate_cloud_config.py @@ -16,7 +16,11 @@ import sunbeam.core.questions from sunbeam.clusterd.client import Client from sunbeam.commands.configure import retrieve_admin_credentials -from sunbeam.core.checks import VerifyBootstrappedCheck, run_preflight_checks +from sunbeam.core.checks import ( + JujuLoginCheck, + VerifyBootstrappedCheck, + run_preflight_checks, +) from sunbeam.core.common import ( BaseStep, Result, @@ -250,9 +254,12 @@ def cloud_config( deployment: Deployment = ctx.obj client = deployment.get_client() - preflight_checks = [] - preflight_checks.append(VerifyBootstrappedCheck(client)) + preflight_checks = [ + VerifyBootstrappedCheck(client), + JujuLoginCheck(deployment.juju_account), + ] run_preflight_checks(preflight_checks, console) + jhelper_keystone = deployment.get_juju_helper(keystone=True) if not jhelper_keystone.model_exists(OPENSTACK_MODEL): LOG.error(f"Expected model {OPENSTACK_MODEL} missing") diff --git a/sunbeam-python/sunbeam/commands/launch.py b/sunbeam-python/sunbeam/commands/launch.py index c501fdde6..0f87bf2bf 100644 --- a/sunbeam-python/sunbeam/commands/launch.py +++ b/sunbeam-python/sunbeam/commands/launch.py @@ -11,6 +11,7 @@ from snaphelpers import Snap from sunbeam.commands.configure import retrieve_admin_credentials +from sunbeam.core.checks import JujuLoginCheck, run_preflight_checks from sunbeam.core.deployment import Deployment from sunbeam.core.openstack import OPENSTACK_MODEL from sunbeam.core.terraform import TerraformException @@ -62,6 +63,9 @@ def launch( if not compute_nodes: raise click.ClickException("No compute role found. Cannot launch instance.") + # Login to the Juju controller + run_preflight_checks([JujuLoginCheck(deployment.juju_account)], console) + jhelper = deployment.get_juju_helper() jhelper_keystone = deployment.get_juju_helper(keystone=True) diff --git a/sunbeam-python/sunbeam/commands/openrc.py b/sunbeam-python/sunbeam/commands/openrc.py index bb627c8b9..a28e57c6b 100644 --- a/sunbeam-python/sunbeam/commands/openrc.py +++ b/sunbeam-python/sunbeam/commands/openrc.py @@ -7,7 +7,11 @@ from rich.console import Console from sunbeam.commands.configure import retrieve_admin_credentials -from sunbeam.core.checks import VerifyBootstrappedCheck, run_preflight_checks +from sunbeam.core.checks import ( + JujuLoginCheck, + VerifyBootstrappedCheck, + run_preflight_checks, +) from sunbeam.core.deployment import Deployment from sunbeam.core.openstack import OPENSTACK_MODEL @@ -21,8 +25,10 @@ def openrc(ctx: click.Context) -> None: """Retrieve openrc for cloud admin account.""" deployment: Deployment = ctx.obj client = deployment.get_client() - preflight_checks = [] - preflight_checks.append(VerifyBootstrappedCheck(client)) + preflight_checks = [ + VerifyBootstrappedCheck(client), + JujuLoginCheck(deployment.juju_account), + ] run_preflight_checks(preflight_checks, console) jhelper = deployment.get_juju_helper(keystone=True) diff --git a/sunbeam-python/sunbeam/commands/plans.py b/sunbeam-python/sunbeam/commands/plans.py index bb9659b2b..1c65564e1 100644 --- a/sunbeam-python/sunbeam/commands/plans.py +++ b/sunbeam-python/sunbeam/commands/plans.py @@ -14,6 +14,7 @@ from sunbeam.clusterd.service import ConfigItemNotFoundException from sunbeam.commands.configure import retrieve_admin_credentials +from sunbeam.core.checks import JujuLoginCheck, run_preflight_checks from sunbeam.core.common import ( FORMAT_TABLE, FORMAT_YAML, @@ -107,6 +108,10 @@ def shell_plan(ctx: click.Context, plan: str | None = None): with the environment variables set for the specified plan. """ deployment: Deployment = ctx.obj + + # Login to the Juju controller + run_preflight_checks([JujuLoginCheck(deployment.juju_account)], console) + jhelper_keystone = deployment.get_juju_helper(keystone=True) with console.status("Fetching OS admin credentials..."): diff --git a/sunbeam-python/sunbeam/commands/proxy.py b/sunbeam-python/sunbeam/commands/proxy.py index cc39d2942..60d85e6fa 100644 --- a/sunbeam-python/sunbeam/commands/proxy.py +++ b/sunbeam-python/sunbeam/commands/proxy.py @@ -13,7 +13,11 @@ ClusterServiceUnavailableException, ConfigItemNotFoundException, ) -from sunbeam.core.checks import VerifyBootstrappedCheck, run_preflight_checks +from sunbeam.core.checks import ( + JujuLoginCheck, + VerifyBootstrappedCheck, + run_preflight_checks, +) from sunbeam.core.common import ( FORMAT_TABLE, FORMAT_YAML, @@ -65,7 +69,10 @@ def _preflight_checks(deployment: Deployment): "completed succesfully. Please run `sunbeam cluster bootstrap`" ) raise click.ClickException(message) - preflight_checks = [VerifyBootstrappedCheck(client)] + preflight_checks = [ + VerifyBootstrappedCheck(client), + JujuLoginCheck(deployment.juju_account), + ] run_preflight_checks(preflight_checks, console) diff --git a/sunbeam-python/sunbeam/commands/refresh.py b/sunbeam-python/sunbeam/commands/refresh.py index d9fb097cc..6fecb325b 100644 --- a/sunbeam-python/sunbeam/commands/refresh.py +++ b/sunbeam-python/sunbeam/commands/refresh.py @@ -11,6 +11,7 @@ from snaphelpers import Snap from sunbeam.clusterd.service import ManifestItemNotFoundException +from sunbeam.core.checks import JujuLoginCheck, run_preflight_checks from sunbeam.core.common import ( ResultType, RiskLevel, @@ -126,6 +127,9 @@ def refresh( deployment: Deployment = ctx.obj client = deployment.get_client() + # Login to the Juju controller + run_preflight_checks([JujuLoginCheck(deployment.juju_account)], console) + if not manifest_path and not clear_manifest: # Warn only when the snap channel risk has changed since the manifest # was last stored (e.g. snap refreshed from stable to beta). We @@ -168,7 +172,9 @@ def refresh( manifest = deployment.get_manifest() LOG.debug(f"Manifest used for deployment - core: {manifest.core}") + jhelper = JujuHelper(deployment.juju_controller) + upgrade_coordinator: UpgradeCoordinator if upgrade_release: upgrade_coordinator = ChannelUpgradeCoordinator( @@ -209,6 +215,10 @@ def refresh_mysql( """Upgrade mysql-k8s charm to latest revision in channel.""" deployment: Deployment = ctx.obj client = deployment.get_client() + + # Login to the Juju controller + run_preflight_checks([JujuLoginCheck(deployment.juju_account)], console) + manifest = None if manifest_path: manifest = deployment.get_manifest(manifest_path) @@ -230,6 +240,7 @@ def refresh_mysql( ) jhelper = JujuHelper(deployment.juju_controller) + upgrade_coordinator = MySQLInChannelUpgradeCoordinator( deployment, client, jhelper, manifest, reset_mysql_upgrade_state ) @@ -255,6 +266,10 @@ def refresh_vault( """Upgrade vault-k8s charm to latest stable channel.""" deployment: Deployment = ctx.obj client = deployment.get_client() + + # Login to the Juju controller + run_preflight_checks([JujuLoginCheck(deployment.juju_account)], console) + jhelper = JujuHelper(deployment.juju_controller) manifest = None diff --git a/sunbeam-python/sunbeam/commands/resize.py b/sunbeam-python/sunbeam/commands/resize.py index d84d5dcca..f219a53d1 100644 --- a/sunbeam-python/sunbeam/commands/resize.py +++ b/sunbeam-python/sunbeam/commands/resize.py @@ -8,6 +8,7 @@ from rich.console import Console from sunbeam.clusterd.client import Client +from sunbeam.core.checks import JujuLoginCheck, run_preflight_checks from sunbeam.core.common import click_option_topology, run_plan from sunbeam.core.deployment import Deployment from sunbeam.core.juju import JujuHelper @@ -46,10 +47,12 @@ def resize( client: Client = deployment.get_client() manifest = deployment.get_manifest() + # Login to the Juju controller + run_preflight_checks([JujuLoginCheck(deployment.juju_account)], console) + openstack_tfhelper = deployment.get_tfhelper("openstack-plan") microceph_tfhelper = deployment.get_tfhelper("microceph-plan") cinder_volume_tfhelper = deployment.get_tfhelper("cinder-volume-plan") - jhelper = JujuHelper(deployment.juju_controller) storage_nodes = client.cluster.list_nodes_by_role("storage") @@ -57,7 +60,9 @@ def resize( if parameter_source == ParameterSource.COMMANDLINE: LOG.warning("WARNING: Option --force is deprecated and the value is ignored.") - plan = [] + jhelper = JujuHelper(deployment.juju_controller) + + plan: list = [] if len(storage_nodes): # Change default-pool-size based on number of storage nodes plan.extend( diff --git a/sunbeam-python/sunbeam/commands/sso.py b/sunbeam-python/sunbeam/commands/sso.py index 7398ed4ba..bc6a91af8 100644 --- a/sunbeam-python/sunbeam/commands/sso.py +++ b/sunbeam-python/sunbeam/commands/sso.py @@ -9,7 +9,11 @@ from rich.table import Table from sunbeam.clusterd.service import ConfigItemNotFoundException -from sunbeam.core.checks import VerifyBootstrappedCheck, run_preflight_checks +from sunbeam.core.checks import ( + JujuLoginCheck, + VerifyBootstrappedCheck, + run_preflight_checks, +) from sunbeam.core.common import ( FORMAT_TABLE, FORMAT_YAML, @@ -146,9 +150,12 @@ def add_sso( ) -> None: """Add a new identity provider.""" deployment: Deployment = ctx.obj - client = deployment.get_client() - preflight_checks = [VerifyBootstrappedCheck(client)] + + preflight_checks = [ + VerifyBootstrappedCheck(client), + JujuLoginCheck(deployment.juju_account), + ] run_preflight_checks(preflight_checks, console) cfg = safe_get_sso_config(client) @@ -219,8 +226,13 @@ def remove_sso( """Remove an identity provider.""" deployment: Deployment = ctx.obj client = deployment.get_client() - preflight_checks = [VerifyBootstrappedCheck(client)] + + preflight_checks = [ + VerifyBootstrappedCheck(client), + JujuLoginCheck(deployment.juju_account), + ] run_preflight_checks(preflight_checks, console) + cfg = safe_get_sso_config(client) provider = cfg.get(protocol, {}).get(name) @@ -233,6 +245,7 @@ def remove_sso( click.confirm(msg, abort=True) jhelper = JujuHelper(deployment.juju_controller) + plan: list[BaseStep] = [ TerraformInitStep(deployment.get_tfhelper("openstack-plan")), ] @@ -287,8 +300,13 @@ def update_sso( """Update identity provider (openid only).""" deployment: Deployment = ctx.obj client = deployment.get_client() - preflight_checks = [VerifyBootstrappedCheck(client)] + + preflight_checks = [ + VerifyBootstrappedCheck(client), + JujuLoginCheck(deployment.juju_account), + ] run_preflight_checks(preflight_checks, console) + try: cfg = read_config(client, SSO_CONFIG_KEY) except ConfigItemNotFoundException: @@ -309,6 +327,7 @@ def update_sso( raise click.ClickException(f"Invalid config supplied: {e}") jhelper = JujuHelper(deployment.juju_controller) + plan = [ TerraformInitStep(deployment.get_tfhelper("openstack-plan")), UpdateExternalProviderStep( @@ -328,6 +347,10 @@ def update_sso( def get_openid_redirect_uri(ctx: click.Context): """Get the OpenID redirect URI.""" deployment: Deployment = ctx.obj + + # Login to the Juju controller + run_preflight_checks([JujuLoginCheck(deployment.juju_account)], console) + jhelper = JujuHelper(deployment.juju_controller) app = "keystone" action_cmd = "get-admin-account" @@ -365,11 +388,16 @@ def purge_sso( ) -> None: """Remove all identity providers.""" deployment: Deployment = ctx.obj - jhelper = JujuHelper(deployment.juju_controller) client = deployment.get_client() - preflight_checks = [VerifyBootstrappedCheck(client)] + + preflight_checks = [ + VerifyBootstrappedCheck(client), + JujuLoginCheck(deployment.juju_account), + ] run_preflight_checks(preflight_checks, console) + jhelper = JujuHelper(deployment.juju_controller) + config = safe_get_sso_config(client) if not yes_i_mean_it and any(config.values()): msg = ( @@ -425,12 +453,17 @@ def set_saml_x509( ) -> None: """Set Keystone SAML x509 SP certificate and key.""" deployment: Deployment = ctx.obj - jhelper = JujuHelper(deployment.juju_controller) client = deployment.get_client() tfhelper = deployment.get_tfhelper("openstack-plan") - preflight_checks = [VerifyBootstrappedCheck(client)] + + preflight_checks = [ + VerifyBootstrappedCheck(client), + JujuLoginCheck(deployment.juju_account), + ] run_preflight_checks(preflight_checks, console) + jhelper = JujuHelper(deployment.juju_controller) + run_plan( [ TerraformInitStep(deployment.get_tfhelper("openstack-plan")), diff --git a/sunbeam-python/sunbeam/features/baremetal/feature.py b/sunbeam-python/sunbeam/features/baremetal/feature.py index f1758b8a5..0bb7efb07 100644 --- a/sunbeam-python/sunbeam/features/baremetal/feature.py +++ b/sunbeam-python/sunbeam/features/baremetal/feature.py @@ -8,6 +8,7 @@ from packaging.version import Version from rich.console import Console +from sunbeam.core.checks import JujuLoginCheck, run_preflight_checks from sunbeam.core.common import ( BaseStep, run_plan, @@ -124,6 +125,9 @@ def run_enable_plans( show_hints: bool, ): """Run the enablement plans.""" + # Login to the Juju controller + run_preflight_checks([JujuLoginCheck(deployment.juju_account)], console) + jhelper = JujuHelper(deployment.juju_controller) tfhelper = deployment.get_tfhelper(self.tfplan) @@ -288,6 +292,9 @@ def conductor_group_add( self, deployment: Deployment, group_name: str, show_hints: bool ): """Add ironic-conductor group.""" + # Login to the Juju controller + run_preflight_checks([JujuLoginCheck(deployment.juju_account)], console) + step = steps.DeployIronicConductorGroupsStep(deployment, self, [group_name]) jhelper = JujuHelper(deployment.juju_controller) temp_url_secret_step = steps.RunSetTempUrlSecretStep( diff --git a/sunbeam-python/sunbeam/features/interface/v1/base.py b/sunbeam-python/sunbeam/features/interface/v1/base.py index d14e3dcf3..6ac67258b 100644 --- a/sunbeam-python/sunbeam/features/interface/v1/base.py +++ b/sunbeam-python/sunbeam/features/interface/v1/base.py @@ -10,10 +10,12 @@ import click from packaging.requirements import Requirement from packaging.version import Version +from rich.console import Console from snaphelpers import Snap from sunbeam.clusterd.client import Client from sunbeam.clusterd.service import ConfigItemNotFoundException +from sunbeam.core.checks import JujuLoginCheck, run_preflight_checks from sunbeam.core.common import SunbeamException, read_config, update_config from sunbeam.core.deployment import Deployment from sunbeam.core.manifest import FeatureConfig, Manifest, SoftwareConfig @@ -23,6 +25,7 @@ from sunbeam.versions import VarMap LOG = logging.getLogger(__name__) +console = Console() _GROUPS: dict[str, Type["BaseFeatureGroup"]] = {} _FEATURES: dict[str, Type["BaseFeature"]] = {} @@ -745,6 +748,7 @@ def pre_enable( self, deployment: Deployment, config: ConfigType, show_hints: bool ) -> None: """Handler to perform tasks before enabling the feature.""" + run_preflight_checks([JujuLoginCheck(deployment.juju_account)], console) self.check_enablement_requirements(deployment) self.enable_requirements(deployment, show_hints) @@ -781,12 +785,14 @@ def enable_feature( current_click_context = current_click_context.parent self.pre_enable(deployment, config, show_hints) + self.run_enable_plans(deployment, config, show_hints) self.post_enable(deployment, config, show_hints) self.update_feature_info(deployment.get_client(), {"enabled": "true"}) def pre_disable(self, deployment: Deployment, show_hints: bool) -> None: """Handler to perform tasks before disabling the feature.""" + run_preflight_checks([JujuLoginCheck(deployment.juju_account)], console) self.check_enablement_requirements(deployment, state="disable") def post_disable(self, deployment: Deployment, show_hints: bool) -> None: @@ -804,6 +810,7 @@ def run_disable_plans(self, deployment: Deployment, show_hints: bool) -> None: def disable_feature(self, deployment: Deployment, show_hints: bool) -> None: """Disable feature command.""" self.pre_disable(deployment, show_hints) + self.run_disable_plans(deployment, show_hints) self.post_disable(deployment, show_hints) self.update_feature_info(deployment.get_client(), {"enabled": "false"}) diff --git a/sunbeam-python/sunbeam/features/ldap/feature.py b/sunbeam-python/sunbeam/features/ldap/feature.py index a8542564f..e1ab8b892 100644 --- a/sunbeam-python/sunbeam/features/ldap/feature.py +++ b/sunbeam-python/sunbeam/features/ldap/feature.py @@ -14,6 +14,7 @@ from sunbeam.clusterd.service import ( ConfigItemNotFoundException, ) +from sunbeam.core.checks import JujuLoginCheck, run_preflight_checks from sunbeam.core.common import ( BaseStep, Result, @@ -370,7 +371,12 @@ def add_domain( "domain-name": domain_name, "tls-ca-ldap": ca, } + + # Login to the Juju controller + run_preflight_checks([JujuLoginCheck(deployment.juju_account)], console) + jhelper = JujuHelper(deployment.juju_controller) + plan = [ TerraformInitStep(deployment.get_tfhelper(self.tfplan)), AddLDAPDomainStep(deployment, FeatureConfig(), jhelper, self, charm_config), @@ -415,7 +421,12 @@ def update_domain( with Path(ca_cert_file).open(mode="r") as f: ca = f.read() charm_config["tls-ca-ldap"] = ca + + # Login to the Juju controller + run_preflight_checks([JujuLoginCheck(deployment.juju_account)], console) + jhelper = JujuHelper(deployment.juju_controller) + plan = [ TerraformInitStep(deployment.get_tfhelper(self.tfplan)), UpdateLDAPDomainStep(deployment, jhelper, self, charm_config), @@ -431,7 +442,11 @@ def remove_domain( self, deployment: Deployment, domain_name: str, show_hints: bool ) -> None: """Remove LDAP backed domain.""" + # Login to the Juju controller + run_preflight_checks([JujuLoginCheck(deployment.juju_account)], console) + jhelper = JujuHelper(deployment.juju_controller) + plan = [ TerraformInitStep(deployment.get_tfhelper(self.tfplan)), DisableLDAPDomainStep( diff --git a/sunbeam-python/sunbeam/features/loadbalancer/feature.py b/sunbeam-python/sunbeam/features/loadbalancer/feature.py index a1498b4ec..bc25ab37d 100644 --- a/sunbeam-python/sunbeam/features/loadbalancer/feature.py +++ b/sunbeam-python/sunbeam/features/loadbalancer/feature.py @@ -19,6 +19,7 @@ from sunbeam.clusterd.service import ConfigItemNotFoundException from sunbeam.commands.configure import retrieve_admin_credentials from sunbeam.core import questions +from sunbeam.core.checks import JujuLoginCheck, run_preflight_checks from sunbeam.core.common import ( FORMAT_TABLE, FORMAT_YAML, @@ -1726,6 +1727,9 @@ def run_configure_plans( TLS certificates are provisioned separately via ``sunbeam loadbalancer provide-certificates``. """ + # Login to the Juju controller + run_preflight_checks([JujuLoginCheck(deployment.juju_account)], console) + openstack_tfhelper = deployment.get_tfhelper(self.tfplan) cni_tfhelper = deployment.get_tfhelper(self.cni_tfplan) setup_tfhelper = deployment.get_tfhelper(self.setup_tfplan) @@ -1971,6 +1975,7 @@ def configure( "Barbican (secrets) feature must be enabled for Octavia Amphora.\n" "Enable it first: sunbeam enable secrets" ) + self.run_configure_plans( deployment, show_hints=show_hints, @@ -2004,6 +2009,10 @@ def provide_certificates( "Barbican (secrets) feature must be enabled for Octavia Amphora.\n" "Enable it first: sunbeam enable secrets" ) + + # Login to the Juju controller + run_preflight_checks([JujuLoginCheck(deployment.juju_account)], console) + jhelper = JujuHelper(deployment.juju_controller) feature_config: LoadbalancerFeatureConfig | None = None if self.manifest: @@ -2043,6 +2052,7 @@ def list_outstanding_csrs(self, deployment: Deployment, format: str) -> None: "Barbican (secrets) feature must be enabled for Octavia Amphora.\n" "Enable it first: sunbeam enable secrets" ) + csrs = handle_list_outstanding_csrs( CA_MANUAL_TLS_CERTIFICATE, CA_MANUAL_TLS_CERTIFICATE_INTERFACE, diff --git a/sunbeam-python/sunbeam/features/maintenance/commands.py b/sunbeam-python/sunbeam/features/maintenance/commands.py index ad16f317c..7e132da37 100644 --- a/sunbeam-python/sunbeam/features/maintenance/commands.py +++ b/sunbeam-python/sunbeam/features/maintenance/commands.py @@ -8,7 +8,7 @@ import click from rich.console import Console -from sunbeam.core.checks import Check, run_preflight_checks +from sunbeam.core.checks import Check, JujuLoginCheck, run_preflight_checks from sunbeam.core.common import ( BaseStep, get_step_message, @@ -639,6 +639,9 @@ def enable( show_hints: bool = False, ) -> None: """Enable maintenance mode for node.""" + # Login to the Juju controller + run_preflight_checks([JujuLoginCheck(deployment.juju_account)], console) + cluster_status = get_cluster_status( deployment=deployment, jhelper=JujuHelper(deployment.juju_controller), @@ -697,6 +700,9 @@ def disable( show_hints: bool = False, ) -> None: """Disable maintenance mode for node.""" + # Login to the Juju controller + run_preflight_checks([JujuLoginCheck(deployment.juju_account)], console) + cluster_status = get_cluster_status( deployment=deployment, jhelper=JujuHelper(deployment.juju_controller), diff --git a/sunbeam-python/sunbeam/features/observability/feature.py b/sunbeam-python/sunbeam/features/observability/feature.py index 4d5c8a4e5..720652bc1 100644 --- a/sunbeam-python/sunbeam/features/observability/feature.py +++ b/sunbeam-python/sunbeam/features/observability/feature.py @@ -26,6 +26,7 @@ from sunbeam.core.checks import ( Check, JujuControllerRegistrationCheck, + JujuLoginCheck, run_preflight_checks, ) from sunbeam.core.common import ( @@ -1066,6 +1067,9 @@ def run_disable_plans(self, deployment: Deployment, show_hints: bool): @pass_method_obj def dashboard_url(self, deployment: Deployment) -> None: """Retrieve COS Dashboard URL.""" + # Login to the Juju controller + run_preflight_checks([JujuLoginCheck(deployment.juju_account)], console) + jhelper = JujuHelper(deployment.juju_controller) with console.status("Retrieving dashboard URL from Grafana service ... "): diff --git a/sunbeam-python/sunbeam/features/tls/common.py b/sunbeam-python/sunbeam/features/tls/common.py index 8746bf01d..8ecdb2258 100644 --- a/sunbeam-python/sunbeam/features/tls/common.py +++ b/sunbeam-python/sunbeam/features/tls/common.py @@ -14,6 +14,7 @@ from sunbeam.clusterd.client import Client from sunbeam.clusterd.service import ConfigItemNotFoundException from sunbeam.core import questions +from sunbeam.core.checks import JujuLoginCheck, run_preflight_checks from sunbeam.core.common import ( BaseStep, Result, @@ -443,6 +444,10 @@ def handle_list_outstanding_csrs( ] """ action_cmd = "get-outstanding-certificate-requests" + + # Login to the Juju controller + run_preflight_checks([JujuLoginCheck(deployment.juju_account)], console) + jhelper = JujuHelper(deployment.juju_controller) try: action_result = get_outstanding_certificate_requests( diff --git a/sunbeam-python/sunbeam/features/validation/feature.py b/sunbeam-python/sunbeam/features/validation/feature.py index f8c4e3cfa..984aaf27d 100644 --- a/sunbeam-python/sunbeam/features/validation/feature.py +++ b/sunbeam-python/sunbeam/features/validation/feature.py @@ -17,6 +17,7 @@ from rich.table import Column, Table from sunbeam.clusterd.client import Client +from sunbeam.core.checks import JujuLoginCheck, run_preflight_checks from sunbeam.core.common import BaseStep, Result, ResultType, StepContext, run_plan from sunbeam.core.deployment import Deployment from sunbeam.core.juju import ( @@ -39,7 +40,6 @@ OpenStackControlPlaneFeature, TerraformPlanLocation, ) -from sunbeam.steps.juju import JujuLoginStep from sunbeam.utils import click_option_show_hints, pass_method_obj from sunbeam.versions import OPENSTACK_CHANNEL @@ -576,7 +576,7 @@ def run_validate_action( if output: # Due to shelling out to the juju cli (rather than using libjuju), # we need to ensure the juju cli is logged in. - run_plan([JujuLoginStep(deployment.juju_account)], console, show_hints) + run_preflight_checks([JujuLoginCheck(deployment.juju_account)], console) self._copy_file_from_tempest_container( deployment, TEMPEST_VALIDATION_RESULT, output @@ -617,7 +617,7 @@ def run_get_last_result( """Get last validation result.""" # Due to shelling out to the juju cli (rather than using libjuju), # we need to ensure the juju cli is logged in. - run_plan([JujuLoginStep(deployment.juju_account)], console, show_hints) + run_preflight_checks([JujuLoginCheck(deployment.juju_account)], console) if not self._check_file_exist_in_tempest_container( deployment, TEMPEST_VALIDATION_RESULT diff --git a/sunbeam-python/sunbeam/features/vault/feature.py b/sunbeam-python/sunbeam/features/vault/feature.py index 99ac78a0c..96e8f8c77 100644 --- a/sunbeam-python/sunbeam/features/vault/feature.py +++ b/sunbeam-python/sunbeam/features/vault/feature.py @@ -21,6 +21,7 @@ ClusterServiceUnavailableException, ConfigItemNotFoundException, ) +from sunbeam.core.checks import JujuLoginCheck, run_preflight_checks from sunbeam.core.common import ( FORMAT_DEFAULT, FORMAT_JSON, @@ -774,7 +775,11 @@ def initialize_vault( format: str, ): """Initialize Vault.""" + # Login to the Juju controller + run_preflight_checks([JujuLoginCheck(deployment.juju_account)], console) + jhelper = JujuHelper(deployment.juju_controller) + plan = [VaultInitStep(jhelper, key_shares, key_threshold)] plan_results = run_plan(plan, console) @@ -813,7 +818,11 @@ def unseal_vault(self, deployment: Deployment, unseal_key: str): if unseal_key == "-": unseal_key = get_stdin_reopen_tty() + # Login to the Juju controller + run_preflight_checks([JujuLoginCheck(deployment.juju_account)], console) + jhelper = JujuHelper(deployment.juju_controller) + plan = [VaultUnsealStep(jhelper, unseal_key)] plan_results = run_plan(plan, console) click.echo(get_step_message(plan_results, VaultUnsealStep)) @@ -829,7 +838,11 @@ def authorize_charm(self, deployment: Deployment, root_token: str): if root_token == "-": root_token = get_stdin_reopen_tty() + # Login to the Juju controller + run_preflight_checks([JujuLoginCheck(deployment.juju_account)], console) + jhelper = JujuHelper(deployment.juju_controller) + plan = [AuthorizeVaultCharmStep(jhelper, root_token)] run_plan(plan, console) click.echo("Vault charm is authorized.") @@ -845,7 +858,11 @@ def authorize_charm(self, deployment: Deployment, root_token: str): @pass_method_obj def vault_status(self, deployment: Deployment, format: str): """Vault status.""" + # Login to the Juju controller + run_preflight_checks([JujuLoginCheck(deployment.juju_account)], console) + jhelper = JujuHelper(deployment.juju_controller) + plan = [VaultStatusStep(jhelper)] plan_results = run_plan(plan, console) diff --git a/sunbeam-python/sunbeam/provider/commands.py b/sunbeam-python/sunbeam/provider/commands.py index 269deb9e9..13ef2a2e2 100644 --- a/sunbeam-python/sunbeam/provider/commands.py +++ b/sunbeam-python/sunbeam/provider/commands.py @@ -12,7 +12,7 @@ from rich.table import Table from snaphelpers import Snap -from sunbeam.core.checks import LocalShareCheck, run_preflight_checks +from sunbeam.core.checks import JujuLoginCheck, LocalShareCheck, run_preflight_checks from sunbeam.core.common import ( CONTEXT_SETTINGS, FORMAT_TABLE, @@ -243,7 +243,12 @@ def update_clusterd_credentials(ctx, show_hints: bool = False): snap = Snap() path = deployment_path(snap) deployments = DeploymentsConfig.load(path) + + # Login to the Juju controller + run_preflight_checks([JujuLoginCheck(deployment.juju_account)], console) + jhelper = JujuHelper(deployment.juju_controller) + plan = [MaasSaveClusterdCredentialsStep(jhelper, deployment.name, deployments)] run_plan(plan, console, show_hints) diff --git a/sunbeam-python/sunbeam/provider/local/commands.py b/sunbeam-python/sunbeam/provider/local/commands.py index 36e8e6fb0..1aa926bd0 100644 --- a/sunbeam-python/sunbeam/provider/local/commands.py +++ b/sunbeam-python/sunbeam/provider/local/commands.py @@ -33,6 +33,7 @@ Check, DaemonGroupCheck, JujuControllerRegistrationCheck, + JujuLoginCheck, JujuSnapCheck, LocalShareCheck, LxdGroupCheck, @@ -1025,6 +1026,10 @@ def configure_sriov( return manifest = deployment.get_manifest(manifest_path) + + # Login to the Juju controller + run_preflight_checks([JujuLoginCheck(deployment.juju_account)], console) + jhelper = deployment.get_juju_helper() jhelper_keystone = deployment.get_juju_helper(keystone=True) @@ -1098,6 +1103,10 @@ def configure_dpdk( return manifest = deployment.get_manifest(manifest_path) + + # Login to the Juju controller + run_preflight_checks([JujuLoginCheck(deployment.juju_account)], console) + jhelper = deployment.get_juju_helper() jhelper_keystone = deployment.get_juju_helper(keystone=True) @@ -1799,9 +1808,11 @@ def list_nodes( show_hints: bool, ) -> None: """List nodes in the cluster.""" - preflight_checks = [DaemonGroupCheck()] - run_preflight_checks(preflight_checks, console) deployment: LocalDeployment = ctx.obj + + preflight_checks = [DaemonGroupCheck(), JujuLoginCheck(deployment.juju_account)] + run_preflight_checks(preflight_checks, console) + jhelper = JujuHelper(deployment.juju_controller) step = LocalClusterStatusStep(deployment, jhelper) results = run_plan([step], console, show_hints) @@ -1827,12 +1838,10 @@ def remove(ctx: click.Context, name: str, force: bool, show_hints: bool) -> None client = deployment.get_client() jhelper = JujuHelper(deployment.juju_controller) - preflight_checks = [DaemonGroupCheck()] + preflight_checks = [DaemonGroupCheck(), JujuLoginCheck(deployment.juju_account)] run_preflight_checks(preflight_checks, console) - plan: list[BaseStep] = [ - JujuLoginStep(deployment.juju_account), - ] + plan: list[BaseStep] = [] if not force: plan.append(PromptCheckNodeExistStep(client, name)) diff --git a/sunbeam-python/sunbeam/provider/maas/commands.py b/sunbeam-python/sunbeam/provider/maas/commands.py index 0cb5124ef..15f028a49 100644 --- a/sunbeam-python/sunbeam/provider/maas/commands.py +++ b/sunbeam-python/sunbeam/provider/maas/commands.py @@ -33,6 +33,7 @@ DiagnosticResultType, DiagnosticsCheck, DiagnosticsResult, + JujuLoginCheck, JujuSnapCheck, LocalShareCheck, VerifyBootstrappedCheck, @@ -589,6 +590,7 @@ def deploy( preflight_checks.append(JujuSnapCheck()) preflight_checks.append(LocalShareCheck()) preflight_checks.append(VerifyClusterdNotBootstrappedCheck()) + preflight_checks.append(JujuLoginCheck(deployment.juju_account)) run_preflight_checks(preflight_checks, console) if ( @@ -603,13 +605,14 @@ def deploy( deployment.juju_controller, ) console.print( - f"{deployment.name!r} deployment is not complete, was bootstrap completed ?" + f"{deployment.name!r} deployment is not complete, was bootstrap completed?" ) sys.exit(1) deployment_location = deployment_path(Snap()) deployments = DeploymentsConfig.load(deployment_location) maas_client = MaasClient.from_deployment(deployment) + jhelper = JujuHelper(deployment.juju_controller) clusterd_plan = [ MaasSaveClusterdCredentialsStep(jhelper, deployment.name, deployments) @@ -1120,6 +1123,10 @@ def configure_cmd( def list_nodes(ctx: click.Context, format: str, show_hints: bool) -> None: """List nodes in the custer.""" deployment: MaasDeployment = ctx.obj + + # Login to the Juju controller + run_preflight_checks([JujuLoginCheck(deployment.juju_account)], console) + jhelper = JujuHelper(deployment.juju_controller) step = MaasClusterStatusStep(deployment, jhelper) results = run_plan([step], console, show_hints) @@ -1749,13 +1756,17 @@ def destroy_deployment_cmd( """ if not no_prompt: click.confirm("This will destroy the deployment. Are you sure?", abort=True) + + deployment: MaasDeployment = ctx.obj + preflight_checks = [ JujuSnapCheck(), LocalShareCheck(), + JujuLoginCheck(deployment.juju_account), ] run_preflight_checks(preflight_checks, console) deployments = DeploymentsConfig.load(deployment_path(Snap())) - deployment: MaasDeployment = ctx.obj + plan = [] client = None @@ -1889,6 +1900,10 @@ def configure_sriov( deployment: MaasDeployment = ctx.obj client = deployment.get_client() manifest = deployment.get_manifest(manifest_path) + + # Login to the Juju controller + run_preflight_checks([JujuLoginCheck(deployment.juju_account)], console) + jhelper = JujuHelper(deployment.juju_controller) admin_credentials = retrieve_admin_credentials(jhelper, deployment, OPENSTACK_MODEL) @@ -1949,6 +1964,10 @@ def configure_dpdk( deployment: MaasDeployment = ctx.obj client = deployment.get_client() manifest = deployment.get_manifest(manifest_path) + + # Login to the Juju controller + run_preflight_checks([JujuLoginCheck(deployment.juju_account)], console) + jhelper = JujuHelper(deployment.juju_controller) admin_credentials = retrieve_admin_credentials(jhelper, deployment, OPENSTACK_MODEL) diff --git a/sunbeam-python/tests/unit/sunbeam/commands/test_refresh.py b/sunbeam-python/tests/unit/sunbeam/commands/test_refresh.py index 2f4ab6535..9caadf25e 100644 --- a/sunbeam-python/tests/unit/sunbeam/commands/test_refresh.py +++ b/sunbeam-python/tests/unit/sunbeam/commands/test_refresh.py @@ -192,6 +192,7 @@ def _patch_upgrade_coordinators(mocker): autospec=True, ) mocker.patch("sunbeam.commands.refresh.JujuHelper", autospec=True) + mocker.patch("sunbeam.commands.refresh.run_preflight_checks") mocker.patch("sunbeam.commands.refresh.run_plan") diff --git a/sunbeam-python/tests/unit/sunbeam/features/baremetal/test_baremetal_feature.py b/sunbeam-python/tests/unit/sunbeam/features/baremetal/test_baremetal_feature.py index 9c9ddafd7..229c9c673 100644 --- a/sunbeam-python/tests/unit/sunbeam/features/baremetal/test_baremetal_feature.py +++ b/sunbeam-python/tests/unit/sunbeam/features/baremetal/test_baremetal_feature.py @@ -78,8 +78,10 @@ def test_set_application_names(self, deployment): @patch.object(steps, "JujuHelper") @patch.object(ironic_feature, "JujuHelper") @patch.object(ironic_feature, "click", Mock()) + @patch.object(ironic_feature, "run_preflight_checks") def test_run_enable_plans( self, + mock_run_preflight_checks, mock_JujuHelper, mock_steps_JujuHelper, mock_switch_apply, diff --git a/sunbeam-python/tests/unit/sunbeam/features/maintenance/test_commands.py b/sunbeam-python/tests/unit/sunbeam/features/maintenance/test_commands.py index f8d9f959d..29bd7d060 100644 --- a/sunbeam-python/tests/unit/sunbeam/features/maintenance/test_commands.py +++ b/sunbeam-python/tests/unit/sunbeam/features/maintenance/test_commands.py @@ -39,6 +39,8 @@ def test_disable_migration_flag_mapping( "sunbeam.features.maintenance.commands.get_cluster_status" ) as mock_get_status, patch("sunbeam.features.maintenance.commands.JujuHelper"), + patch("sunbeam.features.maintenance.commands.run_preflight_checks"), + patch("sunbeam.features.maintenance.commands.run_plan"), ): mock_get_status.return_value = {"test-node": "compute"} mock_instance = Mock() diff --git a/sunbeam-python/tests/unit/sunbeam/features/test_base.py b/sunbeam-python/tests/unit/sunbeam/features/test_base.py index c31b6d9a6..8aa8f70c5 100644 --- a/sunbeam-python/tests/unit/sunbeam/features/test_base.py +++ b/sunbeam-python/tests/unit/sunbeam/features/test_base.py @@ -553,6 +553,44 @@ def test_check_enablement_requirements_with_disabled_dependant( ) feature.check_enablement_requirements(deployment, "disable") + def test_pre_enable_runs_juju_login_preflight_check(self, deployment, mocker): + feature = DummyFeature() + juju_account = Mock() + deployment.juju_account = juju_account + run_preflight_checks = mocker.patch( + "sunbeam.features.interface.v1.base.run_preflight_checks" + ) + mocker.patch.object(feature, "check_enablement_requirements") + mocker.patch.object(feature, "enable_requirements") + + feature.pre_enable(deployment, Mock(), show_hints=False) + + run_preflight_checks.assert_called_once() + checks = run_preflight_checks.call_args.args[0] + assert len(checks) == 1 + assert checks[0].step.juju_account == juju_account + + def test_pre_disable_runs_juju_login_preflight_check(self, deployment, mocker): + feature = DummyFeature() + juju_account = Mock() + deployment.juju_account = juju_account + run_preflight_checks = mocker.patch( + "sunbeam.features.interface.v1.base.run_preflight_checks" + ) + check_enablement_requirements = mocker.patch.object( + feature, "check_enablement_requirements" + ) + + feature.pre_disable(deployment, show_hints=False) + + check_enablement_requirements.assert_called_once_with( + deployment, state="disable" + ) + run_preflight_checks.assert_called_once() + checks = run_preflight_checks.call_args.args[0] + assert len(checks) == 1 + assert checks[0].step.juju_account == juju_account + class TestFeatureManager: """Test FeatureManager methods.""" diff --git a/sunbeam-python/tests/unit/sunbeam/features/test_loadbalancer.py b/sunbeam-python/tests/unit/sunbeam/features/test_loadbalancer.py index 7bda51ade..dea0da6f0 100644 --- a/sunbeam-python/tests/unit/sunbeam/features/test_loadbalancer.py +++ b/sunbeam-python/tests/unit/sunbeam/features/test_loadbalancer.py @@ -956,6 +956,8 @@ def test_configure_proceeds_when_secrets_enabled(self): with ( patch("click.get_current_context", return_value=ctx), patch.object(feature, "run_configure_plans") as mock_run, + patch("sunbeam.features.loadbalancer.feature.run_preflight_checks"), + patch("sunbeam.features.loadbalancer.feature.run_plan"), ): feature.configure.callback(feature, accept_defaults=False, show_hints=False) mock_run.assert_called_once() @@ -973,6 +975,7 @@ def test_provide_certificates_proceeds_when_secrets_enabled(self): with ( patch("click.get_current_context", return_value=ctx), patch("sunbeam.features.loadbalancer.feature.run_plan") as mock_run, + patch("sunbeam.features.loadbalancer.feature.run_preflight_checks"), patch("sunbeam.features.loadbalancer.feature.JujuHelper"), ): feature.provide_certificates.callback(feature, show_hints=False) @@ -994,6 +997,8 @@ def test_list_outstanding_csrs_proceeds_when_secrets_enabled(self): "sunbeam.features.loadbalancer.feature.handle_list_outstanding_csrs", return_value=[], ), + patch("sunbeam.features.loadbalancer.feature.run_preflight_checks"), + patch("sunbeam.features.loadbalancer.feature.run_plan"), ): feature.list_outstanding_csrs.callback(feature, format="table") @@ -1032,6 +1037,7 @@ def test_no_teardown_when_previously_not_enabled(self): "sunbeam.features.loadbalancer.feature.questions.load_answers", side_effect=lambda *_: dict(next(load_answers_iter)), ), + patch("sunbeam.features.loadbalancer.feature.run_preflight_checks"), patch("sunbeam.features.loadbalancer.feature.run_plan") as mock_run_plan, patch("sunbeam.features.loadbalancer.feature.JujuHelper"), patch("click.echo") as mock_echo, @@ -1063,6 +1069,7 @@ def test_teardown_runs_when_previously_enabled(self): "sunbeam.features.loadbalancer.feature.questions.load_answers", side_effect=lambda *_: dict(next(load_answers_iter)), ), + patch("sunbeam.features.loadbalancer.feature.run_preflight_checks"), patch("sunbeam.features.loadbalancer.feature.run_plan") as mock_run_plan, patch("sunbeam.features.loadbalancer.feature.JujuHelper"), patch("click.echo"), @@ -1576,6 +1583,7 @@ def capture_run_plan(plan, *args, **kwargs): "sunbeam.features.loadbalancer.feature.run_plan", side_effect=capture_run_plan, ), + patch("sunbeam.features.loadbalancer.feature.run_preflight_checks"), patch("sunbeam.features.loadbalancer.feature.JujuHelper"), patch("click.echo"), ): diff --git a/sunbeam-python/tests/unit/sunbeam/provider/test_commands.py b/sunbeam-python/tests/unit/sunbeam/provider/test_commands.py index ff90957ae..0bdfaaa5b 100644 --- a/sunbeam-python/tests/unit/sunbeam/provider/test_commands.py +++ b/sunbeam-python/tests/unit/sunbeam/provider/test_commands.py @@ -14,6 +14,7 @@ def test_update_clusterd_credentials_invokes_plan(tmp_path, mocker): deployment = Mock() deployment.juju_controller = Mock() run_plan_spy = mocker.patch.object(provider_commands, "run_plan") + mocker.patch.object(provider_commands, "run_preflight_checks") # Act cmd = provider_commands.update_clusterd_credentials @@ -22,5 +23,5 @@ def test_update_clusterd_credentials_invokes_plan(tmp_path, mocker): cmd.callback(show_hints=False) assert run_plan_spy.call_count == 1 - plan = run_plan_spy.call_args[0][0] + plan = run_plan_spy.call_args_list[0][0][0] assert isinstance(plan[0], MaasSaveClusterdCredentialsStep) From ca95cb2b6179f1ae220d2d3eb1a6cb52dc354b3d Mon Sep 17 00:00:00 2001 From: Himawan Winarto Date: Thu, 30 Apr 2026 18:08:48 -0400 Subject: [PATCH 3/4] refactor: use context manager for pexpect.spawn in Juju step classes Signed-off-by: Himawan Winarto --- sunbeam-python/sunbeam/steps/juju.py | 202 +++++++++--------- .../tests/unit/sunbeam/steps/test_juju.py | 24 ++- 2 files changed, 124 insertions(+), 102 deletions(-) diff --git a/sunbeam-python/sunbeam/steps/juju.py b/sunbeam-python/sunbeam/steps/juju.py index 80258e6e1..12672ca4d 100644 --- a/sunbeam-python/sunbeam/steps/juju.py +++ b/sunbeam-python/sunbeam/steps/juju.py @@ -829,47 +829,49 @@ def run(self, context: StepContext) -> Result: if self.replace: register_args.append("--replace") - try: - child = pexpect.spawn( - self._get_juju_binary(), - register_args, - PEXPECT_TIMEOUT, - ) - with open(log_file, "wb+") as f: - # Record the command output, but only the contents streaming from the - # process, don't record anything sent to the process as it may contain - # sensitive information. - child.logfile_read = f - while True: - index = child.expect(expect_list, PEXPECT_TIMEOUT) - LOG.debug( - "Juju registraton: expect got regex related to " - f"{expect_list[index]}" - ) - if index in (0, 1, 3): - child.sendline(self.juju_account.password) - elif index == 2: - result = child.before.decode() - # If controller already exists, the command keeps on asking - # controller name, so change the controller name to dummy. - # The command errors out at the next stage that controller - # is already registered. - if f'Controller "{self.controller}" already exists' in result: - child.sendline("dummy") - else: - child.sendline(self.controller) - elif index == 4: - result = child.before.decode() - if "ERROR" in result: - str_index = result.find("ERROR") - return Result(ResultType.FAILED, result[str_index:]) - - LOG.debug("User registration completed") - break - except pexpect.TIMEOUT as e: - LOG.exception(f"Error registering user {self.username} in Juju") - LOG.warning(e) - return Result(ResultType.FAILED, str(e)) + with pexpect.spawn( + self._get_juju_binary(), + register_args, + PEXPECT_TIMEOUT, + ) as child: + try: + with open(log_file, "wb+") as f: + # Record the command output, but only the contents streaming + # from the process, don't record anything sent to the process + # as it may contain sensitive information. + child.logfile_read = f + while True: + index = child.expect(expect_list, PEXPECT_TIMEOUT) + LOG.debug( + "Juju registration: input request related to regex %r", + expect_list[index], + ) + if index in (0, 1, 3): + child.sendline(self.juju_account.password) + elif index == 2: + result = child.before.decode() + # If controller already exists, the command keeps on + # asking controller name, so change the controller + # name to placeholder. The command errors out at the next + # stage that controller is already registered. + if ( + f'Controller "{self.controller}" already exists' + in result + ): + child.sendline("placeholder-controller") + else: + child.sendline(self.controller) + elif index == 4: + result = child.before.decode() + if "ERROR" in result: + str_index = result.find("ERROR") + return Result(ResultType.FAILED, result[str_index:]) + + LOG.debug("User registration completed") + break + except pexpect.TIMEOUT as e: + LOG.warning("Timeout registering user %s in Juju: %r", self.username, e) + return Result(ResultType.FAILED, str(e)) return Result(ResultType.COMPLETED) @@ -1072,47 +1074,47 @@ def run(self, context: StepContext) -> Result: log_file = snap.paths.user_common / f"add_juju_machine_{self.machine_ip}.log" auth_message_re = "Are you sure you want to continue connecting" expect_list = [auth_message_re, pexpect.EOF] - try: - child = pexpect.spawn( - self._get_juju_binary(), - ["add-machine", "-m", self.model_with_owner, f"ssh:{self.machine_ip}"], - PEXPECT_TIMEOUT * 5, # 5 minutes - ) - with open(log_file, "wb+") as f: - # Record the command output, but only the contents streaming from the - # process, don't record anything sent to the process as it may contain - # sensitive information. - child.logfile_read = f - while True: - index = child.expect(expect_list) - LOG.debug( - "Juju add-machine: expect got regex related to " - f"{expect_list[index]}" - ) - if index == 0: - child.sendline("yes") - elif index == 1: - result = child.before.decode() - if "ERROR" in result: - str_index = result.find("ERROR") - return Result(ResultType.FAILED, result[str_index:]) - - LOG.debug("Add machine successful") - break - - # TODO(hemanth): Need to wait until machine comes to started state - # from planned state? + + with pexpect.spawn( + self._get_juju_binary(), + ["add-machine", "-m", self.model_with_owner, f"ssh:{self.machine_ip}"], + PEXPECT_TIMEOUT * 5, # 5 minutes + ) as child: try: - machine = self._wait_for_machine(self.machine_ip) - except ValueError: - # respond with machine id as -1 if machine is not reflected in juju - machine = "-1" - - return Result(ResultType.COMPLETED, machine) - except pexpect.TIMEOUT as e: - LOG.exception("Error adding machine {self.machine_ip} to Juju") - LOG.warning(e) - return Result(ResultType.FAILED, "TIMED OUT to add machine") + with open(log_file, "wb+") as f: + # Record the command output, but only the contents streaming + # from the process, don't record anything sent to the process + # as it may contain sensitive information. + child.logfile_read = f + while True: + index = child.expect(expect_list) + LOG.debug( + "Juju add-machine: expect got regex related to %r", + expect_list[index], + ) + if index == 0: + child.sendline("yes") + elif index == 1: + result = child.before.decode() + if "ERROR" in result: + str_index = result.find("ERROR") + return Result(ResultType.FAILED, result[str_index:]) + + LOG.debug("Add machine successful") + break + except pexpect.TIMEOUT: + LOG.warning("Timeout adding machine %s to Juju", self.machine_ip) + return Result(ResultType.FAILED, "TIMED OUT to add machine") + + # TODO(hemanth): Need to wait until machine comes to started state + # from planned state? + try: + machine = self._wait_for_machine(self.machine_ip) + except ValueError: + # respond with machine id as -1 if machine is not reflected in juju + machine = "-1" + + return Result(ResultType.COMPLETED, machine) class RemoveJujuMachineStep(BaseStep, JujuStepHelper): @@ -1481,8 +1483,8 @@ class JujuLoginStep(BaseStep, JujuStepHelper): def __init__(self, juju_account: JujuAccount | None, controller: str | None = None): super().__init__( - "Login to Juju controller: %s" % (controller or "current"), - "Authenticating with Juju controller: %s" % (controller or "current"), + f"Login to Juju controller: {controller or 'current'}", + f"Authenticating with Juju controller: {controller or 'current'}", ) self.juju_account = juju_account self.controller = controller @@ -1505,15 +1507,16 @@ def is_skip(self, context: StepContext) -> Result: ) if self.controller: cmd += f" -c {self.controller}" - LOG.debug(f"Running command {cmd}") expect_list = ["^please enter password", "{}", pexpect.EOF] + + LOG.debug("Running command %s", cmd) with pexpect.spawn(cmd) as process: try: index = process.expect(expect_list, timeout=PEXPECT_TIMEOUT) except pexpect.TIMEOUT as e: LOG.debug("Process timeout") return Result(ResultType.FAILED, str(e)) - LOG.debug(f"Command stdout={process.before}") + LOG.debug("Command stdout=%s", process.before) if index in (0, 1): return Result(ResultType.COMPLETED) elif index == 2: @@ -1543,20 +1546,21 @@ def run(self, context: StepContext) -> Result: ) if self.controller: cmd += f" -c {self.controller}" - LOG.debug(f"Running command {cmd}") - process = pexpect.spawn(cmd) - try: - process.expect("^please enter password", timeout=PEXPECT_TIMEOUT) - process.sendline(self.juju_account.password) - process.expect(pexpect.EOF, timeout=PEXPECT_TIMEOUT) - process.close() - except pexpect.TIMEOUT as e: - LOG.debug("Process timeout") - return Result(ResultType.FAILED, str(e)) - LOG.debug(f"Command stdout={process.before}") - if process.exitstatus != 0: - return Result(ResultType.FAILED, "Failed to login to Juju Controller") - return Result(ResultType.COMPLETED) + + LOG.debug("Running command %s", cmd) + with pexpect.spawn(cmd) as process: + try: + process.expect("^please enter password", timeout=PEXPECT_TIMEOUT) + process.sendline(self.juju_account.password) + process.expect(pexpect.EOF, timeout=PEXPECT_TIMEOUT) + except pexpect.TIMEOUT as e: + LOG.debug("Process timeout", exc_info=True) + return Result(ResultType.FAILED, str(e)) + + LOG.debug("Command stdout=%r", process.before) + if process.exitstatus != 0: + return Result(ResultType.FAILED, "Failed to login to Juju Controller") + return Result(ResultType.COMPLETED) class AddJujuModelStep(BaseStep): diff --git a/sunbeam-python/tests/unit/sunbeam/steps/test_juju.py b/sunbeam-python/tests/unit/sunbeam/steps/test_juju.py index 3ef07d6f1..c31991cc8 100644 --- a/sunbeam-python/tests/unit/sunbeam/steps/test_juju.py +++ b/sunbeam-python/tests/unit/sunbeam/steps/test_juju.py @@ -122,7 +122,13 @@ def test_run(self, step_context): assert step.is_skip(step_context).result_type == ResultType.COMPLETED with patch( - "sunbeam.steps.juju.pexpect.spawn", Mock(return_value=Mock(exitstatus=0)) + "sunbeam.steps.juju.pexpect.spawn", + Mock( + return_value=Mock( + __enter__=Mock(return_value=Mock(exitstatus=0)), + __exit__=Mock(return_value=False), + ) + ), ): result = step.run(step_context) assert result.result_type == ResultType.COMPLETED @@ -144,7 +150,13 @@ def test_run_pexpect_timeout(self, step_context): "sunbeam.steps.juju.pexpect.spawn", Mock( return_value=Mock( - exitstatus=0, expect=Mock(side_effect=pexpect.TIMEOUT("timeout")) + __enter__=Mock( + return_value=Mock( + exitstatus=0, + expect=Mock(side_effect=pexpect.TIMEOUT("timeout")), + ) + ), + __exit__=Mock(return_value=False), ) ), ): @@ -165,7 +177,13 @@ def test_run_pexpect_failed_exitcode(self, step_context): assert step.is_skip(step_context).result_type == ResultType.COMPLETED with patch( - "sunbeam.steps.juju.pexpect.spawn", Mock(return_value=Mock(exitstatus=1)) + "sunbeam.steps.juju.pexpect.spawn", + Mock( + return_value=Mock( + __enter__=Mock(return_value=Mock(exitstatus=1)), + __exit__=Mock(return_value=False), + ) + ), ): result = step.run(step_context) assert result.result_type == ResultType.FAILED From c23920ca3699dcf78ca06f876f847daeec178c83 Mon Sep 17 00:00:00 2001 From: Himawan Winarto Date: Fri, 1 May 2026 08:17:14 -0400 Subject: [PATCH 4/4] refactor: consolidate Juju subprocess calls Signed-off-by: Himawan Winarto --- sunbeam-python/sunbeam/core/juju.py | 123 +++++------ .../sunbeam/features/validation/feature.py | 56 ++--- sunbeam-python/sunbeam/steps/juju.py | 198 ++++-------------- .../tests/unit/sunbeam/steps/test_juju.py | 51 ++--- 4 files changed, 142 insertions(+), 286 deletions(-) diff --git a/sunbeam-python/sunbeam/core/juju.py b/sunbeam-python/sunbeam/core/juju.py index 96a7c9d72..f8a45104e 100644 --- a/sunbeam-python/sunbeam/core/juju.py +++ b/sunbeam-python/sunbeam/core/juju.py @@ -1875,6 +1875,8 @@ def get_relation_map( class JujuStepHelper: + """Base class for Juju step.""" + jhelper: JujuHelper def _get_juju_binary(self) -> str: @@ -1883,12 +1885,12 @@ def _get_juju_binary(self) -> str: juju_binary = snap.paths.snap / "juju" / "bin" / "juju" return str(juju_binary) - def _juju_cmd(self, *args): + def _juju_cmd(self, *args, json_format=True, env=None, cwd=None, timeout=None): """Runs the specified juju command line command. - The command will be run using the json formatter. Invoking functions - do not need to worry about the format or the juju command that should - be used. + The command will be run using the json formatter by default. Invoking + functions do not need to worry about the format or the juju command + that should be used. For example, to run the juju bootstrap k8s, this method should be invoked as: @@ -1896,20 +1898,39 @@ def _juju_cmd(self, *args): self._juju_cmd('bootstrap', 'k8s') Any results from running with json are returned after being parsed. + When json_format is False, the subprocess.CompletedProcess output is returned. + Subprocess execution errors are raised to the calling code. :param args: command to run - :return: + :param json_format: if True, add ``--format json`` and parse output + :param env: optional environment variables for the subprocess + :param cwd: optional working directory for the subprocess + :param timeout: optional timeout in seconds for the subprocess + :return: parsed JSON (dict/list) if json_format, else CompletedProcess """ cmd = [self._get_juju_binary()] cmd.extend(args) - cmd.extend(["--format", "json"]) - - LOG.debug(f"Running command {' '.join(cmd)}") - process = subprocess.run(cmd, capture_output=True, text=True, check=True) - LOG.debug(f"Command finished. stdout={process.stdout}, stderr={process.stderr}") + if json_format: + cmd.extend(["--format", "json"]) + + LOG.debug("Running command %s", " ".join(cmd)) + process = subprocess.run( + cmd, + capture_output=True, + text=True, + check=True, + env=env, + cwd=cwd, + timeout=timeout, + ) + LOG.debug( + "Command finished. stdout=%r, stderr=%r", process.stdout, process.stderr + ) - return json.loads(process.stdout.strip()) + if json_format: + return json.loads(process.stdout.strip()) + return process def get_clouds( self, cloud_type: str, local: bool = False, controller: str | None = None @@ -2020,21 +2041,10 @@ def add_cloud(self, name: str, cloud: dict, controller: str | None) -> bool: with tempfile.NamedTemporaryFile() as temp: temp.write(yaml.dump(cloud).encode("utf-8")) temp.flush() - cmd = [ - self._get_juju_binary(), - "add-cloud", - name, - "--file", - temp.name, - "--client", - ] + args = ["add-cloud", name, "--file", temp.name, "--client"] if controller: - cmd.extend(["--controller", controller, "--force"]) - LOG.debug(f"Running command {' '.join(cmd)}") - process = subprocess.run(cmd, capture_output=True, text=True, check=True) - LOG.debug( - f"Command finished. stdout={process.stdout}, stderr={process.stderr}" - ) + args.extend(["--controller", controller, "--force"]) + self._juju_cmd(*args, json_format=False) return True @@ -2043,22 +2053,15 @@ def add_k8s_cloud_in_client(self, name: str, kubeconfig: dict): with tempfile.NamedTemporaryFile() as temp: temp.write(yaml.dump(kubeconfig).encode("utf-8")) temp.flush() - cmd = [ - self._get_juju_binary(), + env = os.environ.copy() + env.update({"KUBECONFIG": temp.name}) + self._juju_cmd( "add-k8s", name, "--client", "--region=localhost/localhost", - ] - - env = os.environ.copy() - env.update({"KUBECONFIG": temp.name}) - LOG.debug(f"Running command {' '.join(cmd)}") - process = subprocess.run( - cmd, capture_output=True, text=True, check=True, env=env - ) - LOG.debug( - f"Command finished. stdout={process.stdout}, stderr={process.stderr}" + json_format=False, + env=env, ) def add_credential(self, cloud: str, credential: dict, controller: str | None): @@ -2070,22 +2073,12 @@ def add_credential(self, cloud: str, credential: dict, controller: str | None): with tempfile.NamedTemporaryFile() as temp: temp.write(yaml.dump(credential).encode("utf-8")) temp.flush() - cmd = [ - self._get_juju_binary(), - "add-credential", - cloud, - "--file", - temp.name, - ] + args = ["add-credential", cloud, "--file", temp.name] if controller: - cmd.extend(["--controller", controller]) + args.extend(["--controller", controller]) else: - cmd.extend(["--client"]) - LOG.debug(f"Running command {' '.join(cmd)}") - process = subprocess.run(cmd, capture_output=True, text=True, check=True) - LOG.debug( - f"Command finished. stdout={process.stdout}, stderr={process.stderr}" - ) + args.extend(["--client"]) + self._juju_cmd(*args, json_format=False) def integrate( self, @@ -2095,19 +2088,9 @@ def integrate( ignore_error_if_exists: bool = True, ): """Juju integrate applications.""" - cmd = [ - self._get_juju_binary(), - "integrate", - "-m", - model, - provider, - requirer, - ] try: - LOG.debug(f"Running command {' '.join(cmd)}") - process = subprocess.run(cmd, capture_output=True, text=True, check=True) - LOG.debug( - f"Command finished. stdout={process.stdout}, stderr={process.stderr}" + self._juju_cmd( + "integrate", "-m", model, provider, requirer, json_format=False ) except subprocess.CalledProcessError as e: LOG.debug(e.stderr) @@ -2116,17 +2099,9 @@ def integrate( def remove_relation(self, model: str, provider: str, requirer: str): """Juju remove relation.""" - cmd = [ - self._get_juju_binary(), - "remove-relation", - "-m", - model, - provider, - requirer, - ] - LOG.debug(f"Running command {' '.join(cmd)}") - process = subprocess.run(cmd, capture_output=True, text=True, check=True) - LOG.debug(f"Command finished. stdout={process.stdout}, stderr={process.stderr}") + self._juju_cmd( + "remove-relation", "-m", model, provider, requirer, json_format=False + ) def get_charm_deployed_versions(self, model: str) -> dict: """Return charm deployed info for all the applications in model. diff --git a/sunbeam-python/sunbeam/features/validation/feature.py b/sunbeam-python/sunbeam/features/validation/feature.py index 984aaf27d..f1ce94ebb 100644 --- a/sunbeam-python/sunbeam/features/validation/feature.py +++ b/sunbeam-python/sunbeam/features/validation/feature.py @@ -24,6 +24,7 @@ ActionFailedException, ApplicationNotFoundException, JujuHelper, + JujuStepHelper, LeaderNotFoundException, UnitNotFoundException, ) @@ -245,7 +246,7 @@ def run(self, context: StepContext) -> Result: return Result(ResultType.COMPLETED) -class ValidationFeature(OpenStackControlPlaneFeature): +class ValidationFeature(OpenStackControlPlaneFeature, JujuStepHelper): """Deploy tempest to openstack model.""" version = Version(FEATURE_VERSION) @@ -380,21 +381,17 @@ def _check_file_exist_in_tempest_container( # since python-libjuju does not support such feature. See related # bug: https://github.com/juju/python-libjuju/issues/1029 try: - subprocess.run( - [ - "juju", - "ssh", - "--model", - model_name, - "--container", - TEMPEST_CONTAINER_NAME, - unit, - "ls", - TEMPEST_VALIDATION_RESULT, - ], - check=True, + self._juju_cmd( + "ssh", + "--model", + model_name, + "--container", + TEMPEST_CONTAINER_NAME, + unit, + "ls", + TEMPEST_VALIDATION_RESULT, + json_format=False, timeout=30, # 30 seconds should be enough for `ls` - capture_output=True, ) except subprocess.CalledProcessError: return False @@ -416,13 +413,7 @@ def _copy_file_from_tempest_container( with console.status(progress_message): # Note: this is a workaround to cache the model in the juju client try: - subprocess.run( - ["juju", "show-model", model_name], - stdout=subprocess.DEVNULL, - stderr=subprocess.DEVNULL, - check=True, - timeout=30, - ) + self._juju_cmd("show-model", model_name, json_format=False, timeout=30) except subprocess.TimeoutExpired: raise click.ClickException("Timed out while priming Juju model cache") # Note: this is a workaround to run command to payload container @@ -432,18 +423,15 @@ def _copy_file_from_tempest_container( # juju scp does not allow directory as destination destination = str(Path(destination, Path(source).name)) try: - subprocess.run( - [ - "juju", - "scp", - "--model", - model_name, - "--container", - TEMPEST_CONTAINER_NAME, - f"{unit}:{source}", - destination, - ], - check=True, + self._juju_cmd( + "scp", + "--model", + model_name, + "--container", + TEMPEST_CONTAINER_NAME, + f"{unit}:{source}", + destination, + json_format=False, timeout=60, # 60 seconds should be enough for copying a file ) except (subprocess.CalledProcessError, subprocess.TimeoutExpired) as e: diff --git a/sunbeam-python/sunbeam/steps/juju.py b/sunbeam-python/sunbeam/steps/juju.py index 12672ca4d..6448e2392 100644 --- a/sunbeam-python/sunbeam/steps/juju.py +++ b/sunbeam-python/sunbeam/steps/juju.py @@ -313,23 +313,16 @@ def run(self, context: StepContext) -> Result: config_file.write(yaml.dump(config_args).encode("utf-8")) config_file.flush() - cmd = [self._get_juju_binary(), "bootstrap"] - cmd.extend(bootstrap_args_without_configs) - cmd.extend([self.cloud, self.controller]) - cmd.extend(["--config", config_file.name]) + args = ["bootstrap"] + args.extend(bootstrap_args_without_configs) + args.extend([self.cloud, self.controller]) + args.extend(["--config", config_file.name]) env = os.environ.copy() env.update(self.proxy_settings) - LOG.debug(f"Running command {' '.join(cmd)}") LOG.debug(f"Bootstrap configs used: {configs_to_print}") - process = subprocess.run( - cmd, capture_output=True, text=True, check=True, env=env - ) - LOG.debug( - f"Command finished. stdout={process.stdout}, " - f"stderr={process.stderr}" - ) + self._juju_cmd(*args, json_format=False, env=env) return Result(ResultType.COMPLETED) except subprocess.CalledProcessError as e: @@ -360,18 +353,12 @@ def is_skip(self, context: StepContext) -> Result: def run(self, context: StepContext) -> Result: """Enable Juju HA.""" - cmd = [ - self._get_juju_binary(), - "enable-ha", - "-n", - str(self.n), - *self.extra_args, - ] - LOG.debug(f"Running command {' '.join(cmd)}") - process = subprocess.run(cmd, capture_output=True, text=True, check=True) - LOG.debug(f"Command finished. stdout={process.stdout}, stderr={process.stderr}") - cmd = [ - self._get_juju_binary(), + self._juju_cmd( + "enable-ha", "-n", str(self.n), *self.extra_args, json_format=False + ) + self.update_status(context, "scaling controller") + LOG.debug("Waiting for HA to be enabled") + self._juju_cmd( "wait-for", "application", "-m", @@ -379,12 +366,8 @@ def run(self, context: StepContext) -> Result: "controller", "--timeout", self.wait_timeout, - ] - self.update_status(context, "scaling controller") - LOG.debug("Waiting for HA to be enabled") - LOG.debug(f"Running command {' '.join(cmd)}") - process = subprocess.run(cmd, capture_output=True, text=True, check=True) - LOG.debug(f"Command finished. stdout={process.stdout}, stderr={process.stderr}") + json_format=False, + ) return Result(ResultType.COMPLETED) @@ -420,8 +403,7 @@ def is_skip(self, context: StepContext) -> Result: def run(self, context: StepContext) -> Result: """Destroy juju-controller.""" - cmd = [ - self._get_juju_binary(), + args = [ "destroy-controller", "--destroy-all-models", "--destroy-storage", @@ -429,14 +411,13 @@ def run(self, context: StepContext) -> Result: self.deployment.controller, ] if self.destroy_args: - cmd.extend(self.destroy_args) + args.extend(self.destroy_args) try: - process = subprocess.run(cmd, capture_output=True, text=True, check=True) + self._juju_cmd(*args, json_format=False) except subprocess.CalledProcessError as e: LOG.exception("Error destroying controller") return Result(ResultType.FAILED, str(e)) - LOG.debug(f"Command finished. stdout={process.stdout}, stderr={process.stderr}") return Result(ResultType.COMPLETED) @@ -477,12 +458,7 @@ def run(self, context: StepContext) -> Result: :return: """ try: - cmd = [self._get_juju_binary(), "add-user", self.username] - LOG.debug(f"Running command {' '.join(cmd)}") - process = subprocess.run(cmd, capture_output=True, text=True, check=True) - LOG.debug( - f"Command finished. stdout={process.stdout}, stderr={process.stderr}" - ) + process = self._juju_cmd("add-user", self.username, json_format=False) re_groups = re.search( self.registration_token_regex, process.stdout, re.MULTILINE @@ -494,26 +470,12 @@ def run(self, context: StepContext) -> Result: return Result(ResultType.FAILED, "Not able to parse registration token") # Grant superuser access to user. - cmd = [self._get_juju_binary(), "grant", self.username, "superuser"] - LOG.debug(f"Running command {' '.join(cmd)}") - process = subprocess.run(cmd, capture_output=True, text=True, check=True) - LOG.debug( - f"Command finished. stdout={process.stdout}, stderr={process.stderr}" - ) + self._juju_cmd("grant", self.username, "superuser", json_format=False) # Grant write access to controller model # Without this step, the user is not able to view controller model - cmd = [ - self._get_juju_binary(), - "grant", - self.username, - "admin", - CONTROLLER_MODEL, - ] - LOG.debug(f"Running command {' '.join(cmd)}") - process = subprocess.run(cmd, capture_output=True, text=True, check=True) - LOG.debug( - f"Command finished. stdout={process.stdout}, stderr={process.stderr}" + self._juju_cmd( + "grant", self.username, "admin", CONTROLLER_MODEL, json_format=False ) return Result(ResultType.COMPLETED, message=token) @@ -554,16 +516,8 @@ def run(self, context: StepContext) -> Result: return Result(ResultType.FAILED, str(e)) try: - cmd = [ - self._get_juju_binary(), - "change-user-password", - self.username, - "--reset", - ] - LOG.debug(f"Running command {' '.join(cmd)}") - process = subprocess.run(cmd, capture_output=True, text=True, check=True) - LOG.debug( - f"Command finished. stdout={process.stdout}, stderr={process.stderr}" + process = self._juju_cmd( + "change-user-password", self.username, "--reset", json_format=False ) re_groups = re.search(self.reset_token_regex, process.stderr, re.MULTILINE) @@ -612,19 +566,8 @@ def run(self, context: StepContext) -> Result: # Grant write access to the model # Without this step, the user is not able to view the model created # by other users. - cmd = [ - self._get_juju_binary(), - "grant", - self.username, - "admin", - model_with_owner, - ] - LOG.debug("Running command %r", " ".join(cmd)) - process = subprocess.run(cmd, capture_output=True, text=True, check=True) - LOG.debug( - "Command finished. stdout=%r, stderr=%r", - process.stdout, - process.stderr, + self._juju_cmd( + "grant", self.username, "admin", model_with_owner, json_format=False ) return Result(ResultType.COMPLETED) @@ -684,12 +627,7 @@ def run(self, context: StepContext) -> Result: :return: """ try: - cmd = [self._get_juju_binary(), "remove-user", self.username, "--yes"] - LOG.debug(f"Running command {' '.join(cmd)}") - process = subprocess.run(cmd, capture_output=True, text=True, check=True) - LOG.debug( - f"Command finished. stdout={process.stdout}, stderr={process.stderr}" - ) + self._juju_cmd("remove-user", self.username, "--yes", json_format=False) return Result(ResultType.COMPLETED) except subprocess.CalledProcessError as e: @@ -784,19 +722,8 @@ def run(self, context: StepContext) -> Result: # If controller exists with same name, unregister the controller. try: self.get_controller(self.controller) - cmd = [ - self._get_juju_binary(), - "unregister", - self.controller, - "--no-prompt", - ] - LOG.debug(f"Running command {' '.join(cmd)}") - process = subprocess.run( - cmd, capture_output=True, text=True, check=True - ) - LOG.debug( - f"Command finished. stdout={process.stdout}, " - f"stderr={process.stderr}" + self._juju_cmd( + "unregister", self.controller, "--no-prompt", json_format=False ) except ControllerNotFoundException: LOG.debug("No controller found to replace, register the juju user") @@ -988,16 +915,8 @@ def run(self, context: StepContext) -> Result: :return: """ try: - cmd = [ - self._get_juju_binary(), - "unregister", - self.controller, - "--no-prompt", - ] - LOG.debug(f"Running command {' '.join(cmd)}") - process = subprocess.run(cmd, capture_output=True, text=True, check=True) - LOG.debug( - f"Command finished. stdout={process.stdout}, stderr={process.stderr}" + self._juju_cmd( + "unregister", self.controller, "--no-prompt", json_format=False ) self.account_file.unlink(missing_ok=True) except subprocess.CalledProcessError as e: @@ -1176,18 +1095,13 @@ def run(self, context: StepContext) -> Result: "Not able to retrieve machine id from Cluster database", ) - cmd = [ - self._get_juju_binary(), + self._juju_cmd( "remove-machine", "-m", self.model_with_owner, str(self.machine_id), "--no-prompt", - ] - LOG.debug(f"Running command {' '.join(cmd)}") - process = subprocess.run(cmd, capture_output=True, text=True, check=True) - LOG.debug( - f"Command finished. stdout={process.stdout}, stderr={process.stderr}" + json_format=False, ) return Result(ResultType.COMPLETED) @@ -1699,28 +1613,18 @@ def run(self, context: StepContext) -> Result: for charm_file in download_dir.glob("juju-controller*.charm"): charm_file.unlink() - cmd = [ - self._get_juju_binary(), + env = os.environ.copy() + env.update(self.proxy_settings) + self._juju_cmd( "download", "juju-controller", "--channel", JUJU_CHANNEL, "--base", JUJU_BASE, - ] - LOG.debug(f"Running command {' '.join(cmd)}") - env = os.environ.copy() - env.update(self.proxy_settings) - process = subprocess.run( - cmd, - capture_output=True, - cwd=download_dir, - text=True, - check=True, + json_format=False, env=env, - ) - LOG.debug( - f"Command finished. stdout={process.stdout}, stderr={process.stderr}" + cwd=download_dir, ) for charm_file in download_dir.glob("juju-controller*.charm"): @@ -1973,12 +1877,7 @@ def __init__( def run(self, context: StepContext) -> Result: """Switch to juju controller.""" try: - cmd = [self._get_juju_binary(), "switch", self.controller] - LOG.debug(f"Running command {' '.join(cmd)}") - process = subprocess.run(cmd, capture_output=True, text=True, check=True) - LOG.debug( - f"Command finished. stdout={process.stdout}, stderr={process.stderr}" - ) + self._juju_cmd("switch", self.controller, json_format=False) except subprocess.CalledProcessError as e: LOG.exception(f"Error in switching the controller to {self.controller}") return Result(ResultType.FAILED, str(e)) @@ -2211,10 +2110,7 @@ def __init__( self.to_controller = to_controller def _switch_controller(self, controller: str): - cmd = [self._get_juju_binary(), "switch", controller] - LOG.debug(f"Running command {' '.join(cmd)}") - process = subprocess.run(cmd, capture_output=True, text=True, check=True) - LOG.debug(f"Command finished. stdout={process.stdout}, stderr={process.stderr}") + self._juju_cmd("switch", controller, json_format=False) def _get_model_owner(self, model: str, controller: str) -> str: """Determine model owner.""" @@ -2287,17 +2183,13 @@ def _migrate_model( to_controller: str, ): """Migrate and monitor model migration status.""" - cmd = [ - self._get_juju_binary(), - "migrate", - f"{from_controller}:{owner}/{model}", - to_controller, - ] - - LOG.debug(f"Running command {' '.join(cmd)}") - try: - process = subprocess.run(cmd, capture_output=True, text=True, check=True) + self._juju_cmd( + "migrate", + f"{from_controller}:{owner}/{model}", + to_controller, + json_format=False, + ) except subprocess.CalledProcessError as e: LOG.exception( f"Error in migrating model {self.model}" @@ -2307,8 +2199,6 @@ def _migrate_model( raise JujuMigrationFailedError("Juju migrate command failed") from e - LOG.debug(f"Command finished. stdout={process.stdout}, stderr={process.stderr}") - # Check migration status is_model_migrated = self._wait_for_model( model, owner, from_controller, to_controller diff --git a/sunbeam-python/tests/unit/sunbeam/steps/test_juju.py b/sunbeam-python/tests/unit/sunbeam/steps/test_juju.py index c31991cc8..1cc195e3a 100644 --- a/sunbeam-python/tests/unit/sunbeam/steps/test_juju.py +++ b/sunbeam-python/tests/unit/sunbeam/steps/test_juju.py @@ -550,13 +550,12 @@ def test_is_skip(self, step_context): result = step.is_skip(step_context) assert result.result_type == ResultType.COMPLETED - @patch("subprocess.run") - def test_run(self, mock_run, mocker, step_context): + def test_run(self, mocker, step_context): step = juju.ScaleJujuStep("controller", n=3, extra_args=["--arg1", "--arg2"]) - mocker.patch.object(step, "_get_juju_binary", return_value="/juju-mock") + mocker.patch.object(step, "_juju_cmd") result = step.run(step_context) - assert mock_run.call_count == 2 + assert step._juju_cmd.call_count == 2 assert result.result_type == ResultType.COMPLETED @@ -637,20 +636,22 @@ def test_is_skip_controller_not_registered(self, mocker, tmp_path, step_context) result = step.is_skip(step_context) assert result.result_type == ResultType.SKIPPED - @patch("subprocess.run") - def test_run(self, mock_run, mocker, tmp_path, step_context): + def test_run(self, mocker, tmp_path, step_context): step = juju.UnregisterJujuController("testcontroller", tmp_path) - mocker.patch.object(step, "_get_juju_binary", return_value="/juju-mock") + mocker.patch.object(step, "_juju_cmd") result = step.run(step_context) - assert mock_run.call_count == 1 + assert step._juju_cmd.call_count == 1 assert result.result_type == ResultType.COMPLETED - @patch("subprocess.run", side_effect=subprocess.CalledProcessError(1, "command")) - def test_run_unregister_failed(self, mock_run, mocker, tmp_path, step_context): + def test_run_unregister_failed(self, mocker, tmp_path, step_context): step = juju.UnregisterJujuController("testcontroller", tmp_path) - mocker.patch.object(step, "_get_juju_binary", return_value="/juju-mock") + mocker.patch.object( + step, + "_juju_cmd", + side_effect=subprocess.CalledProcessError(1, "command"), + ) result = step.run(step_context) - assert mock_run.call_count == 1 + assert step._juju_cmd.call_count == 1 assert result.result_type == ResultType.FAILED @@ -808,26 +809,28 @@ def test_run_when_boostrap_failed(self, mocker, snap, run, cclient, step_context class TestResetJujuUserStep: - @patch("subprocess.run") - def test_parses_token_success(self, mock_run, step_context): + def test_parses_token_success(self, step_context): step = juju.ResetJujuUserStep("test-user") - step._juju_cmd = Mock(return_value=[{"user-name": "test-user"}]) - step._get_juju_binary = Mock(return_value="/juju") - mock_run.return_value = Mock(stdout="", stderr="juju register new-user\n") + step._juju_cmd = Mock( + side_effect=[ + [{"user-name": "test-user"}], + Mock(stdout="", stderr="juju register new-user\n"), + ] + ) result = step.run(step_context) assert result.result_type == ResultType.COMPLETED assert result.message == "new-user" - @patch( - "subprocess.run", - side_effect=subprocess.CalledProcessError(1, "command", stderr="error"), - ) - def test_called_process_error(self, mock_run, step_context): + def test_called_process_error(self, step_context): step = juju.ResetJujuUserStep("test-user") - step._juju_cmd = Mock(return_value=[{"user-name": "test-user"}]) + step._juju_cmd = Mock( + side_effect=[ + [{"user-name": "test-user"}], + subprocess.CalledProcessError(1, "command", stderr="error"), + ] + ) result = step.run(step_context) assert result.result_type == ResultType.FAILED - assert mock_run.call_count == 1 def test_user_not_found(self, step_context): step = juju.ResetJujuUserStep("missing-user")