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 <12817534+adamrtalbot@users.noreply.github.com>

Signed-off-by: adamrtalbot <12817534+adamrtalbot@users.noreply.github.com>

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

✅ Deploy Preview for nextflow-docs-staging canceled.

Name	Link
🔨 Latest commit	`f93e25b`
🔍 Latest deploy log	https://app.netlify.com/sites/nextflow-docs-staging/deploys/66f3206eb370e8000847153f

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 <12817534+adamrtalbot@users.noreply.github.com>

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

…Env.groovy Signed-off-by: Adam Talbot <12817534+adamrtalbot@users.noreply.github.com>

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 <alberto.miranda@seqera.io>

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 <paolo.ditommaso@gmail.com>

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?

ewels · 2024-09-23T08:56:18Z

Any chance you can look at that?

@pditommaso who is this to? @alberto-miranda ?

alberto-miranda · 2024-09-23T09:36:06Z

Any chance you can look at that?

@pditommaso who is this to? @alberto-miranda ?

Hopefully not, because I have no idea how that works 😅

adamrtalbot · 2024-09-23T09:55:23Z

Tested with:

Keys
Service Principal
Managed Identity

All worked with and without Fusion. Will dig into it a bit more too see if any edge cases pop up it's acceptable to me now.

Signed-off-by: adamrtalbot <12817534+adamrtalbot@users.noreply.github.com>

adamrtalbot · 2024-09-23T10:15:44Z

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?

Tried again with 3139dae. See https://github.com/nextflow-io/nextflow/actions/runs/10992221379/job/30516198828?pr=5287 for an example workflow run 🤞

pditommaso · 2024-09-23T10:26:52Z

Still not working, not understanding why

adamrtalbot · 2024-09-23T10:57:28Z

Still not working, not understanding why

I can't see commit_message in the events context, perhaps it doesn't exist? Docs: https://docs.github.com/en/actions/writing-workflows/choosing-when-your-workflow-runs/events-that-trigger-workflows and contexts: https://docs.github.com/en/actions/writing-workflows/choosing-what-your-workflow-does/accessing-contextual-information-about-workflow-runs#github-context

adamrtalbot · 2024-09-23T11:05:09Z

I would switch it to be a separate workflow, triggered on Github comment containing [test platform prod] or [test platform stage], then add an automatic PR comment to the end of a successful test workflow. That way you get a daisy chained workflow but you can also manually kick it off for a PR.

Signed-off-by: adamrtalbot <12817534+adamrtalbot@users.noreply.github.com>

adamrtalbot · 2024-09-23T11:21:58Z

Here's a test run with the if-statement disabled. Gonna revert that commit now.

https://github.com/nextflow-io/nextflow/actions/runs/10993010551/job/30518841423?pr=5287

This reverts commit cab3ffe. Signed-off-by: adamrtalbot <12817534+adamrtalbot@users.noreply.github.com>

pditommaso · 2024-09-23T11:43:20Z

I would switch it to be a separate workflow

That make sense, any chance to give it a try?

adamrtalbot · 2024-09-23T14:13:22Z

I would switch it to be a separate workflow

That make sense, any chance to give it a try?

I can but can we do a separate PR so we don't block this one?

pditommaso · 2024-09-23T14:24:40Z

Yes, sure

Signed-off-by: Paolo Di Tommaso <paolo.ditommaso@gmail.com>

pditommaso · 2024-09-24T20:52:27Z

It looks tests are not running because is an external PR. Closing in favour of #5328

adamrtalbot added 2 commits September 5, 2024 12:04

Catch when service principal exists but no keys or SAS

89710df

Signed-off-by: adamrtalbot <12817534+adamrtalbot@users.noreply.github.com>

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 <12817534+adamrtalbot@users.noreply.github.com>

Improve comment for Fusion env with SAS

0171fa3

Signed-off-by: adamrtalbot <12817534+adamrtalbot@users.noreply.github.com>

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 <12817534+adamrtalbot@users.noreply.github.com>

Ensure proper SAS tokens are generated for each Azure authentication …

f11d62d

…method Signed-off-by: Alberto Miranda <alberto.miranda@seqera.io>

pditommaso added 3 commits September 16, 2024 11:04

test [platform stage]

dd20318

Signed-off-by: Paolo Di Tommaso <paolo.ditommaso@gmail.com>

Merge branch 'master' into fix_azure_fusion_missing_auth

a832bde

test [platform stage]

61374f8

Signed-off-by: Paolo Di Tommaso <paolo.ditommaso@gmail.com>

adamrtalbot added 2 commits September 23, 2024 10:55

Merge branch 'master' into fix_azure_fusion_missing_auth

5b67abe

[platform stage]

3139dae

Signed-off-by: adamrtalbot <12817534+adamrtalbot@users.noreply.github.com>

debug build workflow

cab3ffe

Signed-off-by: adamrtalbot <12817534+adamrtalbot@users.noreply.github.com>

Revert "debug build workflow"

d05cae5

This reverts commit cab3ffe. Signed-off-by: adamrtalbot <12817534+adamrtalbot@users.noreply.github.com>

pditommaso added 2 commits September 24, 2024 21:25

Merge branch 'master' into fix_azure_fusion_missing_auth

577bfb4

test [e2e stage]

f93e25b

Signed-off-by: Paolo Di Tommaso <paolo.ditommaso@gmail.com>

pditommaso closed this Sep 24, 2024

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

ewels commented Sep 23, 2024 •

edited

Loading

alberto-miranda commented Sep 23, 2024

adamrtalbot commented Sep 23, 2024

adamrtalbot commented Sep 23, 2024 •

edited

Loading

pditommaso commented Sep 23, 2024

adamrtalbot commented Sep 23, 2024

adamrtalbot commented Sep 23, 2024

adamrtalbot commented Sep 23, 2024

pditommaso commented Sep 23, 2024

adamrtalbot commented Sep 23, 2024

pditommaso commented Sep 23, 2024

pditommaso commented Sep 24, 2024

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

Conversation

adamrtalbot commented Sep 5, 2024 • edited Loading

netlify bot commented Sep 5, 2024 • edited Loading

✅ Deploy Preview for nextflow-docs-staging canceled.

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

ewels commented Sep 23, 2024 • edited Loading

alberto-miranda commented Sep 23, 2024

adamrtalbot commented Sep 23, 2024

adamrtalbot commented Sep 23, 2024 • edited Loading

pditommaso commented Sep 23, 2024

adamrtalbot commented Sep 23, 2024

adamrtalbot commented Sep 23, 2024

adamrtalbot commented Sep 23, 2024

pditommaso commented Sep 23, 2024

adamrtalbot commented Sep 23, 2024

pditommaso commented Sep 23, 2024

pditommaso commented Sep 24, 2024

adamrtalbot commented Sep 5, 2024 •

edited

Loading

netlify bot commented Sep 5, 2024 •

edited

Loading

alberto-miranda commented Sep 6, 2024 •

edited

Loading

ewels commented Sep 23, 2024 •

edited

Loading

adamrtalbot commented Sep 23, 2024 •

edited

Loading