Skip to content
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

Iteration on Nomad/Variables <-> Nextflow/Secrets #75

Merged
merged 14 commits into from
Jul 29, 2024
Merged

Conversation

abhi18av
Copy link
Member

@abhi18av abhi18av commented Jul 26, 2024

Use Nomad Variables as Secrets

A user can enable/disable the use of Nomad Variables as Secrets. Nomad Variables are configuration stored in the cluster and used as templates in the job definition. In this way the definition doesn't reveal the values of them

this PR implements a NomadSecretProvider to act as bridge between processes/workflows and the cluster

Also it implements several commands to get/set/delete/list variables (if the token has enough access)

The majority of design level discussion happened on the #73

@abhi18av abhi18av assigned abhi18av and jagedn and unassigned abhi18av Jul 26, 2024
Copy link
Member Author

@abhi18av abhi18av left a comment

Choose a reason for hiding this comment

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

@jagedn, I've added a comment after a test with the nf-nomad secrets at the config level.

Don't think is needs to be a priority but something nice to have and we can address this in the next iteration - whatever you feel best 👍

Comment on lines +5 to +6
secret 'MY_ACCESS_KEY'
secret 'MY_SECRET_KEY'
Copy link
Member Author

@abhi18av abhi18av Jul 26, 2024

Choose a reason for hiding this comment

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

As of tweak the sun-nomadlab config to accommodate variables [ci skip] , the directive level usage of secrets works as expected.

However, I suspect we might need to extend the SecretsLoader class from Nextflow to be able to use nomad variables as secrets in configuration files.

I think we don't need to accommodate multiple sources i.e. local + nomad in the plugin and can switch completely to nomad variables for all secrets.

That is, we can add some words in our plugin level vocabulary like homogenous vs heterogenous secret store.

For example, here's a config file relying purely upon the nomad variables.


#NOTE This is stored in sun-nomadlab cluster
aws {
    accessKey = secrets.SUN_NOMADLAB_MINIO_ACCESS_KEY
    secretKey = secrets.SUN_NOMADLAB_MINIO_SECRET_KEY
    client {
        endpoint = 'http://100.119.165.23:9000'
    }
}

Copy link
Collaborator

Choose a reason for hiding this comment

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

the problem is that current implementation/architecture of Nextflow core doesn't allow custom SecretsLoader to access configurations. This is why our SecretsLoader implementation is a "fake" implementation (it returns null in all get secret requests as all of them will be resolved as nomad templates)

also not sure can mix different SecretsLoader, as far I can see Nextflow expects only 1 active

Copy link
Member Author

Choose a reason for hiding this comment

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

Yup, for our purposes I don't particularly see any issues with the homogenous secret stores so that's all good.

I guess a true prod cluster, would always work in conjunction with the vault setup as @tomiles also mentioned.

Then, let's move ahead with the current implementation and testing for the next release v.0.1.2 ?

Copy link
Collaborator

@jagedn jagedn Jul 27, 2024

Choose a reason for hiding this comment

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

@abhi18av with last commit I think we cover your comment, let me know if it's not clear

Copy link
Member Author

Choose a reason for hiding this comment

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

Okay, so I tested the plugin as of ccb80bd and here are the report.

sun-nomadlab with local NF secrets store + job vars from Nomad

nextflow run -c sun-nomadlab/nextflow.config secrets/main.nf -w 's3://fusionfs/integration-test/work'

And this works as expected, by relying completely upon the following config

nomad.jobs.secrets.enable = false

aws {
// From local Nextflow secret store
    accessKey = secrets.SUN_NOMADLAB_ACCESS_KEY
    secretKey = secrets.SUN_NOMADLAB_SECRET_KEY

}

🔴 sun-nomadlab with Nomad secret store + job vars from Nomad

In this case, the problem is that

nomad.jobs.secrets.enable = true // OR false


aws {
// From Nomad variables secret store
      accessKey = secrets.SUN_NOMADLAB_MINIO_ACCESS_KEY
      secretKey = secrets.SUN_NOMADLAB_MINIO_SECRET_KEY

}

Which raises the following Error, related to unknown variable being used within aws scope.


ERROR ~ The Access Key Id you provided does not exist in our records. (Service: Amazon S3; Status Code: 403; Error Code: InvalidAccessKeyId; Request ID: 17E69CA81533D4ED; S3 Extended Request ID: dd9025bab4ad464b049177c95eb6ebf374d3b3fd1af9251148b658df7ac2e3e8; Proxy: null)

 -- Check '.nextflow.log' file for details

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes, I agree

scenario 3 probably will require some work by core team so .... let's go!!!

Copy link
Member Author

Choose a reason for hiding this comment

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

I think you can't use our SecretProvider (nor any other SecretProvider except LocalProvider) to solve secrets values in the config , at least with current version of Nexflow

Agreed.

So maybe we should revert/stash the changes related to the NomadSecrets implementation so that our plugin cmd commands can work again as expected. As of now, it seems to run into problems during plugin resolution 🤔

$ nextflow -c sun-nomadlab/nextflow.config plugin nf-nomad:secrets list       

Invalid target plugin: nf-nomad

Copy link
Collaborator

Choose a reason for hiding this comment

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

If I'm not wrong will be automagically resolved once we publish the version in the plugin repository

I mean, right now the plugin cmd is trying to use last version from repository and it doesnt implement cmd

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah, okay - then let's merge-tag-pr 😎

Copy link
Member Author

Choose a reason for hiding this comment

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

After the merge to plugins repo nextflow-io/plugins#83 (comment) , I can confirm that the nf-nomad:secrets commands are working as expected ✅

image

@abhi18av abhi18av mentioned this pull request Jul 26, 2024
16 tasks
jagedn added 2 commits July 27, 2024 11:22
if false use Local implementation

Signed-off-by: Jorge Aguilera <[email protected]>
Signed-off-by: Jorge Aguilera <[email protected]>
@jagedn jagedn marked this pull request as ready for review July 27, 2024 09:37
@jagedn jagedn self-requested a review July 27, 2024 09:38
@jagedn jagedn added the enhancement New feature or request label Jul 27, 2024
Channel.of('Bonjour', 'Ciao', 'Hello', 'Hola') | sayHello | view
}
workflow.onComplete {
println("The secret is: ${secrets.MY_ACCESS_KEY}")
Copy link
Collaborator

Choose a reason for hiding this comment

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

with current implementation we can access to nomad variables also in workflows!!!

Signed-off-by: Jorge Aguilera <[email protected]>
Comment on lines 9 to 11
this.enable = map.containsKey('enable') ? map.get('enable') as boolean : false
this.path = map.path ?: "secrets/nf-nomad"
}
Copy link
Member Author

Choose a reason for hiding this comment

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

@jagedn , I've renamed the enable to enabled in order to comply with the general pattern of enabling scopes as per Nextflow configuration

@abhi18av abhi18av merged commit e85940e into master Jul 29, 2024
@abhi18av abhi18av deleted the secrets-template branch July 29, 2024 08:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants