-
Notifications
You must be signed in to change notification settings - Fork 27
[DPE-8005] Handle empty region #1117
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.
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
Changes from all commits
06e754d
0d30ff4
ffe1561
0170beb
984ebfa
47a6bde
e4ad985
c1e8bba
24bdc0c
0915707
c511e3f
03fc000
e8d42b7
6caae64
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -21,10 +21,7 @@ | |
| from botocore.exceptions import ClientError, ConnectTimeoutError, ParamValidationError, SSLError | ||
| from botocore.loaders import create_loader | ||
| from botocore.regions import EndpointResolver | ||
| from charms.data_platform_libs.v0.s3 import ( | ||
| CredentialsChangedEvent, | ||
| S3Requirer, | ||
| ) | ||
| from charms.data_platform_libs.v0.s3 import CredentialsChangedEvent, S3Requirer | ||
| from charms.operator_libs_linux.v2 import snap | ||
| from jinja2 import Template | ||
| from ops.charm import ActionEvent, HookEvent | ||
|
|
@@ -108,11 +105,13 @@ def _tls_ca_chain_filename(self) -> str: | |
| return "" | ||
|
|
||
| def _get_s3_session_resource(self, s3_parameters: dict): | ||
| session = Session( | ||
| aws_access_key_id=s3_parameters["access-key"], | ||
| aws_secret_access_key=s3_parameters["secret-key"], | ||
| region_name=s3_parameters["region"], | ||
| ) | ||
| kwargs = { | ||
| "aws_access_key_id": s3_parameters["access-key"], | ||
| "aws_secret_access_key": s3_parameters["secret-key"], | ||
| } | ||
| if "region" in s3_parameters: | ||
| kwargs["region_name"] = s3_parameters["region"] | ||
| session = Session(**kwargs) | ||
| return session.resource( | ||
| "s3", | ||
| endpoint_url=self._construct_endpoint(s3_parameters), | ||
|
|
@@ -200,7 +199,7 @@ def can_use_s3_repository(self) -> tuple[bool, str]: | |
|
|
||
| else: | ||
| if return_code != 0: | ||
| logger.error(stderr) | ||
| logger.error(f"Failed to run pgbackrest: {stderr}") | ||
| return False, FAILED_TO_INITIALIZE_STANZA_ERROR_MESSAGE | ||
|
|
||
| for stanza in json.loads(stdout): | ||
|
|
@@ -252,7 +251,7 @@ def _construct_endpoint(self, s3_parameters: dict) -> str: | |
|
|
||
| # Construct the endpoint using the region. | ||
| resolver = EndpointResolver(data) | ||
| endpoint_data = resolver.construct_endpoint("s3", s3_parameters["region"]) | ||
| endpoint_data = resolver.construct_endpoint("s3", s3_parameters.get("region")) | ||
|
|
||
| # Use the built endpoint if it is an AWS endpoint. | ||
| if endpoint_data and endpoint.endswith(endpoint_data["dnsSuffix"]): | ||
|
|
@@ -266,7 +265,7 @@ def _create_bucket_if_not_exists(self) -> None: | |
| return | ||
|
|
||
| bucket_name = s3_parameters["bucket"] | ||
| region = s3_parameters.get("region") | ||
| region = s3_parameters.get("region", "") | ||
|
|
||
| try: | ||
| s3 = self._get_s3_session_resource(s3_parameters) | ||
|
|
@@ -640,10 +639,10 @@ def _initialise_stanza(self, event: HookEvent) -> bool: | |
| raise Exception(stderr) | ||
| except TimeoutError as e: | ||
| raise e | ||
| except Exception as e: | ||
| except Exception: | ||
| # If the check command doesn't succeed, remove the stanza name | ||
| # and rollback the configuration. | ||
| logger.exception(e) | ||
| logger.exception("Failed to initialise stanza:") | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Exception context will be logged automatically by |
||
| self._s3_initialization_set_failure(FAILED_TO_INITIALIZE_STANZA_ERROR_MESSAGE) | ||
| return False | ||
|
|
||
|
|
@@ -689,10 +688,10 @@ def check_stanza(self) -> bool: | |
| if return_code != 0: | ||
| raise Exception(stderr) | ||
| self.charm._set_primary_status_message() | ||
| except Exception as e: | ||
| except Exception: | ||
| # If the check command doesn't succeed, remove the stanza name | ||
| # and rollback the configuration. | ||
| logger.exception(e) | ||
| logger.exception("Failed to check stanza:") | ||
| self._s3_initialization_set_failure(FAILED_TO_INITIALIZE_STANZA_ERROR_MESSAGE) | ||
| self.charm.update_config() | ||
| return False | ||
|
|
@@ -975,9 +974,9 @@ def _run_backup( | |
| else: | ||
| try: | ||
| backup_id = list(self._list_backups(show_failed=True).keys())[-1] | ||
| except ListBackupsError as e: | ||
| logger.exception(e) | ||
| except ListBackupsError: | ||
| error_message = "Failed to retrieve backup id" | ||
| logger.exception(error_message) | ||
| logger.error(f"Backup failed: {error_message}") | ||
| event.fail(error_message) | ||
| return | ||
|
|
@@ -1330,8 +1329,6 @@ def _retrieve_s3_parameters(self) -> tuple[dict, list[str]]: | |
|
|
||
| # Add some sensible defaults (as expected by the code) for missing optional parameters | ||
| s3_parameters.setdefault("endpoint", "https://s3.amazonaws.com") | ||
| # Existing behaviour is none not a str | ||
| s3_parameters.setdefault("region", None) # type: ignore | ||
| s3_parameters.setdefault("path", "") | ||
| s3_parameters.setdefault("s3-uri-style", "host") | ||
| s3_parameters.setdefault("delete-older-than-days", "9999999") | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -44,7 +44,7 @@ def get_cloud_config(cloud: str) -> tuple[dict[str, str], dict[str, str]] | None | |
| "endpoint": "https://storage.googleapis.com", | ||
| "bucket": "data-charms-testing", | ||
| "path": f"/postgresql-k8s/{uuid.uuid1()}", | ||
| "region": "", | ||
| "region": "us-east-1", | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Based on headers from 16/edge runs: https://github.com/canonical/postgresql-operator/actions/runs/17406719352/job/49489833563#step:12:3100 |
||
| }, { | ||
| "access-key": os.environ["GCP_ACCESS_KEY"], | ||
| "secret-key": os.environ["GCP_SECRET_KEY"], | ||
|
|
||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Don't create a bucket, so that we can test bucket creation. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add more context so we have an idea where the error happened.