OAK-12219: Upgrade Azure SDK V8 to V12 for oak-blob-azure - rework - address PR apache#2989 review comments

- cap configured presigned URI expiry to the 7-day Azure user delegation
  key lifetime under service-principal auth, with a warning
- honor secondary-location failover in UtilsV12.getRetryOptions when no
  retry count is configured (use SDK default retries instead of dropping
  the secondary host)
- expand @deprecated javadoc on AbstractAzureDataStoreService and
  AzureDataStoreService to explain the replacement
- add tests for expiry capping (SP and non-SP) and secondary-location
  retry options

Author: seropian

oak-blob-cloud-azure/src/main/java/org/apache/jackrabbit/oak/blob/cloud/azure/blobstorage/AbstractAzureDataStoreService.java

@@ -35,7 +35,9 @@
 /**
  * Kept for binary compatibility with existing callers. Use {@link AzureDataStoreWrapper} instead.
  *
- * @deprecated
+ * @deprecated superseded by {@link AzureDataStoreWrapper}, which replaces the dual-service
+ * (v8/v12) OSGi architecture with a single FT-aware component that can toggle between SDK
+ * versions at runtime without restart; retained only for binary compatibility.
  */
 @Deprecated(since = "2.3", forRemoval = true)
 public abstract class AbstractAzureDataStoreService extends AbstractDataStoreService {

oak-blob-cloud-azure/src/main/java/org/apache/jackrabbit/oak/blob/cloud/azure/blobstorage/AzureDataStoreService.java

@@ -26,7 +26,9 @@
  * {@link AzureDataStoreWrapper} owns the {@code AzureDataStore} PID. Use
  * {@link AzureDataStoreWrapper} instead.
  *
- * @deprecated
+ * @deprecated superseded by {@link AzureDataStoreWrapper}, which replaces the dual-service
+ * (v8/v12) OSGi architecture with a single FT-aware component that can toggle between SDK
+ * versions at runtime without restart; retained only for binary compatibility.
  */
 @Deprecated(since = "2.3", forRemoval = true)
 public class AzureDataStoreService extends AbstractAzureDataStoreService {

oak-blob-cloud-azure/src/main/java/org/apache/jackrabbit/oak/blob/cloud/azure/blobstorage/v12/AzureBlobContainerProviderV12.java

@@ -78,7 +78,9 @@ class AzureBlobContainerProviderV12 {
     final AtomicReference<CachedDelegationKey> cachedDelegationKey = new AtomicReference<>();
 
     // Request keys for the full 7-day window so they cover any SAS expiry we'd generate.
-    private static final Duration DELEGATION_KEY_LIFETIME = Duration.ofDays(7);
+    // Also the hard upper bound Azure allows for a user delegation key's lifetime — package-private
+    // so callers can validate configured presigned-URI expiries against it (see AzureBlobStoreBackendV12).
+    static final Duration DELEGATION_KEY_LIFETIME = Duration.ofDays(7);
     // Renew early enough to cover clock skew between this host and Azure.
     private static final Duration DELEGATION_KEY_RENEWAL_BUFFER = Duration.ofSeconds(60);
 
@@ -286,7 +288,9 @@ public void close() {
         log.debug("AzureBlobContainerProviderV12 closed; cached Azure clients released");
     }
 
-    private boolean authenticateViaServicePrincipal() {
+    // Package-private: AzureBlobStoreBackendV12 needs this to know whether presigned URIs will be
+    // signed with a user delegation key (and are therefore bounded by DELEGATION_KEY_LIFETIME).
+    boolean authenticateViaServicePrincipal() {
         return StringUtils.isBlank(azureConnectionString) &&
                 StringUtils.isNoneBlank(accountName, tenantId, clientId, clientSecret);
     }

oak-blob-cloud-azure/src/main/java/org/apache/jackrabbit/oak/blob/cloud/azure/blobstorage/v12/AzureBlobStoreBackendV12.java

@@ -287,11 +287,11 @@ private void initContainerConnection() throws DataStoreException {
     private void initPresignedURIConfig() {
         String putExpiry = properties.getProperty(AzureConstantsV12.PRESIGNED_HTTP_UPLOAD_URI_EXPIRY_SECONDS);
         if (putExpiry != null) {
-            this.setHttpUploadURIExpirySeconds(Integer.parseInt(putExpiry));
+            this.setHttpUploadURIExpirySeconds(capToDelegationKeyLifetime(Integer.parseInt(putExpiry)));
         }
         String getExpiry = properties.getProperty(AzureConstantsV12.PRESIGNED_HTTP_DOWNLOAD_URI_EXPIRY_SECONDS);
         if (getExpiry != null) {
-            this.setHttpDownloadURIExpirySeconds(Integer.parseInt(getExpiry));
+            this.setHttpDownloadURIExpirySeconds(capToDelegationKeyLifetime(Integer.parseInt(getExpiry)));
             String cacheMaxSize = properties.getProperty(AzureConstantsV12.PRESIGNED_HTTP_DOWNLOAD_URI_CACHE_MAX_SIZE);
             if (cacheMaxSize != null) {
                 this.setHttpDownloadURICacheSize(Integer.parseInt(cacheMaxSize));
@@ -303,6 +303,25 @@ private void initPresignedURIConfig() {
         downloadDomainOverride = properties.getProperty(AzureConstantsV12.PRESIGNED_HTTP_DOWNLOAD_URI_DOMAIN_OVERRIDE, null);
     }
 
+    /**
+     * When presigned URIs are signed with a user delegation key (service-principal auth), the SAS
+     * expiry can never exceed the key's lifetime — Azure caps that at 7 days
+     * ({@link AzureBlobContainerProviderV12#DELEGATION_KEY_LIFETIME}). A configured expiry beyond
+     * that would defeat the delegation-key cache (never hits) and the SAS would silently stop
+     * working before its stated expiry. Cap it here and warn instead of failing silently later.
+     */
+    private int capToDelegationKeyLifetime(int configuredExpirySeconds) {
+        long maxSeconds = AzureBlobContainerProviderV12.DELEGATION_KEY_LIFETIME.getSeconds();
+        if (azureBlobContainerProvider.authenticateViaServicePrincipal() && configuredExpirySeconds > maxSeconds) {
+            LOG.warn("Configured presigned URI expiry of {}s exceeds the {}s maximum lifetime of an Azure " +
+                            "user delegation key; capping to {}s to avoid a SAS URI that silently stops working " +
+                            "before its stated expiry.",
+                    configuredExpirySeconds, maxSeconds, maxSeconds);
+            return (int) maxSeconds;
+        }
+        return configuredExpirySeconds;
+    }
+
     private void initReferenceKey() throws DataStoreException {
         // Set to false to defer key creation until the first upload — useful in read-only or cold-standby nodes
         // that should never write to blob storage during startup.
@@ -653,6 +672,11 @@ protected void setHttpDownloadURIExpirySeconds(int seconds) {
         httpDownloadURIExpirySeconds = seconds;
     }
 
+    // Package-private for test assertions on capped/parsed config values.
+    int getHttpDownloadURIExpirySeconds() {
+        return httpDownloadURIExpirySeconds;
+    }
+
     protected void setHttpDownloadURICacheSize(int maxSize) {
         // max size 0 or smaller is used to turn off the cache
         if (maxSize > 0) {
@@ -731,6 +755,11 @@ protected void setHttpUploadURIExpirySeconds(int seconds) {
         httpUploadURIExpirySeconds = seconds;
     }
 
+    // Package-private for test assertions on capped/parsed config values.
+    int getHttpUploadURIExpirySeconds() {
+        return httpUploadURIExpirySeconds;
+    }
+
     private DataIdentifier generateSafeRandomIdentifier() {
         return new DataIdentifier(
                 String.format("%s-%d",

oak-blob-cloud-azure/src/main/java/org/apache/jackrabbit/oak/blob/cloud/azure/blobstorage/v12/UtilsV12.java

@@ -58,7 +58,16 @@ static ProxyOptions computeProxyOptions(@Nullable String proxyHost, @Nullable St
     public static RequestRetryOptions getRetryOptions(final String maxRequestRetryCount, Integer requestTimeout, String secondaryLocation) {
         int retries = PropertiesUtil.toInteger(maxRequestRetryCount, -1);
         if (retries < 0) {
-            return null;
+            if (secondaryLocation == null) {
+                // No retry count and no secondary location configured — let the SDK apply its own defaults.
+                return null;
+            }
+            // Secondary-location failover was explicitly enabled even though no retry count was
+            // configured. Build options with the SDK's default retry count (maxTries=null → 4) so
+            // the secondary host still takes effect instead of being silently dropped.
+            return new RequestRetryOptions(RetryPolicyType.EXPONENTIAL, null,
+                    requestTimeout, null, null,
+                    secondaryLocation);
         }
 
         if (retries == 0) {

oak-blob-cloud-azure/src/test/java/org/apache/jackrabbit/oak/blob/cloud/azure/blobstorage/v12/AzureBlobStoreBackendV12CRUDTest.java

@@ -409,6 +409,42 @@ public void init_downloadExpiryWithoutCacheSize_defaultsCacheToZero() throws Exc
         verify(container).exists();
     }
 
+    // 7 days, the hard limit of an Azure user delegation key's lifetime.
+    private static final int DELEGATION_KEY_LIFETIME_SECONDS = 7 * 24 * 60 * 60;
+
+    @Test
+    public void init_presignedExpiryExceedsDelegationKeyLifetime_withServicePrincipal_capsToMax() throws Exception {
+        when(container.exists()).thenReturn(true);
+        Properties p = baseInitProps();
+        p.setProperty(AzureConstantsV12.AZURE_TENANT_ID, "tenant");
+        p.setProperty(AzureConstantsV12.AZURE_CLIENT_ID, "client");
+        p.setProperty(AzureConstantsV12.AZURE_CLIENT_SECRET, "secret");
+        p.setProperty(AzureConstantsV12.PRESIGNED_HTTP_UPLOAD_URI_EXPIRY_SECONDS, String.valueOf(DELEGATION_KEY_LIFETIME_SECONDS + 100_000));
+        p.setProperty(AzureConstantsV12.PRESIGNED_HTTP_DOWNLOAD_URI_EXPIRY_SECONDS, String.valueOf(DELEGATION_KEY_LIFETIME_SECONDS + 100_000));
+        backend.setProperties(p);
+
+        backend.init();
+
+        assertEquals(DELEGATION_KEY_LIFETIME_SECONDS, backend.getHttpUploadURIExpirySeconds());
+        assertEquals(DELEGATION_KEY_LIFETIME_SECONDS, backend.getHttpDownloadURIExpirySeconds());
+    }
+
+    @Test
+    public void init_presignedExpiryExceedsDelegationKeyLifetime_withoutServicePrincipal_notCapped() throws Exception {
+        when(container.exists()).thenReturn(true);
+        Properties p = baseInitProps();
+        int uncappedExpiry = DELEGATION_KEY_LIFETIME_SECONDS + 100_000;
+        p.setProperty(AzureConstantsV12.PRESIGNED_HTTP_UPLOAD_URI_EXPIRY_SECONDS, String.valueOf(uncappedExpiry));
+        p.setProperty(AzureConstantsV12.PRESIGNED_HTTP_DOWNLOAD_URI_EXPIRY_SECONDS, String.valueOf(uncappedExpiry));
+        backend.setProperties(p);
+
+        backend.init();
+
+        // No service-principal auth configured — SAS is not signed with a delegation key, so no cap applies.
+        assertEquals(uncappedExpiry, backend.getHttpUploadURIExpirySeconds());
+        assertEquals(uncappedExpiry, backend.getHttpDownloadURIExpirySeconds());
+    }
+
     // --- getAllIdentifiers / getAllRecords (list-based) ---
 
     @SuppressWarnings("unchecked")

oak-blob-cloud-azure/src/test/java/org/apache/jackrabbit/oak/blob/cloud/azure/blobstorage/v12/UtilsV12Test.java

@@ -18,6 +18,7 @@
  */
 package org.apache.jackrabbit.oak.blob.cloud.azure.blobstorage.v12;
 
+import com.azure.storage.common.policy.RequestRetryOptions;
 import org.junit.Test;
 
 import java.io.IOException;
@@ -155,6 +156,19 @@ public void getRetryOptions_positiveCount_returnsNonNull() {
         assertNotNull(UtilsV12.getRetryOptions("3", null, null));
     }
 
+    /**
+     * A secondary location alone (no explicit retry count) must not silently disable
+     * secondary-location failover — options should still be built, using the SDK's default
+     * retry count, with the secondary host set.
+     */
+    @Test
+    public void getRetryOptions_negativeCountWithSecondaryLocation_returnsOptionsWithSecondaryHost() {
+        RequestRetryOptions options = UtilsV12.getRetryOptions("-1", null, "https://account-secondary.blob.core.windows.net");
+        assertNotNull(options);
+        assertEquals("https://account-secondary.blob.core.windows.net", options.getSecondaryHost());
+        assertEquals(4, options.getMaxTries());
+    }
+
     @Test(expected = IOException.class)
     public void readConfig_nonExistentFile_throwsIOException() throws IOException {
         UtilsV12.readConfig("/tmp/does-not-exist-" + System.nanoTime() + ".properties");