fix: Azure Fusion env misses credentials when no key or SAS provided #5287

adamrtalbot · 2024-09-05T11:10:11Z

Fix: Azure Fusion missing authentication if accountKey is provided
Catch when service principal exists but no keys or SAS

Changes logic of Azure Environment set up: 1. Is there an account name? (no: error) 2. Is there an accountKey or an accountSas or Managed identity? (no: error) 3. If there is a managed identity yes: return no: Create or add a SAS -> return Signed-off-by: adamrtalbot <[email protected]>

Signed-off-by: adamrtalbot <[email protected]>

netlify · 2024-09-05T11:10:26Z

✅ Deploy Preview for nextflow-docs-staging ready!

Name	Link
🔨 Latest commit	`61374f8`
🔍 Latest deploy log	https://app.netlify.com/sites/nextflow-docs-staging/deploys/66e7f720ff5b8f0008622872
😎 Deploy Preview	https://deploy-preview-5287--nextflow-docs-staging.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

adamrtalbot · 2024-09-05T11:11:16Z

plugins/nf-azure/src/test/nextflow/cloud/azure/fusion/AzFusionEnvTest.groovy

+        def mockStorageObject = Mock(Object) {
+            getOrCreateSasToken() >> 'generatedSasToken'
+        }


@pditommaso o great Spock wizard, why does it not mock the getOrCreateSasToken method properly here?

shouldn't it be Mock(AzStorageOpts)?

Tried that and I get the same result, env.AZURE_STORAGE_SAS_TOKEN is sv=2024-05-04&ss=bf&srt=sco&se=2024-09-07T11%3A24%3A38Z&sp=rwdlacut&sig=SIGNATURE%3D

alberto-miranda · 2024-09-05T11:16:56Z

plugins/nf-azure/src/main/nextflow/cloud/azure/fusion/AzFusionEnv.groovy

-        // If a Managed Identity or Service Principal is configured, Fusion only needs to know the account name
-        if (cfg.managedIdentity().isConfigured() || cfg.activeDirectory().isConfigured()) {
+        // If a Managed Identity is configured, Fusion only needs to know the account name
+        if (cfg.managedIdentity().isConfigured()) {
            return result
        }

        // If a SAS token is configured, instead, Fusion also requires the token value


@adamrtalbot Could we please update this comment to something like:

// Otherwise, Fusion also requires a SAS Token // (yes, even if `cfg.storage().sasToken` has not been explicitly defined)

so that this doesn't happen again?

How about this:

// If Fusion does not use a managed identity, get or create a SAS token for Fusion to use

That's not much different from what the code itself already tells. I would prefer to have a explicit comment telling us why it's needed despite the configuration not requesting it explicitly.

Signed-off-by: adamrtalbot <[email protected]>

plugins/nf-azure/src/main/nextflow/cloud/azure/fusion/AzFusionEnv.groovy

…Env.groovy Signed-off-by: Adam Talbot <[email protected]>

pditommaso · 2024-09-05T17:12:58Z

Not working in my tests

https://cloud.seqera.io/orgs/seqeralabs/workspaces/showcase/watch/4TjgGOuMwaln3w

alberto-miranda · 2024-09-06T07:01:33Z

I tested this patch interactively and it fails when running nextflow from an Azure VM using Service Principal authentication:

java.lang.NullPointerException: 'accountKey' cannot be null.
        at java.base/java.util.Objects.requireNonNull(Objects.java:235)
        at com.azure.storage.common.StorageSharedKeyCredential.<init>(StorageSharedKeyCredent
ial.java:57)
        at nextflow.cloud.azure.batch.AzHelper.memoizedMethodPriv$getOrCreateBlobServiceWithK
eyStringString(AzHelper.groovy:181)
        at nextflow.cloud.azure.batch.AzHelper.access$0(AzHelper.groovy)
        at nextflow.cloud.azure.batch.AzHelper$__clinit__closure1.doCall(AzHelper.groovy)
        at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
        at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccesso
rImpl.java:77)
        at java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMetho
dAccessorImpl.java:43)
        at java.base/java.lang.reflect.Method.invoke(Method.java:568)
        at org.codehaus.groovy.reflection.CachedMethod.invoke(CachedMethod.java:343)
        at groovy.lang.MetaMethod.doMethodInvoke(MetaMethod.java:328)
        at org.codehaus.groovy.runtime.metaclass.ClosureMetaClass.invokeMethod(ClosureMetaCla
ss.java:279)
        at groovy.lang.MetaClassImpl.invokeMethod(MetaClassImpl.java:1007)
        at groovy.lang.Closure.call(Closure.java:433)
        at org.codehaus.groovy.runtime.memoize.Memoize$MemoizeFunction.lambda$call$0(Memoize.
java:137)
        at org.codehaus.groovy.runtime.memoize.ConcurrentCommonCache.getAndPut(ConcurrentComm
onCache.java:137)
        at org.codehaus.groovy.runtime.memoize.ConcurrentCommonCache.getAndPut(ConcurrentComm
onCache.java:113)
        at org.codehaus.groovy.runtime.memoize.Memoize$MemoizeFunction.call(Memoize.java:136)
        at nextflow.cloud.azure.batch.AzHelper.getOrCreateBlobServiceWithKey(AzHelper.groovy)
        at nextflow.cloud.azure.batch.AzHelper.generateAccountSas(AzHelper.groovy:173)
        at nextflow.cloud.azure.config.AzStorageOpts.getOrCreateSasToken(AzStorageOpts.groovy
:68)
[...]

The reason is that the current implementation of getOrCreateSasToken() needs accountKey to be defined to generate the SAS token string, and there's no accountKey when using a Service Principal. I checked the Azure SDK and it seems to be possible to generate SAS tokens with any method of authentication (i.e not just a Shared Access Key) so it should suffice to extend the getOrCreateSasToken() to cover the Managed Identity and Service Principal cases.

I'm currently working on a fix to test this theory.

…method Signed-off-by: Alberto Miranda <[email protected]>

alberto-miranda · 2024-09-06T15:25:55Z

Ok, I think I finally got it working. I just pushed a fix that works for all the interactive tests I could think of using an Azure VM. @pditommaso @adamrtalbot could you please test it with your workflows and see if I finally got it right? 🥺

pditommaso · 2024-09-12T08:19:27Z

Is this PR still valid following the outcome of the discussion on the related Fusion thread?

adamrtalbot · 2024-09-12T14:11:00Z

Is this PR still valid following the outcome of the discussion on the related Fusion thread?

Yes - we need to make sure the workers have a SAS key when using a managed identity. I believe these changes make Nextflow always generate a storage account SAS, even when using a Managed Identity, is that correct @alberto-miranda?

Currently using a Managed Identity + Fusion will fail.

alberto-miranda · 2024-09-14T06:13:59Z

Is this PR still valid following the outcome of the discussion on the related Fusion thread?

Yes - we need to make sure the workers have a SAS key when using a managed identity. I believe these changes make Nextflow always generate a storage account SAS, even when using a Managed Identity, is that correct @alberto-miranda?

Currently using a Managed Identity + Fusion will fail.

It is indeed still valid. In fact, it doesn't make sense to proceed with the implementation mentioned in https://github.com/seqeralabs/fusion/issues/526 unless the current PR is confirmed to work. Did this fix pass all the failing tests?

pditommaso · 2024-09-16T07:59:49Z

Integration tests looks OK in this branch

Signed-off-by: Paolo Di Tommaso <[email protected]>

pditommaso · 2024-09-18T10:59:07Z

I tried to run the e2e tests by adding [platform stage] comment but it didn't run

nextflow/.github/workflows/build.yml

Line 180 in 62e2604

    
           if: ${{ contains(github.event.head_commit.message, '[platform prod]') || contains(github.event.head_commit.message, '[platform stage]') }}

Any chance you can look at that?

adamrtalbot added 2 commits September 5, 2024 12:04

Catch when service principal exists but no keys or SAS

89710df

Signed-off-by: adamrtalbot <[email protected]>

adamrtalbot linked an issue Sep 5, 2024 that may be closed by this pull request

Azure storage authentication errors in 24.08.0-edge with Fusion enabled #5286

Open

adamrtalbot commented Sep 5, 2024

View reviewed changes

alberto-miranda reviewed Sep 5, 2024

View reviewed changes

adamrtalbot added 2 commits September 5, 2024 12:25

Mock AzStorageOpts

a6006f0

Signed-off-by: adamrtalbot <[email protected]>

Improve comment for Fusion env with SAS

0171fa3

Signed-off-by: adamrtalbot <[email protected]>

adamrtalbot commented Sep 5, 2024

View reviewed changes

plugins/nf-azure/src/main/nextflow/cloud/azure/fusion/AzFusionEnv.groovy Outdated Show resolved Hide resolved

Update plugins/nf-azure/src/main/nextflow/cloud/azure/fusion/AzFusion…

8b92f8f

…Env.groovy Signed-off-by: Adam Talbot <[email protected]>

Ensure proper SAS tokens are generated for each Azure authentication …

f11d62d

…method Signed-off-by: Alberto Miranda <[email protected]>

pditommaso added 3 commits September 16, 2024 11:04

test [platform stage]

dd20318

Signed-off-by: Paolo Di Tommaso <[email protected]>

Merge branch 'master' into fix_azure_fusion_missing_auth

a832bde

test [platform stage]

61374f8

Signed-off-by: Paolo Di Tommaso <[email protected]>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: Azure Fusion env misses credentials when no key or SAS provided #5287

fix: Azure Fusion env misses credentials when no key or SAS provided #5287

adamrtalbot commented Sep 5, 2024 •

edited

Loading

netlify bot commented Sep 5, 2024 •

edited

Loading

adamrtalbot Sep 5, 2024

bentsherman Sep 5, 2024

adamrtalbot Sep 5, 2024

alberto-miranda Sep 5, 2024

adamrtalbot Sep 5, 2024

alberto-miranda Sep 5, 2024

pditommaso commented Sep 5, 2024

alberto-miranda commented Sep 6, 2024

alberto-miranda commented Sep 6, 2024 •

edited

Loading

pditommaso commented Sep 12, 2024

adamrtalbot commented Sep 12, 2024

alberto-miranda commented Sep 14, 2024

pditommaso commented Sep 16, 2024

pditommaso commented Sep 18, 2024

fix: Azure Fusion env misses credentials when no key or SAS provided #5287

Are you sure you want to change the base?

fix: Azure Fusion env misses credentials when no key or SAS provided #5287

Conversation

adamrtalbot commented Sep 5, 2024 • edited Loading

netlify bot commented Sep 5, 2024 • edited Loading

✅ Deploy Preview for nextflow-docs-staging ready!

adamrtalbot Sep 5, 2024

Choose a reason for hiding this comment

bentsherman Sep 5, 2024

Choose a reason for hiding this comment

adamrtalbot Sep 5, 2024

Choose a reason for hiding this comment

alberto-miranda Sep 5, 2024

Choose a reason for hiding this comment

adamrtalbot Sep 5, 2024

Choose a reason for hiding this comment

alberto-miranda Sep 5, 2024

Choose a reason for hiding this comment

pditommaso commented Sep 5, 2024

alberto-miranda commented Sep 6, 2024

alberto-miranda commented Sep 6, 2024 • edited Loading

pditommaso commented Sep 12, 2024

adamrtalbot commented Sep 12, 2024

alberto-miranda commented Sep 14, 2024

pditommaso commented Sep 16, 2024

pditommaso commented Sep 18, 2024

adamrtalbot commented Sep 5, 2024 •

edited

Loading

netlify bot commented Sep 5, 2024 •

edited

Loading

alberto-miranda commented Sep 6, 2024 •

edited

Loading