This is an automated email from the ASF dual-hosted git repository. amitj pushed a commit to branch trunk in repository https://gitbox.apache.org/repos/asf/jackrabbit-oak.git
The following commit(s) were added to refs/heads/trunk by this push: new 868e308a1d Issues/oak 10781 (#1518) 868e308a1d is described below commit 868e308a1d7832f086d397d528b2b786bd6e46cf Author: Tushar <145645280+t-r...@users.noreply.github.com> AuthorDate: Tue Jun 18 14:45:58 2024 +0530 Issues/oak 10781 (#1518) OAK-10781: Access Token refresh in oak-blob-cloud-azure * only one access token will exist per class instance, and there will only be one token refresh exectuor that will check and refresh the access token. * close executor * reduce token refresh delay to 1 minute --- oak-blob-cloud-azure/pom.xml | 7 +++ .../blobstorage/AzureBlobContainerProvider.java | 73 +++++++++++++--------- .../azure/blobstorage/AzureBlobStoreBackend.java | 1 + .../azure/blobstorage/AzureDataStoreUtils.java | 10 +-- .../blob/cloud/azure/blobstorage/TestAzureDS.java | 10 +++ 5 files changed, 66 insertions(+), 35 deletions(-) diff --git a/oak-blob-cloud-azure/pom.xml b/oak-blob-cloud-azure/pom.xml index c058e10419..328091dfce 100644 --- a/oak-blob-cloud-azure/pom.xml +++ b/oak-blob-cloud-azure/pom.xml @@ -351,6 +351,13 @@ <artifactId>testcontainers</artifactId> <scope>test</scope> </dependency> + <dependency> + <groupId>org.apache.jackrabbit</groupId> + <artifactId>oak-commons</artifactId> + <version>${project.version}</version> + <classifier>tests</classifier> + <scope>test</scope> + </dependency> </dependencies> diff --git a/oak-blob-cloud-azure/src/main/java/org/apache/jackrabbit/oak/blob/cloud/azure/blobstorage/AzureBlobContainerProvider.java b/oak-blob-cloud-azure/src/main/java/org/apache/jackrabbit/oak/blob/cloud/azure/blobstorage/AzureBlobContainerProvider.java index 2f6d6b4f47..01522248b0 100644 --- a/oak-blob-cloud-azure/src/main/java/org/apache/jackrabbit/oak/blob/cloud/azure/blobstorage/AzureBlobContainerProvider.java +++ b/oak-blob-cloud-azure/src/main/java/org/apache/jackrabbit/oak/blob/cloud/azure/blobstorage/AzureBlobContainerProvider.java @@ -42,7 +42,6 @@ import org.slf4j.Logger; import org.slf4j.LoggerFactory; import java.io.Closeable; -import java.io.IOException; import java.net.URISyntaxException; import java.security.InvalidKeyException; import java.time.Instant; @@ -51,6 +50,7 @@ import java.time.OffsetDateTime; import java.time.format.DateTimeFormatter; import java.util.Date; import java.util.EnumSet; +import java.util.Objects; import java.util.Optional; import java.util.Properties; import java.util.concurrent.Executors; @@ -70,9 +70,12 @@ public class AzureBlobContainerProvider implements Closeable { private final String tenantId; private final String clientId; private final String clientSecret; + private ClientSecretCredential clientSecretCredential; + private AccessToken accessToken; + private StorageCredentialsToken storageCredentialsToken; private static final long TOKEN_REFRESHER_INITIAL_DELAY = 45L; private static final long TOKEN_REFRESHER_DELAY = 1L; - private static final ScheduledExecutorService executorService = Executors.newSingleThreadScheduledExecutor(); + private final ScheduledExecutorService executorService = Executors.newSingleThreadScheduledExecutor(); private AzureBlobContainerProvider(Builder builder) { this.azureConnectionString = builder.azureConnectionString; @@ -177,13 +180,17 @@ public class AzureBlobContainerProvider implements Closeable { public CloudBlobContainer getBlobContainer(@Nullable BlobRequestOptions blobRequestOptions) throws DataStoreException { // connection string will be given preference over service principals / sas / account key if (StringUtils.isNotBlank(azureConnectionString)) { + log.debug("connecting to azure blob storage via azureConnectionString"); return Utils.getBlobContainer(azureConnectionString, containerName, blobRequestOptions); } else if (authenticateViaServicePrincipal()) { + log.debug("connecting to azure blob storage via service principal credentials"); return getBlobContainerFromServicePrincipals(blobRequestOptions); } else if (StringUtils.isNotBlank(sasToken)) { + log.debug("connecting to azure blob storage via sas token"); final String connectionStringWithSasToken = Utils.getConnectionStringForSas(sasToken, blobEndpoint, accountName); return Utils.getBlobContainer(connectionStringWithSasToken, containerName, blobRequestOptions); } + log.debug("connecting to azure blob storage via access key"); final String connectionStringWithAccountKey = Utils.getConnectionString(accountName, accountKey, blobEndpoint); return Utils.getBlobContainer(connectionStringWithAccountKey, containerName, blobRequestOptions); } @@ -205,19 +212,33 @@ public class AzureBlobContainerProvider implements Closeable { @NotNull private StorageCredentialsToken getStorageCredentials() { - ClientSecretCredential clientSecretCredential = new ClientSecretCredentialBuilder() - .clientId(clientId) - .clientSecret(clientSecret) - .tenantId(tenantId) - .build(); - AccessToken accessToken = clientSecretCredential.getTokenSync(new TokenRequestContext().addScopes(AZURE_DEFAULT_SCOPE)); - if (accessToken == null || StringUtils.isBlank(accessToken.getToken())) { - log.error("Access token is null or empty"); - throw new IllegalArgumentException("Could not connect to azure storage, access token is null or empty"); + boolean isAccessTokenGenerated = false; + /* generate access token, the same token will be used for subsequent access + * generated token is valid for 1 hour only and will be refreshed in background + * */ + if (accessToken == null) { + clientSecretCredential = new ClientSecretCredentialBuilder() + .clientId(clientId) + .clientSecret(clientSecret) + .tenantId(tenantId) + .build(); + accessToken = clientSecretCredential.getTokenSync(new TokenRequestContext().addScopes(AZURE_DEFAULT_SCOPE)); + if (accessToken == null || StringUtils.isBlank(accessToken.getToken())) { + log.error("Access token is null or empty"); + throw new IllegalArgumentException("Could not connect to azure storage, access token is null or empty"); + } + storageCredentialsToken = new StorageCredentialsToken(accountName, accessToken.getToken()); + isAccessTokenGenerated = true; + } + + Objects.requireNonNull(storageCredentialsToken, "storage credentials token cannot be null"); + + // start refresh token executor only when the access token is first generated + if (isAccessTokenGenerated) { + log.info("starting refresh token task at: {}", OffsetDateTime.now()); + TokenRefresher tokenRefresher = new TokenRefresher(); + executorService.scheduleWithFixedDelay(tokenRefresher, TOKEN_REFRESHER_INITIAL_DELAY, TOKEN_REFRESHER_DELAY, TimeUnit.MINUTES); } - StorageCredentialsToken storageCredentialsToken = new StorageCredentialsToken(accountName, accessToken.getToken()); - TokenRefresher tokenRefresher = new TokenRefresher(clientSecretCredential, accessToken, storageCredentialsToken); - executorService.scheduleWithFixedDelay(tokenRefresher, TOKEN_REFRESHER_INITIAL_DELAY, TOKEN_REFRESHER_DELAY, TimeUnit.MINUTES); return storageCredentialsToken; } @@ -292,20 +313,7 @@ public class AzureBlobContainerProvider implements Closeable { StringUtils.isNoneBlank(accountName, tenantId, clientId, clientSecret); } - private static class TokenRefresher implements Runnable { - - private final ClientSecretCredential clientSecretCredential; - private AccessToken accessToken; - private final StorageCredentialsToken storageCredentialsToken; - - public TokenRefresher(ClientSecretCredential clientSecretCredential, - AccessToken accessToken, - StorageCredentialsToken storageCredentialsToken) { - this.clientSecretCredential = clientSecretCredential; - this.accessToken = accessToken; - this.storageCredentialsToken = storageCredentialsToken; - } - + private class TokenRefresher implements Runnable { @Override public void run() { try { @@ -315,12 +323,14 @@ public class AzureBlobContainerProvider implements Closeable { log.info("Access token is about to expire (5 minutes or less) at: {}. New access token will be generated", accessToken.getExpiresAt().format(DateTimeFormatter.ISO_LOCAL_DATE_TIME)); AccessToken newToken = clientSecretCredential.getTokenSync(new TokenRequestContext().addScopes(AZURE_DEFAULT_SCOPE)); + log.info("New azure access token generated at: {}", LocalDateTime.now()); if (newToken == null || StringUtils.isBlank(newToken.getToken())) { log.error("New access token is null or empty"); return; } - this.accessToken = newToken; - this.storageCredentialsToken.updateToken(this.accessToken.getToken()); + // update access token with newly generated token + accessToken = newToken; + storageCredentialsToken.updateToken(accessToken.getToken()); } } catch (Exception e) { log.error("Error while acquiring new access token: ", e); @@ -329,7 +339,8 @@ public class AzureBlobContainerProvider implements Closeable { } @Override - public void close() throws IOException { + public void close() { new ExecutorCloser(executorService).close(); + log.info("Refresh token executor service shutdown completed"); } } diff --git a/oak-blob-cloud-azure/src/main/java/org/apache/jackrabbit/oak/blob/cloud/azure/blobstorage/AzureBlobStoreBackend.java b/oak-blob-cloud-azure/src/main/java/org/apache/jackrabbit/oak/blob/cloud/azure/blobstorage/AzureBlobStoreBackend.java index b7338594a3..0eba902e7f 100644 --- a/oak-blob-cloud-azure/src/main/java/org/apache/jackrabbit/oak/blob/cloud/azure/blobstorage/AzureBlobStoreBackend.java +++ b/oak-blob-cloud-azure/src/main/java/org/apache/jackrabbit/oak/blob/cloud/azure/blobstorage/AzureBlobStoreBackend.java @@ -516,6 +516,7 @@ public class AzureBlobStoreBackend extends AbstractSharedBackend { @Override public void close() throws DataStoreException { + azureBlobContainerProvider.close(); LOG.info("AzureBlobBackend closed."); } diff --git a/oak-blob-cloud-azure/src/test/java/org/apache/jackrabbit/oak/blob/cloud/azure/blobstorage/AzureDataStoreUtils.java b/oak-blob-cloud-azure/src/test/java/org/apache/jackrabbit/oak/blob/cloud/azure/blobstorage/AzureDataStoreUtils.java index 8ef8df038e..e5a2a8e620 100644 --- a/oak-blob-cloud-azure/src/test/java/org/apache/jackrabbit/oak/blob/cloud/azure/blobstorage/AzureDataStoreUtils.java +++ b/oak-blob-cloud-azure/src/test/java/org/apache/jackrabbit/oak/blob/cloud/azure/blobstorage/AzureDataStoreUtils.java @@ -182,10 +182,12 @@ public class AzureDataStoreUtils extends DataStoreUtils { Properties props = getAzureConfig(); props.setProperty(AzureConstants.AZURE_BLOB_CONTAINER_NAME, containerName); - CloudBlobContainer container = AzureBlobContainerProvider.Builder.builder(containerName).initializeWithProperties(props) - .build().getBlobContainer(); - boolean result = container.deleteIfExists(); - log.info("Container deleted. containerName={} existed={}", containerName, result); + try (AzureBlobContainerProvider azureBlobContainerProvider = AzureBlobContainerProvider.Builder.builder(containerName).initializeWithProperties(props) + .build()) { + CloudBlobContainer container = azureBlobContainerProvider.getBlobContainer(); + boolean result = container.deleteIfExists(); + log.info("Container deleted. containerName={} existed={}", containerName, result); + } } protected static HttpsURLConnection getHttpsConnection(long length, URI uri) throws IOException { diff --git a/oak-blob-cloud-azure/src/test/java/org/apache/jackrabbit/oak/blob/cloud/azure/blobstorage/TestAzureDS.java b/oak-blob-cloud-azure/src/test/java/org/apache/jackrabbit/oak/blob/cloud/azure/blobstorage/TestAzureDS.java index f809a9e010..14eca1b94a 100644 --- a/oak-blob-cloud-azure/src/test/java/org/apache/jackrabbit/oak/blob/cloud/azure/blobstorage/TestAzureDS.java +++ b/oak-blob-cloud-azure/src/test/java/org/apache/jackrabbit/oak/blob/cloud/azure/blobstorage/TestAzureDS.java @@ -21,12 +21,15 @@ package org.apache.jackrabbit.oak.blob.cloud.azure.blobstorage; import static org.junit.Assume.assumeTrue; import org.apache.jackrabbit.core.data.DataStore; +import org.apache.jackrabbit.oak.commons.junit.LogCustomizer; import org.apache.jackrabbit.oak.plugins.blob.datastore.AbstractDataStoreTest; import org.junit.After; +import org.junit.Assert; import org.junit.Before; import org.junit.BeforeClass; import org.slf4j.Logger; import org.slf4j.LoggerFactory; +import org.slf4j.event.Level; import java.util.Properties; @@ -65,7 +68,14 @@ public class TestAzureDS extends AbstractDataStoreTest { @After public void tearDown() { try { + LogCustomizer customizer = LogCustomizer.forLogger(AzureBlobContainerProvider.class.getName()) + .filter(Level.INFO) + .create(); + customizer.starting(); super.tearDown(); + Assert.assertEquals(1, customizer.getLogs().size()); + Assert.assertEquals("Refresh token executor service shutdown completed", customizer.getLogs().get(0)); + customizer.finished(); AzureDataStoreUtils.deleteContainer(container); } catch (Exception ignore) {