Repository: canonical/operator
Pull request #1290 (2024-07-19 06:25:15)

fix: use temp dir for secret data
Change the mechanism used to pass secret values from ops to hook tools from command line to files in temporary directory.

Removes the 2MB total secret size limit imposed by command line length

Hook tool calls juju agent which in turn reads these files, I've confirmed with the Juju team that juju agent and charm code are run as the same user.


dimaqq: Also tested manually to replacing ops in a running charm venv.

Patch:
diff --git a/ops/model.py b/ops/model.py
index de9b93e..3a9a1cf 100644
--- a/ops/model.py
+++ b/ops/model.py
@@ -3598,11 +3598,13 @@ class _ModelBackend:
             args.extend(['--expire', expire.isoformat()])
         if rotate is not None:
             args += ['--rotate', rotate.value]
-        if content is not None:
-            # The content has already been validated with Secret._validate_content
-            for k, v in content.items():
-                args.append(f'{k}={v}')
-        self._run_for_secret('secret-set', *args)
+        with tempfile.TemporaryDirectory() as tmp:
+            # The content is None or has already been validated with Secret._validate_content
+            for k, v in (content or {}).items():
+                with open(f'{tmp}/{k}', mode='w', encoding='utf-8') as f:
+                    f.write(v)
+                args.append(f'{k}#file={tmp}/{k}')
+            self._run_for_secret('secret-set', *args)
 
     def secret_add(
         self,
@@ -3625,10 +3627,13 @@ class _ModelBackend:
             args += ['--rotate', rotate.value]
         if owner is not None:
             args += ['--owner', owner]
-        # The content has already been validated with Secret._validate_content
-        for k, v in content.items():
-            args.append(f'{k}={v}')
-        result = self._run('secret-add', *args, return_output=True)
+        with tempfile.TemporaryDirectory() as tmp:
+            # The content has already been validated with Secret._validate_content
+            for k, v in content.items():
+                with open(f'{tmp}/{k}', mode='w', encoding='utf-8') as f:
+                    f.write(v)
+                args.append(f'{k}#file={tmp}/{k}')
+            result = self._run('secret-add', *args, return_output=True)
         secret_id = typing.cast(str, result)
         return secret_id.strip()
 


Test patch:
diff --git a/test/test_helpers.py b/test/test_helpers.py
index 393fd75..84f9b20 100644
--- a/test/test_helpers.py
+++ b/test/test_helpers.py
@@ -153,6 +153,15 @@ class FakeScript:
             f.write(
                 """#!/bin/sh
 {{ printf {name}; printf "\\036%s" "$@"; printf "\\034"; }} >> {path}/calls.txt
+
+# Capture key and data from key#file=/some/path arguments
+for word in "$@"; do
+  echo "$word" | grep -q "#file=" || continue
+  key=$(echo "$word" | cut -d'#' -f1)
+  path=$(echo "$word" | cut -d'=' -f2)
+  cp "$path" "{path}/$key.secret"
+done
+
 {content}""".format_map(template_args)
             )
         path.chmod(0o755)
@@ -175,6 +184,9 @@ class FakeScript:
                 f.truncate(0)
         return calls
 
+    def secrets(self) -> typing.Dict[str, str]:
+        return {p.stem: p.read_text() for p in self.path.iterdir() if p.suffix == '.secret'}
+
 
 class FakeScriptTest(unittest.TestCase):
     def test_fake_script_works(self):
diff --git a/test/test_model.py b/test/test_model.py
index b0e4f62..0100570 100644
--- a/test/test_model.py
+++ b/test/test_model.py
@@ -24,7 +24,7 @@ import typing
 import unittest
 from collections import OrderedDict
 from textwrap import dedent
-from unittest.mock import MagicMock, patch
+from unittest.mock import ANY, MagicMock, patch
 
 import pytest
 
@@ -3349,7 +3349,8 @@ class TestSecrets:
         assert secret.id == 'secret:123'
         assert secret.label is None
 
-        assert fake_script.calls(clear=True) == [['secret-add', '--owner', 'application', 'foo=x']]
+        assert fake_script.calls(clear=True) == [['secret-add', '--owner', 'application', ANY]]
+        assert fake_script.secrets() == {'foo': 'x'}
 
     def test_app_add_secret_args(self, fake_script: FakeScript, model: ops.Model):
         fake_script.write('secret-add', 'echo secret:234')
@@ -3379,10 +3380,11 @@ class TestSecrets:
                 'hourly',
                 '--owner',
                 'application',
-                'foo=x',
-                'bar=y',
+                ANY,
+                ANY,
             ]
         ]
+        assert fake_script.secrets() == {'foo': 'x', 'bar': 'y'}
 
     def test_unit_add_secret_simple(self, fake_script: FakeScript, model: ops.Model):
         fake_script.write('secret-add', 'echo secret:345')
@@ -3392,7 +3394,8 @@ class TestSecrets:
         assert secret.id == 'secret:345'
         assert secret.label is None
 
-        assert fake_script.calls(clear=True) == [['secret-add', '--owner', 'unit', 'foo=x']]
+        assert fake_script.calls(clear=True) == [['secret-add', '--owner', 'unit', ANY]]
+        assert fake_script.secrets() == {'foo': 'x'}
 
     def test_unit_add_secret_args(self, fake_script: FakeScript, model: ops.Model):
         fake_script.write('secret-add', 'echo secret:456')
@@ -3422,10 +3425,11 @@ class TestSecrets:
                 'yearly',
                 '--owner',
                 'unit',
-                'foo=w',
-                'bar=z',
+                ANY,
+                ANY,
             ]
         ]
+        assert fake_script.secrets() == {'foo': 'w', 'bar': 'z'}
 
     def test_unit_add_secret_errors(self, model: ops.Model):
         # Additional add_secret tests are done in TestApplication
@@ -3721,10 +3725,11 @@ class TestSecretClass:
             secret.set_content({'s': 't'})  # ensure it validates content (key too short)
 
         assert fake_script.calls(clear=True) == [
-            ['secret-set', 'secret:x', 'foo=bar'],
+            ['secret-set', 'secret:x', ANY],
             ['secret-info-get', '--label', 'y', '--format=json'],
-            ['secret-set', 'secret:z', 'bar=foo'],
+            ['secret-set', 'secret:z', ANY],
         ]
+        assert fake_script.secrets() == {'foo': 'bar', 'bar': 'foo'}
 
     def test_set_info(self, model: ops.Model, fake_script: FakeScript):
         fake_script.write('secret-set', """exit 0""")