Merge ~jfguedez/juju-lint:bug/1903973 into juju-lint:master

[Jose Guedez]

Add the "metal only" condition for subordinates

[Jose Guedez (jfguedez)]

Add the "metal only" condition for subordinates

[🤖 Canonical IS Merge Bot (canonical-is-mergebot)]

This merge proposal is being monitored by mergebot. Change the status to Approved to merge.

[Edin S (exsdev)]

review: Approve

[James Troup (elmo)]

I had to thought check MAC addresses, FWIW; it's also a heuristic, but a fairly reliable one for detecting the most common virt platforms (KVM/VMWare).

diff --git a/contrib/includes/base.yaml b/contrib/includes/base.yaml
index 1815ee2..da558a3 100644
--- a/contrib/includes/base.yaml
+++ b/contrib/includes/base.yaml
@@ -35,6 +35,6 @@ subordinates:
  thruk-agent:
   where: on nagios
  hw-health:
-  where: host only
+  where: metal only
  logrotated:
   where: all
diff --git a/jujulint/lint.py b/jujulint/lint.py
index 68fef78..f680370 100755
--- a/jujulint/lint.py
+++ b/jujulint/lint.py
@@ -33,7 +33,7 @@ import attr
 import dateutil.parser
 from dateutil import relativedelta
 
-from jujulint.util import flatten_list, is_container, extract_charm_name
+import jujulint.util as utils
 from jujulint.logging import Logger
 
 VALID_CONFIG_CHECKS = ("isset", "eq", "neq", "gte")
@@ -133,7 +133,9 @@ class Linter:
                     self.lint_rules["subordinates"][name] = dict(where=where)
 
             # Flatten all entries (to account for nesting due to YAML anchors (templating)
-            self.lint_rules = {k: flatten_list(v) for k, v in self.lint_rules.items()}
+            self.lint_rules = {
+                k: utils.flatten_list(v) for k, v in self.lint_rules.items()
+            }
 
             self._log_with_header(
                 "Lint Rules: {}".format(pprint.pformat(self.lint_rules))
@@ -372,7 +374,7 @@ class Linter:
                 )
                 continue
 
-            charm_name = extract_charm_name(applications[application]["charm"])
+            charm_name = utils.extract_charm_name(applications[application]["charm"])
             if "config" in self.lint_rules:
                 if charm_name in self.lint_rules["config"]:
                     lint_rules = self.lint_rules["config"][charm_name].items()
@@ -393,7 +395,7 @@ class Linter:
                         lint_rules,
                     )
 
-    def check_subs(self):
+    def check_subs(self, machines_data):
         """Check the subordinates in the model."""
         all_or_nothing = set()
         for machine in self.model.subs_on_machines:
@@ -429,7 +431,7 @@ class Linter:
                     self._log_with_header(
                         "requirement is 'host only' form...."
                     )
-                    if is_container(machine):
+                    if utils.is_container(machine):
                         self._log_with_header("... and we are a container, checking")
                         # XXX check alternate names?
                         if required_sub in present_subs:
@@ -438,6 +440,18 @@ class Linter:
                                 self.model.extraneous_subs[required_sub].add(app)
                         continue
                     self._log_with_header("... and we are a host, will fallthrough")
+                elif where == "metal only":
+                    self._log_with_header(
+                        "requirement is 'metal only' form...."
+                    )
+                    if not utils.is_metal(machine, machines_data.get(machine, {})):
+                        self._log_with_header("... and we are not a metal, checking")
+                        if required_sub in present_subs:
+                            self._log_with_header("... found extraneous sub")
+                            for app in self.model.apps_on_machines[machine]:
+                                self.model.extraneous_subs[required_sub].add(app)
+                        continue
+                    self._log_with_header("... and we are a metal, will fallthrough")
 
                 elif where == "all or nothing" and required_sub not in all_or_nothing:
                     self._log_with_header(
@@ -448,7 +462,7 @@ class Linter:
                 # need to change the name we expect to see it as
                 elif where == "container aware":
                     self._log_with_header("requirement is 'container aware'.")
-                    if is_container(machine):
+                    if utils.is_container(machine):
                         suffixes = self.lint_rules["subordinates"][required_sub][
                             "container-suffixes"
                         ]
@@ -719,7 +733,7 @@ class Linter:
         """Process applications in the model, validating and normalising the names."""
         for app in applications:
             if "charm" in applications[app]:
-                charm_name = extract_charm_name(applications[app]["charm"])
+                charm_name = utils.extract_charm_name(applications[app]["charm"])
                 self.model.charms.add(charm_name)
                 self.model.app_to_charm[app] = charm_name
             else:
@@ -973,7 +987,7 @@ class Linter:
             for app in parsed_yaml[applications]:
                 self.process_subordinates(parsed_yaml[applications][app], app)
 
-            self.check_subs()
+            self.check_subs(parsed_yaml["machines"])
             self.check_charms()
 
             if "relations" not in parsed_yaml:
diff --git a/jujulint/util.py b/jujulint/util.py
index c6eb80e..fc45ee6 100644
--- a/jujulint/util.py
+++ b/jujulint/util.py
@@ -50,13 +50,35 @@ def is_container(machine):
         return False
 
 
+def is_virtual_machine(machine, machine_data):
+    """
+    Check if a provided machine is a VM.
+
+    It is not straightforward to determine if a machine is a VM from juju data
+    (bundle/juju status). In some cases a "hardware" key is provided (jsfy),
+    and in those cases we can check for the keyword "virtual" since some
+    provisioners include a tag there (FCE). We use that criteria as a best
+    effort attempt to determine if the machine is a VM.
+    """
+    hardware = machine_data.get("hardware")
+    return bool(hardware and "virtual" in hardware)
+
+
+def is_metal(machine, machine_data):
+    """
+    Check if a provided machine is a bare metal host.
+
+    Leverages the other detection methods, if the others fail (e.g. not a
+    container or VM), we consider the machine to be bare metal.
+    """
+    return not (is_container(machine) or is_virtual_machine(machine, machine_data))
+
+
 def extract_charm_name(charm):
     """Extract the charm name using regex."""
     match = re.match(
         r"^(?:\w+:)?(?:~[\w\.-]+/)?(?:\w+/)?([a-zA-Z0-9-]+?)(?:-\d+)?$", charm
     )
     if not match:
-        raise InvalidCharmNameError(
-            "charm name '{}' is invalid".format(charm)
-        )
+        raise InvalidCharmNameError("charm name '{}' is invalid".format(charm))
     return match.group(1)
diff --git a/tests/conftest.py b/tests/conftest.py
index ba9ed77..fc7093e 100644
--- a/tests/conftest.py
+++ b/tests/conftest.py
@@ -98,6 +98,12 @@ def juju_status():
                     }
                 },
             },
+            "ntp": {
+                "application-status": {"current": "active"},
+                "charm": "cs:ntp-47",
+                "charm-name": "ntp",
+                "relations": {"juju-info": ["ubuntu"]},
+            },
         },
         "machines": {
             "0": {
diff --git a/tests/test_jujulint.py b/tests/test_jujulint.py
index ce123ee..2494c28 100644
--- a/tests/test_jujulint.py
+++ b/tests/test_jujulint.py
@@ -5,41 +5,59 @@ from datetime import datetime, timezone
 import pytest
 
 
-def test_flatten_list(utils):
-    """Test the utils flatten_list function."""
-    unflattened_list = [1, [2, 3]]
-    flattened_list = [1, 2, 3]
-    assert flattened_list == utils.flatten_list(unflattened_list)
-
-    unflattened_list = [1, [2, [3, 4]]]
-    flattened_list = [1, 2, 3, 4]
-    assert flattened_list == utils.flatten_list(unflattened_list)
-
-
-def test_flatten_list_non_list_iterable(utils):
-    """Test the utils flatten_list function."""
-    iterable = {1: 2}
-    assert iterable == utils.flatten_list(iterable)
-
-
-def test_map_charms(linter, utils):
-    """Test the charm name validation code."""
-    applications = {
-        "test-app-1": {"charm": "cs:~USER/SERIES/TEST-CHARM12-123"},
-        "test-app-2": {"charm": "cs:~USER/TEST-CHARM12-123"},
-        "test-app-3": {"charm": "cs:TEST-CHARM12-123"},
-        "test-app-4": {"charm": "local:SERIES/TEST-CHARM12"},
-        "test-app-5": {"charm": "local:TEST-CHARM12"},
-        "test-app-6": {"charm": "cs:~TEST-CHARMERS/TEST-CHARM12-123"},
-    }
-    linter.map_charms(applications)
-    for charm in linter.model.charms:
-        assert "TEST-CHARM12" == charm
-    applications = {
-        "test-app1": {"charm": "cs:invalid-charm$"},
-    }
-    with pytest.raises(utils.InvalidCharmNameError):
-        linter.map_charms(applications)
+class TestUtils:
+    """Test the jujulint utilities."""
+
+    def test_flatten_list(self, utils):
+        """Test the utils flatten_list function."""
+        unflattened_list = [1, [2, 3]]
+        flattened_list = [1, 2, 3]
+        assert flattened_list == utils.flatten_list(unflattened_list)
+
+        unflattened_list = [1, [2, [3, 4]]]
+        flattened_list = [1, 2, 3, 4]
+        assert flattened_list == utils.flatten_list(unflattened_list)
+
+    def test_flatten_list_non_list_iterable(self, utils):
+        """Test the utils flatten_list function."""
+        iterable = {1: 2}
+        assert iterable == utils.flatten_list(iterable)
+
+    def test_is_container(self, utils):
+        """Test the utils is_container function."""
+        assert utils.is_container("1/lxd/0") is True
+        assert utils.is_container("0") is False
+
+    def test_is_virtual_machine(self, utils):
+        """Test the utils is_virtual_machine function."""
+        machine = "0"
+        machine_data = {
+            "hardware": "arch=amd64 cores=2 mem=4096M tags=virtual,pod-console-logging,vault availability-zone=AZ3"
+        }
+        assert utils.is_virtual_machine(machine, machine_data) is True
+
+        machine_data = {}
+        assert utils.is_virtual_machine(machine, machine_data) is False
+
+    def test_is_metal(self, utils):
+        """Test the utils is_metal function."""
+        # A VM should return false
+        machine = "0"
+        machine_data = {
+            "hardware": "arch=amd64 cores=2 mem=4096M tags=virtual,pod-console-logging,vault availability-zone=AZ3"
+        }
+        assert utils.is_metal(machine, machine_data) is False
+
+        # A container should return false
+        assert utils.is_metal("1/lxd/0", {}) is False
+
+        # A bare metal should return true
+        machine = "1"
+        machine_data = {
+            "hardware": "arch=amd64 cores=128 mem=2093056M tags=foundation-nodes,hyper-converged-az2 "
+            "availability-zone=AZ2"
+        }
+        assert utils.is_metal(machine, machine_data) is True
 
 
 class TestLinter:
@@ -77,6 +95,25 @@ class TestLinter:
         assert errors[0]["id"] == "charm-not-mapped"
         assert errors[0]["application"] == "ubuntu2"
 
+    def test_map_charms(self, linter, utils):
+        """Test the charm name validation code."""
+        applications = {
+            "test-app-1": {"charm": "cs:~USER/SERIES/TEST-CHARM12-123"},
+            "test-app-2": {"charm": "cs:~USER/TEST-CHARM12-123"},
+            "test-app-3": {"charm": "cs:TEST-CHARM12-123"},
+            "test-app-4": {"charm": "local:SERIES/TEST-CHARM12"},
+            "test-app-5": {"charm": "local:TEST-CHARM12"},
+            "test-app-6": {"charm": "cs:~TEST-CHARMERS/TEST-CHARM12-123"},
+        }
+        linter.map_charms(applications)
+        for charm in linter.model.charms:
+            assert "TEST-CHARM12" == charm
+        applications = {
+            "test-app1": {"charm": "cs:invalid-charm$"},
+        }
+        with pytest.raises(utils.InvalidCharmNameError):
+            linter.map_charms(applications)
+
     def test_juju_status_unexpected(self, linter, juju_status):
         """Test that juju and workload status is expected."""
         # inject invalid status to the application
@@ -243,6 +280,37 @@ class TestLinter:
         assert errors[0]["machines"] == "0"
         assert errors[0]["subordinate"] == "ntp"
 
+    def test_ops_subordinate_metal_only1(self, linter, juju_status):
+        """
+        Test that missing ops subordinate charms are detected.
+
+        Use the "metal only" rule in a bare metal machine, should report the
+        missing subordinate
+        """
+        linter.lint_rules["subordinates"]["hw-health"] = {"where": "metal only"}
+        linter.do_lint(juju_status)
+
+        errors = linter.output_collector["errors"]
+        assert len(errors) == 1
+        assert errors[0]["id"] == "ops-subordinate-missing"
+        assert errors[0]["principals"] == "ubuntu"
+        assert errors[0]["subordinate"] == "hw-health"
+
+    def test_ops_subordinate_metal_only2(self, linter, juju_status):
+        """
+        Test that missing ops subordinate charms are detected.
+
+        Use the "metal only" rule in a VM, should ignore it
+        """
+        linter.lint_rules["subordinates"]["hw-health"] = {"where": "metal only"}
+
+        # Turn machine "0" into a "VM"
+        juju_status["machines"]["0"]["hardware"] = "tags=virtual availability-zone=rack-1"
+        linter.do_lint(juju_status)
+
+        errors = linter.output_collector["errors"]
+        assert not errors
+
     def test_openstack_charm_missing(self, linter, juju_status):
         """Test that missing openstack mandatory charms are detected."""
         linter.cloud_type = "openstack"
@@ -286,6 +354,7 @@ class TestLinter:
         linter.cloud_type = "kubernetes"
         linter.lint_rules["kubernetes mandatory"] = []
         linter.lint_rules["operations kubernetes mandatory"] = ["ntp"]
+        juju_status["applications"].pop("ntp")  # drop the app from the model
         linter.do_lint(juju_status)
 
         errors = linter.output_collector["errors"]