Fix Azure Fusion env misses credentials when no key or SAS provided (#…

…5328)



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

plugins/nf-azure/src/main/nextflow/cloud/azure/config/AzStorageOpts.groovy

@@ -62,10 +62,4 @@ class AzStorageOpts {
         }
         return result
     }
-
-    synchronized String getOrCreateSasToken() {
-        if( !sasToken )
-            sasToken = AzHelper.generateAccountSas(accountName, accountKey, tokenDuration)
-        return sasToken
-    }
 }

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

@@ -17,47 +17,72 @@
 
 package nextflow.cloud.azure.fusion
 
+import groovy.util.logging.Slf4j
+import nextflow.Global
+import nextflow.cloud.azure.batch.AzHelper
 import groovy.transform.CompileStatic
 import nextflow.cloud.azure.config.AzConfig
 import nextflow.fusion.FusionConfig
 import nextflow.fusion.FusionEnv
 import org.pf4j.Extension
+
 /**
  * Implement environment provider for Azure specific variables
- * 
+ *
  * @author Paolo Di Tommaso <[email protected]>
  */
 @Extension
 @CompileStatic
+@Slf4j
 class AzFusionEnv implements FusionEnv {
 
     @Override
     Map<String, String> getEnvironment(String scheme, FusionConfig config) {
-        if (scheme != 'az')
+        if (scheme != 'az') {
             return Collections.<String, String> emptyMap()
+        }
 
         final cfg = AzConfig.config
         final result = new LinkedHashMap(10)
 
-        if (!cfg.storage().accountName)
+        if (!cfg.storage().accountName) {
             throw new IllegalArgumentException("Missing Azure Storage account name")
+        }
 
-        if (cfg.storage().accountKey && cfg.storage().sasToken)
+        if (cfg.storage().accountKey && cfg.storage().sasToken) {
             throw new IllegalArgumentException("Azure Storage Access key and SAS token detected. Only one is allowed")
+        }
 
-        // the account name is always required
         result.AZURE_STORAGE_ACCOUNT = cfg.storage().accountName
+        // TODO: In theory, generating an impromptu SAS token for authentication methods other than
+        // `azure.storage.sasToken` should not be necessary, because those methods should already allow sufficient
+        // access for normal operation. Nevertheless, #5287 heavily implies that failing to do so causes the Azure
+        // Storage plugin or Fusion to fail. In any case, it may be possible to remove this in the future.
+        result.AZURE_STORAGE_SAS_TOKEN = getOrCreateSasToken()
 
-        // If a Managed Identity or Service Principal is configured, Fusion only needs to know the account name
-        if (cfg.managedIdentity().isConfigured() || cfg.activeDirectory().isConfigured()) {
-            return result
-        }
+        return result
+    }
+
+    /**
+     * Return the SAS token if it is defined in the configuration, otherwise generate one based on the requested
+     * authentication method.
+     */
+    synchronized String getOrCreateSasToken() {
+
+        final cfg = AzConfig.config
 
-        // If a SAS token is configured, instead, Fusion also requires the token value
+        // If a SAS token is already defined in the configuration, just return it
         if (cfg.storage().sasToken) {
-            result.AZURE_STORAGE_SAS_TOKEN = cfg.storage().getOrCreateSasToken()
+            return cfg.storage().sasToken
         }
 
-        return result
+        // For Active Directory and Managed Identity, we cannot generate an *account* SAS token, but we can generate
+        // a *container* SAS token for the work directory.
+        if (cfg.activeDirectory().isConfigured() || cfg.managedIdentity().isConfigured()) {
+            return AzHelper.generateContainerSasWithActiveDirectory(Global.session.workDir, cfg.storage().tokenDuration)
+        }
+
+        // Shared Key authentication can use an account SAS token
+        return AzHelper.generateAccountSasWithAccountKey(Global.session.workDir, cfg.storage().tokenDuration)
     }
 }

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

@@ -21,6 +21,7 @@ import nextflow.Global
 import nextflow.Session
 import nextflow.SysEnv
 import nextflow.fusion.FusionConfig
+
 import spock.lang.Specification
 
 /**
@@ -46,22 +47,117 @@ class AzFusionEnvTest extends Specification {
         env == Collections.emptyMap()
     }
 
-    def 'should return env environment'() {
+    def 'should return env environment with SAS token config when accountKey is provided'() {
         given:
+        def NAME = 'myaccount'
+        def KEY = 'myaccountkey'
         Global.session = Mock(Session) {
-            getConfig() >> [azure: [storage: [accountName: 'x1']]]
+            getConfig() >> [azure: [storage: [accountName: NAME, accountKey: KEY]]]
         }
-        and:
 
         when:
         def config = Mock(FusionConfig)
-        def env = new AzFusionEnv().getEnvironment('az', config)
+        def fusionEnv = Spy(AzFusionEnv)
+        1 * fusionEnv.getOrCreateSasToken() >> 'generatedSasToken'
+        def env = fusionEnv.getEnvironment('az', config)
+
+        then:
+        env.AZURE_STORAGE_ACCOUNT == NAME
+        env.AZURE_STORAGE_SAS_TOKEN
+        env.size() == 2
+    }
+
+    def 'should return env environment with SAS token config when a Service Principal is provided'() {
+        given:
+        def NAME = 'myaccount'
+        def CLIENT_ID = 'myclientid'
+        def CLIENT_SECRET = 'myclientsecret'
+        def TENANT_ID = 'mytenantid'
+        Global.session = Mock(Session) {
+            getConfig() >> [
+                azure: [
+                    activeDirectory: [
+                        servicePrincipalId: CLIENT_ID,
+                        servicePrincipalSecret: CLIENT_SECRET,
+                        tenantId: TENANT_ID
+                    ],
+                    storage: [
+                        accountName: NAME
+                    ]
+                ]
+            ]
+        }
+
+        when:
+        def config = Mock(FusionConfig)
+        def fusionEnv = Spy(AzFusionEnv)
+        1 * fusionEnv.getOrCreateSasToken() >> 'generatedSasToken'
+        def env = fusionEnv.getEnvironment('az', config)
+
+        then:
+        env.AZURE_STORAGE_ACCOUNT == NAME
+        env.AZURE_STORAGE_SAS_TOKEN == 'generatedSasToken'
+        env.size() == 2
+    }
+
+    def 'should return env environment with SAS token config when a user-assigned Managed Identity is provided'() {
+        given:
+        def NAME = 'myaccount'
+        def CLIENT_ID = 'myclientid'
+        Global.session = Mock(Session) {
+            getConfig() >> [
+                azure: [
+                    managedIdentity: [
+                        clientId: CLIENT_ID,
+                    ],
+                    storage: [
+                        accountName: NAME
+                    ]
+                ]
+            ]
+        }
+
+        when:
+        def config = Mock(FusionConfig)
+        def fusionEnv = Spy(AzFusionEnv)
+        1 * fusionEnv.getOrCreateSasToken() >> 'generatedSasToken'
+        def env = fusionEnv.getEnvironment('az', config)
+
         then:
-        env == [AZURE_STORAGE_ACCOUNT: 'x1']
+        env.AZURE_STORAGE_ACCOUNT == NAME
+        env.AZURE_STORAGE_SAS_TOKEN == 'generatedSasToken'
+        env.size() == 2
+    }
 
+    def 'should return env environment with SAS token config when a system-assigned Managed Identity is provided'() {
+        given:
+        def NAME = 'myaccount'
+        Global.session = Mock(Session) {
+            getConfig() >> [
+                azure: [
+                    managedIdentity: [
+                        system: true
+                    ],
+                    storage: [
+                        accountName: NAME
+                    ]
+                ]
+            ]
+        }
+
+        when:
+        def config = Mock(FusionConfig)
+        def fusionEnv = Spy(AzFusionEnv)
+        1 * fusionEnv.getOrCreateSasToken() >> 'generatedSasToken'
+        def env = fusionEnv.getEnvironment('az', config)
+
+        then:
+        env.AZURE_STORAGE_ACCOUNT == NAME
+        env.AZURE_STORAGE_SAS_TOKEN == 'generatedSasToken'
+        env.size() == 2
     }
 
-    def 'should return env environment with SAS token config'() {
+    def 'should return env environment with SAS token config when a sasToken is provided'() {
         given:
         Global.session = Mock(Session) {
             getConfig() >> [azure: [storage: [accountName: 'x1', sasToken: 'y1']]]
@@ -99,4 +195,5 @@ class AzFusionEnvTest extends Specification {
         then:
         thrown(IllegalArgumentException)
     }
+
 }