feat: service ref for trillian service - #2114
Conversation
PR Summary by QodoAdd ServiceReference (ref/url) for Trillian wiring with autodiscovery
AI Description
Diagram
High-Level Assessment
Files changed (46)
|
Code Review by Qodo
1. Conversion double-appends port
|
f3476d4 to
3ec6f8c
Compare
3ec6f8c to
eaa0c58
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #2114 +/- ##
==========================================
- Coverage 56.74% 56.74% -0.01%
==========================================
Files 269 284 +15
Lines 15298 15823 +525
==========================================
+ Hits 8681 8978 +297
- Misses 5731 5916 +185
- Partials 886 929 +43
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
eaa0c58 to
bebcd13
Compare
bebcd13 to
fbda569
Compare
osmman
left a comment
There was a problem hiding this comment.
- CTlog/Rekor controllers now depend on Trillian CRs via ResolveInternalServiceUrl but don't watch them — no event-driven re-reconcile if Trillian is created/updated later.
- Stale
doc.gostill references removed*Servicetypes.
| apiconversion "k8s.io/apimachinery/pkg/conversion" | ||
| ) | ||
|
|
||
| var portRe = regexp.MustCompile(`:(\d+)(?:/|$)`) |
There was a problem hiding this comment.
Both portRe regexes (here and in api/v1alpha1/conversion_overrides.go) duplicate host:port splitting. The stdlib already does this — net.SplitHostPort handles it (including IPv6), so no regex is needed. Just strip the scheme prefix (e.g. dns:///) first, then pass the remainder to net.SplitHostPort. Worth consolidating both call sites onto this in internal/utils/service_ref_resolver.go.
There was a problem hiding this comment.
I know about net.SplitHostPort The problem is with the dns:/// (note 3 /) That means that the host:port is not the host part but the path SplitHostPort does not work on that.
I wanted to create something universal that will work even for normal urls and even for host:port that is technically not valid but it is highly used.
Implementing all workarounds coming from ^^ lead me to the simple regexp solution.
There was a problem hiding this comment.
as you can see on grpc/grpc-go#2403 there was ask to deliver grpc parser but it was not accepted.
I discussed this with the other grpc language leads, and they don't offer anything similar. Target URIs aren't intended to be parsed by users. We are investigating some different options to configure connection settings based on the scheme, but haven't made any concrete plans yet.
The regexp is the easiest option unless we do not want to re-implement the grpc parser.
Trillian never changes its url so no need to watch it for now. |
fbda569 to
fb35120
Compare
Added watcher for consistency and possible future usage. It may also push Services into error when trillian gets removed that is better than wait till the pod dies.. |
fb35120 to
f267ff5
Compare
Assisted-by: Claude Code (claude-opus-4-6)
f267ff5 to
ee90b16
Compare
Assisted-by: Claude Code (claude-opus-4-6)