Abacn commented on code in PR #39940:
URL: https://github.com/apache/beam/pull/39940#discussion_r3915084528
##########
sdks/java/core/src/main/java/org/apache/beam/sdk/util/Secret.java:
##########
@@ -152,38 +172,23 @@ public static Secret fromJson(@Nullable String spec,
@Nullable String secretMana
mapper.configure(com.fasterxml.jackson.core.JsonParser.Feature.ALLOW_SINGLE_QUOTES,
true);
specMap = mapper.readValue(spec, new TypeReference<Map<String,
String>>() {});
} catch (Exception e) {
- logger.debug("Failed to parse secret spec as JSON map", e);
+ LOG.debug("Failed to parse secret spec as JSON map", e);
}
}
if (smManager != null) {
- switch (smManager.toLowerCase()) {
- case "googlecloudsecretmanager":
- case "gcpsecret":
- if (specMap != null) {
- return GcpSecret.fromMap(specMap);
- } else if (spec != null) {
- return new GcpSecret(spec);
- } else {
- throw new IllegalArgumentException("Invalid spec for GcpSecret");
- }
- case "googlecloudhsmgeneratedsecretmanager":
- case "gcphsmgeneratedsecret":
- if (specMap != null) {
- return GcpHsmGeneratedSecret.fromMap(specMap);
- } else {
- throw new IllegalArgumentException("Invalid spec for
GcpHsmGeneratedSecret");
- }
- default:
- throw new IllegalArgumentException(
- String.format(
- "Unsupported secret manager: '%s'. Currently supported
options: 'GoogleCloudSecretManager', 'GoogleCloudHsmGeneratedSecretManager'.",
- smManager));
+ SecretRegistrar.SecretFactory factory =
SECRET_FACTORIES.get(smManager.toLowerCase());
+ if (factory != null) {
+ return factory.createSecret(specMap != null ? specMap :
Collections.emptyMap());
Review Comment:
Previously when specMap = null (jackson parser throws) it falls back to
`return GcpSecret.fromMap(specMap)`, now it becomes a
`factory.createSecret(Collections.emptyMap())` and The raw spec string is
effectively dropped. Any concern here?
##########
sdks/java/core/src/main/java/org/apache/beam/sdk/util/Secret.java:
##########
@@ -35,6 +38,30 @@
* should be able to return a valid byte array representing the secret.
*/
public abstract class Secret implements Serializable {
+ private static final Logger LOG = LoggerFactory.getLogger(Secret.class);
+ private static final Map<String, SecretRegistrar.SecretFactory>
SECRET_FACTORIES =
+ loadSecretFactories();
+
+ private static Map<String, SecretRegistrar.SecretFactory>
loadSecretFactories() {
+ Map<String, SecretRegistrar.SecretFactory> factories = new HashMap<>();
+ for (SecretRegistrar registrar :
ReflectHelpers.loadServicesOrdered(SecretRegistrar.class)) {
+ for (Map.Entry<String, SecretRegistrar.SecretFactory> entry :
+ registrar.getSecretFactories().entrySet()) {
+ String key = entry.getKey().toLowerCase();
+ if (factories.containsKey(key)) {
+ throw new IllegalStateException(
+ String.format(
+ "Duplicate SecretRegistrar for secret manager name '%s': %s
and %s",
+ key,
+ factories.get(key).getClass().getName(),
Review Comment:
When using method references like `GcpSecret::fromMap`, their class names
are compiler-generated lambdas (e.g.,
`org.apache.beam.sdk.util.GcpSecret$$Lambda$142/0x000...`) which is not helpful
Checked by test:
```
@Test
public void testGcpHsmGeneratedSecretRegistrarServiceLoader() {
for (SecretRegistrar registrar :
Lists.newArrayList(ServiceLoader.load(SecretRegistrar.class).iterator())) {
if (registrar instanceof GcpHsmGeneratedSecretRegistrar) {
Map<String, SecretRegistrar.SecretFactory> factories =
registrar.getSecretFactories();
assertThat(
factories.keySet(),
hasItems("googlecloudhsmgeneratedsecretmanager",
"gcphsmgeneratedsecret"));
return;
}
}
fail("Expected to find " + GcpHsmGeneratedSecretRegistrar.class);
}
```
We should print actual SecretRegistrar implementations. A way to do this is
to track the registering registrar in a separate map to report the actual
conflicting SecretRegistrar classes
=========
If user shade Beam and relocated packages, there is still risk of
duplicating classes. Not sure how should we handle them.
##########
sdks/java/core/src/main/java/org/apache/beam/sdk/util/GcpSecretRegistrar.java:
##########
@@ -0,0 +1,34 @@
+/*
+ * 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.util;
+
+import com.google.auto.service.AutoService;
+import java.util.Map;
+import
org.apache.beam.vendor.guava.v32_1_2_jre.com.google.common.collect.ImmutableMap;
+
+/** {@link AutoService} registrar for the {@link GcpSecret}. */
+@AutoService(SecretRegistrar.class)
+public class GcpSecretRegistrar implements SecretRegistrar {
+
+ @Override
+ public Map<String, SecretFactory> getSecretFactories() {
+ return ImmutableMap.of(
+ "googlecloudsecretmanager", GcpSecret::fromMap,
Review Comment:
Error readability: previously, the error displayed the canonical PascalCase
names:
https://github.com/apache/beam/pull/39940/changes#diff-48e6d847440ce6ec0da331ff4d7a2b63bba04d6d2e8ca6686a25d88560e869dcL110
now SECRET_FACTORIES.keySet() returns all lower cases.
consider retaining the original camel case?
##########
sdks/java/core/src/main/java/org/apache/beam/sdk/util/Secret.java:
##########
@@ -35,6 +38,30 @@
* should be able to return a valid byte array representing the secret.
*/
public abstract class Secret implements Serializable {
+ private static final Logger LOG = LoggerFactory.getLogger(Secret.class);
+ private static final Map<String, SecretRegistrar.SecretFactory>
SECRET_FACTORIES =
+ loadSecretFactories();
+
+ private static Map<String, SecretRegistrar.SecretFactory>
loadSecretFactories() {
+ Map<String, SecretRegistrar.SecretFactory> factories = new HashMap<>();
+ for (SecretRegistrar registrar :
ReflectHelpers.loadServicesOrdered(SecretRegistrar.class)) {
+ for (Map.Entry<String, SecretRegistrar.SecretFactory> entry :
+ registrar.getSecretFactories().entrySet()) {
+ String key = entry.getKey().toLowerCase();
+ if (factories.containsKey(key)) {
+ throw new IllegalStateException(
Review Comment:
In general, AutoService scans all classes loaded to JVM and if there are
malformed SecretRegistrar (e.g. from unit test, etc) leaked into class path, it
will crash the whole `loadSecretFactories`. For example, if
`getSecretFactories()` returns null or other situations.
Since here it dynamically loads class and execute codes, consider a fail
safe handling here.
--
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]
