Skip to content

feat: service ref for trillian service - #2114

Merged
bouskaJ merged 1 commit into
mainfrom
jbouska/serviceRefs
Jul 27, 2026
Merged

feat: service ref for trillian service#2114
bouskaJ merged 1 commit into
mainfrom
jbouska/serviceRefs

Conversation

@bouskaJ

@bouskaJ bouskaJ commented Jul 22, 2026

Copy link
Copy Markdown
Member

Assisted-by: Claude Code (claude-opus-4-6)

@qodo-for-securesign

Copy link
Copy Markdown

PR Summary by Qodo

Add ServiceReference (ref/url) for Trillian wiring with autodiscovery

✨ Enhancement 🧪 Tests ⚙️ Configuration changes 🕐 40+ Minutes

Grey Divider

AI Description

• Replace CTlog/Rekor Trillian address/port fields with a ref-or-URL ServiceReference.
• Add resolver infrastructure to compute in-cluster Trillian endpoints via CR refs or autodiscovery.
• Update conversions, CRD schemas, and tests/e2e to cover the new reference model.
Diagram

graph TD
  A["CTlog/Rekor/Securesign CR"] --> B["trillian: ServiceReference"] --> C{"Resolve mode?"}
  C --> D["Use URL"] --> H["Controllers configure Trillian"]
  C --> E["Ref: Get Trillian CR"] --> G["serviceresolver.Resolve"] --> H
  C --> F["Autodiscover: List Trillian CRs"] --> G --> H
Loading
High-Level Assessment

The following are alternative approaches to this PR:

1. Extend existing TrillianService with optional ref field
  • ➕ Smaller API churn (keeps address/port semantics)
  • ➕ Less controller parsing (no host:port extraction)
  • ➖ Harder validation story (multiple partially-overlapping ways to configure)
  • ➖ Continues to bake Trillian-specific shape into CTlog/Rekor specs rather than a reusable reference type
2. Reference Kubernetes Service/Endpoint instead of Trillian CR
  • ➕ Uses standard Kubernetes primitives; avoids custom resolver registry
  • ➕ Naturally supports cross-namespace via ObjectReference + RBAC
  • ➖ Loses higher-level component semantics (operator-managed Trillian CR as the unit)
  • ➖ More user configuration burden (must know Service name/ports)
3. Make trillian a single URL string (no ref/autodiscovery)
  • ➕ Simplest API and implementation
  • ➕ No init-time registration mechanism required
  • ➖ Removes in-cluster component discovery and coupling to managed Trillian instances
  • ➖ Harder multi-namespace and composability story without higher-level references

Recommendation: The PR’s approach (generic ServiceReference + resolver-backed ref/autodiscovery) is the best fit because it standardizes cross-component wiring while preserving backwards compatibility via conversion and providing an ergonomic in-cluster discovery path. The main tradeoff is the init-time resolver registration pattern; it’s acceptable here given the clear type-to-URL mapping and test coverage added.

Files changed (46) +738 / -175

Enhancement (14) +238 / -38
common.goIntroduce ServiceReference and ServiceReferenceRef API types +23/-0

Introduce ServiceReference and ServiceReferenceRef API types

• Adds a reusable ServiceReference type that can point to a component CR via Ref or to an external service via URL. Includes kubebuilder validation to enforce mutual exclusivity between ref and url.

api/v1/common.go

ctlog_defaults.goStop defaulting TrillianService in CTlog defaults +0/-1

Stop defaulting TrillianService in CTlog defaults

• Removes the legacy TrillianService SetDefaults invocation from CTlogSpec defaulting, consistent with the move to ServiceReference-based configuration.

api/v1/ctlog_defaults.go

ctlog_types.goSwitch CTlog spec.trillian to ServiceReference +1/-1

Switch CTlog spec.trillian to ServiceReference

• Replaces CTlogSpec.Trillian from TrillianService (address/port) to the new ServiceReference type, enabling ref/url configuration.

api/v1/ctlog_types.go

rekor_defaults.goStop defaulting TrillianService in Rekor defaults +0/-1

Stop defaulting TrillianService in Rekor defaults

• Removes the legacy TrillianService SetDefaults invocation from RekorSpec defaulting in preparation for ServiceReference usage.

api/v1/rekor_defaults.go

rekor_types.goSwitch Rekor spec.trillian to ServiceReference +1/-1

Switch Rekor spec.trillian to ServiceReference

• Replaces RekorSpec.Trillian from TrillianService (address/port) to ServiceReference, supporting ref/url selection.

api/v1/rekor_types.go

securesign_defaults.goMove defaulting to Securesign and default Trillian refs +13/-1

Move defaulting to Securesign and default Trillian refs

• Changes defaulting entrypoint to Securesign.SetDefaults and sets Ctlog/Rekor Trillian refs to the Securesign resource (name/namespace) when no URL is provided.

api/v1/securesign_defaults.go

conversion_overrides.goAdd TrillianService ↔ ServiceReference conversion overrides +34/-0

Add TrillianService ↔ ServiceReference conversion overrides

• Implements manual conversions between v1alpha1.TrillianService and v1.ServiceReference, including URL building and parsing host/port from URL-like strings.

api/v1alpha1/conversion_overrides.go

action.goResolve Trillian endpoint via ServiceReference resolver utility +2/-8

Resolve Trillian endpoint via ServiceReference resolver utility

• Replaces inline address/port composition with ResolveInternalServiceUrl, allowing URL/ref/autodiscovery resolution for tree actions.

internal/action/tree/action.go

server_config.goResolve Trillian URL via ServiceReference resolver when building CTlog config +5/-12

Resolve Trillian URL via ServiceReference resolver when building CTlog config

• Replaces address defaulting and port validation with ResolveInternalServiceUrl and removes the local resolveTrillianAddress helper.

internal/controller/ctlog/actions/server_config.go

deployment.goResolve Trillian URL and parse host/port for Rekor deployment args +21/-13

Resolve Trillian URL and parse host/port for Rekor deployment args

• Uses ResolveInternalServiceUrl to compute the effective Trillian endpoint, then extracts host and port for Rekor CLI flags.

internal/controller/rekor/actions/server/deployment.go

resolver.goRegister Trillian CR → in-cluster URL resolver +16/-0

Register Trillian CR → in-cluster URL resolver

• Adds an init-time registration that maps a Trillian CR to its canonical dns:/// service URL including default port.

internal/controller/trillian/serviceresolver/resolver.go

trillian_controller.goEnsure Trillian resolver is registered when controller is built +3/-0

Ensure Trillian resolver is registered when controller is built

• Adds a side-effect import of the Trillian serviceresolver package so URL resolution is available at runtime.

internal/controller/trillian/trillian_controller.go

resolver.goIntroduce generic resolver registry for CR → internal URL mapping +34/-0

Introduce generic resolver registry for CR → internal URL mapping

• Adds a small registry keyed by concrete Go type to resolve component CRs into in-cluster service URLs.

internal/serviceresolver/resolver.go

service_ref_resolver.goAdd ResolveInternalServiceUrl utility (URL/ref/autodiscovery) +85/-0

Add ResolveInternalServiceUrl utility (URL/ref/autodiscovery)

• Implements ServiceReference resolution logic: prefer URL, otherwise fetch referenced CR, otherwise autodiscover a single CR instance and resolve via registry.

internal/utils/service_ref_resolver.go

Bug fix (4) +13 / -1
securesign_webhook.goInvoke Securesign.SetDefaults from webhook defaulter +1/-1

Invoke Securesign.SetDefaults from webhook defaulter

• Updates the webhook defaulter to call obj.SetDefaults() after the defaulting function moved off SecuresignSpec.

api/v1/securesign_webhook.go

ctlog_conversion.goPreserve Trillian ref when URL is empty during CTlog ConvertTo +3/-0

Preserve Trillian ref when URL is empty during CTlog ConvertTo

• Ensures restored Trillian Ref is propagated during hub conversion when the destination Trillian URL is not set.

api/v1alpha1/ctlog_conversion.go

rekor_conversion.goPreserve Trillian ref when URL is empty during Rekor ConvertTo +3/-0

Preserve Trillian ref when URL is empty during Rekor ConvertTo

• Mirrors CTlog behavior for Rekor: keeps restored Trillian Ref if no URL was provided on the destination.

api/v1alpha1/rekor_conversion.go

securesign_conversion.goPreserve Trillian refs for Ctlog/Rekor on Securesign ConvertTo +6/-0

Preserve Trillian refs for Ctlog/Rekor on Securesign ConvertTo

• Propagates restored Trillian Ref values for embedded Ctlog/Rekor specs when URLs are unset, improving roundtrip fidelity.

api/v1alpha1/securesign_conversion.go

Refactor (4) +5 / -9
types.goChange tree wrapper to return ServiceReference instead of TrillianService +3/-3

Change tree wrapper to return ServiceReference instead of TrillianService

• Updates the generic wrapper interface so controllers can retrieve a ServiceReference for Trillian regardless of the embedding CR type.

internal/action/tree/types.go

deployment.goRemove legacy Trillian address/port prerequisite checks +0/-4

Remove legacy Trillian address/port prerequisite checks

• Drops explicit CTlog deployment validation for Trillian address/port fields, aligning with resolver-based URL construction elsewhere.

internal/controller/ctlog/actions/deployment.go

resolve_tree.goUse ServiceReference for CTlog resolve-tree action wiring +1/-1

Use ServiceReference for CTlog resolve-tree action wiring

• Updates the CTlog resolve-tree action wrapper to expose *ServiceReference rather than *TrillianService.

internal/controller/ctlog/actions/resolve_tree.go

resolve_tree.goUse ServiceReference for Rekor resolve-tree action wiring +1/-1

Use ServiceReference for Rekor resolve-tree action wiring

• Updates the Rekor resolve-tree action wrapper to expose *ServiceReference rather than *TrillianService.

internal/controller/rekor/actions/server/resolve_tree.go

Tests (19) +353 / -91
ctlog_types_test.goUpdate CTlog type tests for ServiceReference +1/-6

Update CTlog type tests for ServiceReference

• Adjusts CTlog defaulting/manifests tests to reflect the new Trillian field shape and removes assertions tied to legacy port defaulting.

api/v1/ctlog_types_test.go

rekor_types_test.goUpdate Rekor type tests for ServiceReference +3/-5

Update Rekor type tests for ServiceReference

• Updates Rekor tests to validate the ServiceReference default/serialization behavior and updates fully-populated manifests to use URL.

api/v1/rekor_types_test.go

conversion_roundtrip_test.goAdd constrained fuzzers for TrillianService/ServiceReference roundtrips +34/-0

Add constrained fuzzers for TrillianService/ServiceReference roundtrips

• Adds fuzzer functions to generate mutually compatible values across the new conversion boundary and wires them into Securesign/CTlog/Rekor roundtrip tests.

api/v1alpha1/conversion_roundtrip_test.go

conversion_unit_test.goUpdate conversion unit tests for ServiceReference mapping +4/-4

Update conversion unit tests for ServiceReference mapping

• Adjusts unit tests to assert expected translation between v1.ServiceReference URL forms and v1alpha1 TrillianService address/port fields.

api/v1alpha1/conversion_unit_test.go

ctlog_types_test.goAdjust CTlog v1alpha1 tests for port-only conversion behavior +3/-1

Adjust CTlog v1alpha1 tests for port-only conversion behavior

• Updates generated CTlog objects to avoid port-only Trillian values that do not survive conversion, documenting expected defaulting behavior.

api/v1alpha1/ctlog_types_test.go

action_test.goUpdate RBAC action tests to use ServiceReference URLs +2/-5

Update RBAC action tests to use ServiceReference URLs

• Replaces legacy TrillianService test fixtures with ServiceReference URL-based configuration.

internal/action/rbac/action_test.go

action_test.goUpdate tree action tests for ServiceReference fixtures +7/-14

Update tree action tests for ServiceReference fixtures

• Adjusts wrappers and test instances to provide ServiceReference values and removes legacy address/port assumptions.

internal/action/tree/action_test.go

server_config_test.goUpdate CTlog server-config tests for URL/ref/autodiscovery resolution +19/-32

Update CTlog server-config tests for URL/ref/autodiscovery resolution

• Adjusts test inputs to ServiceReference and adds coverage for autodiscovery resolving to a dns:/// URL with annotations.

internal/controller/ctlog/actions/server_config_test.go

ctlog_controller_test.goUpdate CTlog controller tests to use Trillian CR autodiscovery +8/-5

Update CTlog controller tests to use Trillian CR autodiscovery

• Replaces explicit Service creation with creation of a Trillian CR to drive service autodiscovery behavior in tests.

internal/controller/ctlog/ctlog_controller_test.go

ctlog_hot_update_test.goUpdate CTlog hot-update tests for ServiceReference ref resolution +13/-3

Update CTlog hot-update tests for ServiceReference ref resolution

• Updates CTlog test instances to reference Trillian via ServiceReference.Ref and creates a Trillian CR for resolution.

internal/controller/ctlog/ctlog_hot_update_test.go

deployment_test.goAdd Rekor deployment tests for autodiscovery and ServiceReference refs +80/-5

Add Rekor deployment tests for autodiscovery and ServiceReference refs

• Updates existing tests to ServiceReference and adds a new test verifying that an empty ServiceReference autodiscovers Trillian and injects dns:/// + port args.

internal/controller/rekor/actions/server/deployment_test.go

rekor_attestation_test.goSeed Trillian CR for Rekor controller autodiscovery tests +8/-0

Seed Trillian CR for Rekor controller autodiscovery tests

• Creates a Trillian CR before running Rekor controller test flows so autodiscovery can resolve an endpoint.

internal/controller/rekor/rekor_attestation_test.go

rekor_controller_test.goUpdate Rekor controller tests to configure Trillian via ServiceReference URL +2/-1

Update Rekor controller tests to configure Trillian via ServiceReference URL

• Sets Rekor.Spec.Trillian to a ServiceReference URL in controller tests to match the new API shape.

internal/controller/rekor/rekor_controller_test.go

rekor_hot_update_test.goUpdate Rekor hot-update tests to create referenced Trillian CR +10/-3

Update Rekor hot-update tests to create referenced Trillian CR

• Switches Rekor hot-update test setup to ServiceReference.Ref and ensures a Trillian CR exists before resolution occurs.

internal/controller/rekor/rekor_hot_update_test.go

suite.goRegister Trillian service resolver in the controller test suite +1/-0

Register Trillian service resolver in the controller test suite

• Imports the Trillian serviceresolver package for side-effect registration so tests can resolve Trillian URLs.

internal/controller/testonly/suite.go

resolver_test.goAdd unit test for Trillian URL resolver registration +29/-0

Add unit test for Trillian URL resolver registration

• Verifies that resolving a Trillian CR returns the expected dns:///trillian-logserver.<ns>.svc:8091 URL.

internal/controller/trillian/serviceresolver/resolver_test.go

service_ref_resolver_test.goAdd tests for URL, ref, and autodiscovery resolution behaviors +120/-0

Add tests for URL, ref, and autodiscovery resolution behaviors

• Covers precedence rules (URL over Ref), ref lookup failures, autodiscovery empty/multiple cases, and successful dns:/// resolution using a fake client.

internal/utils/service_ref_resolver_test.go

namespaced_test.goUpdate e2e install tests to configure Trillian via ServiceReference +7/-4

Update e2e install tests to configure Trillian via ServiceReference

• Replaces TrillianService address-based configuration with ServiceReference URL/ref forms for cross-namespace component installs.

test/e2e/install/namespaced_test.go

ctlog_recovery_test.goUpdate CTlog recovery e2e test to use ServiceReference URL +2/-3

Update CTlog recovery e2e test to use ServiceReference URL

• Switches CTlog Trillian configuration in lifecycle tests from address/port to a host:port URL string.

test/e2e/lifecycle/ctlog_recovery_test.go

Other (5) +129 / -36
zz_generated.deepcopy.goAdd deepcopy support for ServiceReference types +35/-0

Add deepcopy support for ServiceReference types

• Regenerates deepcopy methods for ServiceReference and ServiceReferenceRef to support controller-runtime object copying and scheme operations.

api/v1/zz_generated.deepcopy.go

zz_generated.conversion.goRegister and use ServiceReference conversion funcs in generated conversions +14/-4

Register and use ServiceReference conversion funcs in generated conversions

• Registers manual conversion funcs and updates autoConvert paths so CTlog/Rekor specs map TrillianService ↔ ServiceReference rather than the legacy TrillianService ↔ TrillianService mapping.

api/v1alpha1/zz_generated.conversion.go

rhtas.redhat.com_ctlogs.yamlUpdate CTlog CRD schema for ServiceReference trillian +19/-8

Update CTlog CRD schema for ServiceReference trillian

• Replaces the trillian schema from address/port to ref/url and adds x-kubernetes-validations enforcing mutual exclusivity.

config/crd/bases/rhtas.redhat.com_ctlogs.yaml

rhtas.redhat.com_rekors.yamlUpdate Rekor CRD schema for ServiceReference trillian +19/-8

Update Rekor CRD schema for ServiceReference trillian

• Replaces the trillian schema from address/port to ref/url and adds x-kubernetes-validations enforcing mutual exclusivity.

config/crd/bases/rhtas.redhat.com_rekors.yaml

rhtas.redhat.com_securesigns.yamlUpdate Securesign CRD schema for ServiceReference trillian (Ctlog/Rekor) +42/-16

Update Securesign CRD schema for ServiceReference trillian (Ctlog/Rekor)

• Updates embedded Ctlog/Rekor trillian schemas to ref/url and adds validations to prevent simultaneously setting ref and url.

config/crd/bases/rhtas.redhat.com_securesigns.yaml

@qodo-for-securesign

qodo-for-securesign Bot commented Jul 22, 2026

Copy link
Copy Markdown

Code Review by Qodo

🐞 Bugs (2) 📘 Rule violations (0) 📎 Requirement gaps (0) 🎨 UX issues (0) 🔗 Cross-repo conflicts (0) 📜 Skill insights (0)

Grey Divider


Action required

1. Conversion double-appends port 🐞 Bug ≡ Correctness
Description
Convert_v1alpha1_TrillianService_To_v1_ServiceReference appends Port to Address whenever both are
set, even if Address already contains a port. This can produce malformed URLs like
"trillian:8091:8091" during v1alpha1↔v1 conversion/upgrade.
Code

api/v1alpha1/conversion_overrides.go[R131-137]

+func Convert_v1alpha1_TrillianService_To_v1_ServiceReference(in *TrillianService, out *v1.ServiceReference, _ apiconversion.Scope) error {
+	if in.Address != "" && in.Port != nil {
+		out.URL = fmt.Sprintf("%s:%d", in.Address, *in.Port)
+	} else if in.Address != "" {
+		out.URL = in.Address
+	}
+	return nil
Relevance

⭐⭐ Medium

No prior accepted/rejected history found for port double-append behavior in conversion code.

ⓘ Recommendations generated based on similar findings in past PRs

Evidence
The v1alpha1 type allows Address to be any string, and the converter blindly appends the port when
Port is non-nil, creating a double-port URL if Address already contains one.

api/v1alpha1/common.go[51-62]
api/v1alpha1/conversion_overrides.go[131-137]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
The v1alpha1 → v1 conversion builds `out.URL` as `fmt.Sprintf("%s:%d", in.Address, *in.Port)` whenever both fields are set. If `in.Address` already includes a `:port` suffix (which the v1alpha1 type does not prohibit), the resulting v1 URL is invalid.

### Issue Context
v1alpha1 `TrillianService.Address` is a free-form endpoint string; older clusters may have stored host:port in Address while also having Port defaulted.

### Fix Focus Areas
- api/v1alpha1/conversion_overrides.go[131-137]
- api/v1alpha1/common.go[51-62]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


2. Defaults overwrite Trillian ref ✓ Resolved 🐞 Bug ≡ Correctness
Description
Securesign.SetDefaults overwrites Spec.Ctlog.Trillian.Ref and Spec.Rekor.Trillian.Ref whenever URL
is empty, even if the user explicitly set a different Ref. This breaks configurations that intend to
reference a non-self Trillian CR via Ref.
Code

api/v1/securesign_defaults.go[R5-16]

+	if s.Spec.Ctlog.Trillian.URL == "" {
+		s.Spec.Ctlog.Trillian.Ref = &ServiceReferenceRef{
+			Name:      s.Name,
+			Namespace: s.Namespace,
+		}
+	}
+	if s.Spec.Rekor.Trillian.URL == "" {
+		s.Spec.Rekor.Trillian.Ref = &ServiceReferenceRef{
+			Name:      s.Name,
+			Namespace: s.Namespace,
+		}
+	}
Relevance

⭐⭐ Medium

No historical review evidence about preserving user-set Ref in defaults; only broad defaulting
changes observed (PR#1982).

PR-#1982

ⓘ Recommendations generated based on similar findings in past PRs

Evidence
The defaulting logic only checks URL emptiness and then unconditionally assigns Ref, so any pre-set
Ref is lost; ServiceReferenceRef.Namespace is optional and Ref is a first-class input so overriding
it is incorrect.

api/v1/securesign_defaults.go[3-16]
api/v1/common.go[155-176]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
`Securesign.SetDefaults()` currently assigns `Spec.Ctlog.Trillian.Ref` / `Spec.Rekor.Trillian.Ref` whenever `URL == ""`, which unintentionally overwrites a user-specified `Ref`.

### Issue Context
The new `ServiceReference` supports selecting Trillian via either `Ref` or `URL`. Defaulting should only fill in a self-reference when neither is provided.

### Fix Focus Areas
- api/v1/securesign_defaults.go[3-16]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


3. Ref namespace not defaulted ✓ Resolved 🐞 Bug ≡ Correctness
Description
ResolveInternalServiceUrl uses serviceRef.Ref.Namespace as-is; if Namespace is omitted (documented
to default to the referencing resource namespace), the Get is performed with an empty namespace and
the referenced CR cannot be resolved. This makes same-namespace refs with omitted namespace fail at
runtime.
Code

internal/utils/service_ref_resolver.go[R19-28]

+func ResolveInternalServiceUrl(ctx context.Context, cl client.Client, serviceRef v1.ServiceReference, autodiscoverNamespaceName string, instance client.Object) (string, error) {
+	if serviceRef.URL != "" {
+		return serviceRef.URL, nil
+	}
+	if serviceRef.Ref != nil {
+		if err := cl.Get(ctx, types.NamespacedName{Namespace: serviceRef.Ref.Namespace, Name: serviceRef.Ref.Name}, instance); err != nil {
+			return "", fmt.Errorf("%w: %w", ErrGetServiceFailed, err)
+		}
+		return serviceresolver.Resolve(instance)
+	}
Relevance

⭐⭐ Medium

No historical evidence found about defaulting empty Ref.Namespace to referencing namespace in
service ref resolver.

ⓘ Recommendations generated based on similar findings in past PRs

Evidence
API type documents that ref.namespace defaults to the referencing namespace, but the resolver passes
the empty string into NamespacedName, so Kubernetes lookup will not target the caller namespace.

api/v1/common.go[167-176]
internal/utils/service_ref_resolver.go[19-28]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
`ResolveInternalServiceUrl()` performs `cl.Get()` using `serviceRef.Ref.Namespace` directly. When the user omits `ref.namespace` (allowed/expected), this becomes `""` and the lookup fails.

### Issue Context
`ServiceReferenceRef.Namespace` is explicitly documented as optional and defaulting to the referencing resource namespace.

### Fix Focus Areas
- internal/utils/service_ref_resolver.go[19-28]
- api/v1/common.go[167-176]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools



Remediation recommended

4. Rekor may miss dns:/// 🐞 Bug ➹ Performance
Description
Rekor deployment uses ResolveInternalServiceUrl() verbatim; if users supply Trillian as a plain
"host:port" (as in e2e), the generated --trillian_log_server.address is not forced to
dns:///..., which can undermine the intended gRPC DNS-based resolution when round_robin is
configured. This reopens the configuration pitfall previously addressed by forcing the dns resolver
scheme.
Code

internal/controller/rekor/actions/server/deployment.go[R65-69]

+	internalTrillianUrl, err := utils.ResolveInternalServiceUrl(ctx, i.Client, instance.Spec.Trillian, instance.Namespace, &rhtasv1.Trillian{})
+	if err != nil {
+		return i.Error(ctx, fmt.Errorf("error resolving Trillian URL: %w", err), instance)
	}
-	i.Logger.V(1).Info("trillian logserver", "address", insCopy.Spec.Trillian.Address)
+	i.Logger.V(1).Info("trillian logserver", "address", internalTrillianUrl)
Relevance

⭐⭐⭐ High

dns:/// scheme for gRPC round_robin was explicitly accepted before (PR#1752); this reintroduces the
same pitfall.

PR-#1752

ⓘ Recommendations generated based on similar findings in past PRs

Evidence
Rekor resolves Trillian URL and uses it directly, while e2e config supplies a plain host:port (no
dns scheme). Prior accepted guidance explicitly required dns:/// to make round_robin effective for
DNS-based endpoint resolution.

internal/controller/rekor/actions/server/deployment.go[65-70]
internal/controller/rekor/actions/server/deployment.go[110-150]
test/e2e/install/namespaced_test.go[98-110]
PR-#1752

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
When `ServiceReference.URL` is provided as `host:port`, Rekor will pass `host` directly to `--trillian_log_server.address` while still configuring round_robin. For in-cluster/headless service usage, this can lead gRPC to not use DNS resolver semantics unless `dns:///` is used.

### Issue Context
Autodiscovery returns `dns:///...:8091`, but explicit URL inputs (including current e2e) may omit it.

### Fix Focus Areas
- internal/controller/rekor/actions/server/deployment.go[65-69]
- internal/controller/rekor/actions/server/deployment.go[112-150]
- internal/utils/service_ref_resolver.go[19-22]
- test/e2e/install/namespaced_test.go[98-110]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


Grey Divider

Qodo Logo

Comment thread api/v1/securesign_defaults.go Outdated
Comment thread internal/utils/service_ref_resolver.go Outdated
Comment thread api/v1alpha1/conversion_overrides.go
@bouskaJ
bouskaJ force-pushed the jbouska/serviceRefs branch from f3476d4 to 3ec6f8c Compare July 22, 2026 13:32
@bouskaJ bouskaJ changed the title feat: servicde ref for trillian service feat: service ref for trillian service Jul 22, 2026
@bouskaJ
bouskaJ requested a review from osmman July 23, 2026 06:56
@bouskaJ
bouskaJ force-pushed the jbouska/serviceRefs branch from 3ec6f8c to eaa0c58 Compare July 23, 2026 08:34
@codecov-commenter

codecov-commenter commented Jul 23, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 56.25000% with 77 lines in your changes missing coverage. Please review.
✅ Project coverage is 56.74%. Comparing base (6452c3d) to head (ee90b16).
⚠️ Report is 2 commits behind head on main.

Files with missing lines Patch % Lines
api/v1/zz_generated.deepcopy.go 10.00% 17 Missing and 1 partial ⚠️
internal/utils/service_ref_resolver.go 65.95% 8 Missing and 8 partials ⚠️
api/v1alpha1/zz_generated.conversion.go 0.00% 6 Missing and 6 partials ⚠️
internal/serviceresolver/resolver.go 0.00% 10 Missing ⚠️
api/v1alpha1/conversion_overrides.go 61.90% 5 Missing and 3 partials ⚠️
...rnal/controller/rekor/actions/server/deployment.go 64.70% 4 Missing and 2 partials ⚠️
internal/controller/ctlog/actions/server_config.go 33.33% 1 Missing and 1 partial ⚠️
internal/controller/ctlog/ctlog_controller.go 75.00% 1 Missing and 1 partial ⚠️
internal/controller/rekor/rekor_controller.go 77.77% 1 Missing and 1 partial ⚠️
internal/action/tree/action.go 50.00% 1 Missing ⚠️
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     
Flag Coverage Δ
e2e 70.31% <72.72%> (-0.81%) ⬇️
unit 34.89% <55.11%> (+0.67%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@osmman osmman left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • 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.go still references removed *Service types.

Comment thread internal/action/tree/action.go
apiconversion "k8s.io/apimachinery/pkg/conversion"
)

var portRe = regexp.MustCompile(`:(\d+)(?:/|$)`)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

grpc/grpc-go#2403 (comment)

The regexp is the easiest option unless we do not want to re-implement the grpc parser.

Comment thread internal/utils/service_ref_resolver.go Outdated
Comment thread api/v1/common.go
@bouskaJ

bouskaJ commented Jul 24, 2026

Copy link
Copy Markdown
Member Author
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.

Trillian never changes its url so no need to watch it for now.

@bouskaJ
bouskaJ force-pushed the jbouska/serviceRefs branch from fbda569 to fb35120 Compare July 24, 2026 14:18
@bouskaJ

bouskaJ commented Jul 24, 2026

Copy link
Copy Markdown
Member Author
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.

Trillian never changes its url so no need to watch it for now.

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..

@bouskaJ
bouskaJ force-pushed the jbouska/serviceRefs branch from fb35120 to f267ff5 Compare July 24, 2026 14:23
Assisted-by: Claude Code (claude-opus-4-6)
@bouskaJ
bouskaJ force-pushed the jbouska/serviceRefs branch from f267ff5 to ee90b16 Compare July 24, 2026 14:27
@bouskaJ
bouskaJ requested a review from osmman July 24, 2026 14:29
@bouskaJ
bouskaJ merged commit a765847 into main Jul 27, 2026
22 of 23 checks passed
@bouskaJ
bouskaJ deleted the jbouska/serviceRefs branch July 27, 2026 08:33
This was referenced Jul 27, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants