Skip to content

[MISC] Always check databag #533

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Jul 18, 2024
Merged

[MISC] Always check databag #533

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
ec502d5
Always check databag
dragomirp Jul 17, 2024
bb52cf7
Merge branch 'main' into always-check-databag
dragomirp Jul 18, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 2 additions & 7 deletions src/charm.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -294,9 +294,7 @@ def get_secret(self, scope: Scopes, key: str) -> Optional[str]:
return None
secret_key = self._translate_field_to_secret_key(key)
# Old translation in databag is to be taken
if key != secret_key and (
result := self.peer_relation_data(scope).fetch_my_relation_field(peers.id, key)
):
Comment on lines -297 to -299
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If key == secret_key we ignored that peer data value

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 😬

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suppose the bug itself is not clear to me.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the explanation. Looks like a specific edge case for which we didn´t have any tests.

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)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmmm..... This line WAS supposed to be checking the databag.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This IS the issue.

https://github.com/canonical/data-platform-libs/blob/main/lib/charms/data_platform_libs/v0/data_interfaces.py#L1873-L1877

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@marceloneppel ☝️

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't self.secrets_enabled from the condition preventing Juju from returning nothing?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it can be kept here.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 😅

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm okay with going the way you commented if it avoids hiding the bug.

Expand All @@ -312,10 +310,7 @@ def set_secret(self, scope: Scopes, key: str, value: Optional[str]) -> Optional[
peers = self.model.get_relation(PEER)
secret_key = self._translate_field_to_secret_key(key)
# Old translation in databag is to be deleted
if key != secret_key and self.peer_relation_data(scope).fetch_my_relation_field(
peers.id, key
):
self.peer_relation_data(scope).delete_relation_data(peers.id, [key])
self.peer_relation_data(scope).delete_relation_data(peers.id, [key])
self.peer_relation_data(scope).set_secret(peers.id, secret_key, value)

def remove_secret(self, scope: Scopes, key: str) -> None:
Expand Down