-
Notifications
You must be signed in to change notification settings - Fork 51
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix: Verify Universe Domain's DirectPath Compatibility after Endpoint Resolution #2412
Conversation
Looking into possible showcase tests to verify client initialization with certain settings. |
I think the configurations with this would revolve around setting flags in |
// If the endpoint is null, then the endpoint hasn't been fully resolved yet (client not | ||
// initialized). Before a call goes through with DirectPath, this validation must succeed. | ||
if (endpoint == null) { | ||
return true; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This would fix the issue but this is a little misleading IMO. If endpoint is null and this method returns true, it does not mean we canUseDirectPathWithUniverseDomain
, but rather we can not validate it.
Can we not log direct path misconfigure on client initialization? And only adding endpoint.contains(Credentials.GOOGLE_DEFAULT_UNIVERSE)
in channel creation where endpoint is guaranteed not null? This should be good enough to meet the requirement for universeDomain validation with DirectPath.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This would fix the issue but this is a little misleading IMO. If endpoint is null and this method returns true, it does not mean we canUseDirectPathWithUniverseDomain, but rather we can not validate it.
Can we not log direct path misconfigure on client initialization?
Makes sense. I think I can just remove the call from logDirectPathMisconfig()
instead of trying to force it in there.
And only adding endpoint.contains(Credentials.GOOGLE_DEFAULT_UNIVERSE) in channel creation where endpoint is guaranteed not null? This should be good enough to meet the requirement for TPC.
sg, will update.
return true; | ||
} | ||
// This is only logged if DirectPath is enabled | ||
LOG.log(Level.WARNING, "DirectPath will only work in the the googleapis.com Universe Domain"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Kept this log statement as it only triggers if these three conditions above are true:
Lines 372 to 374 in 909bdf9
if (isDirectPathEnabled() | |
&& isCredentialDirectPathCompatible() | |
&& isOnComputeEngine() |
Might be beneficial since the user is has all the other DirectPath configurations correct except for Universe Domain.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It could be beneficial, but this check is called on every channel resizing, which by default is every minute, hence could cause spam logs to customers. That's why the logDirectPathMisconfigure
was moved to the constructor initially, so I think we can skip this logging for now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see. Will remove then
|
|
Fixes: #2411