Merge ~vultaire/juju-lint:lp1840814 into juju-lint:master

[Paul Goins]

Added relation space checking

Mismatched spaces will result in warnings by default. If appropriate
rules are set, these warnings can either be coerced to errors, or
marked as ignorable.

Original code of jujulint.check_spaces comes from
https://git.launchpad.net/~sbparke/+git/checkSpaces/tree/checkSpaces.py.

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

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

[Paul Goins (vultaire)]

In addition to code feedback, please provide feedback regarding whether the rule options make sense, whether the error/warning message should be reworded, etc.

[James Troup (elmo)]

a) please black-en in a followup MP
b) consider adding an example to the README as to why this matters (e.g. Ceph sending traffic over 1Gb links vs 25Gb links)

Other than that, this looks good to me, thanks!

review: Approve

[Linda Guo (lihuiguo)]

LGTM

review: Approve

[Xav Paice (xavpaice)]

LGTM

review: Approve

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

Change successfully merged at revision 8d75f2145d27bb1aa5cb4f4e93c7c82f2fa8b7f8

diff --git a/.gitignore b/.gitignore
index 1f49ca6..6b085ea 100644
--- a/.gitignore
+++ b/.gitignore
@@ -28,3 +28,6 @@ stage/
 
 # runtime
 output/
+
+# venv
+/venv
diff --git a/README.md b/README.md
index 9058da9..a99f87c 100644
--- a/README.md
+++ b/README.md
@@ -62,7 +62,41 @@ Supported top-level options for your rules file:
  4. `openstack [mandatory|optional|subordinate]`
  5. `config` - application configuration auditing
  6. `[openstack|kubernetes] config` - config auditing for specific cloud types.
- 7. `!include <relative path>` - Extension to yaml to include files.
+ 7. `space checks` - enforce/ignore checks for relation binding space
+    mismatches.
+ 8. `!include <relative path>` - Extension to yaml to include files.
+
+=== Space checks ===
+
+All relations defined within a bundle, except for cross-model relationships,
+will be checked for mismatches of their space bindings.
+
+By default, mismatches are logged as warnings as they are not necessarily
+critical problems.  If applications can route to each other despite the
+mismatch, there may be no real issue here, and it may be appropriate to ignore
+certain issues.  On the other hand, these mismatches may cause problems ranging
+from impaired throughput due to using suboptimal interfaces to breakages due to
+not being able to route between the related units.
+
+The following options are available to either log such mismatches as errors or
+to ignore them entirely:
+
+ 1. `enforce endpoints` - A list of <charm>:<endpoint> strings.  If a mismatch
+    matches one of these endpoints, it will be flagged as an error.
+ 2. `enforce relations` - A list of two-item <charm>:<endpoint> string lists,
+    representing the two linked endpoints of a relation.  If a mismatch
+    matches one of these relations, it will be flagged as an error.
+ 1. `ignore endpoints` - A list of <charm>:<endpoint> strings.  If a mismatch
+    matches one of these endpoints, it will be ignored.
+ 1. `ignore relations` - A list of two-item <charm>:<endpoint> string lists,
+    representing the two linked endpoints of a relation.  If a mismatch
+    matches one of these relations, it will be ignored.
+
+In the case of a mismatch matching both enforce and ignore rules, the enforce
+rule will "win" and it will be flagged as an error.
+
+Note that all the above checks use charm names rather than application names
+in their endpoint strings.
 
 == License ==
 
diff --git a/jujulint/check_spaces.py b/jujulint/check_spaces.py
 new file mode 100644
index 0000000..e9c850b
--- /dev/null
+++ b/jujulint/check_spaces.py
@@ -0,0 +1,135 @@
+#!/usr/bin/python3
+"""Checks for space mismatches between relation endpoints."""
+
+
+class Relation:
+    """Relation object."""
+
+    def __init__(self, endpoint1, endpoint2):
+        """Object for representing relations."""
+        self.endpoint1 = endpoint1
+        self.endpoint2 = endpoint2
+
+    def __str__(self):
+        """Stringify the object."""
+        return "Relation({} - {})".format(self.endpoint1, self.endpoint2)
+
+    def __eq__(self, other):
+        """Compare relations.
+
+        While Juju does define separate provider and requirer roles, we'll ignore
+        those here.
+        """
+        return set([self.endpoint1, self.endpoint2]) == set([other.endpoint1, other.endpoint2])
+
+    @property
+    def endpoints(self):
+        """Return list of endpoints."""
+        return [self.endpoint1, self.endpoint2]
+
+
+class SpaceMismatch:
+    """Object for representing relation space mismatches."""
+
+    def __init__(self, endpoint1, space1, endpoint2, space2):
+        """Create the object."""
+        self.endpoint1 = endpoint1
+        self.endpoint2 = endpoint2
+        self.space1 = space1
+        self.space2 = space2
+
+    def __str__(self):
+        """Stringify the object."""
+        return "SpaceMismatch({} (space {}) != {} (space {}))".format(
+            self.endpoint1, self.space1, self.endpoint2, self.space2)
+
+    @property
+    def relation(self):
+        """Return a relation object based upon application endpoints."""
+        return Relation(self.endpoint1, self.endpoint2)
+
+    def get_charm_relation(self, app_to_charm_map):
+        """Return a relation object, mapping applications to charms."""
+        app1, endpoint1 = self.endpoint1.split(':')
+        app2, endpoint2 = self.endpoint2.split(':')
+        charm1 = app_to_charm_map[app1]
+        charm2 = app_to_charm_map[app2]
+        return Relation(":".join([charm1, endpoint1]), ":".join([charm2, endpoint2]))
+
+
+def find_space_mismatches(parsed_yaml, debug=False):
+    """Enumerate relations and detect space mismatches.
+
+    Returns a list of objects representing the mismatches.
+
+    """
+    application_list = get_juju_applications(parsed_yaml)
+    app_spaces = get_application_spaces(application_list, parsed_yaml)
+
+    if debug:
+        print("APP_SPACES")
+        for app, map in app_spaces.items():
+            print(app)
+            for key, value in map.items():
+                print("    " + key + " " + value)
+        print("\n")
+
+    relations_list = get_application_relations(parsed_yaml)
+
+    if debug:
+        print("APP_RELATIONS")
+        for relation in relations_list:
+            print(relation)
+        print("\n")
+
+    mismatches = []
+
+    for relation in relations_list:
+        space1 = get_relation_space(relation.endpoint1, app_spaces)
+        space2 = get_relation_space(relation.endpoint2, app_spaces)
+        if space1 != space2:
+            mismatch = SpaceMismatch(relation.endpoint1, space1, relation.endpoint2, space2)
+            mismatches.append(mismatch)
+
+    if debug:
+        print("\nHere are the mismatched relations\n")
+        for mismatch in mismatches:
+            print("Mismatch:", mismatch)
+    return mismatches
+
+
+def get_juju_applications(parsed_yaml):
+    """Return a list of applications in the bundle."""
+    return [name for name in parsed_yaml["applications"]]
+
+
+def get_application_spaces(application_list, parsed_yaml):
+    """Return a dictionary with app.binding=space mappings."""
+    app_spaces = {}
+    for app in application_list:
+        app_spaces[app] = {}
+        bindings = parsed_yaml["applications"][app]['bindings']
+        for name, value in bindings.items():
+            app_spaces[app][name] = value
+    return app_spaces
+
+
+def get_application_relations(parsed_yaml):
+    """Return a list of relations extracted from the bundle."""
+    relation_list = []
+    for provider, requirer in parsed_yaml["relations"]:
+        relation = Relation(provider, requirer)
+        relation_list.append(relation)
+    return relation_list
+
+
+def get_relation_space(endpoint, app_spaces):
+    """Get space for specified app and service."""
+    app, service = endpoint.split(':')
+    if app in app_spaces:
+        if service not in app_spaces[app]:
+            return app_spaces[app][""]
+        else:
+            return app_spaces[app][service]
+    else:
+        return "XModel"
diff --git a/jujulint/cli.py b/jujulint/cli.py
index aafe9a5..af5190a 100755
--- a/jujulint/cli.py
+++ b/jujulint/cli.py
@@ -170,3 +170,7 @@ def main():
         cli.audit_all()
     else:
         cli.usage()
+
+
+if __name__ == "__main__":
+    main()
diff --git a/jujulint/lint.py b/jujulint/lint.py
index a59d66a..c1aa3be 100755
--- a/jujulint/lint.py
+++ b/jujulint/lint.py
@@ -35,6 +35,7 @@ from dateutil import relativedelta
 
 import jujulint.util as utils
 from jujulint.logging import Logger
+from jujulint.check_spaces import find_space_mismatches, Relation
 
 VALID_CONFIG_CHECKS = ("isset", "eq", "neq", "gte")
 
@@ -676,6 +677,53 @@ class Linter:
                         }
                     )
 
+    def check_spaces(self, parsed_yaml):
+        """Check that relations end with the same endpoint."""
+        space_checks = self.lint_rules.get("space checks", {})
+        enforce_endpoints = space_checks.get("enforce endpoints", [])
+        enforce_relations = [
+            Relation(*relation)
+            for relation in space_checks.get("enforce relations", [])]
+        ignore_endpoints = space_checks.get("ignore endpoints", [])
+        ignore_relations = [
+            Relation(*relation)
+            for relation in space_checks.get("ignore relations", [])]
+
+        mismatches = find_space_mismatches(parsed_yaml)
+        for mismatch in mismatches:
+            # By default: treat mismatches as warnings.
+            # If we have a matching enforcement rule, treat as an error.
+            # If we have a matching ignore rule, do not warn.
+            # (Enforcement rules win over ignore rules.)
+            error = False
+            warning = True
+            mismatch_relation = mismatch.get_charm_relation(self.model.app_to_charm)
+
+            for enforce_endpoint in enforce_endpoints:
+                if enforce_endpoint in mismatch_relation.endpoints:
+                    error = True
+            for ignore_endpoint in ignore_endpoints:
+                if ignore_endpoint in mismatch_relation.endpoints:
+                    warning = False
+            for enforce_relation in enforce_relations:
+                if enforce_relation == mismatch_relation:
+                    error = True
+            for ignore_relation in ignore_relations:
+                if ignore_relation == mismatch_relation:
+                    warning = False
+
+            message = "Space binding mismatch: {}".format(mismatch)
+            if error:
+                self.handle_error({
+                    "id": "space-binding-mismatch",
+                    "tags": ["mismatch", "space", "binding"],
+                    "description": "Unhandled space binding mismatch",
+                    "message": message,
+                })
+            elif warning:
+                # DEFAULT: not a critical error, so just warn
+                self._log_with_header(message, level=logging.WARN)
+
     def results(self):
         """Provide results of the linting process."""
         if self.model.missing_subs:
@@ -1028,6 +1076,21 @@ class Linter:
             self.check_subs(parsed_yaml["machines"])
             self.check_charms()
 
+            if "relations" in parsed_yaml:
+                # "bindings" *should* be in exported bundles, *unless* no custom bindings exist,
+                # in which case "juju export-bundle" omits them.
+                if "bindings" in list(parsed_yaml[applications].values())[0]:
+                    self.check_spaces(parsed_yaml)
+                else:
+                    self._log_with_header(
+                        "Relations detected but custom bindings not found; "
+                        "skipping space binding checks."
+                    )
+            else:
+                self._log_with_header(
+                    "Bundle relations data not found; skipping space binding checks."
+                )
+
             if "relations" not in parsed_yaml:
                 self.map_machines_to_az(parsed_yaml["machines"])
                 self.check_azs(parsed_yaml[applications])
diff --git a/tests/requirements.txt b/tests/requirements.txt
index c20bbcd..b40fd2a 100644
--- a/tests/requirements.txt
+++ b/tests/requirements.txt
@@ -10,3 +10,4 @@ pyflakes
 pytest
 pytest-cov
 pytest-html
+pytest-mock
 \ No newline at end of file
diff --git a/tests/test_jujulint.py b/tests/test_jujulint.py
index 228af00..1ec6cdf 100644
--- a/tests/test_jujulint.py
+++ b/tests/test_jujulint.py
@@ -1,6 +1,8 @@
 #!/usr/bin/python3
 """Tests for jujulint."""
 from datetime import datetime, timezone
+from unittest import mock
+import logging
 
 import pytest
 
@@ -693,3 +695,118 @@ applications:
         linter.filename = str(rules_path)
         linter.read_rules()
         assert linter.lint_rules == {"key": "value", "key-inc": "value2"}
+
+    check_spaces_example_bundle = {
+        "applications": {
+            "prometheus-app": {
+                "bindings": {
+                    "target": "internal-space",
+                },
+            },
+            "telegraf-app": {
+                "bindings": {
+                    "prometheus-client": "external-space",
+                },
+            },
+        },
+        "relations": [
+            ["telegraf-app:prometheus-client", "prometheus-app:target"],
+        ],
+    }
+
+    check_spaces_example_app_charm_map = {
+        "prometheus-app": "prometheus",
+        "telegraf-app": "telegraf",
+    }
+
+    def test_check_spaces_detect_mismatches(self, linter, mocker):
+        mock_log: mock.MagicMock = mocker.patch('jujulint.lint.Linter._log_with_header')
+        linter.model.app_to_charm = self.check_spaces_example_app_charm_map
+
+        # Run the space check.
+        # Based on the above bundle, we should have exactly one mismatch.
+        linter.check_spaces(self.check_spaces_example_bundle)
+        
+        # By default the mismatch should only trigger a warning, not an error.
+        errors = linter.output_collector["errors"]
+        assert len(errors) == 0
+        assert mock_log.call_count == 1
+        assert mock_log.mock_calls[0].kwargs['level'] == logging.WARN
+        assert mock_log.mock_calls[0].args[0] == (
+            'Space binding mismatch: SpaceMismatch(telegraf-app:prometheus-client '
+            '(space external-space) != prometheus-app:target (space internal-space))'
+        )
+
+    def test_check_spaces_enforce_endpoints(self, linter):
+        linter.model.app_to_charm = self.check_spaces_example_app_charm_map
+
+        # Run the space check with prometheus:target endpoint enforced.
+        # This should generate an error.
+        linter.lint_rules["space checks"] = {"enforce endpoints": ["prometheus:target"]}
+        linter.check_spaces(self.check_spaces_example_bundle)
+        errors = linter.output_collector["errors"]
+        assert len(errors) == 1
+
+        # Enforce the opposite end of the relation.
+        # This should also generate an error.
+        linter.lint_rules["space checks"] = {"enforce endpoints": ["telegraf:prometheus-client"]}
+        linter.check_spaces(self.check_spaces_example_bundle)
+        errors = linter.output_collector["errors"]
+        assert len(errors) == 2
+
+    def test_check_spaces_enforce_relations(self, linter):
+        linter.model.app_to_charm = self.check_spaces_example_app_charm_map
+
+        # Run the space check with prometheus:target endpoint enforced.
+        # This should generate an error.
+        linter.lint_rules["space checks"] = {"enforce relations": [["prometheus:target", "telegraf:prometheus-client"]]}
+        linter.check_spaces(self.check_spaces_example_bundle)
+        errors = linter.output_collector["errors"]
+        assert len(errors) == 1
+
+        # Reverse the relation's definition order.
+        # This should work the same way and also generate an error.
+        linter.lint_rules["space checks"] = {"enforce relations": [["telegraf:prometheus-client", "prometheus:target"]]}
+        linter.check_spaces(self.check_spaces_example_bundle)
+        errors = linter.output_collector["errors"]
+        assert len(errors) == 2
+
+    def test_check_spaces_ignore_endpoints(self, linter, mocker):
+        mock_log: mock.MagicMock = mocker.patch('jujulint.lint.Linter._log_with_header')
+        linter.model.app_to_charm = self.check_spaces_example_app_charm_map
+
+        # Run the space check with prometheus:target endpoint ignored.
+        # This should generate an error.
+        linter.lint_rules["space checks"] = {"ignore endpoints": ["prometheus:target"]}
+        linter.check_spaces(self.check_spaces_example_bundle)
+        errors = linter.output_collector["errors"]
+        assert len(errors) == 0
+        assert mock_log.call_count == 0
+
+        # Enforce the opposite end of the relation.
+        # This should also generate an error.
+        linter.lint_rules["space checks"] = {"ignore endpoints": ["telegraf:prometheus-client"]}
+        linter.check_spaces(self.check_spaces_example_bundle)
+        errors = linter.output_collector["errors"]
+        assert len(errors) == 0
+        assert mock_log.call_count == 0
+
+    def test_check_spaces_ignore_relations(self, linter, mocker):
+        mock_log: mock.MagicMock = mocker.patch('jujulint.lint.Linter._log_with_header')
+        linter.model.app_to_charm = self.check_spaces_example_app_charm_map
+
+        # Run the space check with prometheus:target endpoint ignored.
+        # This should neither generate an error nor a warning.
+        linter.lint_rules["space checks"] = {"ignore relations": [["prometheus:target", "telegraf:prometheus-client"]]}
+        linter.check_spaces(self.check_spaces_example_bundle)
+        errors = linter.output_collector["errors"]
+        assert len(errors) == 0
+        assert mock_log.call_count == 0
+
+        # Reverse the relation's definition order.
+        # This should work the same way and also not generate errors/warnings.
+        linter.lint_rules["space checks"] = {"ignore relations": [["telegraf:prometheus-client", "prometheus:target"]]}
+        linter.check_spaces(self.check_spaces_example_bundle)
+        errors = linter.output_collector["errors"]
+        assert len(errors) == 0
+        assert mock_log.call_count == 0