gemini-code-assist[bot] commented on code in PR #38928:
URL: https://github.com/apache/beam/pull/38928#discussion_r3398289362
##########
sdks/java/io/google-cloud-platform/src/main/java/org/apache/beam/sdk/io/gcp/spanner/SpannerConfig.java:
##########
@@ -367,6 +376,7 @@ public SpannerConfig
withExperimentalHost(ValueProvider<String> experimentalHost
return toBuilder()
.setInstanceId(EXPERIMENTAL_HOST_INSTANCE_ID)
.setExperimentalHost(experimentalHost)
+
.setCredentials(ValueProvider.StaticValueProvider.of(NoCredentials.getInstance()))
.build();
Review Comment:
Unconditionally setting `NoCredentials` inside `withExperimentalHost` makes
the configuration order-dependent. If a user configures custom credentials
first and then calls `withExperimentalHost`, their credentials will be silently
overwritten.
Instead of forcing `NoCredentials` here, consider removing this line and
defaulting to `NoCredentials` in `SpannerAccessor.buildSpannerOptions` when an
`experimentalHost` is present and no custom credentials have been provided.
```java
return toBuilder()
.setInstanceId(EXPERIMENTAL_HOST_INSTANCE_ID)
.setExperimentalHost(experimentalHost)
.build();
```
##########
sdks/java/io/google-cloud-platform/src/main/java/org/apache/beam/sdk/io/gcp/spanner/SpannerTransformRegistrar.java:
##########
@@ -110,6 +112,14 @@ public void setPlainText(@Nullable Boolean plainText) {
this.plainText = plainText;
}
+ public void setClientCertPath(@Nullable String clientCertPath) {
+ this.clientCertPath = clientCertPath;
+ }
+
+ public void setClientCertKeyPath(@Nullable String clientCertKeyPath) {
+ this.clientCertKeyPath = clientCertKeyPath;
+ }
+
void checkMandatoryFields() {
if (projectId.isEmpty()) {
throw new IllegalArgumentException("projectId can't be empty");
Review Comment:
If a user of the cross-language transform specifies only one of
`clientCertPath` or `clientCertKeyPath`, the configuration will be silently
ignored because the build methods only apply them if both are non-null.
Add a validation check in `checkMandatoryFields()` to ensure that either
both or neither of these fields are specified.
```java
void checkMandatoryFields() {
if (projectId.isEmpty()) {
throw new IllegalArgumentException("projectId can't be empty");
}
if ((clientCertPath != null) != (clientCertKeyPath != null)) {
throw new IllegalArgumentException(
"Both clientCertPath and clientCertKeyPath must be specified
together.");
}
}
```
##########
sdks/java/io/google-cloud-platform/src/main/java/org/apache/beam/sdk/io/gcp/spanner/changestreams/MetadataSpannerConfigFactory.java:
##########
@@ -87,6 +87,15 @@ public static SpannerConfig create(
config = config.withUsingPlainTextChannel(plainText.get());
}
+ ValueProvider<String> clientCert = primaryConfig.getClientCertPath();
+ ValueProvider<String> clientKey = primaryConfig.getClientCertKeyPath();
+ if (clientCert != null
+ && clientCert.get() != null
+ && clientKey != null
+ && clientKey.get() != null) {
+ config = config.withClientCert(clientCert.get(), clientKey.get());
+ }
Review Comment:
Instead of calling `.get()` on the `ValueProvider`s, you can pass the
`ValueProvider` instances directly to `withClientCert(ValueProvider<String>,
ValueProvider<String>)`. This avoids unnecessary evaluation of the providers
and is safer if the configuration is ever initialized or validated during
pipeline construction.
```java
ValueProvider<String> clientCert = primaryConfig.getClientCertPath();
ValueProvider<String> clientKey = primaryConfig.getClientCertKeyPath();
if (clientCert != null && clientKey != null) {
config = config.withClientCert(clientCert, clientKey);
}
```
##########
sdks/java/io/google-cloud-platform/src/test/java/org/apache/beam/sdk/io/gcp/spanner/SpannerTestHelper.java:
##########
@@ -0,0 +1,110 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.beam.sdk.io.gcp.spanner;
+
+import com.google.cloud.spanner.SpannerOptions;
+import org.apache.beam.sdk.extensions.gcp.options.GcpOptions;
+import org.apache.beam.sdk.options.PipelineOptions;
+import org.apache.beam.vendor.guava.v32_1_2_jre.com.google.common.base.Strings;
+
+/** Helper methods for Spanner IT tests to support Spanner Omni natively. */
+public class SpannerTestHelper {
+
+ public static String getOmniEndpoint() {
+ return System.getenv("SPANNER_OMNI_ENDPOINT");
+ }
+
+ public static boolean isOmni() {
+ return !Strings.isNullOrEmpty(getOmniEndpoint());
+ }
+
+ public static String getOmniClientCert() {
+ return System.getenv("SPANNER_OMNI_CLIENT_CERT");
+ }
+
+ public static String getOmniClientKey() {
+ return System.getenv("SPANNER_OMNI_CLIENT_KEY");
+ }
+
+ public static boolean isOmniUsePlainText() {
+ return Boolean.parseBoolean(System.getenv("SPANNER_OMNI_USE_PLAIN_TEXT"));
+ }
+
+ public static String getProject(PipelineOptions options, String
instanceProjectId) {
+ if (isOmni()) {
+ return "default";
+ }
+ String project = instanceProjectId;
+ if (project == null) {
+ project = options.as(GcpOptions.class).getProject();
+ }
+ return project;
+ }
+
+ public static String getInstanceId(String instanceId) {
+ if (isOmni()) {
+ return "default";
+ }
+ return instanceId;
+ }
+
+ public static SpannerOptions.Builder
setUpSpannerOptions(SpannerOptions.Builder builder) {
+ if (isOmni()) {
+ builder.setExperimentalHost(getOmniEndpoint());
+ if (isOmniUsePlainText()) {
+ builder.usePlainText();
+ }
+ String cert = getOmniClientCert();
+ String key = getOmniClientKey();
+ if (!Strings.isNullOrEmpty(cert) && !Strings.isNullOrEmpty(key)) {
+ builder.useClientCert(cert, key);
+ }
+ }
+ return builder;
+ }
Review Comment:
When `isOmni()` is true, we should explicitly set `NoCredentials` on the
`SpannerOptions.Builder` so that the direct Java SDK client doesn't attempt to
load Google Application Default Credentials (ADC), which might fail or slow
down the tests in environments without GCP credentials.
```suggestion
public static SpannerOptions.Builder
setUpSpannerOptions(SpannerOptions.Builder builder) {
if (isOmni()) {
builder.setExperimentalHost(getOmniEndpoint());
builder.setCredentials(com.google.cloud.NoCredentials.getInstance());
if (isOmniUsePlainText()) {
builder.usePlainText();
}
String cert = getOmniClientCert();
String key = getOmniClientKey();
if (!Strings.isNullOrEmpty(cert) && !Strings.isNullOrEmpty(key)) {
builder.useClientCert(cert, key);
}
}
return builder;
}
```
##########
sdks/java/io/google-cloud-platform/src/main/java/org/apache/beam/sdk/io/gcp/spanner/SpannerAccessor.java:
##########
@@ -249,6 +249,14 @@ static SpannerOptions buildSpannerOptions(SpannerConfig
spannerConfig) {
builder.setChannelConfigurator(b -> b.usePlaintext());
builder.setCredentials(NoCredentials.getInstance());
}
+ ValueProvider<String> clientCert = spannerConfig.getClientCertPath();
+ ValueProvider<String> clientKey = spannerConfig.getClientCertKeyPath();
+ if (clientCert != null
+ && clientKey != null
+ && !Strings.isNullOrEmpty(clientCert.get())
+ && !Strings.isNullOrEmpty(clientKey.get())) {
+ builder.useClientCert(clientCert.get(), clientKey.get());
+ }
}
Review Comment:
The client certificate configuration is currently placed inside the `if
(plainText != null)` block (which ends at line 260). Since `plainText` is
`null` by default (unless explicitly configured), this entire block will be
skipped for standard mTLS configurations where `plainText` is not set. This
prevents the client certificates from being applied to the `SpannerOptions`
builder.
To fix this, move the client certificate configuration outside of the
`plainText` block so that it is evaluated independently. Additionally, we
should validate that both `clientCertPath` and `clientCertKeyPath` are provided
together to avoid silent failures.
```java
}
}
ValueProvider<String> clientCert = spannerConfig.getClientCertPath();
ValueProvider<String> clientKey = spannerConfig.getClientCertKeyPath();
boolean hasCert = clientCert != null &&
!Strings.isNullOrEmpty(clientCert.get());
boolean hasKey = clientKey != null &&
!Strings.isNullOrEmpty(clientKey.get());
if (hasCert != hasKey) {
throw new IllegalArgumentException(
"Both clientCertPath and clientCertKeyPath must be provided for
mTLS.");
}
if (hasCert && hasKey) {
builder.useClientCert(clientCert.get(), clientKey.get());
}
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
