Fix plugin secrets in config

[bentsherman]

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

[netlify]

✅ Deploy Preview for nextflow-docs-staging canceled.

Name	Link
🔨 Latest commit	e732f34
🔍 Latest deploy log	https://app.netlify.com/projects/nextflow-docs-staging/deploys/68c6ea5df16b300008d877ee

[bentsherman]

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]

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

[pditommaso]

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]

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]

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

[christopher-hakkaart]

Some very minor suggestions. Feel free to ignore if the meaning changes.

[pditommaso]

@bentsherman can you have a look at the conflicts? I've tried but not sure how address them

[pditommaso]

@claude review

[claude]

Claude finished @pditommaso's task —— View job

---

Code Review Analysis

I'll review this PR that implements plugin secrets support in config by introducing a two-stage config loading approach.

Todo List:

• Read and analyze core implementation files
• Review the ConfigNullProvider "shim" approach
• Analyze the two-stage config loading logic
• Check test coverage and integration tests
• Review documentation updates
• Provide comprehensive review feedback

Review Summary

This is a well-designed solution for a complex problem. The two-stage config loading approach using a "shim" secrets provider is elegant and minimizes performance impact.

✅ Strengths

1. Smart Detection Mechanism: The ConfigNullProvider elegantly tracks secret usage without requiring config parsing twice in most cases.

2. Minimal Performance Impact: Only reloads config when secrets are actually accessed, avoiding unnecessary double parsing.

3. Good Test Coverage: Includes both unit tests and integration tests that verify the end-to-end behavior.

4. Clear Documentation: Well-documented limitations and workarounds for edge cases like config includes.

5. Clean Implementation: The logic in CmdRun.groovy is clear and follows a logical sequence.

🔍 Areas for Consideration

1. Error Handling (CmdRun.groovy:352-361)

The config reload happens unconditionally if secrets were used. Consider adding error handling:

if( secretsProvider.usedSecrets() ) {
    try {
        log.debug "Config file used secrets -- reloading config with secrets provider"
        builder = new ConfigBuilder()...
    } catch (Exception e) {
        throw new AbortOperationException("Failed to reload config with secrets provider: ${e.message}", e)
    }
}

2. ConfigNullProvider Implementation (ConfigNullProvider.groovy:34-37)

The implementation is minimal but correct. The empty string return value aligns with the documented behavior. Consider adding debug logging:

@Override
Secret getSecret(String name) {
    log.debug "Config requested secret: ${name} (returning empty string)"
    accessed = true
    return new SecretImpl(name, '')
}

3. Documentation Enhancement (docs/secrets.md:58-74)

The documentation is comprehensive but could benefit from a concrete example showing the actual log message users should expect:

DEBUG nextflow.cli.CmdRun - Config file used secrets -- reloading config with secrets provider

4. Test Coverage Verification

The integration test at tests/checks/config-secrets.nf/.checks:8 properly verifies the log message, which is good for regression testing.

🚨 Potential Issues

1. Thread Safety

The ConfigNullProvider.accessed field is not thread-safe. While config loading is likely single-threaded, consider using AtomicBoolean:

private final AtomicBoolean accessed = new AtomicBoolean(false)

@Override
Secret getSecret(String name) {
    accessed.set(true)
    return new SecretImpl(name, '')
}

boolean usedSecrets() {
    return accessed.get()
}

2. Plugin Loading Order

The sequence in CmdRun.groovy:344-361 assumes plugins are loaded synchronously. If plugin loading becomes asynchronous in the future, this could break.

📋 Minor Suggestions

1. Consistent Naming: Consider renaming usedSecrets() to wasAccessed() for clarity.

2. Config Validation: The second config build doesn't validate like the first (line 367-368 only applies to the first build).

3. Memory Efficiency: The secretsProvider reference could be nulled after the check to help GC.

🎯 Overall Assessment

This is a solid implementation that solves a complex bootstrap problem elegantly. The approach is pragmatic and the performance considerations are well thought out. The test coverage is appropriate and the documentation clearly explains the limitations and workarounds.

Recommendation: ✅ Approve - The implementation is production-ready with only minor enhancement opportunities.

---

[pditommaso]

Ok, thread safety is not relevant because this code is not expected to run in parallel manner. Added more docs and renamed ConfigNullProvider to EmptySecretProvider to avoid confusing it with NullProvider

[robsyme]

This is great! A huge bonus is that now we can do includeConfig from remote sources, e.g.

plugins { id 'nf-amazon' }

// Force Phase 2 reload by accessing a secret (even if it doesn't exist)
params.dummy = secrets.DUMMY_SECRET ?: 'default'

// Use conditional to defer S3 access until Phase 2 when plugins are loaded
includeConfig nextflow.plugin.Plugins.getManager()
    ? 's3://my-bucket/path/to/nextflow.config'
    : '/dev/null'

Is there a more elegant way to force that second reload of configuration?

[bentsherman]

I was mainly thinking about config includes that reference a secret like the github token. If you don't actually need a secret then you'll need to use a dummy as in your example. But you can use that to simplify your include:

includeConfig secrets.DUMMY_SECRET
    ? 's3://my-bucket/path/to/nextflow.config'
    : '/dev/null'

Accessing internal classes like that probably shouldn't be allowed anyway

[robsyme]

This is great. We can store the S3 path in a workspace secret and then just have configuration:

includeConfig secrets.WORKSPACE_CONFIG ?: '/dev/null'