Repository: canonical/operator (python)

Issue:
Layer object support for log-forwarding as a field
`log-targets` has been added as a field to pebble as of https://github.com/canonical/pebble/pull/267. However, the current implementation of ops.pebble.Layer discards this from the argument of the init. This must be handled properly so that the layer yaml file could be produced.

Pull request:
feat: expose the ability to set Pebble log targets
Exposes the `log-targets` section of the Pebble layer spec, to allow charms to configure log forwarding in Pebble without having to provide layers as plain YAML.

New types:

* `LogTargetDict`: a typed dictionary similar to `ServiceDict` and `CheckDict`.
* `LogTarget`: similar to `Service` and `Check`.

Changed types:

* `LayerDict` and `PlanDict` have (not required) `log-targets` keys.
* `Layer` and `Plan` have `log_targets` attributes (`Plan` is read-only, as with `checks` and `services`).

Fixes #1073

Patch:
diff --git a/CHANGES.md b/CHANGES.md
index 3ff9384..dd9b2c8 100644
--- a/CHANGES.md
+++ b/CHANGES.md
@@ -1,5 +1,6 @@
 # 2.9.0
 
+* Added log target support to `ops.pebble` layers and plans.
 * Added `Harness.run_action()`, `testing.ActionOutput`, and `testing.ActionFailed`
 
 # 2.8.0
diff --git a/HACKING.md b/HACKING.md
index 85a0468..ff556d4 100644
--- a/HACKING.md
+++ b/HACKING.md
@@ -52,20 +52,20 @@ pytest
 
 ## Pebble Tests
 
-The framework has some tests that interact with a real/live pebble server.  To
+The framework has some tests that interact with a real/live Pebble server.  To
 run these tests, you must have [pebble](https://github.com/canonical/pebble)
 installed and available in your path.  If you have the Go toolchain installed,
-you can run `go install github.com/canonical/pebble/cmd/pebble@latest`.  This will
+you can run `go install github.com/canonical/pebble/cmd/pebble@master`.  This will
 install pebble to `$GOBIN` if it is set or `$HOME/go/bin` otherwise.  Add
 `$GOBIN` to your path (e.g. `export PATH=$PATH:$GOBIN` or `export
 PATH=$PATH:$HOME/go/bin` in your `.bashrc`) and you are ready to run the real
-pebble tests:
+Pebble tests:
 
 ```sh
 tox -e pebble
 ```
 
-To do this even more manually, you could start the pebble server yourself:
+To do this even more manually, you could start the Pebble server yourself:
 
 ```sh
 export PEBBLE=$HOME/pebble
diff --git a/ops/pebble.py b/ops/pebble.py
index e1e8e36..ac4278c 100644
--- a/ops/pebble.py
+++ b/ops/pebble.py
@@ -124,16 +124,27 @@ CheckDict = typing.TypedDict('CheckDict',
                               'threshold': Optional[int]},
                              total=False)
 
+# In Python 3.11+ 'services' and 'labels' should be NotRequired, and total=True.
+LogTargetDict = typing.TypedDict('LogTargetDict',
+                                 {'override': Union[Literal['merge'], Literal['replace']],
+                                  'type': Literal['loki'],
+                                  'location': str,
+                                  'services': List[str],
+                                  'labels': Dict[str, str]},
+                                 total=False)
+
 LayerDict = typing.TypedDict('LayerDict',
                              {'summary': str,
                               'description': str,
                               'services': Dict[str, ServiceDict],
-                              'checks': Dict[str, CheckDict]},
+                              'checks': Dict[str, CheckDict],
+                              'log-targets': Dict[str, LogTargetDict]},
                              total=False)
 
 PlanDict = typing.TypedDict('PlanDict',
                             {'services': Dict[str, ServiceDict],
-                             'checks': Dict[str, CheckDict]},
+                             'checks': Dict[str, CheckDict],
+                             'log-targets': Dict[str, LogTargetDict]},
                             total=False)
 
 _AuthDict = TypedDict('_AuthDict',
@@ -718,6 +729,9 @@ class Plan:
                                               for name, service in d.get('services', {}).items()}
         self._checks: Dict[str, Check] = {name: Check(name, check)
                                           for name, check in d.get('checks', {}).items()}
+        self._log_targets: Dict[str, LogTarget] = {
+            name: LogTarget(name, target)
+            for name, target in d.get('log-targets', {}).items()}
 
     @property
     def services(self) -> Dict[str, 'Service']:
@@ -735,11 +749,20 @@ class Plan:
         """
         return self._checks
 
+    @property
+    def log_targets(self) -> Dict[str, 'LogTarget']:
+        """This plan's log targets mapping (maps log target name to :class:`LogTarget`).
+
+        This property is currently read-only.
+        """
+        return self._log_targets
+
     def to_dict(self) -> 'PlanDict':
         """Convert this plan to its dict representation."""
         fields = [
             ('services', {name: service.to_dict() for name, service in self._services.items()}),
             ('checks', {name: check.to_dict() for name, check in self._checks.items()}),
+            ('log-targets', {name: target.to_dict() for name, target in self._log_targets.items()})
         ]
         dct = {name: value for name, value in fields if value}
         return typing.cast('PlanDict', dct)
@@ -766,6 +789,8 @@ class Layer:
     services: Dict[str, 'Service']
     #: Mapping of check to :class:`Check` defined by this layer.
     checks: Dict[str, 'Check']
+    #: Mapping of target to :class:`LogTarget` defined by this layer.
+    log_targets: Dict[str, 'LogTarget']
 
     def __init__(self, raw: Optional[Union[str, 'LayerDict']] = None):
         if isinstance(raw, str):
@@ -780,6 +805,8 @@ class Layer:
                          for name, service in d.get('services', {}).items()}
         self.checks = {name: Check(name, check)
                        for name, check in d.get('checks', {}).items()}
+        self.log_targets = {name: LogTarget(name, target)
+                            for name, target in d.get('log-targets', {}).items()}
 
     def to_yaml(self) -> str:
         """Convert this layer to its YAML representation."""
@@ -792,6 +819,7 @@ class Layer:
             ('description', self.description),
             ('services', {name: service.to_dict() for name, service in self.services.items()}),
             ('checks', {name: check.to_dict() for name, check in self.checks.items()}),
+            ('log-targets', {name: target.to_dict() for name, target in self.log_targets.items()})
         ]
         dct = {name: value for name, value in fields if value}
         return typing.cast('LayerDict', dct)
@@ -1028,6 +1056,45 @@ class CheckStatus(enum.Enum):
     DOWN = 'down'
 
 
+class LogTarget:
+    """Represents a log target in a Pebble configuration layer."""
+
+    def __init__(self, name: str, raw: Optional['LogTargetDict'] = None):
+        self.name = name
+        dct: LogTargetDict = raw or {}
+        self.override: str = dct.get('override', '')
+        self.type = dct.get('type', '')
+        self.location = dct.get('location', '')
+        self.services: List[str] = list(dct.get('services', []))
+        labels = dct.get('labels')
+        if labels is not None:
+            labels = copy.deepcopy(labels)
+        self.labels: Optional[Dict[str, str]] = labels
+
+    def to_dict(self) -> 'LogTargetDict':
+        """Convert this log target object to its dict representation."""
+        fields = [
+            ('override', self.override),
+            ('type', self.type),
+            ('location', self.location),
+            ('services', self.services),
+            ('labels', self.labels),
+        ]
+        dct = {name: value for name, value in fields if value}
+        return typing.cast('LogTargetDict', dct)
+
+    def __repr__(self):
+        return f'LogTarget({self.to_dict()!r})'
+
+    def __eq__(self, other: Union['LogTargetDict', 'LogTarget']):
+        if isinstance(other, dict):
+            return self.to_dict() == other
+        elif isinstance(other, LogTarget):
+            return self.to_dict() == other.to_dict()
+        else:
+            return NotImplemented
+
+
 class FileType(enum.Enum):
     """Enum of file types."""
 


Test patch:
diff --git a/.github/workflows/framework-tests.yaml b/.github/workflows/framework-tests.yaml
index 8a69adf..c3bdd4a 100644
--- a/.github/workflows/framework-tests.yaml
+++ b/.github/workflows/framework-tests.yaml
@@ -77,7 +77,7 @@ jobs:
         run: pip install tox~=4.2
 
       - name: Install Pebble
-        run: go install github.com/canonical/pebble/cmd/pebble@latest
+        run: go install github.com/canonical/pebble/cmd/pebble@master
 
       - name: Start Pebble
         run: |
diff --git a/test/test_framework.py b/test/test_framework.py
index 00a075d..145ecdf 100644
--- a/test/test_framework.py
+++ b/test/test_framework.py
@@ -537,16 +537,26 @@ class TestFramework(BaseTestCase):
         with self.assertRaises(RuntimeError) as cm:
             class OtherEvents(ops.ObjectEvents):  # type: ignore
                 foo = event
+        # Python 3.12+ raises the original exception with a note, but earlier
+        # Python chains the exceptions.
+        if hasattr(cm.exception, "__notes__"):
+            cause = str(cm.exception)
+        else:
+            cause = str(cm.exception.__cause__)
         self.assertEqual(
-            str(cm.exception.__cause__),
+            cause,
             "EventSource(MyEvent) reused as MyEvents.foo and OtherEvents.foo")
 
         with self.assertRaises(RuntimeError) as cm:
             class MyNotifier(ops.Object):  # type: ignore
                 on = MyEvents()  # type: ignore
                 bar = event
+        if hasattr(cm.exception, "__notes__"):
+            cause = str(cm.exception)
+        else:
+            cause = str(cm.exception.__cause__)
         self.assertEqual(
-            str(cm.exception.__cause__),
+            cause,
             "EventSource(MyEvent) reused as MyEvents.foo and MyNotifier.bar")
 
     def test_reemit_ignores_unknown_event_type(self):
diff --git a/test/test_pebble.py b/test/test_pebble.py
index c72431f..c86d266 100644
--- a/test/test_pebble.py
+++ b/test/test_pebble.py
@@ -452,7 +452,12 @@ class TestPlan(unittest.TestCase):
         plan = pebble.Plan('')
         self.assertEqual(plan.services, {})
 
-        plan = pebble.Plan('services:\n foo:\n  override: replace\n  command: echo foo')
+        plan = pebble.Plan("""
+services:
+  foo:
+    override: replace
+    command: echo foo
+""")
 
         self.assertEqual(len(plan.services), 1)
         self.assertEqual(plan.services['foo'].name, 'foo')
@@ -467,8 +472,13 @@ class TestPlan(unittest.TestCase):
         plan = pebble.Plan('')
         self.assertEqual(plan.checks, {})
 
-        plan = pebble.Plan(
-            'checks:\n bar:\n  override: replace\n  http:\n   url: https://example.com/')
+        plan = pebble.Plan("""
+checks:
+  bar:
+    override: replace
+    http:
+      url: https://example.com/
+""")
 
         self.assertEqual(len(plan.checks), 1)
         self.assertEqual(plan.checks['bar'].name, 'bar')
@@ -479,6 +489,29 @@ class TestPlan(unittest.TestCase):
         with self.assertRaises(AttributeError):
             plan.checks = {}  # type: ignore
 
+    def test_log_targets(self):
+        plan = pebble.Plan('')
+        self.assertEqual(plan.log_targets, {})
+
+        location = "https://example.com:3100/loki/api/v1/push"
+        plan = pebble.Plan(f"""
+log-targets:
+  baz:
+    override: replace
+    type: loki
+    location: {location}
+""")
+
+        self.assertEqual(len(plan.log_targets), 1)
+        self.assertEqual(plan.log_targets['baz'].name, 'baz')
+        self.assertEqual(plan.log_targets['baz'].override, 'replace')
+        self.assertEqual(plan.log_targets['baz'].type, "loki")
+        self.assertEqual(plan.log_targets['baz'].location, location)
+
+        # Should be read-only ("can't set attribute")
+        with self.assertRaises(AttributeError):
+            plan.log_targets = {}  # type: ignore
+
     def test_yaml(self):
         # Starting with nothing, we get the empty result
         plan = pebble.Plan('')
@@ -496,6 +529,12 @@ checks:
  bar:
   http:
    https://example.com/
+
+log-targets:
+ baz:
+  override: replace
+  type: loki
+  location: https://example.com:3100/loki/api/v1/push
 '''
         plan = pebble.Plan(raw)
         reformed = yaml.safe_dump(yaml.safe_load(raw))
@@ -503,7 +542,12 @@ checks:
         self.assertEqual(str(plan), reformed)
 
     def test_service_equality(self):
-        plan = pebble.Plan('services:\n foo:\n  override: replace\n  command: echo foo')
+        plan = pebble.Plan("""
+services:
+  foo:
+    override: replace
+    command: echo foo
+""")
 
         old_service = pebble.Service(name="foo",
                                      raw={
@@ -524,6 +568,8 @@ class TestLayer(unittest.TestCase):
         self.assertEqual(layer.summary, '')
         self.assertEqual(layer.description, '')
         self.assertEqual(layer.services, {})
+        self.assertEqual(layer.checks, {})
+        self.assertEqual(layer.log_targets, {})
         self.assertEqual(layer.to_dict(), {})
 
     def test_no_args(self):
@@ -546,6 +592,17 @@ class TestLayer(unittest.TestCase):
                     'summary': 'Bar',
                     'command': 'echo bar',
                 },
+            },
+            'log-targets': {
+                'baz': {
+                    'override': 'merge',
+                    'type': 'loki',
+                    'location': 'https://example.com',
+                    'services': ['foo'],
+                    'labels': {
+                        'key': 'value $VAR',
+                    }
+                },
             }
         }
         s = pebble.Layer(d)
@@ -557,6 +614,12 @@ class TestLayer(unittest.TestCase):
         self.assertEqual(s.services['bar'].name, 'bar')
         self.assertEqual(s.services['bar'].summary, 'Bar')
         self.assertEqual(s.services['bar'].command, 'echo bar')
+        self.assertEqual(s.log_targets['baz'].name, 'baz')
+        self.assertEqual(s.log_targets['baz'].override, 'merge')
+        self.assertEqual(s.log_targets['baz'].type, 'loki')
+        self.assertEqual(s.log_targets['baz'].location, 'https://example.com')
+        self.assertEqual(s.log_targets['baz'].services, ['foo'])
+        self.assertEqual(s.log_targets['baz'].labels, {'key': 'value $VAR'})
 
         self.assertEqual(s.to_dict(), d)
 
@@ -569,6 +632,11 @@ class TestLayer(unittest.TestCase):
     http:
       url: https://example.com/
 description: The quick brown fox!
+log-targets:
+  baz:
+    location: https://example.com:3100
+    override: replace
+    type: loki
 services:
   bar:
     command: echo bar
@@ -604,6 +672,10 @@ summary: Sum Mary
         self.assertEqual(s.checks['chk'].name, 'chk')
         self.assertEqual(s.checks['chk'].http, {'url': 'https://example.com/'})
 
+        self.assertEqual(s.log_targets['baz'].name, 'baz')
+        self.assertEqual(s.log_targets['baz'].override, 'replace')
+        self.assertEqual(s.log_targets['baz'].location, 'https://example.com:3100')
+
         self.assertEqual(s.to_yaml(), yaml)
         self.assertEqual(str(s), yaml)
 
@@ -884,6 +956,65 @@ class TestCheck(unittest.TestCase):
         self.assertNotEqual(one, 5)
 
 
+class TestLogTarget(unittest.TestCase):
+    def _assert_empty(self, target: pebble.LogTarget, name: str):
+        self.assertEqual(target.name, name)
+        self.assertEqual(target.override, '')
+        self.assertEqual(target.type, '')
+        self.assertEqual(target.location, '')
+        self.assertEqual(target.services, [])
+        self.assertIs(target.labels, None)
+
+    def test_name_only(self):
+        target = pebble.LogTarget('tgt')
+        self._assert_empty(target, 'tgt')
+
+    def test_dict(self):
+        d: pebble.LogTargetDict = {
+            'override': 'replace',
+            'type': 'loki',
+            'location': 'https://example.com:3100/loki/api/v1/push',
+            'services': ['+all'],
+            'labels': {'key': 'val', 'key2': 'val2'}
+        }
+        target = pebble.LogTarget('tgt', d)
+        self.assertEqual(target.name, 'tgt')
+        self.assertEqual(target.override, 'replace')
+        self.assertEqual(target.type, 'loki')
+        self.assertEqual(target.location, 'https://example.com:3100/loki/api/v1/push')
+        self.assertEqual(target.services, ['+all'])
+        self.assertEqual(target.labels, {'key': 'val', 'key2': 'val2'})
+
+        self.assertEqual(target.to_dict(), d)
+
+        # Ensure pebble.Target has made copies of mutable objects.
+        assert target.services is not None and target.labels is not None
+        target.services[0] = '-all'
+        self.assertEqual(d['services'], ['+all'])
+        target.labels['key'] = 'val3'
+        assert d['labels'] is not None
+        self.assertEqual(d['labels']['key'], 'val')
+
+    def test_equality(self):
+        d: pebble.LogTargetDict = {
+            'override': 'replace',
+            'type': 'loki',
+            'location': 'https://example.com',
+            'services': ['foo', 'bar'],
+            'labels': {'k': 'v'}
+        }
+        one = pebble.LogTarget('one', d)
+        two = pebble.LogTarget('two', d)
+        self.assertEqual(one, two)
+        self.assertEqual(one, d)
+        self.assertEqual(two, d)
+        self.assertEqual(one, one.to_dict())
+        self.assertEqual(two, two.to_dict())
+        d['override'] = 'merge'
+        self.assertNotEqual(one, d)
+        self.assertNotEqual(one, 5)
+
+
 class TestServiceInfo(unittest.TestCase):
     def test_service_startup(self):
         self.assertEqual(list(pebble.ServiceStartup), [
diff --git a/test/test_real_pebble.py b/test/test_real_pebble.py
index 2fb03a2..40ecdf2 100644
--- a/test/test_real_pebble.py
+++ b/test/test_real_pebble.py
@@ -259,6 +259,32 @@ class TestRealPebble(unittest.TestCase):
 
         self.assertEqual(reads, [b'one\n', b'2\n', b'THREE\n'])
 
+    def test_log_forwarding(self):
+        self.client.add_layer("log-forwarder", {
+            "services": {
+                "tired": {
+                    "override": "replace",
+                    "command": "sleep 1",
+                },
+            },
+            "log-targets": {
+                "pretend-loki": {
+                    "type": "loki",
+                    "override": "replace",
+                    "location": "https://example.com",
+                    "services": ["all"],
+                    "labels": {"foo": "bar"},
+                },
+            },
+        }, combine=True)
+        plan = self.client.get_plan()
+        self.assertEqual(len(plan.log_targets), 1)
+        self.assertEqual(plan.log_targets["pretend-loki"].type, "loki")
+        self.assertEqual(plan.log_targets["pretend-loki"].override, "replace")
+        self.assertEqual(plan.log_targets["pretend-loki"].location, "https://example.com")
+        self.assertEqual(plan.log_targets["pretend-loki"].services, ["all"])
+        self.assertEqual(plan.log_targets["pretend-loki"].labels, {"foo": "bar"})
+
 
 @unittest.skipUnless(os.getenv('RUN_REAL_PEBBLE_TESTS'), 'RUN_REAL_PEBBLE_TESTS not set')
 class TestPebbleStorageAPIsUsingRealPebble(unittest.TestCase, PebbleStorageAPIsTestMixin):