-
Notifications
You must be signed in to change notification settings - Fork 59
feat(publisher): degrade PubSubPublisher to noop when GCP credentials are absent #388
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
base: main
Are you sure you want to change the base?
Changes from 1 commit
05c0cdc
0d5f06c
f7d7fd6
641717a
3b40c28
749156e
f0cee7c
b678c6f
e8e2770
362a860
48cefda
6cbe134
2d40415
6b84b57
e825f47
b1adab9
f0b8f6e
707a180
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 |
|---|---|---|
| @@ -1,13 +1,18 @@ | ||
| import abc | ||
| import logging | ||
| from typing import Dict, Optional, TypeVar | ||
|
|
||
| import google.auth | ||
| from google.auth.exceptions import DefaultCredentialsError | ||
| from google.cloud import pubsub_v1 | ||
| from osprey.worker.lib.instruments import metrics | ||
| from osprey.worker.lib.pubsub.publisher_client import BatchPubsubPublisherClient | ||
| from pydantic import BaseModel | ||
|
|
||
| _PydanticModelT = TypeVar('_PydanticModelT', bound=BaseModel) | ||
|
|
||
| logger = logging.getLogger(__name__) | ||
|
|
||
|
|
||
| class BasePublisher(abc.ABC): | ||
| """ | ||
|
|
@@ -40,6 +45,15 @@ def stop(self) -> None: | |
|
|
||
|
|
||
| class PubSubPublisher(BasePublisher): | ||
| """Publishes Pydantic models to a Google Cloud Pub/Sub topic. | ||
|
|
||
| Degrades to noop mode when GCP credentials cannot be resolved at construction | ||
| time (e.g. local dev or adopter environments without GCP). In noop mode no | ||
| underlying client is built, publish() and stop() return immediately, and a | ||
| `PubSubPublisher.publisher.noop` metric is incremented per call so the inert | ||
| state is visible in dashboards. A one-time warning is logged at construction. | ||
| """ | ||
|
|
||
| def __init__( | ||
| self, | ||
| project_id: str, | ||
|
|
@@ -54,15 +68,23 @@ def __init__( | |
| topic_id=topic_id, | ||
| ) | ||
| self._project = project_id | ||
| self._raise_on_error = raise_on_error | ||
| self._tags = [f'project:{self._project}', f'topic:{self._topic_name}'] | ||
| self._enabled = _check_gcp_credentials() | ||
| if not self._enabled: | ||
| logger.warning( | ||
| 'GCP credentials not detected — PubSubPublisher running in noop mode (project=%s, topic=%s)', | ||
| project_id, | ||
| topic_id, | ||
| ) | ||
| return | ||
| batch_settings = pubsub_v1.types.BatchSettings( | ||
| max_bytes=max_bytes, # default 1MB | ||
| max_messages=max_messages, # default 100 messages | ||
| max_latency=max_latency, | ||
| ) | ||
| # self._publisher = pubsub_v1.PublisherClient(batch_settings=batch_settings) | ||
| self._publisher = BatchPubsubPublisherClient(batch_settings=batch_settings) | ||
| self._raise_on_error = raise_on_error | ||
| self._tags = [f'project:{self._project}', f'topic:{self._topic_name}'] | ||
|
|
||
| def prepare_data(self, data: _PydanticModelT) -> bytes: | ||
| """ | ||
|
|
@@ -72,6 +94,9 @@ def prepare_data(self, data: _PydanticModelT) -> bytes: | |
| return data.json(exclude_none=True).encode() | ||
|
|
||
| def publish(self, data: _PydanticModelT, attributes: Optional[Dict[str, str]] = None) -> None: | ||
| if not self._enabled: | ||
| metrics.increment(f'{self.__class__.__name__}.publisher.noop', tags=self._tags) | ||
| return | ||
| if attributes is None: | ||
| attributes = {} | ||
|
|
||
|
|
@@ -89,6 +114,8 @@ def publish(self, data: _PydanticModelT, attributes: Optional[Dict[str, str]] = | |
| metrics.increment(f'{self.__class__.__name__}.publisher.success', tags=self._tags) | ||
|
|
||
| def stop(self) -> None: | ||
| if not self._enabled: | ||
| return | ||
| self._publisher.stop() | ||
|
|
||
|
|
||
|
|
@@ -103,3 +130,17 @@ def publish(self, data: str, attributes: Optional[Dict[str, str]] = None) -> Non | |
| attributes = {} | ||
|
|
||
| super().publish(data, attributes) # type: ignore[type-var] | ||
|
|
||
|
|
||
| _gcp_credentials_available: Optional[bool] = None | ||
|
|
||
|
|
||
| def _check_gcp_credentials() -> bool: | ||
| global _gcp_credentials_available | ||
| if _gcp_credentials_available is None: | ||
| try: | ||
| google.auth.default() | ||
| _gcp_credentials_available = True | ||
|
Contributor
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. I'm wondering if a better way to do this would be to check for "Google" in Seeing as we actually aren't using the credentials, only checking for them.
Member
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. oooh good catch. you're right that we only need to check whether creds exist here, not actually use them. I looked into what that network call really costs:
it shouldn't slows down a normal startup that has creds, and I only run the check once per process and remember the answer, so it doesn't happen on every publish. The reason I'd hold off on reading In that case, If that startup check does turn out to slow down docker compose up, I think the better fix is to just give it a short time limit so it can't get stuck, vs switch to the hardware check and lose the ability to find key-file creds. Happy to do that if you think it's worth it! What do you think?
Contributor
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.
Yeah, this makes sense.
The network cost might be something we need not worry about tbh, putting a limit sounds like a good idea, |
||
| except DefaultCredentialsError: | ||
| _gcp_credentials_available = False | ||
| return _gcp_credentials_available | ||
|
coderabbitai[bot] marked this conversation as resolved.
Outdated
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,74 @@ | ||
| from typing import Iterator | ||
| from unittest.mock import MagicMock, patch | ||
|
|
||
| import pytest | ||
| from google.auth.exceptions import DefaultCredentialsError | ||
| from osprey.worker.lib import publisher | ||
| from osprey.worker.lib.publisher import PubSubPublisher, _check_gcp_credentials | ||
|
|
||
|
|
||
| @pytest.fixture(autouse=True) | ||
| def reset_cred_cache() -> Iterator[None]: | ||
| publisher._gcp_credentials_available = None | ||
| yield | ||
| publisher._gcp_credentials_available = None | ||
|
|
||
|
|
||
| def test_check_gcp_credentials_true_when_default_resolves() -> None: | ||
| with patch.object(publisher.google.auth, 'default', return_value=(MagicMock(), 'proj')): | ||
| assert _check_gcp_credentials() is True | ||
|
|
||
|
|
||
| def test_check_gcp_credentials_false_when_default_raises() -> None: | ||
| with patch.object(publisher.google.auth, 'default', side_effect=DefaultCredentialsError()): | ||
| assert _check_gcp_credentials() is False | ||
|
|
||
|
|
||
| def test_check_gcp_credentials_is_cached() -> None: | ||
| with patch.object(publisher.google.auth, 'default', return_value=(MagicMock(), 'proj')) as default_mock: | ||
| _check_gcp_credentials() | ||
| _check_gcp_credentials() | ||
| _check_gcp_credentials() | ||
| assert default_mock.call_count == 1 | ||
|
|
||
|
|
||
| def test_pubsub_publisher_constructs_client_when_creds_present() -> None: | ||
| with ( | ||
| patch.object(publisher.google.auth, 'default', return_value=(MagicMock(), 'proj')), | ||
| patch.object(publisher, 'BatchPubsubPublisherClient') as client_cls, | ||
| ): | ||
| pub = PubSubPublisher('proj', 'topic') | ||
| assert pub._enabled is True | ||
| assert client_cls.called | ||
|
|
||
|
|
||
| def test_pubsub_publisher_noops_when_creds_absent(caplog: pytest.LogCaptureFixture) -> None: | ||
| with ( | ||
| patch.object(publisher.google.auth, 'default', side_effect=DefaultCredentialsError()), | ||
| patch.object(publisher, 'BatchPubsubPublisherClient') as client_cls, | ||
| ): | ||
| with caplog.at_level('WARNING', logger=publisher.logger.name): | ||
| pub = PubSubPublisher('proj', 'topic') | ||
| assert pub._enabled is False | ||
| assert not client_cls.called | ||
| assert 'noop mode' in caplog.text | ||
| assert 'project=proj' in caplog.text | ||
| assert 'topic=topic' in caplog.text | ||
|
|
||
|
|
||
| def test_publish_short_circuits_and_emits_noop_metric_when_disabled() -> None: | ||
| with ( | ||
| patch.object(publisher.google.auth, 'default', side_effect=DefaultCredentialsError()), | ||
| patch.object(publisher, 'metrics') as metrics_mock, | ||
| ): | ||
| pub = PubSubPublisher('proj', 'topic') | ||
| pub.publish(MagicMock()) | ||
| metric_names = [call.args[0] for call in metrics_mock.increment.call_args_list] | ||
| assert metric_names == ['PubSubPublisher.publisher.noop'] | ||
|
|
||
|
|
||
| def test_stop_short_circuits_when_disabled() -> None: | ||
| with patch.object(publisher.google.auth, 'default', side_effect=DefaultCredentialsError()): | ||
| pub = PubSubPublisher('proj', 'topic') | ||
| pub.stop() | ||
| assert not hasattr(pub, '_publisher') |
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.
Not sure on the usage of another metric here, and I think the dashboard could probably just surface this as a warning banner or something "Publishing to GCP PubSub is currently disabled due to invalid credentials"
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.
ooooh good idea! I'll work that in
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.
i'm actually going to add that separately, since it's a UI change and i want to keep this and #411 focused on the publisher changes