Skip to content

Commit c85b6bc

Browse files
iting0321iting0321
iting0321
authored and
iting0321
committed
clean up inline catalog secrets on create failure
1 parent a95e164 commit c85b6bc

2 files changed

Lines changed: 239 additions & 67 deletions

File tree

‎runtime/service/src/main/java/org/apache/polaris/service/admin/PolarisAdminService.java‎

Lines changed: 91 additions & 67 deletions
Original file line numberDiff line numberDiff line change
@@ -688,6 +688,24 @@ private Map<String, SecretReference> extractSecretReferences(
688688
return secretReferences;
689689
}
690690

691+
private void deleteSecretReferencesAfterFailedCatalogCreate(
692+
Map<String, SecretReference> secretReferences) {
693+
secretReferences
694+
.values()
695+
.forEach(
696+
secretReference -> {
697+
try {
698+
getUserSecretsManager().deleteSecret(secretReference);
699+
} catch (RuntimeException e) {
700+
LOGGER
701+
.atWarn()
702+
.setCause(e)
703+
.addKeyValue("secretReference", secretReference.urn())
704+
.log("Failed to clean up secret after catalog creation failure");
705+
}
706+
});
707+
}
708+
691709
/**
692710
* @see #extractSecretReferences
693711
*/
@@ -720,77 +738,83 @@ public PolarisEntity createCatalog(CreateCatalogRequest catalogRequest) {
720738
.build();
721739

722740
Catalog catalog = catalogRequest.getCatalog();
723-
if (catalog instanceof ExternalCatalog externalCatalog) {
724-
ConnectionConfigInfo connectionConfigInfo = externalCatalog.getConnectionConfigInfo();
725-
726-
if (connectionConfigInfo != null) {
727-
LOGGER
728-
.atDebug()
729-
.addKeyValue("catalogName", entity.getName())
730-
.log("Creating a federated catalog");
731-
FeatureConfiguration.enforceFeatureEnabledOrThrow(
732-
realmConfig, FeatureConfiguration.ENABLE_CATALOG_FEDERATION);
733-
Map<String, SecretReference> processedSecretReferences = Map.of();
734-
List<String> supportedAuthenticationTypes =
735-
realmConfig
736-
.getConfig(FeatureConfiguration.SUPPORTED_EXTERNAL_CATALOG_AUTHENTICATION_TYPES)
737-
.stream()
738-
.map(s -> s.toUpperCase(Locale.ROOT))
739-
.toList();
740-
if (requiresSecretReferenceExtraction(connectionConfigInfo)) {
741-
// For fields that contain references to secrets, we'll separately process the secrets
742-
// from the original request first, and then populate those fields with the extracted
743-
// secret references as part of the construction of the internal persistence entity.
744-
checkState(
745-
supportedAuthenticationTypes.contains(
746-
connectionConfigInfo
747-
.getAuthenticationParameters()
748-
.getAuthenticationType()
749-
.name()),
750-
"Authentication type %s is not supported.",
751-
connectionConfigInfo.getAuthenticationParameters().getAuthenticationType());
752-
processedSecretReferences = extractSecretReferences(catalogRequest, entity);
753-
} else {
754-
// Support no-auth catalog federation only when the feature is enabled.
755-
checkState(
756-
supportedAuthenticationTypes.contains(
757-
AuthenticationParameters.AuthenticationTypeEnum.IMPLICIT.name()),
758-
"Implicit authentication based catalog federation is not supported.");
741+
Map<String, SecretReference> processedSecretReferences = Map.of();
742+
boolean catalogCreated = false;
743+
try {
744+
if (catalog instanceof ExternalCatalog externalCatalog) {
745+
ConnectionConfigInfo connectionConfigInfo = externalCatalog.getConnectionConfigInfo();
746+
747+
if (connectionConfigInfo != null) {
748+
LOGGER
749+
.atDebug()
750+
.addKeyValue("catalogName", entity.getName())
751+
.log("Creating a federated catalog");
752+
FeatureConfiguration.enforceFeatureEnabledOrThrow(
753+
realmConfig, FeatureConfiguration.ENABLE_CATALOG_FEDERATION);
754+
List<String> supportedAuthenticationTypes =
755+
realmConfig
756+
.getConfig(FeatureConfiguration.SUPPORTED_EXTERNAL_CATALOG_AUTHENTICATION_TYPES)
757+
.stream()
758+
.map(s -> s.toUpperCase(Locale.ROOT))
759+
.toList();
760+
if (requiresSecretReferenceExtraction(connectionConfigInfo)) {
761+
// For fields that contain references to secrets, we'll separately process the secrets
762+
// from the original request first, and then populate those fields with the extracted
763+
// secret references as part of the construction of the internal persistence entity.
764+
checkState(
765+
supportedAuthenticationTypes.contains(
766+
connectionConfigInfo
767+
.getAuthenticationParameters()
768+
.getAuthenticationType()
769+
.name()),
770+
"Authentication type %s is not supported.",
771+
connectionConfigInfo.getAuthenticationParameters().getAuthenticationType());
772+
processedSecretReferences = extractSecretReferences(catalogRequest, entity);
773+
} else {
774+
// Support no-auth catalog federation only when the feature is enabled.
775+
checkState(
776+
supportedAuthenticationTypes.contains(
777+
AuthenticationParameters.AuthenticationTypeEnum.IMPLICIT.name()),
778+
"Implicit authentication based catalog federation is not supported.");
779+
}
780+
781+
// Allocate service identity if needed for the authentication type.
782+
// The provider will determine if a service identity is required based on the connection
783+
// config.
784+
Optional<ServiceIdentityInfoDpo> serviceIdentityInfoDpoOptional =
785+
serviceIdentityProvider.allocateServiceIdentity(connectionConfigInfo);
786+
787+
entity =
788+
new CatalogEntity.Builder(entity)
789+
.setConnectionConfigInfoDpoWithSecrets(
790+
connectionConfigInfo,
791+
processedSecretReferences,
792+
serviceIdentityInfoDpoOptional.orElse(null))
793+
.build();
759794
}
760-
761-
// Allocate service identity if needed for the authentication type.
762-
// The provider will determine if a service identity is required based on the connection
763-
// config.
764-
Optional<ServiceIdentityInfoDpo> serviceIdentityInfoDpoOptional =
765-
serviceIdentityProvider.allocateServiceIdentity(connectionConfigInfo);
766-
767-
entity =
768-
new CatalogEntity.Builder(entity)
769-
.setConnectionConfigInfoDpoWithSecrets(
770-
connectionConfigInfo,
771-
processedSecretReferences,
772-
serviceIdentityInfoDpoOptional.orElse(null))
773-
.build();
774795
}
775-
}
776796

777-
CreateCatalogResult catalogResult =
778-
metaStoreManager.createCatalog(getCurrentPolarisContext(), entity, List.of());
779-
if (catalogResult.alreadyExists()) {
780-
// TODO: Proactive garbage-collection of any inline secrets that were written to the
781-
// secrets manager, here and on any other unexpected exception as well.
782-
throw new AlreadyExistsException(
783-
"Cannot create Catalog %s. Catalog already exists or resolution failed",
784-
entity.getName());
785-
} else if (!catalogResult.isSuccess()) {
786-
throw new IllegalStateException(
787-
String.format(
788-
"Cannot create Catalog %s: %s with extraInfo %s",
789-
entity.getName(),
790-
catalogResult.getReturnStatus(),
791-
catalogResult.getExtraInformation()));
797+
CreateCatalogResult catalogResult =
798+
metaStoreManager.createCatalog(getCurrentPolarisContext(), entity, List.of());
799+
if (catalogResult.alreadyExists()) {
800+
throw new AlreadyExistsException(
801+
"Cannot create Catalog %s. Catalog already exists or resolution failed",
802+
entity.getName());
803+
} else if (!catalogResult.isSuccess()) {
804+
throw new IllegalStateException(
805+
String.format(
806+
"Cannot create Catalog %s: %s with extraInfo %s",
807+
entity.getName(),
808+
catalogResult.getReturnStatus(),
809+
catalogResult.getExtraInformation()));
810+
}
811+
catalogCreated = true;
812+
return PolarisEntity.of(catalogResult.getCatalog());
813+
} finally {
814+
if (!catalogCreated) {
815+
deleteSecretReferencesAfterFailedCatalogCreate(processedSecretReferences);
816+
}
792817
}
793-
return PolarisEntity.of(catalogResult.getCatalog());
794818
}
795819

796820
public void deleteCatalog(String name) {

‎runtime/service/src/test/java/org/apache/polaris/service/admin/PolarisAdminServiceTest.java‎

Lines changed: 148 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -23,24 +23,43 @@
2323
import static org.assertj.core.api.Assertions.assertThatThrownBy;
2424
import static org.mockito.ArgumentMatchers.any;
2525
import static org.mockito.ArgumentMatchers.eq;
26+
import static org.mockito.Mockito.doThrow;
2627
import static org.mockito.Mockito.mock;
28+
import static org.mockito.Mockito.never;
29+
import static org.mockito.Mockito.verify;
2730
import static org.mockito.Mockito.when;
2831

2932
import java.util.List;
33+
import java.util.Map;
34+
import java.util.Optional;
3035
import org.apache.iceberg.catalog.Namespace;
3136
import org.apache.iceberg.catalog.TableIdentifier;
37+
import org.apache.iceberg.exceptions.AlreadyExistsException;
3238
import org.apache.iceberg.exceptions.NoSuchTableException;
3339
import org.apache.iceberg.exceptions.NotFoundException;
3440
import org.apache.polaris.core.PolarisCallContext;
41+
import org.apache.polaris.core.admin.model.AuthenticationParameters;
42+
import org.apache.polaris.core.admin.model.AwsStorageConfigInfo;
43+
import org.apache.polaris.core.admin.model.Catalog;
44+
import org.apache.polaris.core.admin.model.CatalogProperties;
45+
import org.apache.polaris.core.admin.model.ConnectionConfigInfo;
46+
import org.apache.polaris.core.admin.model.CreateCatalogRequest;
47+
import org.apache.polaris.core.admin.model.ExternalCatalog;
48+
import org.apache.polaris.core.admin.model.IcebergRestConnectionConfigInfo;
49+
import org.apache.polaris.core.admin.model.OAuthClientCredentialsParameters;
50+
import org.apache.polaris.core.admin.model.StorageConfigInfo;
3551
import org.apache.polaris.core.auth.PolarisAuthorizer;
3652
import org.apache.polaris.core.auth.PolarisPrincipal;
3753
import org.apache.polaris.core.catalog.PolarisCatalogHelpers;
54+
import org.apache.polaris.core.config.BehaviorChangeConfiguration;
3855
import org.apache.polaris.core.config.FeatureConfiguration;
3956
import org.apache.polaris.core.config.RealmConfig;
4057
import org.apache.polaris.core.context.CallContext;
4158
import org.apache.polaris.core.entity.CatalogEntity;
4259
import org.apache.polaris.core.entity.NamespaceEntity;
60+
import org.apache.polaris.core.entity.PolarisBaseEntity;
4361
import org.apache.polaris.core.entity.PolarisEntity;
62+
import org.apache.polaris.core.entity.PolarisEntityConstants;
4463
import org.apache.polaris.core.entity.PolarisEntitySubType;
4564
import org.apache.polaris.core.entity.PolarisEntityType;
4665
import org.apache.polaris.core.entity.PolarisPrivilege;
@@ -49,13 +68,15 @@
4968
import org.apache.polaris.core.persistence.PolarisMetaStoreManager;
5069
import org.apache.polaris.core.persistence.PolarisResolvedPathWrapper;
5170
import org.apache.polaris.core.persistence.dao.entity.BaseResult;
71+
import org.apache.polaris.core.persistence.dao.entity.CreateCatalogResult;
5272
import org.apache.polaris.core.persistence.dao.entity.EntityResult;
5373
import org.apache.polaris.core.persistence.dao.entity.GenerateEntityIdResult;
5474
import org.apache.polaris.core.persistence.dao.entity.PrivilegeResult;
5575
import org.apache.polaris.core.persistence.resolver.PolarisResolutionManifest;
5676
import org.apache.polaris.core.persistence.resolver.ResolutionManifestFactory;
5777
import org.apache.polaris.core.persistence.resolver.ResolvedPathKey;
5878
import org.apache.polaris.core.persistence.resolver.ResolverStatus;
79+
import org.apache.polaris.core.secrets.SecretReference;
5980
import org.apache.polaris.core.secrets.UserSecretsManager;
6081
import org.apache.polaris.service.config.ReservedProperties;
6182
import org.assertj.core.api.Assertions;
@@ -116,6 +137,62 @@ protected static void assertSuccess(BaseResult result) {
116137
Assertions.assertThat(result.isSuccess()).isTrue();
117138
}
118139

140+
@Test
141+
void testCreateCatalogCleansUpInlineSecretsWhenCatalogAlreadyExists() {
142+
SecretReference secretReference =
143+
new SecretReference("urn:polaris-secret:test:oauth", Map.of());
144+
setupExternalCatalogCreate(secretReference);
145+
when(metaStoreManager.createCatalog(any(), any(), any()))
146+
.thenReturn(new CreateCatalogResult(BaseResult.ReturnStatus.ENTITY_ALREADY_EXISTS, null));
147+
148+
assertThatThrownBy(
149+
() ->
150+
adminService.createCatalog(new CreateCatalogRequest(createExternalOauthCatalog())))
151+
.isInstanceOf(AlreadyExistsException.class);
152+
153+
verify(userSecretsManager).deleteSecret(secretReference);
154+
}
155+
156+
@Test
157+
void testCreateCatalogCleanupFailureDoesNotHideOriginalFailure() {
158+
SecretReference secretReference =
159+
new SecretReference("urn:polaris-secret:test:oauth", Map.of());
160+
setupExternalCatalogCreate(secretReference);
161+
CreateCatalogResult resultWithError =
162+
new CreateCatalogResult(
163+
BaseResult.ReturnStatus.UNEXPECTED_ERROR_SIGNALED, "Unexpected Error Occurred");
164+
when(metaStoreManager.createCatalog(any(), any(), any())).thenReturn(resultWithError);
165+
doThrow(new RuntimeException("cleanup failed")).when(userSecretsManager).deleteSecret(any());
166+
167+
assertThatThrownBy(
168+
() ->
169+
adminService.createCatalog(new CreateCatalogRequest(createExternalOauthCatalog())))
170+
.isInstanceOf(IllegalStateException.class)
171+
.hasMessage(
172+
String.format(
173+
"Cannot create Catalog %s: %s with extraInfo %s",
174+
"external-catalog",
175+
resultWithError.getReturnStatus(),
176+
resultWithError.getExtraInformation()));
177+
178+
verify(userSecretsManager).deleteSecret(secretReference);
179+
}
180+
181+
@Test
182+
void testCreateCatalogDoesNotCleanUpInlineSecretsOnSuccess() {
183+
SecretReference secretReference =
184+
new SecretReference("urn:polaris-secret:test:oauth", Map.of());
185+
setupExternalCatalogCreate(secretReference);
186+
when(metaStoreManager.createCatalog(any(), any(), any()))
187+
.thenReturn(
188+
new CreateCatalogResult(
189+
createBaseCatalog("external-catalog"), createBaseCatalogRole()));
190+
191+
adminService.createCatalog(new CreateCatalogRequest(createExternalOauthCatalog()));
192+
193+
verify(userSecretsManager, never()).deleteSecret(any());
194+
}
195+
119196
@Test
120197
void testGrantPrivilegeOnNamespaceToRole() throws Exception {
121198
String catalogName = "test-catalog";
@@ -677,6 +754,77 @@ private PolarisEntity createEntity(String name, PolarisEntityType type) {
677754
.build();
678755
}
679756

757+
private void setupExternalCatalogCreate(SecretReference secretReference) {
758+
when(realmConfig.getConfig(FeatureConfiguration.ALLOW_OVERLAPPING_CATALOG_URLS))
759+
.thenReturn(true);
760+
when(realmConfig.getConfig(BehaviorChangeConfiguration.STORAGE_CONFIGURATION_MAX_LOCATIONS))
761+
.thenReturn(-1);
762+
when(realmConfig.getConfig(FeatureConfiguration.ENABLE_CATALOG_FEDERATION)).thenReturn(true);
763+
when(realmConfig.getConfig(
764+
FeatureConfiguration.SUPPORTED_EXTERNAL_CATALOG_AUTHENTICATION_TYPES))
765+
.thenReturn(List.of(AuthenticationParameters.AuthenticationTypeEnum.OAUTH.name()));
766+
767+
GenerateEntityIdResult idResult = mock(GenerateEntityIdResult.class);
768+
when(idResult.getId()).thenReturn(2L);
769+
when(metaStoreManager.generateNewEntityId(any())).thenReturn(idResult);
770+
when(reservedProperties.removeReservedProperties(Mockito.<Map<String, String>>any()))
771+
.thenReturn(Map.of());
772+
when(userSecretsManager.writeSecret(any(), any())).thenReturn(secretReference);
773+
when(identityProvider.allocateServiceIdentity(any())).thenReturn(Optional.empty());
774+
}
775+
776+
private Catalog createExternalOauthCatalog() {
777+
AwsStorageConfigInfo awsConfigModel =
778+
AwsStorageConfigInfo.builder()
779+
.setRoleArn("arn:aws:iam::123456789012:role/my-role")
780+
.setExternalId("externalId")
781+
.setUserArn("userArn")
782+
.setStorageType(StorageConfigInfo.StorageTypeEnum.S3)
783+
.setAllowedLocations(List.of("s3://bucket/path/to/data"))
784+
.build();
785+
ConnectionConfigInfo connectionConfigInfo =
786+
IcebergRestConnectionConfigInfo.builder(
787+
ConnectionConfigInfo.ConnectionTypeEnum.ICEBERG_REST)
788+
.setUri("https://example.com/polaris/api/catalog")
789+
.setRemoteCatalogName("remote-catalog")
790+
.setAuthenticationParameters(
791+
OAuthClientCredentialsParameters.builder(
792+
AuthenticationParameters.AuthenticationTypeEnum.OAUTH)
793+
.setClientId("client-id")
794+
.setClientSecret("client-secret")
795+
.setScopes(List.of("PRINCIPAL_ROLE:ALL"))
796+
.build())
797+
.build();
798+
799+
return ExternalCatalog.builder()
800+
.setType(Catalog.TypeEnum.EXTERNAL)
801+
.setName("external-catalog")
802+
.setProperties(CatalogProperties.builder("s3://bucket/path/to/data").build())
803+
.setStorageConfigInfo(awsConfigModel)
804+
.setConnectionConfigInfo(connectionConfigInfo)
805+
.build();
806+
}
807+
808+
private PolarisBaseEntity createBaseCatalog(String name) {
809+
return new PolarisBaseEntity(
810+
PolarisEntityConstants.getNullId(),
811+
2L,
812+
PolarisEntityType.CATALOG,
813+
PolarisEntitySubType.NULL_SUBTYPE,
814+
PolarisEntityConstants.getRootEntityId(),
815+
name);
816+
}
817+
818+
private PolarisBaseEntity createBaseCatalogRole() {
819+
return new PolarisBaseEntity(
820+
2L,
821+
3L,
822+
PolarisEntityType.CATALOG_ROLE,
823+
PolarisEntitySubType.NULL_SUBTYPE,
824+
2L,
825+
PolarisEntityConstants.getNameOfCatalogAdminRole());
826+
}
827+
680828
private PolarisEntity createEntity(String name, PolarisEntityType type, long id) {
681829
return new PolarisEntity.Builder()
682830
.setName(name)

0 commit comments

Comments
 (0)