You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@polaris.apache.org by "sepandr (via GitHub)" <gi...@apache.org> on 2026/07/07 17:47:45 UTC
[PR] Make admin tool bootstrap idempotent for already-bootstrapped realms [polaris]
sepandr opened a new pull request, #4996:
URL: https://github.com/apache/polaris/pull/4996
Running the admin tool's `bootstrap` against a realm that is already bootstrapped currently throws `IllegalArgumentException` ("It appears this metastore manager has already been bootstrapped…") and exits with the generic `EXIT_CODE_BOOTSTRAP_ERROR` (3) — the same code as every real failure. That makes automated deployments (Kubernetes Jobs, ArgoCD hook Jobs that run bootstrap on every deploy) fail on any redeploy, with no reliable way to distinguish the benign case short of matching the error text, and Kubernetes offers no "treat exit N as success" escape hatch. See #4995 for full context.
This change makes re-bootstrap a graceful no-op:
- `AuthBootstrapUtil.createPolarisPrincipalForRealm` returns a `PrincipalSecretsResult` with `ReturnStatus.ENTITY_ALREADY_EXISTS` instead of throwing when the root principal already exists (same for the duplicated check in `LocalPolarisMetaStoreManagerFactory`).
- `PolarisMetaStoreManager.bootstrapPolarisService` propagates that result (javadoc updated).
- `BootstrapCommand` prints `Realm '<realm>' is already bootstrapped; skipping.` for such realms and keeps exit code 0; all other failures still exit 3. `--print-credentials` never prints anything for skipped realms — existing credentials are never returned or altered, and realms in the same invocation that are *not* yet bootstrapped are still bootstrapped normally.
**Testing:** new `testBootstrapAgainIsNoOp` in `BasePolarisMetaStoreManagerTest` (runs for both TreeMap metastore variants) asserts the second bootstrap reports `alreadyExists()` and the realm stays fully intact; existing bootstrap/purge CLI tests pass unchanged (`:polaris-core:test`, `:polaris-admin:test` in-memory variants run locally; an admin-CLI-level double-bootstrap test isn't possible yet because consecutive launches don't reuse the dev-services container — see the existing TODO in `RelationalJdbcBootstrapCommandTest`). Also verified behaviorally against a live 1.5.0 deployment (relational-jdbc on PostgreSQL 16) where re-running bootstrap on an existing realm currently fails.
## Checklist
- [x] 🛡️ Don't disclose security issues! (contact security@apache.org)
- [x] 🔗 Clearly explained why the changes are needed, or linked related issues: Fixes #4995
- [x] 🧪 Added/updated tests with good coverage, or manually tested (and explained how)
- [x] 💡 Added comments for complex logic
- [x] 🧾 Updated `CHANGELOG.md` (if needed)
- [ ] 📚 Updated documentation in `site/content/in-dev/unreleased` (if needed)
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Make admin tool bootstrap idempotent for already-bootstrapped realms [polaris]
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b commented on PR #4996:
URL: https://github.com/apache/polaris/pull/4996#issuecomment-4915727612
> ServiceProducers assumes all bootstrap results have secrets (getPrincipalSecrets()), but with this change some results will be alreadyExists. Worth handling here or quick follow-up?
I'd say the code should not assume that :sweat_smile: Only the first bootstrap should emit auto-generated credentials. If that's what you mean by follow-up then, let's do that :)
My personal preference is to avoid generating user-owned secrets in Polaris completely. Right now the admin use has both options: 1) provide a secret or 2) let Polaris generate it.
Ideally, the schema setup should be idempotent. The "root" user set up should be separate (e.g. via Terraform). With external IdP the root user does not even have to exist, but that requires more work to follow #4356
All in all, +1 to follow-up improvements, but I'm afraid it's a long road :sweat_smile:
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Make admin tool bootstrap idempotent for already-bootstrapped realms [polaris]
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b commented on PR #4996:
URL: https://github.com/apache/polaris/pull/4996#issuecomment-4909690694
FYI: @obelix74
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Make admin tool bootstrap idempotent for already-bootstrapped realms [polaris]
Posted by "sepandr (via GitHub)" <gi...@apache.org>.
sepandr commented on PR #4996:
URL: https://github.com/apache/polaris/pull/4996#issuecomment-4909990395
> Thanks for your contribution, @sepandr ! Very reasonable change.
>
> Given that is affects bootstrap behaviour, let's keep it in review for a few more days for other people to have a chance to comment.
@dimas-b thanks for your review, sounds good!
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Make admin tool bootstrap idempotent for already-bootstrapped realms [polaris]
Posted by "sepandr (via GitHub)" <gi...@apache.org>.
sepandr commented on code in PR #4996:
URL: https://github.com/apache/polaris/pull/4996#discussion_r3544446699
##########
polaris-core/src/testFixtures/java/org/apache/polaris/core/persistence/BasePolarisMetaStoreManagerTest.java:
##########
@@ -83,6 +84,19 @@ protected void validateBootstrap() {
polarisTestMetaStoreManager.validateBootstrap();
}
+ /** re-bootstrapping an already-bootstrapped service must be a graceful no-op */
+ @Test
+ protected void testBootstrapAgainIsNoOp() {
Review Comment:
Good call — switched to asserting `ReturnStatus.ENTITY_ALREADY_EXISTS` directly in d316f18be.
##########
runtime/admin/src/main/java/org/apache/polaris/admintool/BootstrapCommand.java:
##########
@@ -169,6 +168,14 @@ public Integer call() {
result.getValue().getPrincipalSecrets().getMainSecret());
spec.commandLine().getOut().println(msg);
}
+ } else if (result.getValue().alreadyExists()) {
Review Comment:
Done in d316f18be — when realms were skipped the final message is now "Bootstrap completed (N realm(s) already bootstrapped).", and "Bootstrap completed successfully." is kept for full bootstraps so existing output expectations still hold.
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Make admin tool bootstrap idempotent for already-bootstrapped realms [polaris]
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b commented on code in PR #4996:
URL: https://github.com/apache/polaris/pull/4996#discussion_r3555625481
##########
polaris-core/src/main/java/org/apache/polaris/core/auth/AuthBootstrapUtil.java:
##########
Review Comment:
The class name is specific to bootstrapping. Renaming to avoid confusion is also fine. @flyrain : Would you like to propose a PR for that?
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Make admin tool bootstrap idempotent for already-bootstrapped realms [polaris]
Posted by "sepandr (via GitHub)" <gi...@apache.org>.
sepandr commented on PR #4996:
URL: https://github.com/apache/polaris/pull/4996#issuecomment-4930574211
Thanks for the pointer @dimas-b — you're right. I've rewritten the branch to drop the `Co-Authored-By` trailer (no AI tool references remain in the commit messages; the code is unchanged) and added an AI-assistance disclosure to the PR description instead, per the contributing guidelines.
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Make admin tool bootstrap idempotent for already-bootstrapped realms [polaris]
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b merged PR #4996:
URL: https://github.com/apache/polaris/pull/4996
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Make admin tool bootstrap idempotent for already-bootstrapped realms [polaris]
Posted by "flyrain (via GitHub)" <gi...@apache.org>.
flyrain commented on PR #4996:
URL: https://github.com/apache/polaris/pull/4996#issuecomment-4930845540
> > Can we wait until all comments are resolved?
>
> @flyrain : I enabled auto-merge (as noted above) based on the previous approvals and the 2 day review period. I believe you could have disabled the auto-merge.
Got it. I guess auto merge will naturally ignore new comments. I'd suggest using it with caution.
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Make admin tool bootstrap idempotent for already-bootstrapped realms [polaris]
Posted by "vigneshio (via GitHub)" <gi...@apache.org>.
vigneshio commented on code in PR #4996:
URL: https://github.com/apache/polaris/pull/4996#discussion_r3543210710
##########
runtime/admin/src/main/java/org/apache/polaris/admintool/BootstrapCommand.java:
##########
@@ -169,6 +168,14 @@ public Integer call() {
result.getValue().getPrincipalSecrets().getMainSecret());
spec.commandLine().getOut().println(msg);
}
+ } else if (result.getValue().alreadyExists()) {
Review Comment:
LGTM..
nit: the success message says "Bootstrap completed successfully" even when realms were skipped. Consider distinguishing between full bootstrap vs. partial skip? Something like "Bootstrap completed (N realms already initialized)" when applicable.
##########
polaris-core/src/testFixtures/java/org/apache/polaris/core/persistence/BasePolarisMetaStoreManagerTest.java:
##########
@@ -83,6 +84,19 @@ protected void validateBootstrap() {
polarisTestMetaStoreManager.validateBootstrap();
}
+ /** re-bootstrapping an already-bootstrapped service must be a graceful no-op */
+ @Test
+ protected void testBootstrapAgainIsNoOp() {
Review Comment:
Test LGTM.. Would it be worth asserting the specific ReturnStatus directly? Currently using` isSuccess` + `alreadyExists`, but an exact match might be clearer. WDYT ?
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Make admin tool bootstrap idempotent for already-bootstrapped realms [polaris]
Posted by "flyrain (via GitHub)" <gi...@apache.org>.
flyrain commented on code in PR #4996:
URL: https://github.com/apache/polaris/pull/4996#discussion_r3555553705
##########
polaris-core/src/main/java/org/apache/polaris/core/auth/AuthBootstrapUtil.java:
##########
Review Comment:
We may accidentally make this function not suitable for any callers other than boostrapping
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Make admin tool bootstrap idempotent for already-bootstrapped realms [polaris]
Posted by "flyrain (via GitHub)" <gi...@apache.org>.
flyrain commented on PR #4996:
URL: https://github.com/apache/polaris/pull/4996#issuecomment-4930785581
Can we wait until all comments are resolved?
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Make admin tool bootstrap idempotent for already-bootstrapped realms [polaris]
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b commented on PR #4996:
URL: https://github.com/apache/polaris/pull/4996#issuecomment-4930531902
@sepandr : Some of the commits mention Claude as a formal co-author. It is fine to use AI to make contributions, but a human has to assume authorship, I think.
Cf. https://github.com/apache/polaris/pull/4106#issuecomment-4192728124
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Make admin tool bootstrap idempotent for already-bootstrapped realms [polaris]
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b commented on PR #4996:
URL: https://github.com/apache/polaris/pull/4996#issuecomment-4930811008
> Can we wait until all comments are resolved?
@flyrain : I enabled auto-commit (as noted above) based on the previous approvals and the 2 day review period. I believe you could have disabled the auto-merge.
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Make admin tool bootstrap idempotent for already-bootstrapped realms [polaris]
Posted by "flyrain (via GitHub)" <gi...@apache.org>.
flyrain commented on code in PR #4996:
URL: https://github.com/apache/polaris/pull/4996#discussion_r3555553705
##########
polaris-core/src/main/java/org/apache/polaris/core/auth/AuthBootstrapUtil.java:
##########
Review Comment:
We may accidentally make this function not suitable for any callers other than bootstrapping. I understand bootstrapping is the only use case now, should we change the method name to make it explicitly only for bootstrapping?
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org