Skip to content

Fix plugin secrets in config #6249

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Conversation

bentsherman
Copy link
Member

Fix #5494 #5943

This PR fixes support for plugin secrets (i.e. AWS secrets) in the config by loading the config before resolving plugins. If the config attempts to use secrets, it will be reloaded with the complete set of secrets providers.

This is possible because plugin config must be static, unlike other config which can be dynamic and reference secrets. Therefore, we can simply load the config to extract the list of plugins (and other config settings like tower.enabled and process.executor which can trigger additional core plugins.

To minimize the impact of loading the config twice, we only reload the config if we detect that secrets were used by the config on the first load. This way, the double config load should only affect users who want to use secrets in the config.

In summary:

  1. Load config with "shim" secrets provider which only tracks whether secrets are used by config
  2. Load plugins
  3. Load secrets provider
  4. Reload config if secrets were used in config

Copy link

netlify bot commented Jul 3, 2025

Deploy Preview for nextflow-docs-staging ready!

Name Link
🔨 Latest commit dd8a536
🔍 Latest deploy log https://app.netlify.com/projects/nextflow-docs-staging/deploys/68768fc6dfd0940008542c53
😎 Deploy Preview https://deploy-preview-6249--nextflow-docs-staging.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

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

@bentsherman bentsherman linked an issue Jul 3, 2025 that may be closed by this pull request
@bentsherman
Copy link
Member Author

Here is how I've been testing:

  1. Build with make pack
  2. Upload dist binary to s3
  3. Launch this pipeline in platform: https://github.com/bentsherman/nf-tests/tree/plugin-secrets
  4. Download dist binary from s3 and replace default nextflow in pre-run script

However, it's not loading the xpack-amzn plugin:

Jul-03 19:06:49.448 [main] DEBUG nextflow.plugin.PluginsFacade - Plugins resolved requirement=[nf-tower@1.12.0, nf-amazon@2.15.0, nf-wave@1.13.0, nf-cloudcache@0.4.3]

It's at least loading nf-amazon since the workDir is s3, but I can't remember how to trigger the xpack plugin

@bentsherman
Copy link
Member Author

I wonder if the issue is that I'm using the dist release instead of the standard release

@pditommaso pditommaso marked this pull request as draft July 6, 2025 13:34
Copy link
Member

@pditommaso pditommaso left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks smart. Moving to draft until the problem with the plugins is addressed. Also unit test and integration test should be added (it may be convenience setup a test GH repo for this)

@bentsherman bentsherman marked this pull request as ready for review July 7, 2025 16:49
@bentsherman
Copy link
Member Author

It turns out the issue was that I was using the single-VM CE, which doesn't support AWS secrets. It works with AWS Batch.

Regarding testing, I don't think we'll need a git repo unless you want to do an e2e test with platform. I'll add a local e2e test that just tests the detection of config secrets -> config reloading.

@bentsherman
Copy link
Member Author

One minor issue with this double-loading approach is that you can't use secrets in a config include (#5312). On the first load, the secret will be null and the include will likely fail, which means the run will fail

However, you should be able to work around this using a dynamic include:

includeConfig secrets.GITHUB_TOKEN
    ? "https://raw.githubusercontent.com/markpanganiban/nf-test/master/ubuntu.config?token=${secrets.GITHUB_TOKEN}"
    : '/dev/null'

On the first load, it will access the secret, triggering a reload, but won't attempt to include the secured config. On the second load it will include the config.

I will add this to the docs on config secrets

@bentsherman bentsherman force-pushed the fix-plugin-secrets-config branch from e8f25b9 to 5d5ef8a Compare July 7, 2025 17:39
@bentsherman bentsherman requested a review from a team as a code owner July 7, 2025 17:39
Signed-off-by: Ben Sherman <bentshermann@gmail.com>
@bentsherman bentsherman force-pushed the fix-plugin-secrets-config branch from 5d5ef8a to f03a097 Compare July 10, 2025 17:18
@bentsherman
Copy link
Member Author

The e2e test that I added is failing in GitHub, but not sure why because it works for me locally

@bentsherman bentsherman linked an issue Jul 10, 2025 that may be closed by this pull request
@bentsherman bentsherman requested a review from pditommaso July 14, 2025 14:12
@bentsherman bentsherman added this to the 25.10 milestone Jul 14, 2025
@pditommaso
Copy link
Member

For me does not work locally

» cd tests/checks
» b qrun.sh config-secrets.nf/
> Running test: config-secrets.nf

~ Test 'config-secrets.nf' run failed
   + set -e
   + /Users/pditommaso/Projects/nextflow/launch.sh secrets set MY_SECRET hello-world
   + /Users/pditommaso/Projects/nextflow/launch.sh -q run ../../config-secrets.nf -c ../../config-secrets.config
   + tee stdout
   + grep 'Config file used secrets -- reloading config with secrets provider'
   + false

Jul-15 17:54:46.610 [main] DEBUG nextflow.cli.Launcher - $> /Users/pditommaso/Projects/nextflow/launch.sh -q run ../../config-secrets.nf -c ../../config-secrets.config
Jul-15 17:54:46.634 [main] INFO  nextflow.cli.CmdRun - N E X T F L O W  ~  version 25.06.0-edge
Jul-15 17:54:46.646 [main] DEBUG nextflow.plugin.PluginsFacade - Setting up plugin manager > mode=dev; embedded=false; plugins-dir=/Users/pditommaso/Projects/nextflow/plugins; core-plugins: nf-amazon@3.0.0,nf-azure@1.18.0,nf-cloudcache@0.4.3,nf-codecommit@0.2.3,nf-console@1.2.1,nf-google@1.22.1,nf-k8s@1.1.0,nf-tower@1.13.0,nf-wave@1.14.0

Signed-off-by: Ben Sherman <bentshermann@gmail.com>
@bentsherman
Copy link
Member Author

I see now, I wasn't testing it correctly on local. The e2e test runs with -q which suppresses log.info but not println. Updated the testing docs

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants