core: Credential Vending Refactor

[tokoko]

• Decouples credential vending from StorageCredentialCache by moving orchestration logic into StorageAccessConfigProvider
• Remove credential vending from MetaStoreManager: PolarisCredentialVendor interface, StorageCredentialsVendor, and getSubscopedCredsForEntity() implementations are removed from MetaStoreManager implementations. Credential vending no longer round-trips through the persistence layer.
• Moves caching into storage integrations: Each PolarisStorageIntegration subclass now owns its StorageCredentialCache interaction and builds cloud-specific cache keys.
• Simplify StorageAccessConfigProvider: Now directly resolves the storage integration and delegates to it, instead of going through StorageCredentialsVendor → PolarisCredentialVendor → MetaStoreManager → persistence layer → integration provider. Entity is no longer re-loaded from persistence during credential vending.

[dimas-b]

Thanks for keeping this refactoring going, @tokoko !

From my POV this PR is moving in the right direction. I've got just a couple of relatively minor comments.

[dimas-b]

The Supplier in this .get method in Caffeine is supposed to be fast, but I believe in our case it may hit network (e.g. STS), which is not desirable... It might be best to go back to LoadingCache, which allows loaders to be blocking.

[tokoko]

I can revert, but I have to say my limited (ai-assisted) research convinced me that there's no point in using a LoadingCache with a dummy loader if you're always calling a get with an explicit loader. The behavior should be the same with either. might be wrong, though. happy to trust you on this.

[dimas-b]

I agree, that the old code was using LoadingCache in a strange way... yet, I believe LoadingCache in itself is the right tool here 😅

[dimas-b]

This is related to using the "key" as input for Storage Config generation. If we could achieve that, LoadingCache would fit naturally.

[dimas-b]

I believe this comment thread still applies... WDYT?

[tokoko]

Don't you have a "chicken and egg" problem there? you need StorageIntegration for cache key first... Or do you mean we can create them twice effectively?

[dimas-b]

Sorry, I do not see the "chicken", only the "egg" 😉 which is the key. It is created first, then we go through the cache, then the loader function takes the key, creates a light-weight AwsCredentialsStorageIntegration and feeds the data from the key into it.

[tokoko]

I took a shot at it. basically added a load method to the cache key interface. each key carries it's own loader method and that's what loader method of the LoadingCache is calling. let me know what you think.

[dimas-b]

Sorry, not quite there yet from my POV 😅

So we derive storage config like this: entity -> PolarisStorageConfigurationInfo -> (1) PolarisStorageIntegration -> include CredentialVendingContext -> (cache) -> StorageAccessConfig

I believe we need to first combine PolarisStorageConfigurationInfo and CredentialVendingContext into a cache key. This requires (2) a concrete PolarisStorageIntegration (same as step 1 above). However, the key should only contain the data that acts as direct input into the config generation. So, it should not hold PolarisStorageIntegration. The cache loader function will re-derive (3) PolarisStorageIntegration from the cache key at load time (if it happens). This is a fast operation as far as I understand.

This is a fine point, I admit, but I think it is critical for caching correctness and indirectly for future improvement in this code.

[tokoko]

you're right, that was a poor implementation, I shouldn't have included integration itself as an aux. still I don't like the idea of re-deriving entire PolarisStorageIntegration object. the reason is that the first time it's instantiated, the point it to build a key, the second time around you build a credential. inputs for each are not necessarily identical, I changed session name derivation in AWS to highlight it. I switched to deriving session name during cache key generation and then simply using resolved string in the cache key directly. in other words, cache key generation needs entire CredentialVendingContext (which includes principal name), while credential compute needs only the session name string that's already resolved. Reinstantiating PolarisStorageIntegration with entire CredentialVendingContext is therefore wasteful.

I switched to providing fields like stsClientProvider and credentialResolver to cache key as aux fields instead of integration itself. the key then calls a static compute method on the integration which takes in only what it absolutely needs and params are coming from the fields in the cache key. let me know what you think.

[dimas-b]

Ideally, I think this should be something like loader.get(key). That is to ensure we produce StorageAccessConfig from exactly the same data that is used as the cache key, i.e. any change in key results in regeneration of StorageAccessConfig and any two StorageAccessConfig objects are logically equivalent if their keys match. WDYT?

[tokoko]

while I agree that it would be ideal, we probably still wouldn't be able to make a loader dependent 100% on the key, for example the key only contains catalogId, rather than a resolved polaris entity. In the current codebase, while key is available in the loader, it's only partially used. My thinking was that unless we make loader fully dependent on the key it makes little sense to make the code more confusing when some of the params depend on the key and others don't.

[dimas-b]

Re: confusing code - TBH, IMHO the current (on main) code is very confusing because the reader has to think very carefully about which parameters go through the cache and which go outside and whether they are consistent 😅

I think we're pretty close to "ideal" here. The "entity" this code deals with, has already been "found" and loaded, we only need to extract the storage configuration JSON from it in front of the cache.

PolarisStorageIntegration is loaded based on the storage config, which can be done past the cache.

In each call, the key will be used in full as required for a particular storage backend + feature flags. Different calls will have different data in their keys, of course, but that is the idea :) It will allow one cache per JVM to handle all use cases. We could even have different expiry strategies per storage type/config, but that's looking to far into the future 😅

Would renaming StorageCredentialCacheKey to StorageAccessConfigParameters make the code clearer?

[dimas-b]

ping :)

[dimas-b]

@tokoko : it looks like this PR has a lot of conflicts now... CI will not run because of that 😅

[tokoko]

@dimas-b that's fine, this one was more for you to take a look 😆 I'll take care of the conflicts and bunch of other stuff that's still unfinished

[github-actions]

This PR is stale because it has been open 30 days with no activity. Remove stale label or comment or this will be closed in 5 days.

[dimas-b]

@tokoko : The general approach in this PR is fine from my POV 👍 If you have time I think we can iterate.

[tokoko]

yeah, I'm gonna rebase and make it ready for a review this weekend.

[dimas-b]

I like the idea of using application-scoped beans for producing StorageAccessConfig in runtime.

Posting some preliminary comments below. I did not review every single line, but the general approach LGTM 👍

[flyrain]

Thanks @tokoko for working on it. Is there a dev mailing thread for it? If not, I'd recommend to have one for better visibility. It is generally a good practice to engage with the community earlier to avoid last minute surprises.

[dimas-b]

@flyrain : This PR was mentioned on the S3 Tables thread: https://lists.apache.org/thread/c5nc7144zlsb5vdjmpsgbc0h0g5no9c0

Do you think we need a separate discussion for this?

[tokoko]

@flyrain sure, I'll make a bit more changes, change PR description to be more accurate with latest changes and send out an email on the devlist. For me, the main goal of this PR is to demo the overall direction and build consensus on it. P.S. I'd be okay to split these changes up over a series of smaller PRs if people think that's more appropriate.

[tokoko]

@dennishuo @dimas-b I changed the PR and reinstated PolarisCredentialVendor SPI, but the thing that bothers me now is that PolarisCredentialVendor.getStorageAccessConfig and PolarisStorageIntegration.getSubscopedCreds signatures are converging to essentially the same thing — both take locations, actions, refresh endpoint, context and return StorageAccessConfig. I think the current design is coupling PolarisStorageIntegrationProvider (which effectively includes PolarisStorageIntegration) and PolarisCredentialVendor too much. Having two SPIs that mirror each other adds complexity without buying us anything.

The root cause is that PolarisCredentialVendor is trying to be a unified SPI that wraps both "get the right integration" and "vend credentials" into one call. So what if instead of putting PolarisStorageIntegrationProvider behind PolarisCredentialVendor, we make PolarisStorageIntegrationProvider a first-class citizen?

Planned flow will look like this:
Vending:

• PolarisStorageIntegrationProvider.getStorageIntegration(List resolvedEntityPath)

  • oss: resolves StorageConfig from full entity path and gets cached per-config PolarisStorageIntegration singleton from the cache.
  • custom: gets entity (with findInHierarchy) and calls IntegrationPersistence.loadPolarisStorageIntegration directly

• StorageIntegration.getSubscopedCreds(...) - since persistence question is out of the way and PolarisStorageIntegrationProvider SPI gives an implementer full control over the integration objects that are retrieved, I think we no longer need another PolarisCredentialVendor SPI for the next step. it would essentially be identical to PolarisStorageIntegration anyway.

CreateCatalog:

• createStorageIntegration / persistStorageIntegrationIfNeeded:
  • oss: we make both a no-op, createStorageIntegration no longer defers to PolarisStorageIntegrationProvider since PolarisStorageIntegrationProvider signature has changed and current impl discards the result anyway.
  • custom: this is unchanged, implements lease-commit steps here.

This change would give us one SPI (PolarisStorageIntegrationProvider) instead of two that mirror each other and also keep IntegrationPersistence storage methods around for custom impls that need them.

Curious what you guys think...

[dennishuo]

Sorry for the delay responding to this! I've been digesting this more and I think there's a bigger convergence we can align with.

I agree with what you said that if we only reintroduce PolarisCredentialVendor in the proposed way, it's a bit confusing whether a StorageIntegration itself is also fundamentally a CredentialVendor interface (with partially-bound request-scoped state). I'm mulling over whether what you describe is topologically equivalent to the other long-term direction we wanted to go, just with different class names.

The TL;DR: We should incrementally try to converge with how the analogous ConnectionConfig flow works for federation, especially since the sigv4-to-Glue federation credentials are pretty much exactly the same as sigv4-to-s3 storage credentials. Your proposal does seem to align somewhat, so I'd say go ahead and try to have StorageIntegration be the first-class entrypoint. We should consider accordingly moving the items related to request scope (realmConfig, storageConfig) out of the getSubscopedCreds arguments and force them to be bound at construction-time, if StorageIntegrationProvider.getStorageIntegration(resolvedEntityPath) is intended to be the stateful lookup.

Extended analysis:

Very abstractly, there are these four things:

1. "User-Requested Config" -- Basically the StorageConfingInfo that comes in from a CreateCatalog request initially
2. "Full config with system-assigned internal references" -- This is currently a confusing "hidden" ghost entity but only hinted at due to createStorageIntegration and persistStorageIntegrationIfNeeded and the fact that PolarisCredentialVendor takes the PolarisEntity when getting subscoped credentials. The StorageIntegration object itself kind of doubles as an effective "Data Access Object" (DAO) that wraps this "ghost entity"
3. "Handle into the entity-specific credential vending layer" - The StorageIntegration, by virtue of being the DAO into the hidden layer, basically fills this role. Importantly, the IntegrationPersistence.loadPolarisStorageIntegration method is implied to interact with persistence because it takes PolarisBaseEntity as an argument.
4. "Factory for the credential-vending handle, that requires full config with system-assigned state" - This is the IntegrationPersistence.loadPolarisStorageIntegration method basically

The fact that StorageIntegrationProvider at HEAD only takes what amounts to (1) - the user-requested config - and serves the purpose of (4) to provide (3) is what causes confusion.

What I remembered is that we actually already did try to redesign the flow of such credential-handling to accommodate this same concept in a more explicit way. See:

1. Secrets management for Catalog Federation: https://docs.google.com/document/d/1JPNx5vL4vM8DqwRwnBIPiQxwN4MXOdGx4Ki0j7vgwSM/edit?tab=t.0
2. The sigv4 support for that secrets management that is really the same flow as storage-credential vending, but for a different set of IAM actions: SigV4 Auth Support for Catalog Federation - Part 2: Connection Config Persistence #2190

Reposting @XJDKC 's diagram here:

In this world:

1. User-Requested config is the ConnectionConfigInfoModel
2. Config with System-assigned references is the ConnectionConfigInfoDpo that contains an "identityReference"
3. Handle into credential-vending - This is actually multi-step - AwsIamServiceIdentityCredential (concrete ServiceIdentityCredential class) is basically the handle into the service identity that can be used to further mint short-lived access, and SigV4ConnectionCredentialVendor (concrete ConnectionCredentialVendor class) is the thing that actually gets the short-lived credentials, as evidenced by it holding an StsClient. (Not explicitly depicted in the diagram, have to look at the code)
4. Factory that takes internal config/reference to get the handle - ServiceIdentityProvider (not exactly in the diagram since it's outdated, but roughly, the interaction between "ServiceIdentityRegistry" and "ResolvedServiceItentity" is captured in ServiceIdentityProvider)

I guess the rough equivalences are:

• ServiceIdentityProvider is kind of like StorageIntegrationProvider but it's also kind of like IntegrationPersistence since it also defines the allocate method
• ServiceIdentityCredential is like StorageIntegration
• ConnectionCredentialVendor is like a combination of StorageIntegration, PolarisCredentialVendor, StorageCredentialsVendor, and StorageAccessConfigProvider

One key difference in behavior is in the ConnectionConfig path, I guess we ended up putting the following all the way in PolarisAdminService:

    Optional<ServiceIdentityInfoDpo> serviceIdentityInfoDpoOptional =
        serviceIdentityProvider.allocateServiceIdentity(connectionConfigInfo);

    entity =
        new CatalogEntity.Builder(entity)
            .setConnectionConfigInfoDpoWithSecrets(
                connectionConfigInfo,
                processedSecretReferences,
                serviceIdentityInfoDpoOptional.orElse(null))
            .build();

This is in contrast to putting the "allocate" hook into the inner transaction-aware block in PolarisMetaStoreManager. So it doesn't support as good of transactional semantics as how we handle StorageConfig, but on the plus side, it forces the referential service-identity info to be first-class in the CatalogEntity itself, so we don't have fully invisible "ghost entities" representing the underlying credential allocation.

Overall, fully merging the Storage credential flow into the Connection credential flow is probably too big of an undertaking to do here, and maybe we also want to adjust the Connection credential code at the same time, but I think any incremental refactoring to make it structurally compatible is probably the right direction.

All this to say, yes, I think your proposal is somewhat structurally compatible, with a couple key points:

1. StorageIntegrationProvider changes from being semantically allocateForConfig(userSpecifiedConfig) to being fetchForEntity(entityPath)
2. StorageIntegration instances should be thought of as fully representing the resolved entityPath already, rather than being application/type-scoped -- this might mean we want realmConfig and storageConfig to be constructor-time bindings rather than arguments to getSubscopedCreds (where the storageConfig will actually be derived from entityPath during construction by StorageIntegrationProvider)

[tokoko]

@dennishuo thanks for the explanation. some sort of convergence between normal credential vending and federation makes sense. I'm not too familiar with federation codebase so I'll leave that out of scope for now, but I'll keep that in mind. that would probably make something like #4052 cleaner.

I went ahead and made quite a few changes. technically StorageIntegration instances returned by StorageIntegrationProvider now are storageConfig-scoped rather than entity-scoped which probably doesn't make much of a difference as of now. (other than the fact that storage locations need to be passed as parameters, which they might not have been otherwise, not sure about that though).

I'm thinking of trimming PolarisStorageIntegration itself next:

• @dimas-b I saw you had previously removed Remove unused PolarisCredentialVendor.validateAccessToLocations() #1480 from Vendor but left it on the integration layer, any reason for that? from what I saw it's completely unused and can be deleted from there as well. with that gone InMemoryStorageIntegration class also no longer makes sense to be kept around. The only active code there is a static method that can be refactored somewhere else.
• getStorageIdentifierOrId is another piece of unused dead code that I think can be removed.
• On the other hand, I'm thinking of splitting current PolarisStorageIntegration into a slimmer interface and renaming current code to CachingStorageIntegration. I think that should be better for persistence-based credentials that are unlikely to require caching (?). Also in general if we're making PolarisStorageIntegration a primary SPI, it should really be an interface.
• All in all, this is the interface I'm going for (also factoring in @dimas-b's suggestion of passing raw locations and actions rather than separate read/write locations):

public interface PolarisStorageIntegration {

    StorageAccessConfig getStorageAccessConfig(
        @Nonnull Set<String> locations,
        @Nonnull Set<PolarisStorageActions> actions,
        @Nonnull Optional<String> refreshEndpoint,
        @Nonnull CredentialVendingContext context);
  }

[dimas-b]

@dimas-b I saw you had previously removed #1480 from Vendor but left it on the integration layer, any reason for that? from what I saw it's completely unused and can be deleted from there as well. [...]

I did not touch PolarisStorageIntegration.validateAccessToLocations() because it is considered part of the historical "core" and I am now aware of the full implications of such a change.

From my POV that method can be deleted, but I'd appreciate it if @dennishuo could share his view on that.

[dimas-b]

The current state of this PR looks reasonable to me. If @dennishuo is ok with the general direction, I'll take a deeper look 😉

[dennishuo]

Sorry for the delays, took a some deep diving archaeology to assess the impact of all the suggested changes downstream, though I only just realized earlier this evening that you were just saying you'd remove getStorageIdentifierOrId in a future PR, not this one. I've also always disliked that method, and confirmed there will be some downstream refactoring required in custom code if that's removed, but it should be feasible to do (the cleaner way to achieve the more general functionality there is to add a method that abstracts out injecting "book-keeping" info into a PolarisEntity based on the StorageIntegration, like mentioned in the Catalog ConnectionConfig stuff, but that's for a longer-term refactor).

For the current PR:

Removing loadPolarisStorageIntegration is feasible, but since it is indeed an SPI which is intended to be called by encouraged customizations (e.g. custom MetaStoreManager classes calling ((IntegrationPersistence) ms).loadPolarisStorageIntegration), it would be good to exercise the intended SPI-evolution process, which is to put it to a vote on the dev mailing list in case others want to assess impact as well.

For the suggested changes:

Removing getStorageIdentiferOrId - workable for the customization cases I know of, but since this is also used by customizations of MetaStoreManager, if we're going to have a dev mailing list vote for loadPolarisStorageIntegration anyways, perhaps we can also include this method's removal in that same vote thread (so that we don't have to have multiple threads in case it's most likely anyone impacted would feel equally about both methods)

Removing validateAccessToLocations - this one seems fairly clean to remove, since the TODO about plumbing the check through to a persistence impl instead of conjuring an InMemoryStorageIntegration never materialized. Personally I could go either way on whether to include this one in the vote. I suppose one could argue that since the intended caller of the validateAccessToLocations wouldn't have been from polaris-core but rather polaris-service (i.e. BasePolarisCatalog) that validateAccessToLocations isn't part of the SPIs that customizations against polaris-core would rely on. It's a bit of a blurry line here.

The main deciding factor would be starting from the core SPIs called out in https://lists.apache.org/thread/0nj24zro7kyctqfnlml08ppo7zs9xcqs and going one level deeper into their arguments and if the interaction between those core classes and the argument (including a PolarisStorageIntegration) is part of the core SPI then we should have it in the vote.

Regarding removal of read vs write and just having one locations list - I'm not sure I understand this proposal. Subscoped policy strings give different sets of allowed locations for read vs write locations. @dimas-b what was the suggestion about no longer differentiating between read vs write? I must have missed this

[tokoko]

@dennishuo thanks for the detailed response. If we're going in the ML vote direction, we might as well bundle all SPI changes in a single vote. we can then either remove everything that's no longer needed or I can simply deprecate them as part of this PR (storage id for example) and push removal for a follow up if you think that'll make adjusting to the changes easier for you. let me know.

to summarize:

• we are dropping PolarisCredentialVendor (and StorageCredentialsVendor) SPI entirely.
• PolarisStorageIntegrationProvider is made into a top-level SPI with a single method: PolarisStorageIntegration getStorageIntegration(List<PolarisEntity> resolvedEntityPath);
• we are dropping loadPolarisStorageIntegration from IntegrationPersistence because it's not called from oss code.
• we are dropping everything from PolarisStorageIntegration except for StorageAccessConfig getStorageAccessConfig(...) method.

The only thing left is to agree on getStorageAccessConfig signature. let me know if I missed anything.

I'm not sure I understand this proposal. Subscoped policy strings give different sets of allowed locations for read vs write locations

let me try to make the case for changing current params from:

boolean allowListOperation,
@Nonnull Set<String> allowedReadLocations,
@Nonnull Set<String> allowedWriteLocations

to

@Nonnull Set<String> locations,
@Nonnull Set<PolarisStorageActions> actions,

the motivation for the change stems from two observations:

• we never set allowedReadLocations and allowedWriteLocations to different sets of strings. the two lists are always identical in practice. the fact that storage policies (like iam policy strings) expect them to be supplied in some particular way is an implementation detail.
• credential vending flow only allows read/write locations and a list boolean to be set (which is extremely ugly as well imho 😆) when PolarisStorageActions is actually more expressive, it also contains a delete action. One hypothetical use case that would benefit from passing PolarisStorageActions.DELETE down to the storage integration layer is more fine-grained control and distinction between write/delete actions which are currently impossible to distinguish. For example a normal iceberg writer principal would get only PutObject privilege, while one that also can remove orphans files or expire snapshots would get DeleteObject privilege as well. (PutObject can also be destructive unless we mandate conditional writes like this but that's different story and would probably require upstream iceberg change that would allow setting s3.write.if-none-match=true in S3FileIO to signal clients to use conditional writes) Anyway, The point is that the current design discards important details (like DELETE action) and duplicates storage locations that are (at least currently) always identical.
• If we want to be even more future-proof we could introduce something like this that would allow setting different storage actions for different sets of locations, but I couldn't really come up with a use case:

public record LocationGrant(
        @Nonnull Set<String> locations, 
        @Nonnull Set<PolarisStorageActions> actions
)

public interface PolarisStorageIntegration {
    StorageAccessConfig getStorageAccessConfig(
        @Nonnull List<LocationGrant> grants,
        @Nonnull Optional<String> refreshEndpoint,
        @Nonnull CredentialVendingContext context);
}

I get that this will introduce even more changes and headache for downstream implementations. It's just that if we're breaking things anyway and trying to build consensus around new SPI design, it's unlikely we will have better opportunity in the future for these sorts of changes.

[dennishuo]

@tokoko +1 to bundling up SPI changes for a vote.

For the locations, I'm okay with refactoring a more advanced way to set different storage actions for different sets of locations. Keeping the muscle under the hood for policy-scoping logic to handle different sets of actions for different locations will be important to avoid that door inadvertently closing with other architectural decisions that start assuming a uniform action-set, even if we failed to properly capitalize on it so far.

The original use case in mind was in the case of cloned tables or other tables that originate from some legacy dataset in an "old" location that is no longer the new write-location, it's important to keep the legacy location "read-only" and only provide write access to the new location. It's a layer of protection against misbehaving client-side engines that may unexpectedly delete things from the legacy dataset location, for example.

In general with the future of better intent-conveyance in the APIs (e.g. loadTable for a subsequent write vs just for a SELECT), we'd also want to better distinguish whether to include write permissions at all. Some finer-grained operations, such as things that only want to compact a snapshot might only need write access to the metadata/ directory but not the data/ directory.

With better specification of intent in the API spec, there's some discussion of providing finer-grained privileges under TABLE_WRITE_PROPERTIES that distinguish between data-writes vs metadata-writes, and even within metadata writes, distinguishing between some sensitive ones like setLocation from more innocuous ones like snapshot tags.

In some cases you might even want to allow DELETE but not READ on a subdirectory. For example, you might run a shared snapshot-expiry maintenance engine on a highly sensitive dataset. It would need read + write on metadata/ files but you might even only want to give it DELETE on data/.

[dimas-b]

Re: voting for SPI changes - I think it would be fine, however, I'd suggest to have concrete changes in a PR first, then review the PR, then vote. This is because voting is supposed to be on specific changes. If some adjustments are called for during a vote, the whole process has to restart, so we'd better sort out all concerns before the vote even starts 😅

That said, from my personal POV posting a notice about changes on the dev ML and approving in GH is just as good.

[dimas-b]

LGTM overall 👍 Please fix conflicts 🙂

[dimas-b]

ping :)