chore(core): Log names of registered trust.mgrs #2792
Conversation
This assists with debugging misspelled or forgotten mgrs
Summary of ChangesHello @dmihalcik-virtru, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This PR enhances the KAS service's logging to provide better visibility into registered Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces logging of registered trust.KeyManagers to assist with debugging misspelled or forgotten managers. The changes involve modifying kas.go to collect the names of registered key managers and log them, and updating delegating_key_service.go to include key manager names in debug logs when a key manager factory is not found. I have identified some opportunities to improve the code and have provided suggestions.
| factory, factoryExists := d.managerFactories[name] | ||
| // Read defaultMode under lock for comparison. | ||
| currentDefaultMode := d.defaultMode | ||
| allNames := slices.Collect(maps.Keys(d.managerFactories)) | ||
| d.mutex.Unlock() |
There was a problem hiding this comment.
Moving allNames outside the lock scope can lead to a race condition if d.managerFactories is modified between the collection of names and its use in the debug log. To avoid this, collect the names while holding the lock.
| factory, factoryExists := d.managerFactories[name] | |
| // Read defaultMode under lock for comparison. | |
| currentDefaultMode := d.defaultMode | |
| allNames := slices.Collect(maps.Keys(d.managerFactories)) | |
| d.mutex.Unlock() | |
| d.mutex.Unlock() | |
| currentDefaultMode := d.defaultMode | |
| // Collect key manager names while holding the lock. | |
| allNames := slices.Collect(maps.Keys(d.managerFactories)) |
| factory, factoryExists := d.managerFactories[name] | ||
| // Read defaultMode under lock for comparison. | ||
| currentDefaultMode := d.defaultMode | ||
| allNames := slices.Collect(maps.Keys(d.managerFactories)) |
There was a problem hiding this comment.
It would be more efficient to collect the names of the key managers once, outside of the lock, and then use that collected list here. This avoids holding the lock while collecting the names, improving performance.
| allNames := slices.Collect(maps.Keys(d.managerFactories)) | |
| d.mutex.Unlock() | |
| allNames := slices.Collect(maps.Keys(d.managerFactories)) |
| p.KeyDelegator.SetDefaultMode(inProcessService.Name()) | ||
| kmgrNames = append(kmgrNames, inProcessService.Name()) | ||
| } | ||
| srp.Logger.Info("kas registered trust.KeyManagers", slog.Any("key_managers", kmgrNames)) |
There was a problem hiding this comment.
Consider using a more structured log message. Instead of just logging the names, include additional context or metadata that might be helpful for debugging. For example, you could include the number of key managers registered or the types of key managers.
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Standard Benchmark Metrics Skipped or FailedBulk Benchmark Results
TDF3 Benchmark Results:
NANOTDF Benchmark Results:
|
1 participant
Proposed Changes
Checklist
Testing Instructions