Skip to content

fix: fallback on VPC#13567

Merged
mutianf merged 4 commits into
googleapis:mainfrom
mutianf:dp
Jun 29, 2026
Merged

fix: fallback on VPC#13567
mutianf merged 4 commits into
googleapis:mainfrom
mutianf:dp

Conversation

@mutianf

@mutianf mutianf commented Jun 29, 2026

Copy link
Copy Markdown
Contributor

No description provided.

@mutianf mutianf requested review from a team as code owners June 29, 2026 14:51

@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 adds a check to identify if a PERMISSION_DENIED exception is caused by a VPC Service Controls (VPC-SC) policy violation in ClassicDirectAccessChecker.java. It inspects the error details or falls back to string matching, logging a warning if a violation is found. The review feedback correctly identifies that omitting the assignment of isEligible in the VPC-SC violation branch could lead to a compilation error if the variable is not pre-initialized, and suggests explicitly setting it to false.

@sushanb

sushanb commented Jun 29, 2026

Copy link
Copy Markdown
Contributor

I think in case of PERMISSION_DENIED exception is caused by a VPC Service Controls,
if i understand correctly, it will never send the request to AFE so isALTS should be false, so it should fallback to regular transport.

}

/** Checks if the exception is due to a VPC Service Controls policy violation. */
private boolean isAllowed(StatusRuntimeException e) {

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.

add isAllowedFromVPCServiceControls

String description = e.getStatus().getDescription();
String message = e.getMessage();
return (description != null
&& description.contains("Request is prohibited by organization's policy"))

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.

can you see if this is the authorative string?

I see "request is prohibited by " as well.

@mutianf

mutianf commented Jun 29, 2026

Copy link
Copy Markdown
Contributor Author

/gemini review

@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 introduces a check in ClassicDirectAccessChecker to identify if a PERMISSION_DENIED exception is caused by a VPC Service Controls policy violation, logging a warning instead of resorting to an ALTS check. The review feedback suggests renaming the helper method isAllowedFromVPCServiceControls to isBlockedByVpcServiceControls to accurately reflect its logic, and using toLowerCase(Locale.ROOT) to prevent locale-sensitive string comparison issues.

@mutianf mutianf enabled auto-merge (squash) June 29, 2026 19:24
@mutianf mutianf merged commit e8c428d into googleapis:main Jun 29, 2026
200 of 203 checks passed
@mutianf mutianf deleted the dp branch June 29, 2026 20:10
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.

3 participants