-
Notifications
You must be signed in to change notification settings - Fork 5.6k
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
feat(secretstores): Add Docker Secrets #13035
Conversation
This feature adds the secret-store for Docker Secrets to Telegraf. This PR is a resultant effort based on discussion for a feature-request in influxdata#12848. Upon acceptance this PR closes influxdata#12848 Signed-off-by: Shantanoo 'Shan' Desai <shantanoo.desai@gmail.com>
Signed-off-by: Shantanoo 'Shan' Desai <shantanoo.desai@gmail.com>
Thanks so much for the pull request! |
TestsOne of the major concern with introducing tests for I would like some guidance on how to proceed with testing this plugin which is inherently meant to be run in a docker container. Is there some provision to achieve the testing for |
!signed-cla |
Hi, there is the |
I think |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Even though this is still marked as a draft, this PR looks quite solid. I added some comments and would love to see a basic test with a dummy directory to get some more test coverage.
- add `path` variable to the plugin to cover better test coverage This `path` variable mentioned in the `sample.conf` comes with a note for the current docker compose where the value is default to `/run/secrets` - refactor `docker.go` to address review points and improve the error messages - add `docker_test.go` to conduct Tests for `List`, `Get`, `Resolver` and `Set`. These tests rely heavily on the Jose Plugin since the core functionality is similar since the introduction of `path` variable in the configuration Signed-off-by: Shantanoo 'Shan' Desai <shantanoo.desai@gmail.com>
Signed-off-by: Shantanoo 'Shan' Desai <shantanoo.desai@gmail.com>
- set the default path to `/run/secrets` if no other path is explicitly mentioned. - Improve the error handling for files / directory by return explicity what is the reason as opposed to generic error statements - update test coverage by removing the path error checks, test for a non-existent path and check for substring in non-existent resolution Signed-off-by: Shantanoo 'Shan' Desai <shantanoo.desai@gmail.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @shantanoo-desai for the awesome plugin. A few comments from my side...
- add `docker` in the main README.md for secretstore plugins - add `dynamic` configuration for dynamic secrets in `sample.conf` - add additional example Compose file in README.md to provide a usage scenario with plugin - add some additional links for Docker Secrets Signed-off-by: Shantanoo 'Shan' Desai <shantanoo.desai@gmail.com>
- add `dynamic` configuration for dynamic secrets - provide checks for directory traversal errors - adapt resolver to use dynamic configuration value instead of hard-coded boolean value Signed-off-by: Shantanoo 'Shan' Desai <shantanoo.desai@gmail.com>
- add `testdata` directory that contains some dummy secret files - better error handling using `Error.Contains` as opposed to simply `Error` wherever possible - set `testdir` variable within Test cases with absolute path to `testdata` directory - conduct separate `TestSetNotAvailable` case Signed-off-by: Shantanoo 'Shan' Desai <shantanoo.desai@gmail.com>
Signed-off-by: Shantanoo 'Shan' Desai <shantanoo.desai@gmail.com>
Change Summary
|
Local TestPerform tests for plugin with a local docker container using the following files:
|
Signed-off-by: Shantanoo 'Shan' Desai <shantanoo.desai@gmail.com>
it is essential to let the container run with `user` parameter in the compose file since `/run/` directory is accessible only when the user within the container is `root`. By providing the `user` value which is typically the user id (via `id -u`), telegraf is able to read the values Signed-off-by: Shantanoo 'Shan' Desai <shantanoo.desai@gmail.com>
Signed-off-by: Shantanoo 'Shan' Desai <shantanoo.desai@gmail.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@shantanoo-desai thanks for the update, we are almost there... Just have one comment regarding the error handling in Init
.
- Catch all the errors in order to provide a plausible explanation as to why the plugin failed to access Signed-off-by: Shantanoo 'Shan' Desai <shantanoo.desai@gmail.com>
Download PR build artifacts for linux_amd64.tar.gz, darwin_amd64.tar.gz, and windows_amd64.zip. 📦 Click here to get additional PR build artifactsArtifact URLs |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perfect. Thanks a lot @shantanoo-desai for the nice contribution!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for taking the time to both propose this and work through the process of creating a PR! This will go out in v1.27.0 in June.
This feature adds the secret-store for Docker Secrets to Telegraf. This PR is a resultant effort based on discussion for a feature-request in #12848.
In a nutshell, Docker can inject secrets to a container during boot into files under
/run/secrets/<secret_file_x>
depending on the configuration mentioned within adocker-compose.yml
file. This plugin uses these _injected_f files to read credentials and passes them to the respective resolver strings wherever they are mentioned in plugins.Changes
Following files are introduced with this PR, which complies with other secret-store plugins file structure:
plugins/secretstores/all/docker.go
plugins/secretstores/docker/docker.go
plugins/secretstores/docker/docker_test.go
plugins/secretstores/docker/README.md
plugins/secretstores/docker/sample.conf
Required for all PRs
resolves #12848