Prototype Spanner Omni Login functionality#13023
Prototype Spanner Omni Login functionality#13023sagnghos wants to merge 2 commits intogoogleapis:mainfrom
Conversation
There was a problem hiding this comment.
Code Review
This pull request implements Spanner Omni authentication using the OPAQUE protocol, introducing a new login client, credential management with background refresh, and necessary cryptographic utilities. Feedback focuses on improving resource efficiency by reusing ManagedChannel and SecureRandom instances, and cleaning up redundant dependency declarations in the pom.xml. Additionally, suggestions were made to simplify code using Arrays.copyOf, remove unreachable logic in the OPAQUE utilities, and refactor the token refresh retry mechanism to be more robust.
I am having trouble creating individual review comments. Click here to see my feedback.
java-spanner/google-cloud-spanner/src/main/java/com/google/cloud/spanner/omni/SpannerOmniCredentials.java (67)
Creating a new ManagedChannel for each token refresh is inefficient. A ManagedChannel is a heavyweight object that manages connections and should be reused. Consider creating the channel once and reusing it for the lifetime of the SpannerOmniCredentials instance.
References
- To improve resource efficiency, cache and share resource-intensive clients, such as MetricServiceClient, instead of creating a new instance for every client.
java-spanner/google-cloud-spanner/pom.xml (562-571)
These dependencies (bcprov-jdk18on and tink) are already declared in the main section (lines 540-549). Declaring them again inside this profile is redundant and can lead to maintenance issues. Please remove this block.
java-spanner/google-cloud-spanner/pom.xml (747-756)
These dependencies (bcprov-jdk18on and tink) are already declared in the main section (lines 540-549). Declaring them again inside this profile is redundant and can lead to maintenance issues. Please remove this block.
java-spanner/google-cloud-spanner/src/main/java/com/google/cloud/spanner/SpannerOptions.java (1852-1853)
This manual array copy can be simplified by using java.util.Arrays.copyOf.
byte[] passwordBytes = java.util.Arrays.copyOf(rawBytes, len);
java-spanner/google-cloud-spanner/src/main/java/com/google/cloud/spanner/omni/LoginClient.java (66)
Creating a new SecureRandom instance on each call can be inefficient as it may need to gather entropy. It's recommended to create a single, static SecureRandom instance for the class and reuse it.
References
- To improve resource efficiency, cache and share resource-intensive clients instead of creating a new instance for every client.
java-spanner/google-cloud-spanner/src/main/java/com/google/cloud/spanner/omni/LoginClient.java (252-257)
This block contains redundant error handling. The if (error != null) check on line 249 already covers the error case, so the inner check on line 253 is unreachable. The block can be simplified to only handle the completed case.
if (response == LoginResponse.getDefaultInstance() && completed) {
return null;
}java-spanner/google-cloud-spanner/src/main/java/com/google/cloud/spanner/omni/SpannerOmniCredentials.java (139)
The way scheduleRefresh is called for retries is a bit of a hack. It relies on the internal logic of scheduleRefresh to calculate the actual delay from a 'fake' token lifetime. This makes the code harder to understand and more brittle. It would be clearer to schedule the retry directly with SHARED_EXECUTOR.schedule() using the calculated retryDelay.
java-spanner/google-cloud-spanner/src/main/java/com/google/cloud/spanner/omni/opaque/OpaqueUtil.java (307-309)
The condition iBytes.length == 0 is unreachable. BigInteger.toByteArray() will never return an empty array. For i=0 it returns [0], and since the loop starts from i=1, i is always positive and the byte array will be non-empty. This else if branch can be removed.
No description provided.