Skip to content

fix(core): DSPX-4694 honor reflection configuration - #4021

Merged
jakedoublev merged 4 commits into
mainfrom
fix/dspx-4694-reflection-config
Sep 10, 2026
Merged

fix(core): DSPX-4694 honor reflection configuration#4021
jakedoublev merged 4 commits into
mainfrom
fix/dspx-4694-reflection-config

Conversation

@jakedoublev

@jakedoublev jakedoublev commented Sep 9, 2026

Copy link
Copy Markdown
Contributor

Honors server.grpc.reflectionEnabled for external handlers and omits reflection from the in-process mux.

Tested: make test HAND_MODS=service/internal/server

Summary by CodeRabbit

  • Configuration

    • gRPC reflection is now registered only when explicitly enabled.
    • Reflection handlers are no longer registered on the in-process server.
  • Bug Fixes

    • Prevents unintended exposure of reflection endpoints when reflection is disabled.

Signed-off-by: jakedoublev <jake.vanvorhis@virtru.com>
@coderabbitai

coderabbitai Bot commented Sep 9, 2026

Copy link
Copy Markdown

Review Change StackReview Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: ASSERTIVE

Plan: Advanced

Run ID: edf02d32-54f4-4622-8cf3-60c55b797dd8

📥 Commits

Reviewing files that changed from the base of the PR and between da8450b and e127a2c.

📒 Files selected for processing (2)
  • service/internal/server/server.go
  • service/internal/server/server_test.go

Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review.


📝 Walkthrough

Walkthrough

The server now conditionally registers gRPC reflection handlers. Enabled reflection registers V1 and V1Alpha handlers on the external mux only. Disabled reflection registers no reflection handlers. Tests cover both configurations.

Changes

gRPC reflection registration

Layer / File(s) Summary
Reflection configuration and registration flow
service/internal/server/server.go
OpenTDFServer stores the reflection setting from GRPCConfig. Start() calls conditional registration. Enabled reflection registers V1 and V1Alpha handlers on the external mux only.
Reflection registration tests
service/internal/server/server_test.go
Tests verify enabled registration on the external mux and no registration on the in-process mux. Disabled configuration registers no reflection handlers.

Priority: ⬇️ Low

Estimated code review effort: 2 (Simple) | ~10 minutes

Merge Risk: ⚪ Minimal · up to e127a

gRPC reflection now follows the configured setting: it is exposed externally only when enabled and is unavailable in-process. The change has focused coverage for both configuration states and presents no remaining merge-blocking risk.

Suggested reviewers: pflynn-virtru

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 8 functions across 2 files. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly identifies the main change: honoring the reflection configuration in the core server. It is concise and specific.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
  • Fix all pre-merge checks with AI
✨ Finishing Touches 💡 1
📝 Generate docstrings 💡
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/dspx-4694-reflection-config

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.

❤️ Share

A rabbit checks the mux at night
V1 and V1Alpha hop into sight
The inner path stays clear and still
Config decides the handler’s will
Tests guard every route with skill

Comment @coderabbitai help to get the list of available commands.

@github-actions github-actions Bot added the size/s label Sep 9, 2026
@github-actions

github-actions Bot commented Sep 9, 2026

Copy link
Copy Markdown
Contributor
Benchmark results, click to expand

Benchmark authorization.GetDecisions Results:

Metric Value
Approved Decision Requests 1000
Denied Decision Requests 0
Total Time 186.625781ms

Benchmark authorization.v2.GetMultiResourceDecision Results:

Metric Value
Approved Decision Requests 1000
Denied Decision Requests 0
Total Time 93.93854ms

Benchmark Statistics

Name № Requests Avg Duration Min Duration Max Duration

Bulk Benchmark Results

Metric Value
Total Decrypts 100
Successful Decrypts 100
Failed Decrypts 0
Total Time 347.752995ms
Throughput 287.56 requests/second

TDF3 Benchmark Results:

Metric Value
Total Requests 5000
Successful Requests 5000
Failed Requests 0
Concurrent Requests 50
Total Time 44.472697624s
Average Latency 443.585498ms
Throughput 112.43 requests/second

Signed-off-by: jakedoublev <jake.vanvorhis@virtru.com>
@github-actions

github-actions Bot commented Sep 9, 2026

Copy link
Copy Markdown
Contributor

Signed-off-by: jakedoublev <jake.vanvorhis@virtru.com>
@github-actions

github-actions Bot commented Sep 9, 2026

Copy link
Copy Markdown
Contributor

@github-actions

github-actions Bot commented Sep 9, 2026

Copy link
Copy Markdown
Contributor
Benchmark results, click to expand

Benchmark authorization.GetDecisions Results:

Metric Value
Approved Decision Requests 1000
Denied Decision Requests 0
Total Time 182.572131ms

Benchmark authorization.v2.GetMultiResourceDecision Results:

Metric Value
Approved Decision Requests 1000
Denied Decision Requests 0
Total Time 96.79521ms

Benchmark Statistics

Name № Requests Avg Duration Min Duration Max Duration

Bulk Benchmark Results

Metric Value
Total Decrypts 100
Successful Decrypts 100
Failed Decrypts 0
Total Time 335.231153ms
Throughput 298.30 requests/second

TDF3 Benchmark Results:

Metric Value
Total Requests 5000
Successful Requests 5000
Failed Requests 0
Concurrent Requests 50
Total Time 49.588527885s
Average Latency 495.01781ms
Throughput 100.83 requests/second

Signed-off-by: jakedoublev <jake.vanvorhis@virtru.com>
@github-actions

github-actions Bot commented Sep 9, 2026

Copy link
Copy Markdown
Contributor

@jakedoublev
jakedoublev marked this pull request as ready for review September 9, 2026 22:42
@jakedoublev
jakedoublev requested a review from a team as a code owner September 9, 2026 22:42
@github-actions

github-actions Bot commented Sep 9, 2026

Copy link
Copy Markdown
Contributor
Benchmark results, click to expand

Benchmark authorization.GetDecisions Results:

Metric Value
Approved Decision Requests 1000
Denied Decision Requests 0
Total Time 189.900173ms

Benchmark authorization.v2.GetMultiResourceDecision Results:

Metric Value
Approved Decision Requests 1000
Denied Decision Requests 0
Total Time 100.78233ms

Benchmark Statistics

Name № Requests Avg Duration Min Duration Max Duration

Bulk Benchmark Results

Metric Value
Total Decrypts 100
Successful Decrypts 100
Failed Decrypts 0
Total Time 355.401448ms
Throughput 281.37 requests/second

TDF3 Benchmark Results:

Metric Value
Total Requests 5000
Successful Requests 5000
Failed Requests 0
Concurrent Requests 50
Total Time 50.318028107s
Average Latency 502.056992ms
Throughput 99.37 requests/second

@github-actions

github-actions Bot commented Sep 9, 2026

Copy link
Copy Markdown
Contributor
Benchmark results, click to expand

Benchmark authorization.GetDecisions Results:

Metric Value
Approved Decision Requests 1000
Denied Decision Requests 0
Total Time 212.866802ms

Benchmark authorization.v2.GetMultiResourceDecision Results:

Metric Value
Approved Decision Requests 1000
Denied Decision Requests 0
Total Time 115.352678ms

Benchmark Statistics

Name № Requests Avg Duration Min Duration Max Duration

Bulk Benchmark Results

Metric Value
Total Decrypts 100
Successful Decrypts 100
Failed Decrypts 0
Total Time 436.176649ms
Throughput 229.26 requests/second

TDF3 Benchmark Results:

Metric Value
Total Requests 5000
Successful Requests 5000
Failed Requests 0
Concurrent Requests 50
Total Time 55.903547006s
Average Latency 557.783853ms
Throughput 89.44 requests/second

@github-actions

github-actions Bot commented Sep 9, 2026

Copy link
Copy Markdown
Contributor

⚠️ Govulncheck found vulnerabilities ⚠️

The following modules have known vulnerabilities:

  • otdfctl
  • service
  • tests-bdd

See the workflow run for details.

@jakedoublev
jakedoublev added this pull request to the merge queue Sep 10, 2026
Merged via the queue into main with commit 8743070 Sep 10, 2026
47 checks passed
@jakedoublev
jakedoublev deleted the fix/dspx-4694-reflection-config branch September 10, 2026 13:16
@jakedoublev jakedoublev added the backport release/service/v0.25 Backport PR to release service/v0.25.0 label Sep 10, 2026
@jakedoublev

Copy link
Copy Markdown
Contributor Author

/backport

@opentdf-automation

Copy link
Copy Markdown
Contributor

Successfully created backport PR for release/service/v0.25:

opentdf-automation Bot added a commit that referenced this pull request Sep 10, 2026
Honors `server.grpc.reflectionEnabled` for external handlers and omits
reflection from the in-process mux.

Tested: `make test HAND_MODS=service/internal/server`

<!-- This is an auto-generated comment: release notes by coderabbit.ai
-->

## Summary by CodeRabbit

* **Configuration**
  * gRPC reflection is now registered only when explicitly enabled.
* Reflection handlers are no longer registered on the in-process server.

* **Bug Fixes**
* Prevents unintended exposure of reflection endpoints when reflection
is disabled.

<!-- end of auto-generated comment: release notes by coderabbit.ai -->

---------

Signed-off-by: jakedoublev <jake.vanvorhis@virtru.com>
(cherry picked from commit 8743070)
jakedoublev pushed a commit that referenced this pull request Sep 10, 2026
…ase/service/v0.25] (#4025)

# Description
Backport of #4021 to `release/service/v0.25`.

Signed-off-by: jakedoublev <jake.vanvorhis@virtru.com>
Co-authored-by: opentdf-automation[bot] <149537512+opentdf-automation[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

backport release/service/v0.25 Backport PR to release service/v0.25.0 size/s

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants