fix: if secret ID and label are known, only provide secret-get with ID

[tonyandrewmeyer]

Juju secret-get accepts an ID/URI or a label, but errors out if both are provided. If an ops secret object knows both the ID and the label, then only provide the ID in the secret-get call rather than both.

Prefer ID over label to avoid the lookup when not required.

Fixes #1058

[benhoyt]

I'm concerned that this isn't correct, at least for secret observers: when calling get_secret, the observer looks it up by ID, but the label given is applied as a label when get_secret(id=..., label=label_to_apply) is called. See the docs for Model.get_secret:

    def get_secret(self, *, id: Optional[str] = None, label: Optional[str] = None) -> 'Secret':
        """Get the :class:`Secret` with the given ID or label.

        The caller must provide at least one of `id` (the secret's locator ID)
        or `label` (the charm-local "name").

        If both are provided, the secret will be fetched by ID, and the
        secret's label will be updated to the label provided. Normally secret
        owners set a label using ``add_secret``, whereas secret observers set
        a label using ``get_secret`` (see an example at :attr:`Secret.label`).

Let's discuss with @wallyworld to see how we should solve the underlying issue.

[benhoyt]

I spoke further with Ian about this, and I realized by looking at the original bug that this is happening when calling peek_content, where you don't supply the ID and label in the call (they're filled in from self.id and self.label). So I think what we need to do is a change like this in Secret.get_content and Secret.peek_content:

diff --git a/ops/model.py b/ops/model.py
index 33f7f12..213bee9 100644
--- a/ops/model.py
+++ b/ops/model.py
@@ -1296,7 +1296,7 @@ class Secret:
         """
         if refresh or self._content is None:
             self._content = self._backend.secret_get(
-                id=self.id, label=self.label, refresh=refresh)
+                id=self.id, label=self.label if self.id is None else None, refresh=refresh)
         return self._content.copy()
 
     def peek_content(self) -> Dict[str, str]:
@@ -1305,7 +1305,7 @@ class Secret:
         This returns the content of the latest revision without updating the
         tracking.
         """
-        return self._backend.secret_get(id=self.id, label=self.label, peek=True)
+        return self._backend.secret_get(id=self.id, label=self.label if self.id is None else None, peek=True)
 
     def get_info(self) -> SecretInfo:
         """Get this secret's information (metadata).

In other words, when calling get_content or peek_content, the charm author shouldn't be supplying a label, so only pass the label to secret-get if there's no ID. So that should fix those two. The only other place secret-get is called is in Model.get_secret itself, where the caller explicitly provides the id and label arguments. If this comes up in that case, the charm author will know if they're owner or observer so can specify only the ID if appropriate.

In addition, as we've discussed, Juju is planning to make a change so that owner secrets work like observer secrets and secret-get will apply the given label if given both ID and label (for secret owners; it already does this for secret observers now). But doing the above change for get_content and peek_content won't hurt in any case.

[jameinel]

I feel like Juju should only error if both label and id are supplied and they don't match the existing recorded information.
You have to supply both the first time in order to establish a correspondence. eg, the first time you ask about a secret, if you want it to be labeled, you clearly must supply both the id and the label. Once you've established that relationship, it is probably incorrect to supply a different label and id pair (because then there is ambiguity in whether you want the content of the id, or the content of the label). But as long as they match, why would we error?

[jameinel]

Actually, I think the ops change is subtly worse than supplying it. Because ops isn't verifying that you haven't skewed the label and id. And then you have the problem that if I supply (label='foo', id='1234'), and then later supply (label='bar', id='1234'), nobody notices that I've broken my linkage, because it happily returns the content of '1234'.

So I'm actually against this patch, and feel like we need to fix juju.

[tonyandrewmeyer]

So I'm actually against this patch, and feel like we need to fix juju.

Works for me :). @benhoyt do you agree? If so I can close this PR and we can close the linked issue (and either open a Juju ticket or request the OP to do so).

[benhoyt]

Yep, I agree @tonyandrewmeyer -- John's rationale makes sense to me. Let's close this and the linked issue and open a Juju issue.