[Java] Support dynamic secret provider registration via SecretRegistrar - #39940
[Java] Support dynamic secret provider registration via SecretRegistrar#39940shunping wants to merge 5 commits 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?
There was a problem hiding this comment.
The new behavior is correct.
For fromJson(), we only initiate secret manager classes (except for RawSecret) via their fromMap function, which has parameter validation.
GcpSecret(spec) is only used as a shortcut for testing or when users want to directly initialize GcpSecret (not via Secret factory method).
| 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.
There was a problem hiding this comment.
Added code to give warning message if duplicating classes are seen and use the first-seen class in the factory.
| 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.
There was a problem hiding this comment.
Added code for fail-safe handling
| @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?
ad68566 to
41ad48d
Compare
| } | ||
|
|
||
| @Test | ||
| public void testSupportedTypesRetainsPascalCase() { |
There was a problem hiding this comment.
It's good to restore original naming, though no need every follow up change adding a trivial test case. Consider run a negative diff
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.