[BUG] azure-keyvault-extensions.jar and guava.jar version 28 support

[vinayak-kuchekar]

Hi,
We have integrated our product with Azure Blob Storage and communicate using the Azure Java SDK jars. As a jar dependency, we are using the guava.jar (version 26). This version of guava.jar uses unsafe API from Oracle Java SDK and hence not compatible with java 11. The jar provider has fixed these issues in latest version 28.1-jre. But azure-keyvault-extensions.jar(version 1.1) is not compatible with guava.jar (version 28.1-jre).

During Key Vault Key retrieval, request fails with CancellationException. I did analysis of this failure:
https://github.com/Azure/azure-keyvault-java/blob/master/azure-keyvault-extensions/src/main/java/com/microsoft/azure/keyvault/extensions/KeyVaultKeyResolver.java#L108
private ListenableFuture resolveKeyFromKeyAsync(String kid) {
ListenableFuture futureCall = client.getKeyAsync(kid, null);
return Futures.transform(futureCall, new FutureKeyFromKey(), MoreExecutors.directExecutor());
}
Here, futureCall is able to return the result, but it also tell that the task was cancelled.
Futures.transform – API from Google’s guava.jar which is responsible for chaining for async calls, is working fine till 26th version of guava.jar - although task was cancelled.
But in 28th version of guava.jar, Google has made improvements over cancelled task. They just transfer the cancellation status to caller

Nested exception is: wt.objectstorage.exception.ObjectStorageException: java.util.concurrent.CancellationException: Task was cancelled.
Nested exception is: java.util.concurrent.CancellationException: Task was cancelled.
at com.google.common.util.concurrent.AbstractFuture.cancellationExceptionWithCause(AbstractFuture.java:1349)
at com.google.common.util.concurrent.AbstractFuture.getDoneValue(AbstractFuture.java:550)
at com.google.common.util.concurrent.AbstractFuture.get(AbstractFuture.java:513)
at com.google.common.util.concurrent.FluentFuture$TrustedFuture.get(FluentFuture.java:86)
at com.ptc.windchill.objectstorage.azureblob.encryption.csekeyvault.CSEKeyVaultBlobEncryptConfig.getKeyVaultKey(CSEKeyVaultBlobEncryptConfig.java:98)

To Reproduce
Steps to reproduce the behavior:
Get Key from Key Vault Service using azure-keyvault-extensions.jar and guava.jar (version 28).

Code Snippet
KeyVaultConnectionParams kvConParams = keyProvider.getKeyVaultConnectionDetails(containerName, blobName);
KeyVaultCredentials kvCred = new KeyVaultCredentials() {
public String doAuthenticate(String authorization, String resource, String scope) {
AuthenticationResult token = getAccessTokenFromClientCredentials(authorization, resource, "", "");
return token.getAccessToken();
}
};
KeyVaultClient vc = new KeyVaultClient(kvCred);
KeyVaultKeyResolver cloudResolver = new KeyVaultKeyResolver(vc);
CachingKeyResolver cachingResolver = new CachingKeyResolver(1, cloudResolver);
IKey encryptionKey = cachingResolver.resolveKeyAsync("").get();

Expected behavior
We should be able to fetch the Key from Key Vault Service using guava.jar (version 28) and Azure Java SDK libraries.

No Screenshots

Setup :

• OS: CentOS 7
• IDE : Eclipse
• azure-storage.jar(8.3.0)
  azure-keyvault.jar (1.2.0)
  azure-keyvault-core.jar (1.2.0)
  azure-keyvault-cryptography.jar (1.2.0)
  azure-keyvault-extensions.jar (1.2.0)
  azure-keyvault-webkey.jar (1.2.0)
  guava.jar (version 28.1-jre)

Additional context
We are upgrading the java support to version 11 for our product.

[ghost]

Thanks for the feedback! We are routing this to the appropriate team for follow-up. cc

[ghost]

Thanks for the feedback! We are routing this to the appropriate team for follow-up. cc

[joshfree]

@g2vinay can you please help route this "track 1" question?

[joshfree]

@vinayak-kuchekar please note that we have shipped new major releases of the Azure SDK for Java including KeyVault and Storage. The new versions have names starting with 'com.azure' instead of 'com.microsoft.azure'. The new SDK versions have new features, better performance, and are more usable and consistent following the Azure SDK Design Guidelines.

• There is also a blog post about these new versions here: https://techcommunity.microsoft.com/t5/Azure-SDK/November-2019-unified-Azure-SDK-GA/ba-p/1020875

• You can find samples, migration guides, and documentation for the new major versions of the libraries here: https://github.com/azure/azure-sdk-for-java#client-ga-november-2019-releases

• A community "Gitter" channel for Azure SDK for Java is also available here: https://gitter.im/Azure/azure-sdk-for-java?utm_source=badge&utm_medium=badge&utm_campaign=pr-badge&utm_content=badge

[vcolin7]

After doing some debugging and with the help of @anuchandy, I managed to pinpoint the culprit: the ServiceFuture class, which extends Google's AbstractFuture.

In line 214 we override the method isCancelled() and define a future as cancelled in the case its subscription is unsubscribed, which could happen in one of three events: completion, failure or cancellation.

@anuchandy and myself were thinking we could add an extra check there to see if the set() method was called at some point (even for null values from empty responses) to determine if this future was in fact cancelled or not. Since you wrote this class, do you have any thoughts on this @jianghaolu?

[anuchandy]

@jianghaolu - The only owner of the underlying Rx Subscription should be ServiceFuture, but as of today we're allowing consumers to get a hold on that Subscription through getSubscription method, not sure why it has to be exposed. Had it not exposed, we could just remove the isCancelled override and rely on Abstract Future::isCancelled. It may be good to double-check getSubscription can be deprecated.

Keeping that deprecation possibility aside, In the current implementation, it seems the Subscriptioncan be in a canceled state in 3 cases:

Explicit cancel

1. If it was canceled explicitly by calling cancel method.
2. if it was canceled explicitly by calling getSubscription().cancel method.

Implicit cancel

2. If the result (success or error) is resolved hence the Subscription is "auto-unsubscribed", and this is treated as canceled in ServiceFuture.

The new Guava v28 Future chain seems considering a Future as canceled if isCancelled return true even after it produces a result. Like Victor pointed, wondering we should not treat "auto-unsubscription" as canceled since the result is already produced. Thoughts?

[jianghaolu]

With the rate of development in that repo, a 2 minor release deprecation will take forever, if it ever happens.
To me it feels like a bug for isCanceled() - in my opinion any potential behavior change to fix a bug, breaking or not, is justified.

[anuchandy]

right, Guava's transform operators behavior changed FROM "ignoring" the cancel state of a completed task TO "propagating" the cancel state of a completed task, which uncovered this bug in ServiceFuture::isCancelled.

[vinayak-kuchekar]

Hello,
Thanks for the bug fix. When the fix will be GA?
I assume that the fix will come via com.microsoft.rest » client-runtime library.

Thanks & Regards,
Vinayak Kuchekar

[vcolin7]

Hi Vinayak,

We will release a patch for this later this month, I will let you know once we do so :) The fix would be indeed a part of the com.microsoft.rest:client-runtime library, which is a dependency of the com.microsoft.azure:azure-keyvault library.

[vinayak-kuchekar]

Hi, Thanks for the bug fix. The latest client-runtime.jar is working very well along with guava.jar version 28. It helped us to solve the critical issue in our product in time. Thanks and regards, Vinayak Kuchekar

…

On Fri, May 8, 2020, 1:42 AM vcolin7 ***@***.***> wrote: Hi Vinayak, We will release a patch for this later this month, I will let you know once we do so. The fix would be indeed a part of the com.microsoft.rest:client-runtime library, which is a dependency of the com.microsoft.azure:azure-keyvault library. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#6553 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ACXAR35AAHFAG5KETV3DKVLRQMIZ5ANCNFSM4JRTW64Q> .

[vcolin7]

Glad to hear that! We will be releasing a patch to include this dependency in KeyVault in the upcoming days as well.

[vcolin7]

The new com.microsoft.azure:azure-keyvault version (1.2.4) that includes this fix has been released :)