[Java] Support dynamic secret provider registration via SecretRegistrar - #39940
[Java] Support dynamic secret provider registration via SecretRegistrar#39940shunping wants to merge 1 commit into
Conversation
shunping
changed the title
2f5bde7 to
47e95fd
Compare
shunping
changed the title
|
r: @Abacn |
|
Stopping reviewer notifications for this pull request: review requested by someone other than the bot, ceding control. If you'd like to restart, comment |
Follow the FileSystems registration pattern by introducing SecretRegistrar SPI and auto-service discovery in Secret.java. This eliminates hardcoded secret provider logic in Secret.java and allows modular extension for new secret managers.
47e95fd to
e2a5e59
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #39940 +/- ##
=========================================
Coverage 58.37% 58.38%
- Complexity 13491 13500 +9
=========================================
Files 2568 2571 +3
Lines 268743 268751 +8
Branches 11029 11032 +3
=========================================
+ Hits 156888 156898 +10
- Misses 105904 105905 +1
+ Partials 5951 5948 -3
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
| smManager)); | ||
| SecretRegistrar.SecretFactory factory = SECRET_FACTORIES.get(smManager.toLowerCase()); | ||
| if (factory != null) { | ||
| return factory.createSecret(specMap != null ? specMap : Collections.emptyMap()); |
There was a problem hiding this 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?
| String.format( | ||
| "Duplicate SecretRegistrar for secret manager name '%s': %s and %s", | ||
| key, | ||
| factories.get(key).getClass().getName(), |
There was a problem hiding this 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.
| registrar.getSecretFactories().entrySet()) { | ||
| String key = entry.getKey().toLowerCase(); | ||
| if (factories.containsKey(key)) { | ||
| throw new IllegalStateException( |
There was a problem hiding this 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.
| @Override | ||
| public Map<String, SecretFactory> getSecretFactories() { | ||
| return ImmutableMap.of( | ||
| "googlecloudsecretmanager", GcpSecret::fromMap, |
There was a problem hiding this comment.
Error readability: previously, the error displayed the canonical PascalCase names:
now SECRET_FACTORIES.keySet() returns all lower cases.
consider retaining the original camel case?
2 participants
Previously,
Secret.javahad hardcoded switch statements and factory calls for specific secret managers (such as Google Cloud Secret Manager and HSM-generated secrets). Supporting a new secret manager required directly modifying Secret.java.This PR refactors
Secretto follow Apache Beam's standard Service Provider Interface (SPI) pattern (similar toFileSystems/FileSystemRegistrar). It allows secret providers to register dynamically at runtime viaServiceLoaderand@AutoService, decoupling the core Secret management from specific provider implementations.