-
-
Notifications
You must be signed in to change notification settings - Fork 2
feat(objectstore): enable objectstore auth if key is configured #535
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 all commits
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 |
|---|---|---|
|
|
@@ -9,6 +9,8 @@ | |
|
|
||
| from dataclasses import dataclass | ||
|
|
||
| from objectstore_client import Permission as ObjectstorePermission | ||
|
|
||
| from launchpad.sentry_client import SentryClient | ||
| from launchpad.utils.logging import get_logger | ||
| from launchpad.utils.statsd import NullStatsd, StatsdInterface, get_statsd | ||
|
|
@@ -121,29 +123,46 @@ def _shutdown_server(self) -> None: | |
| self._server_thread.join(timeout=10) | ||
|
|
||
|
|
||
| @dataclass | ||
| class ObjectstoreConfig: | ||
| """Objectstore client configuration data.""" | ||
|
|
||
| objectstore_url: str | None | ||
| key_id: str | None = None | ||
| key_file: str | None = None | ||
| token_expiry_seconds: int = 60 | ||
|
|
||
|
|
||
| @dataclass | ||
| class ServiceConfig: | ||
| """Service configuration data.""" | ||
|
|
||
| sentry_base_url: str | ||
| projects_to_skip: list[str] | ||
| objectstore_url: str | None | ||
| objectstore_config: ObjectstoreConfig | ||
|
Comment on lines
137
to
+142
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. Bug: Instantiations of the Suggested FixUpdate all test instantiations of Prompt for AI AgentDid we get this right? 👍 / 👎 to inform future reviews. |
||
|
|
||
|
|
||
| def get_service_config() -> ServiceConfig: | ||
| """Get service configuration from environment.""" | ||
| sentry_base_url = os.getenv("SENTRY_BASE_URL") | ||
| projects_to_skip_str = os.getenv("PROJECT_IDS_TO_SKIP") | ||
| projects_to_skip = projects_to_skip_str.split(",") if projects_to_skip_str else [] | ||
| objectstore_url = os.getenv("OBJECTSTORE_URL") | ||
|
|
||
| objectstore_config = ObjectstoreConfig( | ||
| objectstore_url=os.getenv("OBJECTSTORE_URL"), | ||
| key_id=os.getenv("OBJECTSTORE_SIGNING_KEY_ID"), | ||
| key_file=os.getenv("OBJECTSTORE_SIGNING_KEY_FILE"), | ||
| ) | ||
| if expiry_seconds := os.getenv("OBJECTSTORE_TOKEN_EXPIRY_SECONDS"): | ||
| objectstore_config.token_expiry_seconds = int(expiry_seconds) | ||
|
|
||
| if sentry_base_url is None: | ||
| sentry_base_url = "http://getsentry.default" | ||
|
|
||
| return ServiceConfig( | ||
| sentry_base_url=sentry_base_url, | ||
| projects_to_skip=projects_to_skip, | ||
| objectstore_url=objectstore_url, | ||
| objectstore_config=objectstore_config, | ||
| ) | ||
|
|
||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,50 @@ | ||
| from objectstore_client import ( | ||
| Client as ObjectstoreClient, | ||
| ) | ||
| from objectstore_client import ( | ||
| Permission, | ||
| TokenGenerator, | ||
| ) | ||
|
|
||
| import launchpad | ||
|
|
||
| from launchpad.utils.logging import get_logger | ||
|
|
||
| logger = get_logger(__name__) | ||
|
|
||
| _cached_keyfile: str | None = None | ||
|
|
||
| TOKEN_PERMISSIONS: list[Permission] = [ | ||
| Permission.OBJECT_READ, | ||
| Permission.OBJECT_WRITE, | ||
| Permission.OBJECT_DELETE, | ||
| ] | ||
|
|
||
|
|
||
| def _read_keyfile(path: str) -> str | None: | ||
| global _cached_keyfile | ||
| if not _cached_keyfile: | ||
| try: | ||
| with open(path) as f: | ||
| _cached_keyfile = f.read() | ||
|
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. Key file content not stripped of trailing whitespaceMedium Severity
|
||
| except Exception: | ||
| logger.exception(f"Failed to load objectstore keyfile at {path}") | ||
|
|
||
| return _cached_keyfile | ||
|
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. Global keyfile cache ignores path parameter changesLow Severity
|
||
|
|
||
|
|
||
| def create_objectstore_client(config: "launchpad.service.ObjectstoreConfig") -> ObjectstoreClient | None: | ||
| if not config.objectstore_url: | ||
| return None | ||
|
|
||
| token_generator = None | ||
| if config.key_id and config.key_file: | ||
| if secret_key := _read_keyfile(config.key_file): | ||
| token_generator = TokenGenerator( | ||
| config.key_id, | ||
| secret_key, | ||
| config.token_expiry_seconds, | ||
| TOKEN_PERMISSIONS, | ||
| ) | ||
|
|
||
| return ObjectstoreClient(config.objectstore_url, token_generator=token_generator) | ||


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.
Unused import
ObjectstorePermissionin service moduleLow Severity
Permission as ObjectstorePermissionis imported inservice.pybut never referenced anywhere in the file. ThePermissionclass is only used inutils/objectstore.py, where it's imported separately. This is dead code left over from development.