feat(objectstore): enable objectstore auth if key is configured#535
feat(objectstore): enable objectstore auth if key is configured#535matt-codecov wants to merge 1 commit intomainfrom
Conversation
NicoHinderling
left a comment
NicoHinderling
left a comment
There was a problem hiding this comment.
lgtm but worth having @rbro112 take a look as well
matt-codecov
force-pushed
the
matth/objectstore-enable-auth
branch
from
422a71f to
20fea25
Compare
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
| class ServiceConfig: | ||
| """Service configuration data.""" | ||
|
|
||
| sentry_base_url: str | ||
| projects_to_skip: list[str] | ||
| objectstore_url: str | None | ||
| objectstore_config: ObjectstoreConfig |
There was a problem hiding this comment.
Bug: Instantiations of the ServiceConfig dataclass in tests use the old objectstore_url parameter, which was removed, causing a TypeError when tests run.
Severity: HIGH
Suggested Fix
Update all test instantiations of ServiceConfig to pass an ObjectstoreConfig instance to the new objectstore_config parameter instead of passing a string to the removed objectstore_url parameter.
Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent.
Verify if this is a real issue. If it is, propose a fix; if not, explain why it's not
valid.
Location: src/launchpad/service.py#L137-L142
Potential issue: The `ServiceConfig` dataclass was refactored to replace the
`objectstore_url` string parameter with a structured `objectstore_config:
ObjectstoreConfig` parameter. However, multiple test files
(`test_artifact_processor.py`, `test_kafka_service.py`) were not updated to reflect this
change. These tests continue to instantiate `ServiceConfig` using the old
`objectstore_url` keyword argument. Since this argument no longer exists on the
dataclass's `__init__` method, a `TypeError` will be raised at runtime when these tests
are executed, causing the test suite to fail.
Did we get this right? 👍 / 👎 to inform future reviews.
Sentry Build Distribution
|
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 3 potential issues.
Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
|
|
||
| from dataclasses import dataclass | ||
|
|
||
| from objectstore_client import Permission as ObjectstorePermission |
There was a problem hiding this comment.
Unused import ObjectstorePermission in service module
Low Severity
Permission as ObjectstorePermission is imported in service.py but never referenced anywhere in the file. The Permission class is only used in utils/objectstore.py, where it's imported separately. This is dead code left over from development.
| except Exception: | ||
| logger.exception(f"Failed to load objectstore keyfile at {path}") | ||
|
|
||
| return _cached_keyfile |
There was a problem hiding this comment.
Global keyfile cache ignores path parameter changes
Low Severity
_read_keyfile uses a single module-level _cached_keyfile global that ignores the path parameter after the first successful read. If ever called with a different path (e.g., key rotation or tests), the stale cached content from the first call is returned. The cache is not keyed by path, making the function's contract misleading.
| if not _cached_keyfile: | ||
| try: | ||
| with open(path) as f: | ||
| _cached_keyfile = f.read() |
There was a problem hiding this comment.
Key file content not stripped of trailing whitespace
Medium Severity
_read_keyfile passes the raw result of f.read() to TokenGenerator without stripping whitespace. The PR description states the signing key is mounted as a file via GCP Secret Manager CSI driver. File-mounted secrets commonly include a trailing newline, which would become part of the key material passed to TokenGenerator, likely causing token signing failures or invalid auth tokens in production.
2 participants
ObjectstoreConfigdataclass to config which can configure Objectstore signing key/token creation optionssomething to note: in INF-844 our secrets were provisioned with GCP Secret Manager. @rgibert says for now we have to mount them as files until secret syncing in the GCP SM CSI driver is GA, so that's what this PR does.
working with @rgibert to get these into launchpad's config. https://github.com/getsentry/ops/pull/18653 mounts the signing key as a file, next up i need to figure out where to inject these config values