feat: enrich Check span with auth decision attributes - #670
Open
asanzgom wants to merge 2 commits into
Open
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe authorisation service now enriches OpenTelemetry spans with decision, response, denial, AuthConfig, and identity attributes. Tests verify these attributes for allowed, denied, unknown-service, and invalid requests. ChangesAuthentication trace enrichment
Estimated code review effort: 3 (Moderate) | ~20 minutes Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/service/auth_test.go`:
- Around line 408-548: Complete the span attribute assertions in the four tests:
add IdentityTypeAttr validation to TestCheckSpanAttributes_AllowedRequest,
AuthConfigNamespaceAttr validation to TestCheckSpanAttributes_DeniedRequest,
AuthResponseCodeAttr validation with the NOT_FOUND value to
TestCheckSpanAttributes_ServiceNotFound, and AuthDenialReasonAttr validation to
TestCheckSpanAttributes_InvalidRequest. Use the expected values emitted by each
path and preserve the existing assertions.
- Around line 390-393: Update the t.Cleanup callback in the tracer-provider test
setup to handle the error returned by tp.Shutdown, preserving restoration of
prev and ensuring shutdown failures are reported rather than ignored.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: db624617-f7df-44f9-8e18-0767f8697f95
📒 Files selected for processing (3)
pkg/service/auth.gopkg/service/auth_test.gopkg/trace/trace.go
Add domain-specific span attributes to the existing Check span so traces carry actionable auth metadata instead of only authorino.request_id. New attributes: authorino.auth.result (ALLOW/DENY), authorino.auth.response_code, authorino.auth.denial_reason, authorino.auth_config.name, authorino.auth_config.namespace, authorino.identity.source, and authorino.identity.type. Closes Kuadrant#669 Signed-off-by: Adrian Sanz Gomiz <asanzgom@redhat.com> Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Signed-off-by: Adrián Sanz Gómiz <asanzgom@redhat.com>
Add missing assertions to the four span attribute tests: IdentityTypeAttr in AllowedRequest, AuthConfigNamespaceAttr in DeniedRequest, AuthResponseCodeAttr in ServiceNotFound, and AuthDenialReasonAttr in InvalidRequest. Handle the error returned by TracerProvider.Shutdown in the test cleanup callback. Signed-off-by: Adrian Sanz Gomiz <asanzgom@redhat.com> Signed-off-by: Adrián Sanz Gómiz <asanzgom@redhat.com>
asanzgom
force-pushed
the
feat/check-span-enrichment
branch
from
August 11, 2026 13:35
774ec95 to
ea4a1f8
Compare
Comment on lines
+296
to
+299
| span.SetAttributes( | ||
| otel_attr.String(trace.AuthConfigNameAttr, authConfig.Labels["authconfig"]), | ||
| otel_attr.String(trace.AuthConfigNamespaceAttr, authConfig.Labels["namespace"]), | ||
| ) |
Collaborator
There was a problem hiding this comment.
I wonder if we should iterate over the AuthConfig labels and set them dynamically into the span, similarly to what we do with the metrics.
I understand the keys wouldn't be the same as proposed, but perhaps this is also an opportunity to standardise them across the different types of observability sources?
E.g.:
Suggested change
| span.SetAttributes( | |
| otel_attr.String(trace.AuthConfigNameAttr, authConfig.Labels["authconfig"]), | |
| otel_attr.String(trace.AuthConfigNamespaceAttr, authConfig.Labels["namespace"]), | |
| ) | |
| for k, v := range authConfig.Labels { | |
| span.SetAttributes(otel_attr.String(fmt.Sprintf("authorino.authconfig.%s", k), v)) | |
| } |
Comment on lines
+524
to
+534
| func setAuthResultSpanAttrs(span otel_trace.Span, result auth.AuthResult) { | ||
| if result.Success() { | ||
| span.SetAttributes(otel_attr.String(trace.AuthResultAttr, "ALLOW")) | ||
| } else { | ||
| span.SetAttributes(otel_attr.String(trace.AuthResultAttr, "DENY")) | ||
| if result.Message != "" { | ||
| span.SetAttributes(otel_attr.String(trace.AuthDenialReasonAttr, result.Message)) | ||
| } | ||
| } | ||
| span.SetAttributes(otel_attr.String(trace.AuthResponseCodeAttr, result.Code.String())) | ||
| } |
Collaborator
There was a problem hiding this comment.
This needs documentation at https://github.com/Kuadrant/authorino/blob/main/docs/user-guides/observability.md#data-plane-tracing.
Other than the new attributes, a couple of things that seem important to get covered IMO:
DENYis also used along with other GRPC response codes such asUNAVAILABLEandNOT_FOUND.- Note: either
ALLOWorDENY, the PEP can still behave otherwise. E.g.: a 50x that falls back to access granted, flipping the defaultfailure_mode_allow
- Note: either
result.Codeis the ext_authz GRPC response code, not the HTTP status code one may expect
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Enrich the existing
Checkspan with domain-specific auth decision attributes so traces carry actionable metadata for debugging auth flows in production.Before: Check span only had
authorino.request_idandguid:x-request-id.After: Check span includes auth result, denial reason, AuthConfig name/namespace, and resolved identity source/type.
New span attributes
authorino.auth.resultALLOW/DENYauthorino.auth.response_codeauthorino.auth.denial_reasonauthorino.auth_config.nameauthorino.auth_config.namespaceauthorino.identity.sourceauthorino.identity.typeUses the
authorino.*namespace, consistent with existingauthorino.request_id. OTel has no general-purpose auth semantic conventions (only framework-specificaspnetcore.*), so a custom namespace avoids future collisions.Attributes are set at every exit path in
Check()(invalid request, service not found, context timeout, pipeline result) so spans always carry the decision.Changes
pkg/trace/trace.go: Add 7 attribute name constantspkg/service/auth.go: Set span attributes at each return path inCheck(); addsetAuthResultSpanAttrshelperpkg/service/auth_test.go: 4 new tests verifying span attributes for allowed, denied, not-found, and invalid request scenariosRelated
Test plan
TestCheckSpanAttributes_*)go build ./pkg/service/compiles cleanlySummary by CodeRabbit