Support IMPLICIT authentication type for federated catalogs

[poojanilangekar]

Previously, the ConnectionConfigInfo required explicit AuthenticationParameters for every federated catalog. However, certain catalogs types that Polaris federates to (either now or in the future) allow IMPLICIT authentication, wherein the authentication parameters are picked from the environment or configuration files. This change enables federating to such catalogs without passing dummy secrets.

The IMPLICIT option is guarded by the SUPPORTED_EXTERNAL_CATALOG_AUTHENTICATION_TYPES. Hence users may create federated catalogs with IMPLICIT authentication only when the administrator explicitly enables this feature.

[poojanilangekar]

CC @eric-maynard @dennishuo

[dimas-b]

How is it different from NULL_TYPE?

[eric-maynard]

With some connection types like HIVE, you may either have or not have an authentication mechanism in place. It's reasonable that admins might want to limit which authentication types can be used for their service (e.g. disallow auth-less federation), but they can't disallow having a null authentication type because that is also used by "classic" EXTERNAL catalogs that don't federate at all.

In other words, if you have a non-null connection, you should have a non-null authentication type configured. We should still leave room however for users to be explicit about certain federation modes like HIVE that may not have authentication in use.

[dimas-b]

I agree that we need to distinguish defined auth from undefined auth (i.e. null). However, a null value is not the same as a NULL_TYPE value. This is probably a concern about the existing codebase rather than this particular PR 🤔 but I did not find any usages of NULL_TYPE in the current codebase.

So, I wonder whether introducing the NO_AUTH enum value is really necessary (as opposed to renaming NULL_TYPE). WDYT?

[poojanilangekar]

From my understanding AuthenticationType (and ConnectionType) should keep the NULL_TYPE to indicate an unsupported/missing value. Specifically to handle the case pointed out in the discussion that follows. If a newer version of AuthenticationType is used to create a federated catalog and the type is persisted in the ConnectionConfigInfoDPO. Later, an older client tries to read this DPO, it will return NULL_TYPE; which is an appropriate response considering it read an unknown/missing type from the DPO.

[dimas-b]

I question the usefulness of NULL_TYPE to represent an "unsupported" value. ATM, I do not see any existing usage of NULL_TYPE in Polaris code.

As for REST API clients, they do not use this enum at all, as far as I understand. This enum appears to used by Polaris server code only internally.

[poojanilangekar]

While NULL_TYPE is not explicitly used in the codebase, it is implied (and therefore used) in the default case of switch statements. I can go over a more explicit case for ConnectionType here:

1. Assume that sometime in the future, Polaris adds support for HIVE connections.
2. The user then creates an external catalog called hive-catalog of type HIVE. At which point, Polaris will store the corresponding ConnectionConfigInfoDpo into the underlying persistence layer. While creating the persistence object, the constructor similar to IcebergRestConnectionConfigInfoDpo, called HiveConnectionConfigInfoDpo will create this object with enums defined internally by the Polaris server. It'd make a call of the form ConnectionType.HIVE.getCode().
3. Further, let's assume that one of the two happens (1) we rollback the hive change the newer versions of Polaris don't support Hive connections, or (2) the Polaris instance is rebooted with an older version. In both cases, the Polaris instance won't recognize the HIVE connection type.
4. Lastly, let's say the user wants to perform some operation on the hive-catalog. The IcebergCatalogHandler::initializeCatalog() would now read the persisted object and parse it's information, specifically, let's look at this snippet:

          ConnectionType.fromCode(connectionConfigInfoDpo.getConnectionTypeCode());     
        switch (connectionType) {
        case ICEBERG_REST:
          SessionCatalog.SessionContext context = SessionCatalog.SessionContext.createEmpty();
          federatedCatalog =
              new RESTCatalog(
                  context,
                  (config) ->
                      HTTPClient.builder(config)
                          .uri(config.get(org.apache.iceberg.CatalogProperties.URI))
                          .build());
          federatedCatalog.initialize(
              ((IcebergRestConnectionConfigInfoDpo) connectionConfigInfoDpo).getRemoteCatalogName(),
              connectionConfigInfoDpo.asIcebergCatalogProperties(getUserSecretsManager()));
          break;
        case HADOOP:
          federatedCatalog = new HadoopCatalog();
          federatedCatalog.initialize(
              ((HadoopConnectionConfigInfoDpo) connectionConfigInfoDpo).getWarehouse(),
              connectionConfigInfoDpo.asIcebergCatalogProperties(getUserSecretsManager()));
          break;
        default:
          throw new UnsupportedOperationException(
              "Connection type not supported: " + connectionType);
      }

Since HIVE is not a valid connection type in this Polaris instance, the ConnectionType::fromCode() function will return NULL_TYPE and hence activate the default case.

This was my understanding of why we need a NULL_TYPE. Essentially, since we always want to support not just backward compatibility but also forward compatibility wherein the data store might contain some objects/types that the current Polaris instance does not recognize. The NULL_TYPE provide a nice catch-all type to allow appropriate error handling.

[snazy]

Beside that this is a breaking change, any user facing API change requires a discussion on the dev mailing list.

[dimas-b]

+1 to dev list discussion

[eric-maynard]

This is not a breaking change btw

[snazy]

It is an enum in older clients - and Java enum valueOf throws and does not gracefully handle unknown values. Hence it is a breaking change.

[eric-maynard]

In the semver sense, I'm not sure that needs to qualify as a breaking change -- at least it hasn't been called out as such when we've made similar changes in the past. The REST API is not breaking, even if some clients implemented in a fragile way might. Our Python client seems to fail quite gracefully. See #1506 and the associated doc here for a similar change made recently.

Separately, if you think it's a good idea to harden our Java client against this kind of enum change, I think that would be a great contribution.

[snazy]

Separately, if you think it's a good idea to harden our Java client against this kind of enum change, I think that would be a great contribution.

I guess that it would be a hard fight to remove the code generator, mind proposing that change to get that implemented?

[eric-maynard]

In fact, is the Java client already set up to treat this as NULL_TYPE?

if (authTypeCode < 0 || authTypeCode >= REVERSE_MAPPING_ARRAY.length) {
  return NULL_TYPE;
}

[snazy]

And the other way around?

[eric-maynard]

Sorry, what do you mean by the other way around? Does this relate to the doc linked above?

edit: Oh, are you referring to what happens if a client tries to provide this new enum value to an older version of the service? How is that different from a client calling an API that doesn't exist on the service? I don't think that typically qualifies as a breaking change under semver

Regardless of whether or not it is a breaking change however, we do need to discuss the spec change on the ML just like the linked PR

[dimas-b]

@snazy : the API-level enum is auto-generated from this YAML spec: AuthenticationTypeEnum.
Decoding code:

polaris/polaris-core/src/main/java/org/apache/polaris/core/connection/AuthenticationParametersDpo.java

Line 66 in 24683d8

switch (authenticationParameters.getAuthenticationType()) {

Encoding:

polaris/polaris-core/src/main/java/org/apache/polaris/core/connection/OAuthClientCredentialsParametersDpo.java

Line 120 in cc7ffb6

.setAuthenticationType(AuthenticationParameters.AuthenticationTypeEnum.OAUTH)

[poojanilangekar]

Thanks everyone for looking into this PR, I have started a devlist discussion as suggested.

[poojanilangekar]

Hi everyone,

@dennishuo suggested that instead of calling the this AuthenticationType::NONE, we can call it AuthenticationType::ENVIRONMENT because really the motivation behind adding NONE was to enable federation without explicitly passing OAUTH/BEARER tokens. Essentially, this authentication type tells Polaris that when federating to this external catalog don't expect any tokens, instead use whatever information you find in the environment/configuration files. I prefer this over NONE because it better represents what is happening under the hood.

@eric-maynard @dimas-b What do you think?

[dimas-b]

I like the idea, but I think term ENVIRONMENT is not well-defined. What is implied here looks like it's PROVIDER... Meaning whoever makes a catalog with this auth type (user) relies on the Polaris administrator (service provider) to perform some per-arranged kind of auth flow... Did get this right?

[poojanilangekar]

relies on the Polaris administrator (service provider) to perform some per-arranged kind of auth flow

This part isn't accurate. The Polaris service does not perform any pre-arranged authentication. It relies on the underlying connection library to carry out the authentication flow based on the environment or configuration file. Really what we want to convey here is that:

1. Polaris does not store or manage the authentication parameters.
2. It does not apply any authentication flow explicitly.

To elaborate on 2: If the config file provides no authentication parameters, then it will use an unauthenticated mechanism, on the other hand, let's say the config file contains auth method Kerberos and provides a path to a key-tab file, then the connection library will connect to the kerberized hadoop instance. In both cases Polaris is oblivious to what happens under the hood.

[dimas-b]

environment is specific to how the service provider runs Polaris. Any credentials and auth flows based on that are what I termed "pre-arranged" because the Polaris user does not control them.

From the user's perspective the term ENVIRONMENT is not well defiled because it is not clear whether it is the user's env. or some other env. and the user does not have visibility into how the service provider runs Polaris.

[poojanilangekar]

I agree that the user does not control the ENVIRONMENT on the polaris instance. Would you be okay with the term DEFAULT_CONFIG? I am also open to suggestions. We could also do PROVIDER but I don't think it's immediately apparent in this context. However, if you think that's the correct term, I can augment the API description and use it.

[dimas-b]

@poojanilangekar : could you post the enum name options on the dev ML thread for visibility? This is an API change, so extra care is necessary.

[poojanilangekar]

Hi everyone, just a gentle reminder: can you please respond to the mailing list with your naming preference?

[poojanilangekar]

I've updated the PR by renaming the AuthenticationType to IMPLICIT.

[dimas-b]

Why is this change required?

[poojanilangekar]

Polaris assumes any catalog that is not defined as "INTERNAL" is by default "EXTERNAL", it ends up activating the code-path in PolarisAdminService.java:733

Granted that since we allow legacy EXTERNAL catalogs without ConnectionConfigInfos, the test would still pass without the change. But I think that wasn't the intention of the test.

For this test and other tests in catalog/, I think it's ideal to not activate that code path in the first place. If you want me to I can revert this and other changes. Please let me know what you think.

[dimas-b]

I'm not so much worried about this test code change, but about defaulting to EXTERNAL. Existing test code appears to assume that the default is/was INTERNAL (which makes sense to me).

@dennishuo @eric-maynard : WDYT?

[poojanilangekar]

I think the fix is simple, I can create a separate PR to fix it.

polaris/polaris-core/src/main/java/org/apache/polaris/core/entity/CatalogEntity.java

Line 109 in 9cb71f8

return catalogType == Catalog.TypeEnum.INTERNAL

The builder checks if the catalogType is INTERNAL, if not, it assumes that the type is EXTERNAL, we can flip the condition to check if the type is set to EXTEERNAL. I also think that the change only affects tests, because in other cases, it's generated from polaris-management-service.yml which defaults to INTERNAL.

[dimas-b]

Opening a PR for that would be great. Thx!

Let's review the impact of that change on that specific PR (I hope it's ok).

[poojanilangekar]

Done.

[poojanilangekar]

Since I opened #1998, I reverted this and other similar changes. The build still passes but I think we should try to get 1998 merged before this change.

[dimas-b]

Thx! Let's merge #1998 first and rebase.

[poojanilangekar]

Done.

[dimas-b]

LGTM overall with one more minor comment.

[dimas-b]

Please add a brief note to the New Features section in CHANGELOG.md about this.

[poojanilangekar]

Done.