-
Notifications
You must be signed in to change notification settings - Fork 290
Integration tests for Catalog Federation #2344
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, @poojanilangekar ! The PR LGTM overall, but I have a concern with the IT test location (as commented).
...tests/src/main/java/org/apache/polaris/service/it/test/CatalogFederationIntegrationTest.java
Outdated
Show resolved
Hide resolved
runtime/service/src/intTest/java/org/apache/polaris/service/it/CatalogFederationIT.java
Outdated
Show resolved
Hide resolved
...tests/src/main/java/org/apache/polaris/service/it/test/CatalogFederationIntegrationTest.java
Outdated
Show resolved
Hide resolved
...tests/src/main/java/org/apache/polaris/service/it/test/CatalogFederationIntegrationTest.java
Outdated
Show resolved
Hide resolved
public Map<String, String> getConfigOverrides() { | ||
return Map.of( | ||
"polaris.features.\"ENABLE_CATALOG_FEDERATION\"", "true", | ||
"polaris.features.\"ALLOW_OVERLAPPING_CATALOG_URLS\"", "true"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it be possible to use 2 different base locations + 2 different storage configs for the two catalogs, and thus avoid setting this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need to use the same base location because we are using a single Polaris instance. So unless we have two different Polaris instances (which we can't do in the current integration test setup), we need this configuration.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@poojanilangekar : Do you mean that federated catalogs check for location overlaps with internal catalogs?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, that is correct.
Unless the ALLOW_OVERLAPPING_CATALOG_URLS
flag is set, Polaris always checks if the base location of a CreateCatalog (irrespective of whether its INTERNAL vs EXTERNAL) request overlaps with an existing catalog. Therefore we need this flag to be set, at least for the JUnit tests.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(This has come up before, but I think the fix is that federating EXTERNAL catalogs shouldn't need or support a location)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not at the moment. Making this change would require changing the API spec and making subsequent changes in the CreateCatalog validations.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would you be able to open an issue for sorting this out later?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done: #2356
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thx 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This PR is good to merge from my POV 👍
public static class CatalogFederationProfile implements QuarkusTestProfile { | ||
@Override | ||
public Map<String, String> getConfigOverrides() { | ||
return Map.of( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It might be good to explicitly set SUPPORTED_CATALOG_CONNECTION_TYPES
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
newUserCredentials = managementApi.createPrincipal(PRINCIPAL_NAME); | ||
|
||
FileStorageConfigInfo storageConfig = | ||
FileStorageConfigInfo.builder() | ||
.setStorageType(StorageConfigInfo.StorageTypeEnum.FILE) | ||
.setAllowedLocations(List.of(baseLocation.toString())) | ||
.build(); | ||
|
||
CatalogProperties catalogProperties = new CatalogProperties(baseLocation.toString()); | ||
|
||
Catalog localCatalog = | ||
PolarisCatalog.builder() | ||
.setType(Catalog.TypeEnum.INTERNAL) | ||
.setName(LOCAL_CATALOG_NAME) | ||
.setProperties(catalogProperties) | ||
.setStorageConfigInfo(storageConfig) | ||
.build(); | ||
managementApi.createCatalog(localCatalog); | ||
|
||
CatalogGrant catalogGrant = | ||
CatalogGrant.builder() | ||
.setType(CatalogGrant.TypeEnum.CATALOG) | ||
.setPrivilege(CatalogPrivilege.TABLE_WRITE_DATA) | ||
.build(); | ||
managementApi.addGrant(LOCAL_CATALOG_NAME, CATALOG_ROLE_NAME, catalogGrant); | ||
managementApi.assignPrincipalRole(PRINCIPAL_NAME, PRINCIPAL_ROLE_NAME); | ||
CatalogRole localCatalogAdminRole = | ||
managementApi.getCatalogRole(LOCAL_CATALOG_NAME, CATALOG_ROLE_NAME); | ||
managementApi.grantCatalogRoleToPrincipalRole( | ||
PRINCIPAL_ROLE_NAME, LOCAL_CATALOG_NAME, localCatalogAdminRole); | ||
|
||
AuthenticationParameters authParams = | ||
OAuthClientCredentialsParameters.builder() | ||
.setAuthenticationType(AuthenticationParameters.AuthenticationTypeEnum.OAUTH) | ||
.setTokenUri(endpoints.catalogApiEndpoint().toString() + "/v1/oauth/tokens") | ||
.setClientId(newUserCredentials.getCredentials().getClientId()) | ||
.setClientSecret(newUserCredentials.getCredentials().getClientSecret()) | ||
.setScopes(List.of("PRINCIPAL_ROLE:ALL")) | ||
.build(); | ||
ConnectionConfigInfo connectionConfig = | ||
IcebergRestConnectionConfigInfo.builder() | ||
.setConnectionType(ConnectionConfigInfo.ConnectionTypeEnum.ICEBERG_REST) | ||
.setUri(endpoints.catalogApiEndpoint().toString()) | ||
.setRemoteCatalogName(LOCAL_CATALOG_NAME) | ||
.setAuthenticationParameters(authParams) | ||
.build(); | ||
ExternalCatalog externalCatalog = | ||
ExternalCatalog.builder() | ||
.setType(Catalog.TypeEnum.EXTERNAL) | ||
.setName(EXTERNAL_CATALOG_NAME) | ||
.setConnectionConfigInfo(connectionConfig) | ||
.setProperties(catalogProperties) | ||
.setStorageConfigInfo(storageConfig) | ||
.build(); | ||
managementApi.createCatalog(externalCatalog); | ||
|
||
managementApi.addGrant(EXTERNAL_CATALOG_NAME, CATALOG_ROLE_NAME, catalogGrant); | ||
CatalogRole externalCatalogAdminRole = | ||
managementApi.getCatalogRole(EXTERNAL_CATALOG_NAME, CATALOG_ROLE_NAME); | ||
managementApi.grantCatalogRoleToPrincipalRole( | ||
PRINCIPAL_ROLE_NAME, EXTERNAL_CATALOG_NAME, externalCatalogAdminRole); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We only have one test right now so it's fine, but if we add more this should get refactored out
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, but let's resolve outstanding conversations before merging.
Adds a Junit5 integration test for catalog federation.