patch: backup credentials validation on recompute by smethnani · Pull Request #226 · canonical/opensearch-single-kernel-library

smethnani · 2026-10-05T20:40:36Z

Fixes #215

Description of issue or feature:

SnapshotsManager.get_statuses was calling validate_storage_config on every hook.
On relating with a backup integrator, a Backup relation data missing or incomplete. status was displayed immediately, followed by Backup configuration error: bad credentials before clearing itself. This is because the connection info empty during the first hooks, and later s3-integrator's legacy lib adds a data field (which is a copy of our request) which the charm was mistaking for invalid connection_info
If credentials were correct but other fields were missing, a Backup configuration error: bad credentials was being shown when it should be Backup relation data missing or incomplete

Some uncaught exceptions were causing hook failures:

Setting an empty endpoint was crashing a unit because boto raised an uncaught ValueError
In large deployments, if a backup conflict exists, peer-cluster-relation-changed caused a hook failure in subclusters because they raised an uncaught OpenSearchSnapshotsPeerClusterDataConflictError

Solution:

only validate storage config on get_statuses when recompute=True (commit 1)
Ignore the s3 legacy libdata field and add a new BACKUP_WAITING_FOR_CONNECTION_INFO status which is displayed when first relating to a backup integrator while the connection info is empty. If connection_info is still missing when recompute=True its considered an incomplete configuration (commit 2)
Catch the raised exceptions (commits 3 & 4)
Distinguish between bad credentials and missing fields (commit 5)

commits 6 and 7 are removing statuses on relation-removal and adding the missing fields message to the backup actions

How was this change tested?

Manually
Unit tests
Integration tests

Mehdi-Bendriss

Thanks Shams, left some small comments and questions

Mehdi-Bendriss · 2026-10-06T12:01:43Z


        return None

+    def _set_credentials_incorrect_status(self, failed: bool) -> None:


nit, missing docstrings

Mehdi-Bendriss · 2026-10-06T12:03:09Z

+        message="Waiting for settings from backup integrator",
+        short_message="Waiting for backup settings",


nit, backup => object storage

Mehdi-Bendriss · 2026-10-06T12:06:13Z

+                # s3-integrator legacy-lib adds copy of our request to a "data" field
+                # before filling connection info (its not part of the connection info)
+                info = self.s3_requirer.get_storage_connection_info(self.s3_relation) or {}
+                info.pop("data", None)


what does this field represent and why do we remove it?

I believe it's for internal bookkeeping, the relation data looks like this for the first few hooks:

{} { "data": "{}"} { "data": "{\"requested-secrets\": \"[\\\"access-key\\\", \\\"secret-key\\\"]\"}" } { "bucket": "opensearch-bucket", "data": "{\"bucket\": \"relation-42\", \"requested-secrets\": \"[\\\"access-key\\\", \\\"secret-key\\\"]\", \"version\": \"1\"}", "endpoint": "9200", ... }

We remove it to be able to check if the connection info is empty, it's the only key we get before we get the actual configuration. Upstream drops it the same way in their v1 provider tests to compare connection info for backwards compatibility

Mehdi-Bendriss · 2026-10-06T12:09:13Z

    def get_endpoint_protocol(endpoint: str) -> str:
        """Returns the protocol based on the endpoint."""
-        if not endpoint:
+        if not isinstance(endpoint, str) or not endpoint:


when not None, when would this be anything but string?

Yes, s3 v1 publishes raw strings and the requirer lib json decodes them so we can get things that aren't strings. Setting endpoint=9200 caused a hook failure for example.

Mehdi-Bendriss · 2026-10-06T12:10:42Z

+                self.charm.snapshots_manager.read_snapshots_data_from_peer_cluster()
+            )
+        except OpenSearchSnapshotsPeerClusterDataConflictError:
+            return None


we should perhaps log that we're ignoring this due to ongoing conflict

Mehdi-Bendriss · 2026-10-06T12:11:27Z

            interpolated=True,
        )

+    def _clear_relation_data_incomplete_status(self) -> None:


nit, missing docstrings

Mehdi-Bendriss · 2026-10-06T12:12:23Z

-            except OpenSearchObjectStorageConfigValidationError:
+            except OpenSearchObjectStorageConfigValidationError as e:
+                if e.missing_fields:
+                    return "Object storage configuration missing fields."


Let's add the list of missing fields in the message for clarity

akram09

Thanks Shams !

akram09 · 2026-10-07T06:29:50Z

+        self.missing_fields: list[str] = (
+            [field.strip() for msg in msgs for field in msg.split(":", 1)[1].split(",")]
+            if all("Missing field" in msg for msg in msgs)
+            else []
+        )


nit: "Missing fields" . I personally don't like relying on str since it can be error prone I documented a bit on this and seems we can use PydanticCustomError which is more structured and can give you data access. I never used it so to be confirmed

akram09 · 2026-10-07T06:35:14Z

+                # connection_info may be empty for a few hooks before being filled
+                # if its empty on recompute then we consider it unconfigured
+                if not connection_info:
+                    if recompute or cached_non_running_statuses(


I am not sure we should rely on recompute like this. Because it can be fired at any time with running status-detail or just jhack ffwd (which sets the refresh to less time)

smethnani added 7 commits October 5, 2026 21:20

patch: only validate credentials on recompute

d78c524

patch: fix backup error on relate

cbe1ebc

patch: fix missing fields hook failure

4a5f7ba

patch: fix conflict hook failure peer cluster relation changed

d29dc47

patch: distinguish missing fields from bad credentials

6c75d44

patch: clear status on relation removal

ddf83d6

patch: add missing fields message to actions

836096a

smethnani marked this pull request as ready for review October 6, 2026 10:57

smethnani requested review from Mehdi-Bendriss and akram09 October 6, 2026 11:51

Mehdi-Bendriss reviewed Oct 6, 2026

View reviewed changes

smethnani added 2 commits October 6, 2026 17:12

patch: pr feedback

57818db

patch: include list of missing fields

42b043a

smethnani requested a review from Mehdi-Bendriss October 6, 2026 16:55

akram09 reviewed Oct 7, 2026

View reviewed changes


		return None

		def _set_credentials_incorrect_status(self, failed: bool) -> None:

		message="Waiting for settings from backup integrator",
		short_message="Waiting for backup settings",

Conversation

smethnani commented Oct 5, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description of issue or feature:

Solution:

How was this change tested?

Uh oh!

Mehdi-Bendriss left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

akram09 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

This branch has not been deployed

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

smethnani commented Oct 5, 2026 •

edited

Loading