[FSN-13] feat(nf-tower): Retrieve and set Fusion license tokens in a process environment #5614

alberto-miranda · 2024-12-16T11:03:35Z

Description

This PR adds a new TowerFusionEnv class to nf-tower that enables setting Platform-specific environment variables to Nextflow processes running with Fusion enabled. The class implements the FusionEnv interface and, as such, provides a getEnvironment() method that returns the relevant key-value pairs.

Currently, the provider is only being used to set the FUSION_LICENSE_TOKEN environment variable, the value of which is retrieved by sending a specific HTTP request to Platform in order to validate the run. Once the run is validated, Platform will reply with a corresponding token which will be injected into the process environment.

Note

Since fusion licenses are still not mandatory, if the license cannot be validated (or there's a configuration error affecting this validation process) a warning is emitted:

user@host$ nextflow run hello

N E X T F L O W   ~  version 24.11.0-edge

Launching `https://github.com/nextflow-io/hello` [mighty_liskov] DSL2 - revision: afff16a9b4 [master]

executor >  local (fusion enabled) (4)
[ec/6b82ba] process > sayHello (4) [100%] 4 of 4 ✔
Hello world!

Hello world!

Ciao world!

Ciao world!

Bonjour world!

Bonjour world!

Hola world!

Hola world!

WARN: Error retrieving Fusion license information: Missing personal access token -- Make sure there's a variable >TOWER_ACCESS_TOKEN in your environment
WARN: Error retrieving Fusion license information: Missing personal access token -- Make sure there's a variable >TOWER_ACCESS_TOKEN in your environment
WARN: Error retrieving Fusion license information: Missing personal access token -- Make sure there's a variable >TOWER_ACCESS_TOKEN in your environment
WARN: Error retrieving Fusion license information: Missing personal access token -- Make sure there's a variable >TOWER_ACCESS_TOKEN in your environment

Testing

This PR can be tested by following this guide.

netlify · 2024-12-16T11:03:54Z

✅ Deploy Preview for nextflow-docs-staging canceled.

Name	Link
🔨 Latest commit	`43f8bba`
🔍 Latest deploy log	https://app.netlify.com/sites/nextflow-docs-staging/deploys/67659be67516e400080a0dea

modules/nextflow/src/main/groovy/nextflow/fusion/FusionClient.groovy

alberto-miranda · 2024-12-17T10:40:47Z

@pditommaso I have refactored the implementation into TowerClient as you suggested. A couple of comments:

I'm not very fond of making TowerClient implement FusionEnv: I'd rather have a dedicated TowerFusionEnv class with the getEnvironment() method and helpers, but then I cannot reuse the client itself or the sendHttpMessage() and co. functions. Any advice here?
It doesn't seem to be picking up TowerClient as an extension despite having defined it in plugins/nf-tower/src/resources/META-INF/extensions.idx. As such, it's not using it (e.g. this should fail and it's not failing). Is there anything else I need to do to register the extension?

pditommaso · 2024-12-17T13:25:02Z

I'm not very fond of making TowerClient implement FusionEnv: I'd rather have a dedicated TowerFusionEnv class

I agree, sorry if I may have suggested in that direction. My point was to move this logic in the tower client module

then I cannot reuse the client itself or the sendHttpMessage()

don't worry about that, use the java provider client for example

nextflow/plugins/nf-tower/src/main/io/seqera/tower/plugin/TowerXAuth.groovy

Lines 56 to 62 in 6183fdf

    
               this.httpClient = HttpClient.newBuilder() 
        
                       .version(HttpClient.Version.HTTP_1_1) 
        
                       .followRedirects(HttpClient.Redirect.NORMAL) 
        
                       .cookieHandler(cookieManager) 
        
                       .connectTimeout(Duration.ofSeconds(10)) 
        
                       .build() 
        
           }

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

alberto-miranda · 2024-12-18T13:06:01Z

@pditommaso I refactored the implementation based on our discussions. The PR is ready to review.

alberto-miranda · 2024-12-18T13:08:44Z

plugins/nf-tower/src/main/io/seqera/tower/plugin/TowerFusionEnv.groovy

+    /**
+     * Get the configured Platform API endpoint: if the endpoint is not provided in the configuration, we fallback to the
+     * environment variable `TOWER_API_ENDPOINT`. If neither is provided, we fallback to the default endpoint.
+     *
+     * @param opts the configuration options for Platform
+     * @param env the applicable environment variables
+     * @return the Platform API endpoint
+     */
+    protected static String endpoint0(Map opts, Map<String, String> env) {
+        def result = opts.endpoint as String
+        if (!result || result == '-') {
+            result = env.get('TOWER_API_ENDPOINT') ?: TowerClient.DEF_ENDPOINT_URL
+        }
+        return result.stripEnd('/')
+    }
+
+    /**
+     * Get the configured Platform access token: if `TOWER_WORKFLOW_ID` is provided in the environment, we are running
+     * in a Platform-made run and we should ONLY retrieve the token from the environment. Otherwise, check
+     * the configuration file or fallback to the environment. If no token is found, returns null.
+     *
+     * @param opts the configuration options for Platform
+     * @param env the applicable environment variables
+     * @return the Platform access token
+     */
+    protected static String accessToken0(Map opts, Map<String, String> env) {
+        def token = env.get('TOWER_WORKFLOW_ID')
+            ? env.get('TOWER_ACCESS_TOKEN')
+            : opts.containsKey('accessToken') ? opts.accessToken as String : env.get('TOWER_ACCESS_TOKEN')
+        return token
+    }


This basically replicates what TowerConfig does in nf-wave, but I didn't want to add a explicit dependency on that. If adding this dependency is not a problem, it would be better to reuse the code there.

Could make sense to turn those into PlatformHelper class shared across modules & plugins

I agree. Do you want it done in the context of this PR?

it could be better

Refactored with c1147cd. I created the PlatformHelper class under modules/nextflow/src/main/groovy/nextflow/platform since it seemed to fit with the existing conventions for common code. Please double check.

plugins/nf-tower/src/main/io/seqera/tower/plugin/TowerFusionEnv.groovy

pditommaso

Looks good, left some comments. Question, what happens when using Fusion and Tower plugin is not enabled?

plugins/nf-tower/src/main/io/seqera/tower/plugin/TowerFusionEnv.groovy

pditommaso · 2024-12-18T13:49:31Z

plugins/nf-tower/src/main/io/seqera/tower/plugin/TowerFusionEnv.groovy

+        try {
+            final token = getLicenseToken(product, version)
+            return [
+                'FUSION_LICENSE_TOKEN': token,


Suggested change

'FUSION_LICENSE_TOKEN': token,

FUSION_LICENSE_TOKEN: token,

Groovy magic

Fixed with f32f0ca

plugins/nf-tower/src/main/io/seqera/tower/plugin/TowerFusionEnv.groovy

pditommaso · 2024-12-18T13:52:57Z

plugins/nf-tower/src/main/io/seqera/tower/plugin/TowerFusionEnv.groovy

+                    new Gson().toJson(
+                        new LicenseTokenRequest(
+                            product: product,
+                            version: version
+                        ),
+                        LicenseTokenRequest.class
+                    ),


Built-in JsonOutout.toJson(Map) can be easier

I did it this way for symmetry with the deserialization but I don't have a preference for one or the other.

I'm fine keeping gson, then it would be better to isolate into its own method

Done with 2f11171

pditommaso · 2024-12-18T13:54:07Z

plugins/nf-tower/src/main/io/seqera/tower/plugin/TowerFusionEnv.groovy

+    /**
+     * Get the configured Platform API endpoint: if the endpoint is not provided in the configuration, we fallback to the
+     * environment variable `TOWER_API_ENDPOINT`. If neither is provided, we fallback to the default endpoint.
+     *
+     * @param opts the configuration options for Platform
+     * @param env the applicable environment variables
+     * @return the Platform API endpoint
+     */
+    protected static String endpoint0(Map opts, Map<String, String> env) {
+        def result = opts.endpoint as String
+        if (!result || result == '-') {
+            result = env.get('TOWER_API_ENDPOINT') ?: TowerClient.DEF_ENDPOINT_URL
+        }
+        return result.stripEnd('/')
+    }
+
+    /**
+     * Get the configured Platform access token: if `TOWER_WORKFLOW_ID` is provided in the environment, we are running
+     * in a Platform-made run and we should ONLY retrieve the token from the environment. Otherwise, check
+     * the configuration file or fallback to the environment. If no token is found, returns null.
+     *
+     * @param opts the configuration options for Platform
+     * @param env the applicable environment variables
+     * @return the Platform access token
+     */
+    protected static String accessToken0(Map opts, Map<String, String> env) {
+        def token = env.get('TOWER_WORKFLOW_ID')
+            ? env.get('TOWER_ACCESS_TOKEN')
+            : opts.containsKey('accessToken') ? opts.accessToken as String : env.get('TOWER_ACCESS_TOKEN')
+        return token
+    }


Could make sense to turn those into PlatformHelper class shared across modules & plugins

alberto-miranda · 2024-12-18T14:11:15Z

Looks good, left some comments. Question, what happens when using Fusion and Tower plugin is not enabled?

Nothing special, if nf-tower is enabled but Fusion is not, the TowerFusionEnv itself is never instantiated, and there are no calls made to retrieve a token. The same happens if both are disabled, but in this case nf-tower is also never loaded.

pditommaso · 2024-12-18T14:13:50Z

Will Fusion be able to run?

alberto-miranda · 2024-12-18T14:21:02Z

Will Fusion be able to run?

The license is not yet enforced in the Fusion side, so yes.

A side effect we didn't consider is that if fusion is run without nf-tower the license is not enforced currently (because TowerFusionEnv is not loaded in this case). Is this a use case that should concern us?

EDIT: Even if not enforced by nextflow, fusion will still fail once licenses become mandatory, but the error received by the user will probably be uglier.

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

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

pditommaso · 2024-12-18T16:33:03Z

Even if not enforced by nextflow, fusion will still fail once licenses become mandatory, but the error received by the user will probably be uglier

and slower, ie. after the job started. we should consider adding a "default" TowerFusionEnv implementation of this execution reporting a warning message.

The "default" TowerFusionEnv is "overridden" by the one provided by tower plugin when it's enabled

See here for example

nextflow/plugins/nf-wave/src/main/io/seqera/wave/plugin/resolver/WaveContainerResolver.groovy

Line 41 in 9eefd20

    
           @Priority(-10)  // <-- lower is higher, this is needed to override default provider behavior

alberto-miranda · 2024-12-18T17:05:05Z

Even if not enforced by nextflow, fusion will still fail once licenses become mandatory, but the error received by the user will probably be uglier

and slower, ie. after the job started. we should consider adding a "default" TowerFusionEnv implementation of this execution reporting a warning message.

The "default" TowerFusionEnv is "overridden" by the one provided by tower plugin when it's enabled

See here for example

nextflow/plugins/nf-wave/src/main/io/seqera/wave/plugin/resolver/WaveContainerResolver.groovy

Line 41 in 9eefd20

@Priority(-10) // <-- lower is higher, this is needed to override default provider behavior

It's a great idea, but if I understand correctly the mechanism you propose, it prioritizes between implementations of a specific interface doesn't it? For our case, we need to override one implementer of FusionEnv (i.e. TowerFusionEnv) but maintain any other implementers such as AwsFusionEnv. Can we do that automatically?

pditommaso · 2024-12-18T18:15:40Z

You are right, it may need some customisation in the plugins handling. Ignore it for now.

plugins/nf-tower/src/main/io/seqera/tower/plugin/TowerFusionEnv.groovy

stefanoboriero

Nice, left a few comments but nothing blocking

plugins/nf-tower/src/main/io/seqera/tower/plugin/TowerFusionEnv.groovy

stefanoboriero · 2024-12-20T08:23:50Z

plugins/nf-tower/src/main/io/seqera/tower/plugin/TowerFusionEnv.groovy

+            () -> {
+                log.debug "Request: method:=${req.method()}; uri:=${req.uri()}; request:=${req}"
+                final resp = httpClient.send(req, HttpResponse.BodyHandlers.ofString())
+                log.debug "Response: statusCode:=${resp.statusCode()}; body:=${resp.body()}"


This response is going to contain an authentication token right? Do we want to log it?

Good question. I'm not sure about what the preferred policy is in Nextflow logs (cc @pditommaso)

stefanoboriero · 2024-12-20T08:28:40Z

plugins/nf-tower/src/main/io/seqera/tower/plugin/TowerFusionEnv.groovy

+    private static final String LICENSE_TOKEN_PATH = 'license/token/'
+
+    // Server errors that should trigger a retry
+    private static final List<Integer> SERVER_ERRORS = [429, 500, 502, 503, 504]


I'm not sure how I feel about retrying 503 and 504 errors, if Seqera Platform is under heavy load wouldn't this make it worse?

I took this from Wave's client to follow the existing approach. I don't have a specific preference 🤷‍♂️ (cc @pditommaso)

That's why it's used an exponential backoff. Actually think it should be added 408 (connection timeout) to that list

Makes sense, consider this resolved 👍

Error 408 added with 6ce5be3

stefanoboriero · 2024-12-20T08:29:29Z

plugins/nf-tower/src/main/io/seqera/tower/plugin/TowerFusionEnv.groovy

+     * Send a request to Platform to obtain a license-scoped JWT for Fusion. The request is authenticated using the
+     * Platform access token provided in the configuration of the current session.
+     *
+     * @throws AbortOperationException if a Platform access token cannot be found


It also throws other exceptions if the token is not valid or malformed

Addressed with 1fed859 and refactored into a9849f5

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

alberto-miranda · 2024-12-20T16:02:01Z

As discussed this morning, I have added caching of license token responses with a9849f5 and re-scoped the license-related error messages as debug instead of warning with eaa985e.

Having said that, I'm not at all convinced that the caching is working as it should, so I would appreciate more experienced eyes looking into it.

alberto-miranda · 2024-12-20T16:03:59Z

~~It looks like I broke some tests in the process. Fixing.~~ Tests should be fixed! 🚀

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

pditommaso reviewed Dec 16, 2024

View reviewed changes

modules/nextflow/src/main/groovy/nextflow/fusion/FusionClient.groovy Outdated Show resolved Hide resolved

jordeu self-requested a review December 16, 2024 13:33

bentsherman added the storage/fusion label Dec 16, 2024

alberto-miranda force-pushed the fsn-13/fetch-fusion-license-token-from-platform-and-pass-it-to-fusion-using-envvar branch from bee1e99 to 6fc8fbe Compare December 17, 2024 10:33

feat(nf-tower): Add TowerFusionEnv provider to set required env vars

e7b1082

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

alberto-miranda force-pushed the fsn-13/fetch-fusion-license-token-from-platform-and-pass-it-to-fusion-using-envvar branch from 6fc8fbe to e7b1082 Compare December 18, 2024 12:53

alberto-miranda changed the title ~~[FSN-13] feat: Add FusionClient to send fusion-related HTTP requests to Platform~~ [FSN-13] feat: Add Fusion environment provider to nf-tower plugin Dec 18, 2024

alberto-miranda changed the title ~~[FSN-13] feat: Add Fusion environment provider to nf-tower plugin~~ [FSN-13] feat(nf-tower): Retrieve and set Fusion license tokens in a process environment Dec 18, 2024

alberto-miranda commented Dec 18, 2024

View reviewed changes

plugins/nf-tower/src/main/io/seqera/tower/plugin/TowerFusionEnv.groovy Show resolved Hide resolved

pditommaso requested changes Dec 18, 2024

View reviewed changes

alberto-miranda added 5 commits December 18, 2024 17:26

refactor: Move common Platform code to `nextflow.platform.PlatformHel…

c1147cd

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

chore: Rename LICENSE_TOKEN_ENDPOINT to LICENSE_TOKEN_PATH

20c8a53

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

style: Remove quotes from key name

f32f0ca

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

style: Prefer Map.of() to [:]

25cd5ae

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

refactor: Encapsulate request/response serialization/deserialization

2f11171

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

jordeu approved these changes Dec 20, 2024

View reviewed changes

plugins/nf-tower/src/main/io/seqera/tower/plugin/TowerFusionEnv.groovy Outdated Show resolved Hide resolved

stefanoboriero approved these changes Dec 20, 2024

View reviewed changes

alberto-miranda added 2 commits December 20, 2024 16:43

chore: Get fusion's SKU from FusionConfig

94f6075

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

chore: Improve javadoc for getLicenseToken()

1fed859

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

alberto-miranda added 3 commits December 20, 2024 16:43

chore: Ensure requests are retried on 408 error

6ce5be3

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

refactor: Cache license token responses

a9849f5

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

chore: Scope license-related errors as debug level

eaa985e

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

chore: Fix failing tests

43f8bba

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

[FSN-13] feat(nf-tower): Retrieve and set Fusion license tokens in a process environment #5614

Are you sure you want to change the base?

[FSN-13] feat(nf-tower): Retrieve and set Fusion license tokens in a process environment #5614

Conversation

alberto-miranda commented Dec 16, 2024 • edited Loading

Description

Testing

netlify bot commented Dec 16, 2024 • edited Loading

✅ Deploy Preview for nextflow-docs-staging canceled.

alberto-miranda commented Dec 17, 2024 • edited Loading

pditommaso commented Dec 17, 2024 • edited Loading

alberto-miranda commented Dec 18, 2024

alberto-miranda Dec 18, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pditommaso left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alberto-miranda commented Dec 18, 2024

pditommaso commented Dec 18, 2024

alberto-miranda commented Dec 18, 2024 • edited Loading

pditommaso commented Dec 18, 2024

alberto-miranda commented Dec 18, 2024

pditommaso commented Dec 18, 2024

stefanoboriero left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alberto-miranda commented Dec 20, 2024 • edited Loading

alberto-miranda commented Dec 20, 2024 • edited Loading

alberto-miranda commented Dec 16, 2024 •

edited

Loading

netlify bot commented Dec 16, 2024 •

edited

Loading

alberto-miranda commented Dec 17, 2024 •

edited

Loading

pditommaso commented Dec 17, 2024 •

edited

Loading

alberto-miranda Dec 18, 2024 •

edited

Loading

alberto-miranda commented Dec 18, 2024 •

edited

Loading

alberto-miranda commented Dec 20, 2024 •

edited

Loading

alberto-miranda commented Dec 20, 2024 •

edited

Loading