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 support for returning ACL secret IDs for accessors with acl:write #10546

Merged
merged 7 commits into from
Jul 8, 2021
Merged

Add support for returning ACL secret IDs for accessors with acl:write #10546

merged 7 commits into from
Jul 8, 2021

Conversation

eculver
Copy link
Contributor

@eculver eculver commented Jul 1, 2021

In order to support copying of secret IDs from the UI, we need to include them in the API response with the caveat of when the accessor has only has acl:read in which case the redacted secret ID will be returned instead.

@eculver eculver requested a review from a team July 1, 2021 21:38
@github-actions github-actions bot added theme/api Relating to the HTTP API interface type/docs Documentation needs to be created/updated/clarified labels Jul 1, 2021
@hashicorp-ci
Copy link
Contributor

🤔 This PR has changes in the website/ directory but does not have a type/docs-cherrypick label. If the changes are for the next version, this can be ignored. If they are updates to current docs, attach the label to auto cherrypick to the stable-website branch after merging.

@vercel vercel bot temporarily deployed to Preview – consul July 1, 2021 21:41 Inactive
@vercel vercel bot temporarily deployed to Preview – consul-ui-staging July 1, 2021 21:41 Inactive
@vercel vercel bot temporarily deployed to Preview – consul July 1, 2021 21:58 Inactive
@vercel vercel bot temporarily deployed to Preview – consul-ui-staging July 1, 2021 21:58 Inactive
mkeeler
mkeeler reviewed Jul 6, 2021
View reviewed changes
Copy link
Member

@mkeeler mkeeler left a comment

Choose a reason for hiding this comment

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

Functionally this looks to do exactly what it set out to and is well done. There is one small bit missing from the implementation to get the pretty cli output format to show the secrets for the consul acl token list command but otherwise things look great.

I started to write a small bit about whether we need the ACLTokenListStub type anymore since it holds pretty much everything the full ACLToken does. While that is true I am not sure we could just get rid of the type and have the ACL.TokenList RPC return a []ACLToken instead due to upgrade concerns. It might be possible but I am not positive.

As a side note I noticed a bug that I think I introduced a while back. Essentially the ACLAuthMethodEnterpriseMeta field from the token isn't being propagated to the stub. Not something to fix in this PR but its one of the things that got me thinking we would be better off without the dual types.

command/acl/token/formatter_test.go Show resolved Hide resolved
@mkeeler
Copy link
Member

mkeeler commented Jul 6, 2021

The issue for the extra bug in the token list stub conversion is here: #10557

@eculver
Copy link
Contributor Author

eculver commented Jul 6, 2021

The issue for the extra bug in the token list stub conversion is here: #10557

Ok, I can take a stab it this next.

@vercel vercel bot temporarily deployed to Preview – consul July 6, 2021 17:46 Inactive
@vercel vercel bot temporarily deployed to Preview – consul-ui-staging July 6, 2021 17:46 Inactive
mkeeler
mkeeler approved these changes Jul 6, 2021
View reviewed changes
Copy link
Member

@mkeeler mkeeler left a comment

Choose a reason for hiding this comment

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

LGTM :shipit:

@eculver eculver merged commit 13bd865 into main Jul 8, 2021
@eculver eculver deleted the include-acl-secret-ids branch July 8, 2021 22:13
@hc-github-team-consul-core
Copy link
Collaborator

🍒 If backport labels were added before merging, cherry-picking will start automatically.

To retroactively trigger a backport after merging, add backport labels and re-run https://circleci.com/gh/hashicorp/consul/406153.

@hc-github-team-consul-core
Copy link
Collaborator

🍒✅ Cherry pick of commit 13bd865 onto release/1.10.x succeeded!

hc-github-team-consul-core pushed a commit that referenced this pull request Jul 8, 2021
@eculver @hc-github-team-consul-core
Add support for returning ACL secret IDs for accessors with acl:write (…
940419a 
…#10546)
@kaxcode kaxcode mentioned this pull request Jul 30, 2021
Merged
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mkeeler mkeeler mkeeler approved these changes

@freddygv freddygv freddygv approved these changes

Assignees
No one assigned
Labels
theme/api Relating to the HTTP API interface type/docs Documentation needs to be created/updated/clarified
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@eculver @hashicorp-ci @mkeeler @hc-github-team-consul-core @freddygv