Skip to content

feat(storage): add enable_bucket_metadata_cache opt-out - #18203

Open
ayam04 wants to merge 4 commits into
googleapis:mainfrom
ayam04:feat/storage-metadata-cache-optout
Open

feat(storage): add enable_bucket_metadata_cache opt-out#18203
ayam04 wants to merge 4 commits into
googleapis:mainfrom
ayam04:feat/storage-metadata-cache-optout

Conversation

@ayam04

@ayam04 ayam04 commented Aug 23, 2026

Copy link
Copy Markdown

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):

  • cache enabled by default
  • opt-out sets _bucket_metadata_cache to None
  • close() works with the cache disabled

Validation:

  • test_client.py + test_bucket.py + test__bucket_metadata_cache.py: 423 passed
  • black --check: clean
  • (gRPC/gapic suites need grpc deps not installable here; unrelated to this change)

ayam04 added 4 commits August 22, 2026 19:09
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
@ayam04
ayam04 requested review from a team as code owners August 23, 2026 20:11
@ayam04
ayam04 requested review from julianxu-goog and removed request for a team August 23, 2026 20:11
@ayam04

ayam04 commented Aug 23, 2026

Copy link
Copy Markdown
Author

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.

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment on lines +304 to +306
self._bucket_metadata_cache = (
BucketMetadataCache(self) if enable_bucket_metadata_cache else None
)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

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
  1. 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.

Comment on lines +186 to +187
from types import MethodType
from google.cloud.bigquery.retry import INSERT_ROWS_DEFAULT_RETRY, _should_retry_insert_rows

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The import from types import MethodType is unused in this test and should be removed to keep the code clean.

References
  1. Imports should be clean and unused imports should be removed (PEP 8). (link)

@ayam04

ayam04 commented Aug 23, 2026

Copy link
Copy Markdown
Author

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):

  • google/cloud/storage/_helpers.py:160-161hasattr(client, "_bucket_metadata_cache") and client._bucket_metadata_cache before .get() / .get_or_queue_fetch()
  • _helpers.py:197-198 — same guard for .check_and_evict()
  • google/cloud/storage/_http.py:90-91 — same guard for .get()

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).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

google-cloud-storage: BucketMetadataCache (ACO) triggers storage.buckets.get on object-only operations; no supported opt-out

1 participant