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

Expose secret_id_accessor as WrappedAccessor when wrapping secret-id creation. #12425

Merged
merged 6 commits into from Sep 16, 2021

Conversation

melbit-michaelw
Copy link
Contributor

Resolves #12173 for Secret-Ids.

I'm not a Go developer (nor indeed any kind of developer), and was unable to find the right place to add a test for this functionality. Could you please point me in the right direction as to where this should be tested ?

@hashicorp-cla
Copy link

hashicorp-cla commented Aug 25, 2021

CLA assistant check
All committers have signed the CLA.

@vercel vercel bot temporarily deployed to Preview – vault August 25, 2021 00:48 Inactive
@vercel vercel bot temporarily deployed to Preview – vault-storybook August 25, 2021 00:48 Inactive
@pmmukh
Copy link
Contributor

pmmukh commented Sep 8, 2021

Hi @melbit-michaelw ! Thanks for submitting this PR! For adding a test for this functionality, I think you are good to skip unit tests, as we don't really have unit tests on wrapping.go. For integration tests, I think adding a new folder for approle under https://github.com/hashicorp/vault/tree/main/vault/external_tests, and adding a test there that exercises setting up a wrapped secret and uses the accessor in the response like how hashicorp/terraform-provider-vault#976 plans to use it, would make sense.

vault/wrapping.go Outdated Show resolved Hide resolved
vault/wrapping.go Outdated Show resolved Hide resolved
@pmmukh
Copy link
Contributor

pmmukh commented Sep 8, 2021

Think this comment https://github.com/hashicorp/vault/blob/main/sdk/helper/wrapping/wrapinfo.go#L20-L21 might need to be updated, to reflect the dual nature of WrappedAccessor with this change.

@vercel vercel bot temporarily deployed to Preview – vault-storybook September 9, 2021 03:20 Inactive
@vercel vercel bot temporarily deployed to Preview – vault September 9, 2021 03:20 Inactive
@melbit-michaelw
Copy link
Contributor Author

Thank you very much for the feedback. I've done the minor fixes suggested, and will start looking at the integration testing.

@vercel vercel bot temporarily deployed to Preview – vault September 9, 2021 22:34 Inactive
@vercel vercel bot temporarily deployed to Preview – vault-storybook September 9, 2021 22:34 Inactive
@melbit-michaelw
Copy link
Contributor Author

@pmmukh - Added tests to confirm WrappedAccessor set as expected, and not set when wrapping isn't performed.

Let me know if anything else is required.

@pmmukh
Copy link
Contributor

pmmukh commented Sep 14, 2021

hey! Just wanted to drop a note that there's one little point we're hashing out internally on this change, after which I'll give this another pass sometime this week!

vault/wrapping.go Outdated Show resolved Hide resolved
@vercel vercel bot temporarily deployed to Preview – vault September 15, 2021 21:54 Inactive
@vercel vercel bot temporarily deployed to Preview – vault-storybook September 15, 2021 21:54 Inactive
@vercel vercel bot temporarily deployed to Preview – vault September 15, 2021 21:57 Inactive
@vercel vercel bot temporarily deployed to Preview – vault-storybook September 15, 2021 21:57 Inactive
Copy link
Contributor

@pmmukh pmmukh left a comment

Choose a reason for hiding this comment

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

lgtm! Thanks for the quick responses on the feedback!

@pmmukh pmmukh merged commit db8cc30 into hashicorp:main Sep 16, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Return wrapped accessor when creating wrapped AppRole secret-ids
4 participants