-
Notifications
You must be signed in to change notification settings - Fork 4.4k
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
Add accessorID of token when ops are denied by ACL system #7117
Conversation
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.
Overall this looks great. I had handful of really minor comments or suggestions, as well as one slightly more important one about the new RPC and its usage in the agent local state.
Would it be possible to have tests for any of this? I have my doubts about whether it is feasible without major alterations to our logging and then whether its even desirable. Its probably not worth it but have you given it any consideration?
Co-Authored-By: Matt Keeler <[email protected]>
95dbaf7
to
2cc71f5
Compare
@mkeeler I believe I hit all of your feedback! Biggest change is that I extracted the code to grab the There's a decent amount of copy-paste repetition in this approach, but it didn't feel like a complex enough operation to pull up to a utility package/repeatable helper fn. As a result of this change, the call sites for the logs are cleaned up significantly, getting a value to work directly and not having to fuss w/ errors/nil behaviors unrelated to logging the accessorID. |
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.
LGTM. This is going to make debugging ACL issues much easier.
@pierresouchay you might like this :) Great job @mkcp 🎉 |
Co-Authored-By: R.B. Boyer <[email protected]>
The last unaddressed item from review is: how do we test logs, and perhaps other telemetry-type functionality that's off core functionality but still providers user value. My impression is that it seems a bit far off the critical path to warrant adding weight to the integration tests, and straightforward enough to see the results of that we'll hear back pretty fast from users if it's broken. And, given it's non-criticality, if it's broken it's not terribly destructive (e.g. dc downtime, lost business-critical records, etc.) I'm going to squash and merge here this PR without testing it, but I'm willing to continue the discussion and follow up with tests if we decide to go that route. |
Intended to resolve customer asks in #7010, plus wherever else I could find ACL debug logs with access to a token.
We employ a few different ways of getting the accessorID, depending on where we are in the agent.
agent/local/state.go
acquires it via internal RPC because local is a subcomponent of theagent
and we don't have access to one. Adding an additional network hop in theseservice
andcheck
sync loops might be prohibitively expensive on a critical path, but I haven't profiled it. I would appreciate feedback on my impl here from folks w/ more Go and consul experience.Then various delegate and srv calls to
ResolveIdentityFromToken
so we can pull the accessorID off of it.Added some doc comments early on when I was still researching the problem. Can split into its own PR if needed.
And important note is that none of this is particularly tested, because I couldn't find any place that felt like it needed unit testing (could be wrong there) and integration testing logging felt... bizarre? I also had some difficulty building up a test case to induce the logs themselves so much of what I'm submitting here is taken on faith, the type checker, and a few thousand
Test{Agent,ACL}
runs to ensure I didn't break something important.I would appreciate that whoever review this PR get about as nitpicky as possible. Treat this code like someone with 5 years of Go experience got a little {drunk/tired from a new baby/etc.} and rushed through this. I would like to learn as much as possible from whatever I missed. Thanks!
Will squash before merge.