feat(authz): ensure logging parity between authz v2 and v1

[jakedoublev]

Proposed Changes

• WIP: audit logs

Checklist

• I have added or updated unit tests
• I have added or updated integration tests (if appropriate)
• I have added or updated documentation

Testing Instructions

[gemini-code-assist]

Hello @jakedoublev, I'm Gemini Code Assist¹! 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!

Summary of Changes

Hello! Gemini-code-assist has analyzed this pull request. The primary goal, as indicated by the title and description, is to achieve logging parity between the new authorization v2 code and the existing v1 implementation. While the description mentions "audit logs" as a work in progress, the current changes focus on refining the debug logging within the authorization evaluation logic. This includes standardizing logger variable names, ensuring the correct logger with context is passed down through function calls, and adding specific debug logs for the hierarchy rule evaluation.

Highlights

• Standardize Logger Variable: Renamed the logger parameter to l in several functions (getResourceDecision, evaluateResourceAttributeValues) for consistency across the v2 access package.
• Correct Logger Propagation: Ensured the correct logger instance (l which likely carries request-specific context) is passed down to helper functions like populateHigherValuesIfHierarchy and getResourceDecision within the PolicyDecisionPoint evaluation flow.
• Add Debug Logging: Added a new debug log statement within the hierarchyRule function to provide visibility when the rule is satisfied, including details about the entitled attribute value and the highest hierarchy resource value involved.
• Remove Redundant Log Context: Removed the action name from the logger context in the evaluateDefinition function, potentially to avoid duplication or simplify the log output for this specific context.

Changelog

• service/internal/access/v2/evaluate.go
  • Renamed the logger parameter to l in the getResourceDecision function signature and its usages.
  • Renamed the logger parameter to l in the evaluateResourceAttributeValues function call within getResourceDecision.
  • Removed the line adding the action name to the logger context in evaluateDefinition.
  • Updated the hierarchyRule function signature to accept context.Context and *logger.Logger.
  • Introduced a local variable attrValues for attrDefinition.GetValues() in hierarchyRule for slightly cleaner code.
  • Added a debug log statement within hierarchyRule when an entitlement is found, detailing the entitled value and the relevant resource value.
• service/internal/access/v2/pdp.go
  • Changed the call to populateHigherValuesIfHierarchy to pass the local logger variable l instead of p.logger.
  • Changed the call to getResourceDecision to pass the local logger variable l instead of p.logger.

Using Gemini Code Assist

The 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 in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.

Feature	Command	Description
Code Review	/gemini review	Performs a code review for the current pull request in its current state.
Pull Request Summary	/gemini summary	Provides a summary of the current pull request in its current state.
Comment	@gemini-code-assist	Responds in comments when explicitly tagged, both in issue comments and review comments.
Help	/gemini help	Displays a list of available commands.

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 .gemini/ folder in the base of the repository. Detailed instructions can be found here.

Limitations & Feedback

Gemini Code Assist is currently in preview and 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 to provide feedback.

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.

---

Code changes flow,
Logs reveal the path they take,
Parity achieved.

Footnotes

1. Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩

[gemini-code-assist]

Code Review

This pull request aims to ensure logging parity between authz v2 and v1, which is a valuable goal for consistency and debuggability. The code changes themselves, such as renaming logger variables for brevity (e.g., logger to l), removing redundant log field initializations, and adding new debug logs in hierarchyRule, appear to be reasonable and align with Go best practices.

However, there are a few points to consider before this PR is ready for merge:

1. WIP Status: The PR description includes "WIP: audit logs". Could you please clarify the current status of this work? Is this PR a part of that WIP, or is the audit log work separate and subsequent?
2. Checklist Completion: The PR checklist for tests (unit and integration) and documentation is currently unchecked. For changes related to logging parity and potentially audit logs, having corresponding tests is crucial to verify correctness and prevent regressions. Documentation updates would also be beneficial for maintainability.

Given these points, especially the WIP status and the incomplete checklist, I recommend addressing these before merging.

Summary of Findings

• PR Readiness: The pull request is marked as 'WIP' in its description, and the standard checklist items (unit tests, integration tests, documentation) are not yet completed. These aspects are important for ensuring the quality, correctness (especially for logging parity), and maintainability of the changes.
• Code Changes Analysis: The specific code modifications in the diff are generally positive:
• Renaming logger to l in service/internal/access/v2/evaluate.go is idiomatic Go.
• Removing the redundant l.With("action", action.GetName()) in evaluateDefinition is a good cleanup as the action context is already present in the logger.
• The new debug logging in hierarchyRule enhances debuggability.
• Using the more specific contextual logger l instead of the general p.logger in service/internal/access/v2/pdp.go is a good practice.

Merge Readiness

While the code changes themselves are generally sound and improve logging, the pull request is not yet ready for merging. The 'WIP' status mentioned in the description needs clarification, and more importantly, the checklist items for adding/updating unit tests, integration tests, and documentation should be completed. These are particularly important for a feature focused on 'logging parity' and potentially related to 'audit logs' to ensure correctness and maintainability. I am unable to approve pull requests, but I recommend that these points be addressed, and the code be reviewed by others before it is merged.

[github-actions]

Benchmark results, click to expand

Benchmark authorization.GetDecisions Results:

Metric	Value
Approved Decision Requests	5000
Denied Decision Requests	0
Total Time	458.331946ms

Benchmark authorization.v2.GetMultiResourceDecision Results:

Metric	Value
Approved Decision Requests	5000
Denied Decision Requests	0
Total Time	271.405419ms

Bulk Benchmark Results

Metric	Value
Total Decrypts	100
Successful Decrypts	100
Failed Decrypts	0
Total Time	347.03651ms
Throughput	288.15 requests/second

TDF3 Benchmark Results:

Metric	Value
Total Requests	5000
Successful Requests	5000
Failed Requests	0
Concurrent Requests	50
Total Time	37.176563516s
Average Latency	369.817451ms
Throughput	134.49 requests/second

NANOTDF Benchmark Results:

Metric	Value
Total Requests	5000
Successful Requests	5000
Failed Requests	0
Concurrent Requests	50
Total Time	25.923512796s
Average Latency	258.018107ms
Throughput	192.88 requests/second

Standard Benchmark Metrics Skipped or Failed

[jakedoublev]

OBE