Feature: Send secrets to Secrets-Store-Driver ("write_secrets" bool flag) #89
Conversation
|
|
There was a problem hiding this comment.
Thanks for the contribution! Generally looks pretty good to me so far, and thanks for the bug fix in there.
There is some unfortunate flakiness in the integration tests that I've had trouble reproducing outside of CI, although the failure in this PR looks systematic and likely related to the CSI driver version update. Line 182 in provider.bats expects two owners, and I think we may now expect 1 due to kubernetes-sigs/secrets-store-csi-driver#493, although I haven't fully dug into that.
Feel free to self-review on GitHub to start off discussions about specific questions, now that the PR's open that may be a better workflow for you than the code comments.
Some of my comments are suggesting slightly broader improvement as well, LMK what you think.
In terms of things we should add before landing:
- Some test coverage - as discussed offline this may be a little tricky and we can sync some more about the approach
- A plan for documentation - I think we should probably add a page to https://www.vaultproject.io/docs/platform/k8s/csi/ for command line flags at this stage, I can help you sort that with a PR on the hashicorp/vault repo.
- A changelog note
|
Added some of the recommended changes, as well as tried to isolate the problem from the breaking test (test 2 in bats) Changes:
Appreciate the feedback |
There was a problem hiding this comment.
Thanks for the updates, the code is looking good, just a few minor follow up suggestions. I think the biggest thing left to address is tests. I need to give that some more thought on how best to cover this feature while not abandoning the old code path, but I'm open to suggestions.
| // MountSecretsStoreObjectContent mounts content of the vault object to target path | ||
| func (p *provider) MountSecretsStoreObjectContent(ctx context.Context, cfg config.Config, writeSecrets bool) (*MountSecretsStoreObjectContentResponse, error) { | ||
| func (p *provider) MountSecretsStoreObjectContent(ctx context.Context, cfg config.Config, writeSecrets bool) (*pb.MountResponse, error) { | ||
| versions := make(map[string]string) |
There was a problem hiding this comment.
May as well make this of type []*pb.ObjectVersion and remove ov now that versions and ov both get created in the same function.
Which issue does this PR address?
#88
What does this feature/enhancement do?
By setting the
write_secretsflag tofalse, the provider sends secrets to the Secrets-Store-Driver instead of writing to file system.Note: You will see that I've commented some code out where I made changes, as well as a "change here" comment in several places. I intend to remove these once I have received a review and the changes are ready for merging. I will also squash the changes into one commit.
I did want an opinion on
MountSecretsStoreObjectContentResponsestruct I made, I was wondering if there were better practices I could be following there. I was also wondering if I should upgrade the version of Go to1.16.3?