feat(storage): add enable_bucket_metadata_cache opt-out - #18203
Conversation
Sorry, something went wrong.
googleapis#17489 made SSLError globally non-retryable, but _should_retry is the base predicate for every retry surface: a single transient TLS reset (SSLEOFError) now fails jobs.get / result() polling outright, where 3.40.1 retried it as a ConnectionError subclass. Remove SSLError from the global non-retryable set and keep the googleapis#17489 carve-out only on the insertAll path via a scoped predicate (INSERT_ROWS_DEFAULT_RETRY): malformed streaming payloads still fail fast, while job polling keeps retrying transient transport resets. Fixes googleapis#18178
The background bucket-metadata cache (ACO) probes storage.buckets.get on object-level operations, producing denied-audit-log noise for principals with object-only IAM roles and no supported way to disable it. Add a keyword-only constructor flag backed by the existing pattern: when False, no cache is instantiated, so the probe never fires. close() already guards None. Fixes googleapis#17650
|
Hi @zhixiangli @ohmayr — this adds the supported opt-out requested in #17650: enable_bucket_metadata_cache: bool = True keyword-only on the storage Client (mirrors the api_key pattern). When False, the ACO background probe never fires, eliminating denied storage.buckets.get audit-log noise for object-only IAM principals. Tests added (default-on, opt-out, close-with-disabled), 423 passed across client/bucket/cache suites, black-clean. CLA signed. Happy to adjust to maintainers' preferred shape (env var instead/also) if you'd rather. |
Sorry, something went wrong.
There was a problem hiding this comment.
Code Review
This pull request scopes the SSLError retry carve-out in google-cloud-bigquery specifically to the streaming-insert path, ensuring other operations continue to retry transient TLS resets. It also adds an option to disable the background bucket-metadata cache in google-cloud-storage. The review feedback recommends using a No-Op cache implementation or adding explicit None checks to avoid potential AttributeErrors when the cache is disabled, and suggests removing an unused import in the unit tests.
Sorry, something went wrong.
| self._bucket_metadata_cache = ( | ||
| BucketMetadataCache(self) if enable_bucket_metadata_cache else None | ||
| ) |
There was a problem hiding this comment.
Setting _bucket_metadata_cache to None when enable_bucket_metadata_cache is False can lead to AttributeErrors if other parts of the codebase (such as bucket operations or trace helpers) attempt to access its methods (e.g., get, set, clear) without checking for None. To prevent potential runtime crashes, consider using a No-Op cache implementation that conforms to the BucketMetadataCache interface but performs no operations, or add explicit None checks before all accesses to _bucket_metadata_cache across the codebase.
References- Specifically enforce defensive programming: for languages that support nullable references (e.g., Go, Python, Java), ensure appropriate null/nil/None checks or other language-idiomatic guards exist before object property accesses.
Sorry, something went wrong.
| from types import MethodType | ||
| from google.cloud.bigquery.retry import INSERT_ROWS_DEFAULT_RETRY, _should_retry_insert_rows |
There was a problem hiding this comment.
The import from types import MethodType is unused in this test and should be removed to keep the code clean.
References- Imports should be clean and unused imports should be removed (PEP 8). (link)
Sorry, something went wrong.
|
Thanks for the review. Verified the None-safety concern against the code before responding: All three access sites already guard against a missing/None cache (the codebase has done this since the cache was introduced):
So a None cache is already inert by design (the issue's own docs used the private-attr = None workaround successfully, and close() checks truthiness too). A No-Op cache would be a second mechanism for the same behavior; the None value rides the existing guards. I also added a test_close_ok_with_disabled_bucket_metadata_cache covering the one path people might worry about. Unused MethodType import in the bigquery test — fixed in f3849af (that file belongs to the sibling PR #18202; the import fix went onto that branch's HEAD). |
Sorry, something went wrong.
Reviewers
At least 1 approving review is required to merge this pull request.
Fixes #17650
The background bucket-metadata cache (ACO) triggers storage.buckets.get on object-only operations like list_blobs / download_*, producing a continuous stream of denied-storage.buckets.get audit-log entries (ERROR severity) for principals granted object-only IAM roles — with no supported opt-out (the only workaround is mutating the private _bucket_metadata_cache attribute).
Change: new keyword-only constructor flag enable_bucket_metadata_cache: bool = True (mirroring the existing api_key pattern). When False, no BucketMetadataCache is instantiated, so create_trace_span_helper's probe is inert — no background thread, no denied audit entries. close() already guards against a missing cache; transfer_manager's client reconstruction is unaffected (it only carries _initial_client_info/).
Tests added (test_client.py):
Validation: