Skip to content
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

feat: improve aiohttp client error messages #1201

Merged
merged 10 commits into from
Dec 3, 2024
30 changes: 26 additions & 4 deletions google/cloud/sql/connector/client.py
Original file line number Diff line number Diff line change
Expand Up @@ -128,8 +128,19 @@ async def _get_metadata(
resp = await self._client.get(url, headers=headers)
if resp.status >= 500:
resp = await retry_50x(self._client.get, url, headers=headers)
resp.raise_for_status()
ret_dict = await resp.json()
# try to get response json for better error message
try:
ret_dict = await resp.json()
if resp.status >= 400:
# if detailed error message is in json response, use as error message
message = ret_dict.get("error", {}).get("message")
if message:
resp.reason = message
# skip, raise_for_status will catch all errors in finally block
except Exception:
pass
finally:
resp.raise_for_status()

if ret_dict["region"] != region:
raise ValueError(
Expand Down Expand Up @@ -198,8 +209,19 @@ async def _get_ephemeral(
resp = await self._client.post(url, headers=headers, json=data)
if resp.status >= 500:
resp = await retry_50x(self._client.post, url, headers=headers, json=data)
resp.raise_for_status()
ret_dict = await resp.json()
# try to get response json for better error message
try:
ret_dict = await resp.json()
if resp.status >= 400:
# if detailed error message is in json response, use as error message
message = ret_dict.get("error", {}).get("message")
if message:
resp.reason = message
# skip, raise_for_status will catch all errors in finally block
except Exception:
pass
finally:
resp.raise_for_status()

ephemeral_cert: str = ret_dict["ephemeralCert"]["cert"]

Expand Down
11 changes: 0 additions & 11 deletions google/cloud/sql/connector/instance.py
Original file line number Diff line number Diff line change
Expand Up @@ -22,8 +22,6 @@
from datetime import timezone
import logging

import aiohttp

from google.cloud.sql.connector.client import CloudSQLClient
from google.cloud.sql.connector.connection_info import ConnectionInfo
from google.cloud.sql.connector.connection_name import _parse_instance_connection_name
Expand Down Expand Up @@ -128,15 +126,6 @@ async def _perform_refresh(self) -> ConnectionInfo:
f"expiration = {connection_info.expiration.isoformat()}"
)

except aiohttp.ClientResponseError as e:
logger.debug(
f"['{self._conn_name}']: Connection info "
f"refresh operation failed: {str(e)}"
)
if e.status == 403:
e.message = "Forbidden: Authenticated IAM principal does not seem authorized to make API request. Verify 'Cloud SQL Admin API' is enabled within your GCP project and 'Cloud SQL Client' role has been granted to IAM principal."
raise

except Exception as e:
logger.debug(
f"['{self._conn_name}']: Connection info "
jackwotherspoon marked this conversation as resolved.
Show resolved Hide resolved
Expand Down
72 changes: 72 additions & 0 deletions tests/unit/test_client.py
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,9 @@
import datetime
from typing import Optional

from aiohttp import ClientResponseError
from aioresponses import aioresponses
from google.auth.credentials import Credentials
from mocks import FakeCredentials
import pytest

Expand Down Expand Up @@ -138,3 +141,72 @@ async def test_CloudSQLClient_user_agent(
assert client._user_agent == f"cloud-sql-python-connector/{version}+{driver}"
# close client
await client.close()


async def test_cloud_sql_error_messages_get_metadata(
fake_credentials: Credentials,
) -> None:
"""
Test that Cloud SQL Admin API error messages are raised for _get_metadata.
"""
# mock Cloud SQL Admin API calls with exceptions
client = CloudSQLClient(
sqladmin_api_endpoint="https://sqladmin.googleapis.com",
quota_project=None,
credentials=fake_credentials,
)
get_url = "https://sqladmin.googleapis.com/sql/v1beta4/projects/my-project/instances/my-instance/connectSettings"
resp_body = {
"error": {
"code": 403,
"message": "Cloud SQL Admin API has not been used in project 123456789 before or it is disabled",
}
}
with aioresponses() as mocked:
mocked.get(
get_url,
status=403,
payload=resp_body,
repeat=True,
)
with pytest.raises(ClientResponseError) as exc_info:
await client._get_metadata("my-project", "my-region", "my-instance")
assert exc_info.value.status == 403
assert (
exc_info.value.message
== "Cloud SQL Admin API has not been used in project 123456789 before or it is disabled"
)
await client.close()


async def test_cloud_sql_error_messages_get_ephemeral(
fake_credentials: Credentials,
) -> None:
"""
Test that Cloud SQL Admin API error messages are raised for _get_ephemeral.
"""
# mock Cloud SQL Admin API calls with exceptions
client = CloudSQLClient(
sqladmin_api_endpoint="https://sqladmin.googleapis.com",
quota_project=None,
credentials=fake_credentials,
)
post_url = "https://sqladmin.googleapis.com/sql/v1beta4/projects/my-project/instances/my-instance:generateEphemeralCert"
resp_body = {
"error": {
"code": 404,
"message": "The Cloud SQL instance does not exist.",
}
}
with aioresponses() as mocked:
mocked.post(
post_url,
status=404,
payload=resp_body,
repeat=True,
)
with pytest.raises(ClientResponseError) as exc_info:
await client._get_ephemeral("my-project", "my-instance", "my-key")
assert exc_info.value.status == 404
assert exc_info.value.message == "The Cloud SQL instance does not exist."
await client.close()
57 changes: 0 additions & 57 deletions tests/unit/test_instance.py
Original file line number Diff line number Diff line change
Expand Up @@ -17,10 +17,6 @@
import asyncio
import datetime

from aiohttp import ClientResponseError
from aiohttp import RequestInfo
from aioresponses import aioresponses
from google.auth.credentials import Credentials
from mock import patch
import mocks
import pytest # noqa F401 Needed to run the tests
Expand Down Expand Up @@ -266,59 +262,6 @@ async def test_get_preferred_ip_CloudSQLIPTypeError(cache: RefreshAheadCache) ->
instance_metadata.get_preferred_ip(IPTypes.PSC)


@pytest.mark.asyncio
async def test_ClientResponseError(
fake_credentials: Credentials,
) -> None:
"""
Test that detailed error message is applied to ClientResponseError.
"""
# mock Cloud SQL Admin API calls with exceptions
keys = asyncio.create_task(generate_keys())
client = CloudSQLClient(
sqladmin_api_endpoint="https://sqladmin.googleapis.com",
quota_project=None,
credentials=fake_credentials,
)
get_url = "https://sqladmin.googleapis.com/sql/v1beta4/projects/my-project/instances/my-instance/connectSettings"
post_url = "https://sqladmin.googleapis.com/sql/v1beta4/projects/my-project/instances/my-instance:generateEphemeralCert"
with aioresponses() as mocked:
mocked.get(
get_url,
status=403,
exception=ClientResponseError(
RequestInfo(get_url, "GET", headers=[]), history=[], status=403 # type: ignore
),
repeat=True,
)
mocked.post(
post_url,
status=403,
exception=ClientResponseError(
RequestInfo(post_url, "POST", headers=[]), history=[], status=403 # type: ignore
),
repeat=True,
)
cache = RefreshAheadCache(
"my-project:my-region:my-instance",
client,
keys,
)
try:
await cache._current
except ClientResponseError as e:
assert e.status == 403
assert (
e.message == "Forbidden: Authenticated IAM principal does not "
"seem authorized to make API request. Verify "
"'Cloud SQL Admin API' is enabled within your GCP project and "
"'Cloud SQL Client' role has been granted to IAM principal."
)
finally:
await cache.close()
await client.close()


@pytest.mark.asyncio
async def test_AutoIAMAuthNotSupportedError(fake_client: CloudSQLClient) -> None:
"""
Expand Down
Loading