-
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 16 commits
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,17 @@ | ||||||||||||||||||||
| import abc | ||||||||||||||||||||
| import logging | ||||||||||||||||||||
| from typing import TypeVar | ||||||||||||||||||||
|
|
||||||||||||||||||||
| 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 +44,13 @@ def stop(self) -> None: | |||||||||||||||||||
|
|
||||||||||||||||||||
|
|
||||||||||||||||||||
| class PubSubPublisher(BasePublisher): | ||||||||||||||||||||
| """Publishes Pydantic models to a Google Cloud Pub/Sub topic. | ||||||||||||||||||||
|
|
||||||||||||||||||||
| This is a plain transport: it assumes GCP is configured and builds its client | ||||||||||||||||||||
| eagerly. Callers that may run without GCP should build publishers via | ||||||||||||||||||||
| make_publisher(), which hands back a NullPublisher when publishing is off. | ||||||||||||||||||||
| """ | ||||||||||||||||||||
|
|
||||||||||||||||||||
| def __init__( | ||||||||||||||||||||
| self, | ||||||||||||||||||||
| project_id: str, | ||||||||||||||||||||
|
|
@@ -103,3 +114,28 @@ def publish(self, data: str, attributes: dict[str, str] | None = None) -> None: | |||||||||||||||||||
| attributes = {} | ||||||||||||||||||||
|
|
||||||||||||||||||||
| super().publish(data, attributes) # type: ignore[type-var] | ||||||||||||||||||||
|
|
||||||||||||||||||||
|
|
||||||||||||||||||||
| def make_publisher(project_id: str, topic_id: str) -> BasePublisher: | ||||||||||||||||||||
| """Build a Pub/Sub publisher, or a NullPublisher when publishing is off. | ||||||||||||||||||||
|
|
||||||||||||||||||||
| Publishing is opt-in: unless OSPREY_PUBSUB_ENABLED is true, this returns a | ||||||||||||||||||||
| NullPublisher (the default, no-GCP experience). When it is true but GCP | ||||||||||||||||||||
| credentials cannot be resolved, PubSubPublisher construction raises; that is a | ||||||||||||||||||||
| misconfiguration, so we log, emit a one-time configuration.errors metric, and | ||||||||||||||||||||
| fall back to a NullPublisher rather than crash. | ||||||||||||||||||||
| """ | ||||||||||||||||||||
| # Imported here to avoid pulling the config/singletons stack into this low-level module. | ||||||||||||||||||||
| from osprey.worker.lib.singletons import CONFIG | ||||||||||||||||||||
|
|
||||||||||||||||||||
| if not CONFIG.instance().get_bool('OSPREY_PUBSUB_ENABLED', False): | ||||||||||||||||||||
| return NullPublisher() | ||||||||||||||||||||
| try: | ||||||||||||||||||||
| return PubSubPublisher(project_id, topic_id) | ||||||||||||||||||||
|
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. To retain what was previously here:
Suggested change
Though these perhaps have default values?
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. ty! They do have defaults, but no reason to drop the passthrough. Done in f0b8f6e |
||||||||||||||||||||
| except DefaultCredentialsError: | ||||||||||||||||||||
| logger.warning('GCP credentials not detected, publishing disabled (project=%s, topic=%s)', project_id, topic_id) | ||||||||||||||||||||
|
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.
Suggested change
|
||||||||||||||||||||
| metrics.increment( | ||||||||||||||||||||
| 'configuration.errors', | ||||||||||||||||||||
| tags=[f'project:{project_id}', f'topic:{topic_id}', 'reason:gcp_credentials_missing'], | ||||||||||||||||||||
| ) | ||||||||||||||||||||
| return NullPublisher() | ||||||||||||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,53 @@ | ||
| 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 NullPublisher, PubSubPublisher, make_publisher | ||
|
|
||
|
|
||
| def _mock_config(pubsub_enabled: bool) -> MagicMock: | ||
| config = MagicMock() | ||
| config.instance.return_value.get_bool.return_value = pubsub_enabled | ||
| return config | ||
|
|
||
|
|
||
| def test_make_publisher_returns_null_when_pubsub_not_enabled() -> None: | ||
| config = _mock_config(pubsub_enabled=False) | ||
| with ( | ||
| patch('osprey.worker.lib.singletons.CONFIG', config), | ||
| patch.object(publisher, 'BatchPubsubPublisherClient') as client_cls, | ||
| patch.object(publisher, 'metrics') as metrics_mock, | ||
| ): | ||
| pub = make_publisher('proj', 'topic') | ||
| assert isinstance(pub, NullPublisher) | ||
| # Opt-in and default-off: no client is built and it is not a misconfiguration. | ||
| config.instance.return_value.get_bool.assert_called_once_with('OSPREY_PUBSUB_ENABLED', False) | ||
| assert not client_cls.called | ||
| metrics_mock.increment.assert_not_called() | ||
|
|
||
|
|
||
| def test_make_publisher_returns_pubsub_when_enabled_and_creds_present() -> None: | ||
| with ( | ||
| patch('osprey.worker.lib.singletons.CONFIG', _mock_config(pubsub_enabled=True)), | ||
| patch.object(publisher, 'BatchPubsubPublisherClient') as client_cls, | ||
| ): | ||
| pub = make_publisher('proj', 'topic') | ||
| assert isinstance(pub, PubSubPublisher) | ||
| assert client_cls.called | ||
|
|
||
|
|
||
| def test_make_publisher_falls_back_to_null_and_metric_when_creds_absent(caplog: pytest.LogCaptureFixture) -> None: | ||
| with ( | ||
| patch('osprey.worker.lib.singletons.CONFIG', _mock_config(pubsub_enabled=True)), | ||
| patch.object(publisher, 'BatchPubsubPublisherClient', side_effect=DefaultCredentialsError()), | ||
| patch.object(publisher, 'metrics') as metrics_mock, | ||
| ): | ||
| with caplog.at_level('WARNING', logger=publisher.logger.name): | ||
| pub = make_publisher('proj', 'topic') | ||
| assert isinstance(pub, NullPublisher) | ||
| assert 'credentials not detected' in caplog.text | ||
| metrics_mock.increment.assert_called_once_with( | ||
| 'configuration.errors', | ||
| tags=['project:proj', 'topic:topic', 'reason:gcp_credentials_missing'], | ||
| ) |
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.
Current behavior would be defaulting to True, I think, if I understand/remember this method correctly:
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.
agreed: since the construction-catch already returns a NullPublisher when credentials can't be resolved, a no-GCP setup still runs either way. defaulting to True avoids silently disabling publishing for deployments that already have creds. Done in f0b8f6e.