[MISC] Always check databag #533
Conversation
| if key != secret_key and ( | ||
| result := self.peer_relation_data(scope).fetch_my_relation_field(peers.id, key) | ||
| ): |
There was a problem hiding this comment.
If key == secret_key we ignored that peer data value
There was a problem hiding this comment.
I'm a bit confused: If key == secret_key, then this means we should check inside the secret, not the databag (wasn't this the original intention of this code?). Then how come we can have key == secret_key and still have to check the databag? Doesn´t this mean something went wrong when setting the secret in the first place? Sorry I think I never understood this workflow properly 😬
There was a problem hiding this comment.
I suppose the bug itself is not clear to me.
There was a problem hiding this comment.
Migrating from Juju 2 to Juju 3 would switch the charm from trying to use secrets, but all the values are still in the peer databag. The issue here is that we only check if the key and secret_key are different, but the data would still be in the peer databag even if the key is the same (no translation needed).
There was a problem hiding this comment.
Thanks for the explanation. Looks like a specific edge case for which we didn´t have any tests.
dragomirp
requested review from
lucasgameiroborges,
taurus-forever and
marceloneppel
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #533 +/- ##
==========================================
+ Coverage 70.05% 70.11% +0.05%
==========================================
Files 11 11
Lines 3016 3015 -1
Branches 535 534 -1
==========================================
+ Hits 2113 2114 +1
+ Misses 785 784 -1
+ Partials 118 117 -1 ☔ View full report in Codecov by Sentry. |
| if result := self.peer_relation_data(scope).fetch_my_relation_field(peers.id, key): | ||
| return result | ||
|
|
||
| return self.peer_relation_data(scope).get_secret(peers.id, secret_key) |
There was a problem hiding this comment.
Hmmm..... This line WAS supposed to be checking the databag.
There was a problem hiding this comment.
There was a problem hiding this comment.
Well... Having said all that...
It becomes a conceptual question.
Should the lib's get_secret() return secret fields ONLY?
Or should it return the key requested also in case it's in the databag?
(I consider the current situation a bug on my side, as my original intent was along the lines of the latter. However once we are at this point, I think the question is valid regarding policy.)
There was a problem hiding this comment.
In my mind, we'd expect that lib's get_secret() would return secret fields only if Juju Secrets are enabled (Juju 3+). It would return the databag field value only on Juju 2 (it's still sensitive data, even if not protected by any means). So, the code in the lib is right; no bug there.
IMO, the issue here may be fixed with Dragomir changes.
There was a problem hiding this comment.
Isn't self.secrets_enabled from the condition preventing Juju from returning nothing?
There was a problem hiding this comment.
IIRC we added those checks here, so that we don't have to deal with translation etc. in the lib. Do we want to push that back to the lib?
There was a problem hiding this comment.
I think it can be kept here.
There was a problem hiding this comment.
So.
We should deal with the translarion.
But no more.
Just like in MySQL: https://github.com/canonical/mysql-k8s-operator/blob/main/lib/charms/mysql/v0/mysql.py#L742-L745
We should check for the new key (using lib get_secret() but thechnically fetch_relation_data() can work as well).
Then we should do the same for the old name.
The current "fix" is covering up for the bug on the libs side. IT's not the way the code should be wrtiten ideally.
Huh I don't know if I made my point clear 😅
There was a problem hiding this comment.
I'm okay with going the way you commented if it avoids hiding the bug.
| if result := self.peer_relation_data(scope).fetch_my_relation_field(peers.id, key): | ||
| return result | ||
|
|
||
| return self.peer_relation_data(scope).get_secret(peers.id, secret_key) |
There was a problem hiding this comment.
In my mind, we'd expect that lib's get_secret() would return secret fields only if Juju Secrets are enabled (Juju 3+). It would return the databag field value only on Juju 2 (it's still sensitive data, even if not protected by any means). So, the code in the lib is right; no bug there.
IMO, the issue here may be fixed with Dragomir changes.
Fix a missing case for peer to secrets translation.