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.

Sign up for GitHub

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

Jump to bottom

Add additional info for secret drivers #2738

Merged
merged 1 commit into from
Oct 31, 2018
Merged

Add additional info for secret drivers #2738

merged 1 commit into from
Oct 31, 2018

Conversation

sirlatrom
Copy link
Contributor

@sirlatrom sirlatrom commented Sep 4, 2018 •
edited
Loading

- What I did
I added the following fields to the type SecretsProviderRequest:

  • SecretLabels
  • ServiceID
  • TaskID
  • TaskName
  • TaskImage
  • NodeID

This provides more context for the secret driver when it is requested the value for the secret. It is useful both for audit purposes, e.g. an external system (such as HashiCorp Vault) logging which task requested what secret, as well as in a scenario where the plugin would return a different value (or error) based on e.g. labels on the secret.

- How I did it
I added the fields to the type and to the struct when it is built for the request to be made.

- How to test it
Write a plugin that makes use of the new fields. This would be made easier if docker/go-plugins-helpers#111 were merged.

- Description for the changelog

Provide additional secret, task and node context for secret driver plugins.

@sirlatrom
Copy link
Contributor Author

Could it be that the CI integration test is not related to this PR?

@sirlatrom sirlatrom force-pushed the additional-info-for-secret-drivers branch from 9448225 to 1bb52fa Compare September 6, 2018 09:37
@codecov
Copy link

codecov bot commented Sep 6, 2018 •
edited
Loading

Codecov Report

Merging #2738 into master will increase coverage by 0.16%.
The diff coverage is n/a.

@@            Coverage Diff            @@
##           master   #2738      +/-   ##
=========================================
+ Coverage   61.84%     62%   +0.16%     
=========================================
  Files         137     137              
  Lines       22047   22047              
=========================================
+ Hits        13634   13670      +36     
+ Misses       6945    6898      -47     
- Partials     1468    1479      +11

@sirlatrom
Copy link
Contributor Author

I added TaskImage to the struct as well

@sirlatrom sirlatrom mentioned this pull request Sep 10, 2018
Merged
@sirlatrom sirlatrom force-pushed the additional-info-for-secret-drivers branch from 1bb52fa to 4369539 Compare September 10, 2018 15:06
@sirlatrom
Copy link
Contributor Author

With docker/go-plugins-helpers#111 now merged, here's an example plugin that would use the additional info: https://gitlab.com/sirlatrom/docker-secretprovider-plugin-vault

@sirlatrom
Copy link
Contributor Author

sirlatrom commented Sep 10, 2018 •
edited
Loading

When this PR is merged, I can move on with integrating it in my plugin at https://gitlab.com/sirlatrom/docker-secretprovider-plugin-vault/merge_requests/4

@sirlatrom sirlatrom force-pushed the additional-info-for-secret-drivers branch 3 times, most recently from d80c1c1 to 69dea6e Compare September 16, 2018 22:25
@sirlatrom
Copy link
Contributor Author

@dperny Any chance you could have a look at this? It would greatly enhance secret drivers' ability to deliver a timely and well-informed response.

wk8
wk8 approved these changes Oct 11, 2018
View reviewed changes
Copy link
Contributor

@wk8 wk8 left a comment

Choose a reason for hiding this comment

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

LGTM!

@sirlatrom
Copy link
Contributor Author

@wk8 @dperny So could this be merged?

@sirlatrom sirlatrom force-pushed the additional-info-for-secret-drivers branch 4 times, most recently from 1fdde89 to a1b2f46 Compare October 30, 2018 16:27
@sirlatrom
Copy link
Contributor Author

I cannot reproduce the CI failure; seems to be flaky. I saw the same happening on #2735.

@sirlatrom
Copy link
Contributor Author

sirlatrom commented Oct 30, 2018 •
edited
Loading

@wk8 @dperny @anshulpundir Can either of you help to rebuild this PR and possibly have it merged? I managed to get it to pass by changing some code comments(!), so it's ready as seen from my perspective. It's rebased on origin/master as per today.

@olljanat
Copy link
Contributor

@sirlatrom you can btw trigger rebuild example by changing commit text a bit and using git push -f after that.

Failing test looks to be same than on #2559

@olljanat olljanat mentioned this pull request Oct 30, 2018
Open
@sirlatrom
Copy link
Contributor Author

@olljanat Yeah, thanks, I tried that a couple of times but ran out of creativity as for what I could mix up. What's funny is, the PR passed CI earlier, and I just wanted to rebase on master before pinging folks at Docker to merge.

I'll change some random text until it passes, I guess.

@sirlatrom sirlatrom force-pushed the additional-info-for-secret-drivers branch from a1b2f46 to 3d21b49 Compare October 31, 2018 10:37
@sirlatrom
Add additional info for secret drivers
537000a 
This provides more context for the secret driver when it is requested
the value for the secret. It is useful both for audit purposes, e.g. an
external system logging which task requested what secret, as well as
in a scenario where the plugin would return a different value (or
error) based on e.g. labels on the secret.

Signed-off-by: Sune Keller <absukl@almbrand.dk>
@sirlatrom sirlatrom force-pushed the additional-info-for-secret-drivers branch from 3d21b49 to 537000a Compare October 31, 2018 10:39
@sirlatrom
Copy link
Contributor Author

Yay, I won the CI lottery! 😅

@dperny
Copy link
Collaborator

dperny commented Oct 31, 2018

jackpot!

@dperny dperny merged commit 7b5833c into moby:master Oct 31, 2018
@sirlatrom sirlatrom deleted the additional-info-for-secret-drivers branch October 31, 2018 14:35
thaJeztah added a commit to thaJeztah/docker that referenced this pull request Nov 1, 2018
@thaJeztah
Bump SwarmKit to 8d8689d
be3843c 
Changes included;

- moby/swarmkit#2735 Assign secrets individually to each task
- moby/swarmkit#2759 Adding a new `Deallocator` component
- moby/swarmkit#2738 Add additional info for secret drivers
- moby/swarmkit#2775 Increase grpc max recv message size
  - addresses moby#37941
  - addresses moby#37997
  - follow-up to moby#38103

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
@thaJeztah thaJeztah mentioned this pull request Nov 1, 2018
Merged
docker-jenkins pushed a commit to docker-archive/docker-ce that referenced this pull request Nov 12, 2018
@thaJeztah
Bump SwarmKit to 8d8689d5a94ac42406883a4cef89b3a5eaec3d11
d5edbf8 
Changes included;

- moby/swarmkit#2735 Assign secrets individually to each task
- moby/swarmkit#2759 Adding a new `Deallocator` component
- moby/swarmkit#2738 Add additional info for secret drivers
- moby/swarmkit#2775 Increase grpc max recv message size
  - addresses moby/moby#37941
  - addresses moby/moby#37997
  - follow-up to moby/moby#38103

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Upstream-commit: be3843c8c8fb30b4a604dae9d0dad3d393db717c
Component: engine
adhulipa pushed a commit to adhulipa/docker that referenced this pull request Apr 11, 2019
@thaJeztah @adhulipa
Bump SwarmKit to 8d8689d
51030ce 
Changes included;

- moby/swarmkit#2735 Assign secrets individually to each task
- moby/swarmkit#2759 Adding a new `Deallocator` component
- moby/swarmkit#2738 Add additional info for secret drivers
- moby/swarmkit#2775 Increase grpc max recv message size
  - addresses moby#37941
  - addresses moby#37997
  - follow-up to moby#38103

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@wk8 wk8 wk8 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@sirlatrom @olljanat @dperny @wk8