[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	`073ebde`
🔍 Latest deploy log	https://app.netlify.com/sites/nextflow-docs-staging/deploys/678fd873dcdf780007e865f0

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.

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

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)

Is the token temporary, right? then it should not be a problem

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

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 · 2025-01-21T11:36:30Z

Does this depends on Fusion 2.5?

jordeu · 2025-01-21T13:39:27Z

Does this depends on Fusion 2.5?

No, it's also supported on Fusion 2.4

…tform-and-pass-it-to-fusion-using-envvar

Signed-off-by: Paolo Di Tommaso <[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 Outdated Show resolved Hide resolved

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]>

pditommaso force-pushed the master branch from 1f834a0 to 6454605 Compare January 20, 2025 15:13

pditommaso added 2 commits January 21, 2025 17:53

Merge branch 'master' into fsn-13/fetch-fusion-license-token-from-pla…

2f2923f

…tform-and-pass-it-to-fusion-using-envvar

Minor changes

073ebde

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

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 commented Dec 18, 2024

pditommaso left a 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

stefanoboriero Dec 20, 2024

alberto-miranda Dec 20, 2024

pditommaso Jan 21, 2025

alberto-miranda commented Dec 20, 2024 •

edited

Loading

alberto-miranda commented Dec 20, 2024 •

edited

Loading

pditommaso commented Jan 21, 2025

jordeu commented Jan 21, 2025

[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

pditommaso left a 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

stefanoboriero Dec 20, 2024

Choose a reason for hiding this comment

alberto-miranda Dec 20, 2024

Choose a reason for hiding this comment

pditommaso Jan 21, 2025

Choose a reason for hiding this comment

alberto-miranda commented Dec 20, 2024 • edited Loading

alberto-miranda commented Dec 20, 2024 • edited Loading

pditommaso commented Jan 21, 2025

jordeu commented Jan 21, 2025

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 commented Dec 18, 2024 •

edited

Loading

alberto-miranda commented Dec 20, 2024 •

edited

Loading

alberto-miranda commented Dec 20, 2024 •

edited

Loading