Skip to content

Blocking Regional Access Boundary Lookup and Seed Support#16720

Draft
macastelaz wants to merge 4 commits intogoogleapis:mainfrom
macastelaz:clean-rab-gcloud
Draft

Blocking Regional Access Boundary Lookup and Seed Support#16720
macastelaz wants to merge 4 commits intogoogleapis:mainfrom
macastelaz:clean-rab-gcloud

Conversation

@macastelaz
Copy link
Copy Markdown

In order for the gcloud CLI to support Regional Access Boundary, the Python auth SDK needs to support blocking lookups as well as allowing an initial seed RAB to be provided (gcloud will set this seed if the CLI has a locally cached valid RAB available).

Additional details can be found at go/rab-python-gcloud-one-pager

Copy link
Copy Markdown
Contributor

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

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 renames the "Trust Boundary" feature to "Regional Access Boundary" (RAB) and introduces a centralized management system for it. Key changes include the implementation of _RegionalAccessBoundaryManager for thread-safe state handling and background refreshes, and the update of multiple credential types to support this new mechanism. The with_trust_boundary method is deprecated in favor of with_regional_access_boundary. Review feedback recommends updating a type hint in the utility module to correctly reflect the expected data types and removing redundant error logging in the base credentials class to reduce log noise.

"""Manually sets the regional access boundary to the client provided seed

Args:
seed (Mapping[str, str]): The regional access boundary to use for the
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 type hint for the seed parameter, Mapping[str, str], is incorrect. The docstring on line 136 states that the expiry key should map to a datetime.datetime object, not a string. This is misleading for users of this method.

Please update the type hint to Mapping[str, Any] to accurately reflect the expected types. You'll also need to import Any from the typing module at the top of the file (line 23).

Suggested change
seed (Mapping[str, str]): The regional access boundary to use for the
seed (Mapping[str, Any]): The regional access boundary to use for the

Comment on lines 481 to +483
if not url:
raise exceptions.InvalidValue("Failed to build trust boundary lookup URL.")
_LOGGER.error("Failed to build Regional Access Boundary lookup URL.")
return 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.

medium

This error log is redundant. All implementations of _build_regional_access_boundary_lookup_url() that can fail and return None already log a more specific error message. This generic log entry adds noise to the logs.

To avoid duplicate logging, please remove this _LOGGER.error call.

Suggested change
if not url:
raise exceptions.InvalidValue("Failed to build trust boundary lookup URL.")
_LOGGER.error("Failed to build Regional Access Boundary lookup URL.")
return None
if not url:
return None
References
  1. Remove duplicate lines of code, especially duplicate assertions in tests, to keep the codebase clean and avoid redundancy.

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.

1 participant