feat: Synchronizer: pluggable CredentialWriter for principal credentials - #259
Open
saidixith002 wants to merge 6 commits into
Open
feat: Synchronizer: pluggable CredentialWriter for principal credentials#259saidixith002 wants to merge 6 commits into
saidixith002 wants to merge 6 commits into
Conversation
Replace hardcoded plaintext credential logging in create-omnipotent-principal and sync-polaris with a pluggable CredentialWriter interface (mirrors the existing ETagManager pattern), supporting console (default, behavior- preserving), JSON Lines file output with owner-only permissions, and a CUSTOM classname escape hatch for future backends without CLI changes.
dimas-b
changed the title
dimas-b
reviewed
Jul 23, 2026
dimas-b
left a comment
dimas-b
left a comment
There was a problem hiding this comment.
Nice enhancement 👍 Thanks, @saidixith002 !
| * | ||
| * @param properties properties to configure instance with | ||
| */ | ||
| void initialize(Map<String, String> properties); |
There was a problem hiding this comment.
This looks like a factory concern. "Users" of CredentialWriter should not need to worry about initializing it. The factory should return a pre-configured instance.
Remove initialize() from the CredentialWriter interface so the factory returns fully pre-configured instances instead of requiring callers to configure them after construction. Replace the reflective Class.forName instantiation for the CUSTOM type with java.util.ServiceLoader-based discovery, matching the pattern used elsewhere in the main Polaris codebase.
dimas-b
approved these changes
Jul 24, 2026
dimas-b
left a comment
dimas-b
left a comment
There was a problem hiding this comment.
LGTM 👍 Thanks again, @saidixith002 !
| } | ||
|
|
||
| return ServiceLoader.load(CredentialWriter.class).stream() | ||
| .filter(provider -> provider.type().getName().equals(customWriterClassname)) |
There was a problem hiding this comment.
nit: do you expect more than one custom class to be available?
| implementation("org.slf4j:log4j-over-slf4j:2.0.17") | ||
| implementation("org.apache.iceberg:iceberg-spark-runtime-3.3_2.12:1.7.1") | ||
| implementation("org.apache.commons:commons-csv:1.13.0") | ||
| implementation("com.fasterxml.jackson.core:jackson-databind:2.18.3") |
There was a problem hiding this comment.
nit: Do you want to use Jackson 3? It looks like it's the future :)
Merging main (which added skipIcebergContent via apache#256) into this branch duplicated the constructor parameter and call-site lists instead of combining them with the credentialWriter parameter already on this branch. Merge them into a single correct signature, update all call sites, and pass a no-op credentialWriter in the skip-iceberg-content tests since they only exercise syncCatalogs().
Contributor
Author
|
@dimas-b , Resolved all the conflicts. Please merge |
dimas-b
approved these changes
Jul 27, 2026
|
@saidixith002 : please check CI failure out |
ServiceLoader provider-config files are plain classnames with no comment syntax to hold a license header, so Rat was flagging the new CredentialWriter test service file as an unapproved license, failing the :rat task in CI.
Contributor
Author
|
@dimas-b , I'm not able to run the CI workflows. Seems like need approval to run them. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Pluggable
CredentialWriterfor principal credentialsMotivation
polaris-synchronizercurrently logs newly generated/rotated principal credentials (clientId/clientSecret) as plaintext directly via slf4j, in two places:create-omnipotent-principal— a one-shot banner logged after creating the omnipotent principal.sync-polaris --sync-principals— logs credentials for every principal created or overwritten on the target during a sync.This means secrets always land wherever the logger happens to be configured (console, log file, log aggregator, CI output, etc.), with no way to redirect them somewhere safer, and no way to plug in an organization-specific secrets backend without editing tool internals.
What changed
Introduces a small, pluggable
CredentialWriterabstraction — mirroring the existingETagManager/ETagManagerFactorypattern already used in this module for ETag storage — so credential output is configurable instead of hardcoded.CredentialWriter(api/.../access/CredentialWriter.java): generic interface (initialize(properties),writeCredentials(...),AutoCloseable) so the destination is decoupled from the tool.ConsoleCredentialWriter(default): reproduces the existing banner output byte-for-byte — no behavior change for existing invocations.JsonFileCredentialWriter: writes credentials as JSON Lines (one object per principal, flushed immediately) to a file created with owner-only (rw-------) permissions, since it holds plaintext secrets. Supportsappendfor accumulating across runs.CredentialWriterFactory:CONSOLE/FILE/CUSTOMtypes, withCUSTOMreflectively loading a user-supplied classname — so a future backend (e.g. a secrets manager) can be added with zero changes to this repo, no new CLI flags required.create-omnipotent-principalandsync-polarisvia two new options:--credential-output-type(defaultCONSOLE) and--credential-output-properties.PolarisSynchronizer.syncPrincipals()no longer routes secrets throughclientLogger— progress logging ("Created principal X on target.") and credential output are now fully separated.Backward compatibility
Default
--credential-output-type=CONSOLEon both commands preserves the exact current banner text, logger name, log level, and placement in program flow. No behavior changes for existing invocations unless the new flags are explicitly used.Testing
CredentialWriterFactoryTest,JsonFileCredentialWriterTest(JSON Lines format, append vs. overwrite semantics, POSIX file permission enforcement)../gradlew :polaris-synchronizer-api:test :polaris-synchronizer-cli:testpasses.create-omnipotent-principalwith defaultCONSOLEoutput — banner unchanged.create-omnipotent-principal --credential-output-type FILE— file created with600permissions, valid JSON line with correctclientId/clientSecret.sync-polaris --sync-principals --credential-output-type FILE— output file accumulates one JSON line per synced principal; confirmed noclientSecretvalues appear anywhere inclientLoggeroutput.--credential-output-type CUSTOMwith a classname — confirmed the reflective loading path works.Checklist
README.md) updated with new flags and examples