Add a command to explicitly purge the metastore #59

eric-maynard · 2024-08-01T17:17:27Z

Description

This adds a new command, purge, that must be run in order to purge entities from the metastore.

Additionally, bootstrapping will now fail if entities exist in the metastore instead of purging them.

Type of change

Please delete options that are not relevant.

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to not work as expected)
This change requires a documentation update

How Has This Been Tested?

Previous behavior:

java -jar .app/polaris-service-1.0.0-all.jar bootstrap polaris-server.yml
. . .
(Succeeds)

New behavior:

java -jar .app/polaris-service-1.0.0-all.jar bootstrap polaris-server.yml
. . .
It appears this metastore manager has already been bootstrapped...

New behavior + purge:

java -jar .app/polaris-service-1.0.0-all.jar purge polaris-server.yml
. . .
(Succeeds)
. . .
java -jar .app/polaris-service-1.0.0-all.jar bootstrap polaris-server.yml
(Succeeds)

Checklist:

I have performed a self-review of my code
I have commented my code, particularly in hard-to-understand areas
My changes generate no new warnings
New and existing unit tests pass locally with my changes

snazy

I'm on the fence with some --overwrite flag that purges (some) state.

bootstrap is a one-off operation that, I think, should fail when the system's already bootstrapped.

I think, it's better to have an explicit command or tool to purge / reset to avoid accidentally resetting principals. WDYT?

eric-maynard · 2024-08-01T17:39:49Z

Thanks for taking a look @snazy -- to be clear, the current behavior is that bootstrap will silently purge the state. This PR proposes a flag you have to set in order to confirm that you really want to do this. And without that flag bootstrap will fail when the system is already bootstrapped.

snazy · 2024-08-01T17:41:57Z

Oh - right - that "bark at you" code part is new. Can we add the "barking" part and have a "purge root-principals" command or so?

snazy · 2024-08-01T17:43:10Z

I feel that's a bit safer - humans tend to be lazy (and forget that they have such --overwrite flags set) and wonder why things break.

RussellSpitzer · 2024-08-05T06:52:17Z

polaris-core/src/main/java/io/polaris/core/persistence/LocalPolarisMetaStoreManagerFactory.java

    // While bootstrapping we need to act as a fake privileged context since the real
    // CallContext hasn't even been resolved yet.
    PolarisCallContext polarisContext =
        new PolarisCallContext(
            sessionSupplierMap.get(realmContext.getRealmIdentifier()).get(), diagServices);
    CallContext.setCurrentContext(CallContext.of(realmContext, polarisContext));

+    if (!overwrite) {
+      PolarisMetaStoreManager.EntityResult preliminaryRootPrincipalLookup =
+          metaStoreManager.readEntityByName(


Will this be version safe in the future? For now I assume this is safe but in the future could readEntityByName fail for a Polaris catalog bootstrapped from an earlier version?

This seems like a good check to have for now, I'm just more worried about what happens if this call fails because the current version of Polaris just doesn't understand how to look up the Principal in the past.

This is an important concern and it's why we don't rely on a check like this to auto-bootstrap... but it seems to me that this check is much less critical. That scenario would be very bad for other reasons, but here you would only be losing the protection of the --overwrite flag.

polaris-core/src/main/java/io/polaris/core/persistence/MetaStoreManagerFactory.java

RussellSpitzer · 2024-08-05T07:27:49Z

@snazy I think we probably can't safely always validate (or require that an implementation can validate) that existing information is in the destination. I like this though because it is at least safer than the current behavior.

snazy · 2024-08-05T09:51:24Z

I think we probably can't safely always validate (or require that an implementation can validate) that existing information is in the destination. I like this though because it is at least safer than the current behavior.

Sure, having this check is definitely a good thing. What I'm thinking of is:

not having an --overwrite flag, but let bootstrap always fail with that check but
have a new command to explicitly purge the principal

Thinking of humans leaving the --overwrite in for all deployments - (and complain that authn no longer works after a restart).

eric-maynard · 2024-08-05T17:01:51Z

I don't think we want to break bootstrap into purge + bootstrap. Not all metastore managers will necessarily purge on bootstrap. We want to just gate bootstrapping behind an additional flag on a best-effort basis in the case that we detect bootstrapping may have already been performed.

--overwrite doesn't appear in the docs and only appears in this error message, so for now it's hard to imagine someone always leaving --overwrite on their bootstrap command without knowing what it does. And if we can imagine such a state, it's not such a stretch to also imagine that they leave the purge command in there too.

collado-mike · 2024-08-05T20:17:13Z

IDK, I like the option to purge because I feel it is very explicit and we should expect the implementation to be thorough. bootstrap is necessarily an unauthenticated command, whereas the purge operation ought to require root credentials (there's a question of how does someone bootstrap if they lost their root credentials, at which point I think they should require direct database access).

If bootstrap --overwrite merely focuses on replacing the root principal/secrets, there's a risk that someone could bootstrap an existing installation but not delete the pre-existing data. In my mind, purge means delete all the things, so purge then bootstrap should result in the expected behavior.

…bootstrap

eric-maynard · 2024-08-06T01:56:52Z

Thanks for taking a look @collado-mike & @snazy -- I have implemented a purge command based on my understanding of the discussion above. Please take a look at let me know if this matches what you had in mind. Thanks!

snazy

One nit, but overall LGTM.
Deferring review to @collado-mike, because this related code base a bit better ;)

polaris-server.yml

…bootstrap

polaris-core/src/main/java/io/polaris/core/persistence/MetaStoreManagerFactory.java

polaris-core/src/main/java/io/polaris/core/persistence/PolarisMetaStoreManager.java

polaris-core/src/main/java/io/polaris/core/persistence/PolarisMetaStoreManagerImpl.java

polaris-service/src/main/java/io/polaris/service/BootstrapRealmsCommand.java

polaris-service/src/main/java/io/polaris/service/PurgeRealmsCommand.java

polaris-service/src/main/java/io/polaris/service/BootstrapRealmsCommand.java

MonkeyCanCode · 2024-08-17T19:43:59Z

@flyrain can we have this merge if not other blockers so the bootstrap won't wipe the backend if accidently got ran more than once. Then I can test if anything additional will be needed on the helm side as well to handle those.

flyrain

LGTM with minor comments.

flyrain · 2024-08-17T22:17:04Z

polaris-service/src/main/java/io/polaris/service/BootstrapRealmsCommand.java

+    Map<String, PolarisMetaStoreManager.PrincipalSecretsResult> results =
+        metaStoreManagerFactory.bootstrapRealms(configuration.getDefaultRealms());


Nit: we can use var to simplify a bit.

flyrain · 2024-08-17T22:17:33Z

polaris-service/src/main/java/io/polaris/service/BootstrapRealmsCommand.java

+    for (Map.Entry<String, PolarisMetaStoreManager.PrincipalSecretsResult> result :
+        results.entrySet()) {


Nit: We can use var to simplify and keep them in one line

eric-maynard · 2024-08-17T22:24:02Z

I haven’t seen any use of `var` outside of tests in this project; might there be compatibility concerns? Is this merge blocking?

…

On Sat, Aug 17, 2024 at 6:20 PM Yufei Gu ***@***.***> wrote: ***@***.**** approved this pull request. LGTM with minor comments. ------------------------------ In polaris-service/src/main/java/io/polaris/service/BootstrapRealmsCommand.java <#59 (comment)> : > + Map<String, PolarisMetaStoreManager.PrincipalSecretsResult> results = + metaStoreManagerFactory.bootstrapRealms(configuration.getDefaultRealms()); Nit: we can use var to simplify a bit. ------------------------------ In polaris-service/src/main/java/io/polaris/service/BootstrapRealmsCommand.java <#59 (comment)> : > + for (Map.Entry<String, PolarisMetaStoreManager.PrincipalSecretsResult> result : + results.entrySet()) { Nit: We can use var to simplify and keep them in one line — Reply to this email directly, view it on GitHub <#59 (review)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AFRE3SGK6EGIGJAZYNFAK6DZR7EDDAVCNFSM6AAAAABL27URICVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDENBUGE2TOOJYGI> . You are receiving this because you authored the thread.Message ID: ***@***.***>

flyrain · 2024-08-17T22:27:13Z

I think it's fine to use var as Polaris requires jdk 11 for core and jdk21 for others. var was introduced in jdk 10. To be clear, they are not blockers. I will merge it tonight if no more comments.

flyrain · 2024-08-18T04:06:12Z

Thanks @eric-maynard for the change. Thanks @collado-mike @snazy @RussellSpitzer @MonkeyCanCode for the review.

initial commit

ea89eaf

eric-maynard requested a review from a team as a code owner August 1, 2024 17:17

snazy reviewed Aug 1, 2024

View reviewed changes

eric-maynard mentioned this pull request Aug 1, 2024

Implement missing doc "Configuring Polaris for Production" #43

Merged

2 tasks

style

7bb0f01

RussellSpitzer reviewed Aug 5, 2024

View reviewed changes

polaris-core/src/main/java/io/polaris/core/persistence/MetaStoreManagerFactory.java Show resolved Hide resolved

preserve old interface

897dc5b

RussellSpitzer previously approved these changes Aug 5, 2024

View reviewed changes

eric-maynard dismissed RussellSpitzer’s stale review via 94d4232 August 5, 2024 07:31

lint

94d4232

eric-maynard changed the title ~~Add a flag to verify existing entities should be purged by bootstrapping~~ Add a command to explicitly purge the metastore Aug 6, 2024

eric-maynard added 4 commits August 5, 2024 18:09

start implementing purge command

02caf5b

check in

8b5ae70

Merge branch 'main' of github.com:polaris-catalog/polaris into force-…

d04c2c8

…bootstrap

check in

4cb0079

eric-maynard requested a review from snazy August 6, 2024 01:57

edits

7c27efe

snazy reviewed Aug 6, 2024

View reviewed changes

polaris-server.yml Outdated Show resolved Hide resolved

eric-maynard added 2 commits August 6, 2024 14:51

revert a file

0837987

Merge branch 'main' of github.com:polaris-catalog/polaris into force-…

16c9f1d

…bootstrap

collado-mike requested changes Aug 7, 2024

View reviewed changes

eric-maynard added 2 commits August 7, 2024 16:40

changes per review

e66fcd3

lint

9b89b1c

collado-mike previously approved these changes Aug 8, 2024

View reviewed changes

polaris-service/src/main/java/io/polaris/service/BootstrapRealmsCommand.java Outdated Show resolved Hide resolved

eric-maynard dismissed collado-mike’s stale review via ef8fbe7 August 9, 2024 17:42

warn -> error

ef8fbe7

eric-maynard requested review from collado-mike and RussellSpitzer August 9, 2024 21:05

collado-mike enabled auto-merge (squash) August 9, 2024 22:39

fix test

5829aa6

auto-merge was automatically disabled August 13, 2024 20:54
Head branch was pushed to by a user without write access

flyrain approved these changes Aug 17, 2024

View reviewed changes

flyrain merged commit d6278e2 into apache:main Aug 18, 2024
3 checks passed

flyrain mentioned this pull request Aug 18, 2024

Fix the compilation failure #157

Merged

2 tasks

MonkeyCanCode mentioned this pull request Aug 18, 2024

Add helm support #135

Merged

13 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add a command to explicitly purge the metastore #59

Add a command to explicitly purge the metastore #59

eric-maynard commented Aug 1, 2024 •

edited

Loading

snazy left a comment

eric-maynard commented Aug 1, 2024

snazy commented Aug 1, 2024

snazy commented Aug 1, 2024

RussellSpitzer Aug 5, 2024

eric-maynard Aug 5, 2024

RussellSpitzer commented Aug 5, 2024

snazy commented Aug 5, 2024

eric-maynard commented Aug 5, 2024

collado-mike commented Aug 5, 2024 •

edited

Loading

eric-maynard commented Aug 6, 2024

snazy left a comment

MonkeyCanCode commented Aug 17, 2024

flyrain left a comment

flyrain Aug 17, 2024

flyrain Aug 17, 2024

eric-maynard commented Aug 17, 2024 via email

flyrain commented Aug 17, 2024 •

edited

Loading

flyrain commented Aug 18, 2024

		Map<String, PolarisMetaStoreManager.PrincipalSecretsResult> results =
		metaStoreManagerFactory.bootstrapRealms(configuration.getDefaultRealms());

		for (Map.Entry<String, PolarisMetaStoreManager.PrincipalSecretsResult> result :
		results.entrySet()) {

Add a command to explicitly purge the metastore #59

Add a command to explicitly purge the metastore #59

Conversation

eric-maynard commented Aug 1, 2024 • edited Loading

Description

Type of change

How Has This Been Tested?

Checklist:

snazy left a comment

Choose a reason for hiding this comment

eric-maynard commented Aug 1, 2024

snazy commented Aug 1, 2024

snazy commented Aug 1, 2024

RussellSpitzer Aug 5, 2024

Choose a reason for hiding this comment

eric-maynard Aug 5, 2024

Choose a reason for hiding this comment

RussellSpitzer commented Aug 5, 2024

snazy commented Aug 5, 2024

eric-maynard commented Aug 5, 2024

collado-mike commented Aug 5, 2024 • edited Loading

eric-maynard commented Aug 6, 2024

snazy left a comment

Choose a reason for hiding this comment

MonkeyCanCode commented Aug 17, 2024

flyrain left a comment

Choose a reason for hiding this comment

flyrain Aug 17, 2024

Choose a reason for hiding this comment

flyrain Aug 17, 2024

Choose a reason for hiding this comment

eric-maynard commented Aug 17, 2024 via email

flyrain commented Aug 17, 2024 • edited Loading

flyrain commented Aug 18, 2024

eric-maynard commented Aug 1, 2024 •

edited

Loading

collado-mike commented Aug 5, 2024 •

edited

Loading

flyrain commented Aug 17, 2024 •

edited

Loading