Secrets #840

PietroPasotti · 2022-10-07T10:17:11Z

NOTE: new PR is #861. Note that if you're building charms, you'll need to update your branch to benhoyt/operator@secrets, so update your charm's requirements.txt to the following:

git+https://github.com/benhoyt/operator@secrets#egg=ops

ops wrappers for Juju Secrets (3.0.53.0.2+)

QA steps

A couple of tester/demo charms that can be deployed to try the functionality out:
https://github.com/PietroPasotti/secrets-demo-charms

Documentation changes

Added:

Bug reference

#836

Changelog

Added support for juju secrets.

* Finish secret hook tool calls, * Create secret add/get methods, * mods to accomadate spec evolution * Add in secret-event creation plumbing

The idea is to implement the secret hook tools in the testing backend as normal, but to have this pseudo-secret object that represents the juju secret semantics - e.g. tracking which units are on which revisions, whether a secret is expired, etc. This allows a test writer to do things like force-expire a secret at a desired point in time and then see how a charm reacts to it in various hook calls. Or to control the contents or access granting of a secret that is being consumed, and not explicitly controlled by the charm being tested.

sed-i · 2022-10-07T13:19:20Z

This is exciting stuff @PietroPasotti!
Are you aware of any sequnce/activity diagrams for secrets? There's quite a bit of new terminology (revision, owner, label, id, update, prune, remove, rotate, expired, changed, grant, revoke) and it could be handy to be able to see in a glance where all pieces fit.

PietroPasotti · 2022-10-07T13:21:35Z

@sed-i not yet I'm afraid, the coming weeks while I wait for some juju bugs to be fixed and the implementation on their side to settle, I'll get working on the documentation.

benhoyt · 2022-10-11T02:10:41Z

Pre-review comment: it'd be great to fix the formatting and tests before going too much further.

I'm also getting errors when running tox -e docs, but it seems like a Sphinx error on Python 3.10 -- odd.

$ tox -e docs
docs installed: alabaster==0.7.12,Babel==2.10.3,certifi==2022.9.24,charset-normalizer==2.1.1,docutils==0.16,idna==3.4,imagesize==1.4.1,Jinja2==3.1.2,MarkupSafe==2.1.1,packaging==21.3,Pygments==2.13.0,pyparsing==3.0.9,pytz==2022.4,PyYAML==6.0,requests==2.28.1,snowballstemmer==2.2.0,Sphinx==3.5.4,sphinx-rtd-theme==1.0.0,sphinxcontrib-applehelp==1.0.2,sphinxcontrib-devhelp==1.0.2,sphinxcontrib-htmlhelp==2.0.0,sphinxcontrib-jsmath==1.0.1,sphinxcontrib-qthelp==1.0.3,sphinxcontrib-serializinghtml==1.1.5,urllib3==1.26.12
docs run-test-pre: PYTHONHASHSEED='2153479159'
docs run-test: commands[0] | sphinx-build -M html docs/ docs/_build
Traceback (most recent call last):
  File "/home/ben/w/operator/.tox/docs/bin/sphinx-build", line 5, in <module>
    from sphinx.cmd.build import main
  File "/home/ben/w/operator/.tox/docs/lib/python3.10/site-packages/sphinx/cmd/build.py", line 25, in <module>
    from sphinx.application import Sphinx
  File "/home/ben/w/operator/.tox/docs/lib/python3.10/site-packages/sphinx/application.py", line 32, in <module>
    from sphinx.config import Config
  File "/home/ben/w/operator/.tox/docs/lib/python3.10/site-packages/sphinx/config.py", line 23, in <module>
    from sphinx.util import logging
  File "/home/ben/w/operator/.tox/docs/lib/python3.10/site-packages/sphinx/util/__init__.py", line 35, in <module>
    from sphinx.util import smartypants  # noqa
  File "/home/ben/w/operator/.tox/docs/lib/python3.10/site-packages/sphinx/util/smartypants.py", line 33, in <module>
    from sphinx.util.docutils import __version_info__ as docutils_version
  File "/home/ben/w/operator/.tox/docs/lib/python3.10/site-packages/sphinx/util/docutils.py", line 31, in <module>
    from sphinx.util.typing import RoleFunction
  File "/home/ben/w/operator/.tox/docs/lib/python3.10/site-packages/sphinx/util/typing.py", line 34, in <module>
    from types import Union as types_Union
ImportError: cannot import name 'Union' from 'types' (/usr/lib/python3.10/types.py)
ERROR: InvocationError for command /home/ben/w/operator/.tox/docs/bin/sphinx-build -M html docs/ docs/_build (exited with code 1)
_________________________________________________________________________ summary _________________________________________________________________________
ERROR:   docs: commands failed

PietroPasotti · 2022-10-11T14:42:51Z

Pre-review comment: it'd be great to fix the formatting and tests before going too much further.

Will do it first thing tomorrow.

I'm also getting errors when running tox -e docs, but it seems like a Sphinx error on Python 3.10 -- odd.

Will look into that, didn't notice.

ops/main.py

ops/model.py

PietroPasotti · 2022-10-12T10:22:43Z

@sed-i I started working on some docs: https://discourse.charmhub.io/t/secret-events/7191

PietroPasotti · 2022-10-20T14:29:48Z

I was about to close for the day when I realized that I'd forgotten to add a bunch of optional 'label' args to all sorts of Secret.remove/revoke etc methods, to be used if an ID is not provided.
Fixed that and the testing framework as well.

benhoyt

I have not reviewed the ops/testing.py or test/test_secrets.py changes, but the rest of this looks good to me now. Nice work!

ops/model.py

ops/log.py

ops/charm.py

ops/model.py

wallyworld · 2022-10-31T14:09:25Z

ops/model.py

+
+
+class SecretRotateValueError(SecretsError):
+    """Raised when one attempts to create a secret with a bad `rotate` value."""


Do we also check that we cannot create a secret with an expiry time in the past

ops/charm.py

wallyworld · 2022-10-31T15:11:27Z

ops/model.py

+                meta=True)
+            meta = typing.cast('_SecretGetMetadataDict', meta)
+
+        except SecretOwnershipError:  # let other types raise


We should not be using "programme by exception"

wallyworld · 2022-10-31T15:13:31Z

ops/model.py

+        label = metadata.get('label')
+        revision = int(metadata.get('revision'))
+        return Secret(self._backend, secret_id, label=label,
+                      revision=revision, is_owned_by_this_unit=True)


I keep thinking "is owned by this unit" isn't quite right. In the case of the secret being owned by the application, the leader unit is allowed to manage the secret, but it doesn't own it.

Would we be better talking in terms of "managed_by_this_unit"?

wallyworld · 2022-10-31T15:18:16Z

ops/model.py

+            ...    expiration=datetime.datetime(day=23, year=2045, month=12))
+        """
+        if not self._backend._hook_is_running:  # noqa
+            raise RuntimeError("This method is meant to be called from charm code only; "


Is this a standard error message used by the OF? Is there a const somewhere?

wallyworld · 2022-10-31T15:37:21Z

ops/model.py

+        """The revision number for this secret.
+
+        This will only be visible to the owner of the secret.
+        Secret consumers will get ``None``.


I wonder if we need 2 classes:
Secret (for owners)
SecretContent (for consumers, returned from secret_get).
It seems like a suboptimal charmer experience to have to "know" what apis work and which don't.
A consumer will always only get the content. The owner always gets the full secret.

wallyworld · 2022-10-31T15:39:30Z

ops/model.py

+            # fixme: ATM you can't secret-set without content (e.g. to only update the label),
+            #  but that is a bug in juju.
+            #  when that is fixed, `secret-set [id] --label X` will be valid.


This is fixed now

wallyworld · 2022-10-31T15:46:36Z

ops/model.py

+
+    def _validate_ownership(self, operation_name: str,
+                            should_own: bool = True):
+        owns = self._is_owned_by_this_unit


So checking - this "is owned" value is set if the current unit is the leader (for app owned secrets)?

wallyworld · 2022-10-31T15:48:45Z

ops/model.py

+            raise TypeError(
+                'cannot {} secret {} to a unit or application '
+                'target without specifying a relation as its scope'.format(operation, self))
+        if relation and isinstance(target, Application) and relation.app is not target:


Target application should come from the "remote application" property of the relation right? So we can never get this error?

wallyworld · 2022-10-31T15:50:41Z

ops/model.py

+                    operation_name, self))
+        if not should_own and owns:
+            raise SecretOwnershipError(
+                'Cannot {} secret {!r} as an owner. Only consumers can do that.'.format(


Remind me, when would we get this?

All this logic here is a bit convoluted and would be much simpler if we had separate classes for owners and consumers.

Tests and testing harness broken right now

benhoyt · 2022-11-28T03:24:27Z

This PR is getting too messy -- many commits over a long period of time, and many commits to main since the branch was created. Going to clean things up by deleting this branch and recreating on a new PR without the test harness for now.

Based on canonical#840 with rework for the new API design and simplifications.

rwcarlsen and others added 16 commits September 9, 2022 09:14

skeleton of secrets API

761ebf7

start filling out hook tool calls

a25a826

Finish sketching out secrets API:

0f24109

* Finish secret hook tool calls, * Create secret add/get methods, * mods to accomadate spec evolution * Add in secret-event creation plumbing

fix snapshotting test

6889a22

fix secret-add bugs

f9b61de

testing backend for secrets

a042b4c

unified testing backends

b3318ae

some minor lints

d29087d

added some _TestingSecret obj

43568c2

added some charm-pov tests

5850e49

fixed some revision issues

59b60c2

revision available to owners only

ee05ed4

wip closed until juju fixes bug

c81960e

refactored some label things

a687a22

tests green, api semifinal

a0b0358

PietroPasotti requested review from jameinel and benhoyt October 10, 2022 06:23

benhoyt reviewed Oct 11, 2022

View reviewed changes

lint

f43cd14

PietroPasotti force-pushed the secrets branch from acc8e96 to 230116b Compare October 12, 2022 10:21

PietroPasotti force-pushed the secrets branch from 230116b to 04a8b84 Compare October 12, 2022 10:23

PietroPasotti added 2 commits October 12, 2022 12:29

pr comments - lint and static

bc9d0af

fixed main tests

a022e06

PietroPasotti force-pushed the secrets branch from 04a8b84 to a022e06 Compare October 12, 2022 11:49

more pr comments, loads of fixes

f9d50fc

benhoyt approved these changes Oct 21, 2022

View reviewed changes

ops/model.py Outdated Show resolved Hide resolved

PietroPasotti added 3 commits October 27, 2022 10:23

SecretRotate Enum refactored, added test and new error class

26b8a0e

SecretOwner enum names capitalized

95a495c

fixed static env

2af0907

wallyworld reviewed Oct 27, 2022

View reviewed changes

ops/log.py Outdated Show resolved Hide resolved

PietroPasotti added 4 commits October 27, 2022 13:41

uncapitalized juju-log [ci skip]

bdfdc8a

Secret holder --> consumer

98aeb16

fixed secretEvent.snapshotting methods inheritance

af319eb

static

644b5cb

wallyworld reviewed Oct 31, 2022

View reviewed changes

wallyworld approved these changes Oct 31, 2022

View reviewed changes

benhoyt mentioned this pull request Nov 15, 2022

Testing: add support for network-get to harness #846

Merged

Simplify and tweak docs for secret events

c47ca80

sed-i mentioned this pull request Nov 17, 2022

Add support for private repositories canonical/cos-configuration-k8s-operator#31

Closed

Update and simplify secrets code based on latest API design doc

5281fb6

Tests and testing harness broken right now

benhoyt marked this pull request as draft November 20, 2022 22:55

benhoyt added 6 commits November 21, 2022 16:19

Fix add_secret rotate bug; add Secret.__repr__

486f71d

Fix secret-grant / secret-revoke issues

e628664

Temp fix for get_secret issue

a9a6a85

Various fixes from testing against Juju

fe096a4

Fix some type checking issues

eb529a5

test/test_main.py tests passing (and a few other tweaks)

0d89002

benhoyt changed the base branch from ops-2.0 to main November 27, 2022 23:32

benhoyt force-pushed the secrets branch from 40a53cb to 0d89002 Compare November 28, 2022 03:17

benhoyt closed this Nov 28, 2022

benhoyt deleted the secrets branch November 28, 2022 03:25

benhoyt added a commit to benhoyt/operator that referenced this pull request Nov 28, 2022

Updated implementation of secrets API (without harness for now)

772862b

Based on canonical#840 with rework for the new API design and simplifications.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Secrets #840

Secrets #840

PietroPasotti commented Oct 7, 2022 •

edited by benhoyt

Loading

sed-i commented Oct 7, 2022

PietroPasotti commented Oct 7, 2022

benhoyt commented Oct 11, 2022

PietroPasotti commented Oct 11, 2022

PietroPasotti commented Oct 12, 2022

PietroPasotti commented Oct 20, 2022

benhoyt left a comment

wallyworld Oct 31, 2022

wallyworld Oct 31, 2022

wallyworld Oct 31, 2022

wallyworld Oct 31, 2022

wallyworld Oct 31, 2022

wallyworld Oct 31, 2022

wallyworld Oct 31, 2022

wallyworld Oct 31, 2022

wallyworld Oct 31, 2022

benhoyt commented Nov 28, 2022



		class SecretRotateValueError(SecretsError):
		"""Raised when one attempts to create a secret with a bad `rotate` value."""

Secrets #840

Secrets #840

Conversation

PietroPasotti commented Oct 7, 2022 • edited by benhoyt Loading

QA steps

Documentation changes

Bug reference

Changelog

sed-i commented Oct 7, 2022

PietroPasotti commented Oct 7, 2022

benhoyt commented Oct 11, 2022

PietroPasotti commented Oct 11, 2022

PietroPasotti commented Oct 12, 2022

PietroPasotti commented Oct 20, 2022

benhoyt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

benhoyt commented Nov 28, 2022

PietroPasotti commented Oct 7, 2022 •

edited by benhoyt

Loading