Add grpc dns transport #114
Conversation
WalkthroughAdds DNS protobuf definitions and codegen; introduces gRPC and Redis transports; refactors DNS action handling to use an unexported transport interface with Publish/PublishAsync; provides protobuf↔domain adapters and a TLS-capable gRPC client helper; updates tests and proto docs. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Caller
participant DNS as DNS Client
participant T as transport (private)
rect rgb(245,248,255)
note right of DNS: New flow: action-based Publish / PublishAsync
Caller->>DNS: Execute(action)
DNS->>T: Publish(ctx, action)
alt transport = grpc
T->>GRPCsvc: RPC(Add/Delete/RequestCert) (protobuf)
GRPCsvc-->>T: RPC Response
else transport = redis
T->>Redis: PUBLISH request (and SUB for sync)
Redis-->>T: reply message
end
T-->>DNS: JSON DNSResponse bytes or error
DNS-->>Caller: Parsed result or error
end
sequenceDiagram
autonumber
participant Test as Test
participant DNS as DNS Client
participant GRPC as grpcTransport
Note right of GRPC: Configure via WithGRPC(pb.DNSServiceClient)
Test->>DNS: Configure(WithGRPC(mockClient))
Test->>DNS: Execute(DNSAddAction)
DNS->>GRPC: Publish(ctx, DNSAddAction)
GRPC->>mockSvc: Add(AddRequest)
mockSvc-->>GRPC: AddResponse
GRPC-->>DNS: JSON Add DNSResponse
DNS-->>Test: Assert success
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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: 5
🧹 Nitpick comments (4)
dns/grpc_adapter.go (2)
15-17: Potential integer overflow when converting int to int32.While unlikely for Priority, Weight, and Port values, converting from
inttoint32could theoretically overflow on 64-bit systems if values exceedmath.MaxInt32(2,147,483,647).Consider either:
- Changing the field types in
DNSAddActiontoint32if the protobuf limits are intentional- Adding overflow checks if you want to support the full
intrange- Documenting that values must not exceed int32 range
Example validation:
+import "math" + func (a *DNSAddAction) ToProto() *pb.AddRequest { + if a.Priority > math.MaxInt32 || a.Weight > math.MaxInt32 || a.Port > math.MaxInt32 { + // handle overflow + } return &pb.AddRequest{
93-108: Consider handling zero time value in timestamp conversion.Line 103 uses
timestamppb.New(cert.Expires)which will create a timestamp for the Unix epoch (1970-01-01) ifcert.Expiresis a zerotime.Time. This might not be the intended behavior if a zero time should be represented differently (e.g., nil timestamp or validation error).If zero time should be handled specially:
func ToProtoCertResponse(cert *DNSCert, err error) *pb.CertResponse { resp := &pb.CertResponse{} if err != nil { resp.Success = false resp.Error = err.Error() } else { resp.Success = true if cert != nil { resp.Certificate = cert.Certificate resp.PrivateKey = cert.PrivateKey - resp.Expires = timestamppb.New(cert.Expires) + if !cert.Expires.IsZero() { + resp.Expires = timestamppb.New(cert.Expires) + } resp.Domain = cert.Domain } } return resp }dns/aether_test.go (2)
19-43: Consider adding explicit handling for DNSDeleteAction.The
Publishmethod handlesDNSCertActionandDNSAddActionbut returnsnil, nilfor any other action type (includingDNSDeleteAction). While this may work for current tests, explicitly handlingDNSDeleteActionwould make the mock more complete and prevent confusion.Apply this diff to add explicit handling:
func (t *testTransport) Publish(ctx context.Context, action DNSAction) ([]byte, error) { t.lastAction = action // Return different responses based on action type switch action.(type) { case *DNSCertAction: var cert DNSCert cert.Certificate = []byte("cert") cert.Expires = time.Now().Add(time.Hour * 24 * 365 * 2) cert.PrivateKey = []byte("private") var response DNSResponse[DNSCert] response.Success = true response.Data = &cert return json.Marshal(response) case *DNSAddAction: var record DNSRecord record.IDs = []string{uuid.New().String()} var response DNSResponse[DNSRecord] response.Success = true response.Data = &record return json.Marshal(response) + case *DNSDeleteAction: + var response DNSResponse[string] + response.Success = true + return json.Marshal(response) default: return nil, nil } }
91-93: Remove unrelated ActionFromChannel utility test
The assertion at dns/aether_test.go:91–93 tests the standaloneActionFromChannelhelper, which isn’t affected by this PR’s transport changes; remove it or move it into a dedicated utility test suite.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
dns/proto/dns.pb.gois excluded by!**/*.pb.godns/proto/dns_grpc.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (8)
Makefile(1 hunks)dns/aether.go(8 hunks)dns/aether_test.go(3 hunks)dns/grpc_adapter.go(1 hunks)dns/grpc_transport.go(1 hunks)dns/proto/README.md(1 hunks)dns/proto/dns.proto(1 hunks)dns/redis_transport.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (5)
dns/redis_transport.go (2)
dns/aether.go (2)
DNSAction(183-192)ErrClosed(280-280)string/json.go (1)
JSONStringify(8-20)
dns/grpc_adapter.go (2)
dns/aether.go (8)
DNSAddAction(24-44)DNSAddAction(290-290)DNSDeleteAction(81-88)DNSDeleteAction(291-291)DNSCertAction(92-95)DNSCertAction(292-292)DNSRecord(112-114)DNSCert(104-110)dns/proto/dns.pb.go (18)
AddRequest(25-42)AddRequest(55-55)AddRequest(70-72)DeleteRequest(190-196)DeleteRequest(209-209)DeleteRequest(224-226)CertRequest(294-299)CertRequest(312-312)CertRequest(327-329)AddResponse(130-137)AddResponse(150-150)AddResponse(165-167)DeleteResponse(242-248)DeleteResponse(261-261)DeleteResponse(276-278)CertResponse(338-348)CertResponse(361-361)CertResponse(376-378)
dns/aether.go (1)
dns/proto/dns_grpc.pb.go (1)
DNSServiceClient(32-39)
dns/grpc_transport.go (3)
dns/proto/dns_grpc.pb.go (2)
DNSServiceClient(32-39)NewDNSServiceClient(45-47)dns/aether.go (8)
DNSAction(183-192)DNSAddAction(24-44)DNSAddAction(290-290)NewDNSResponse(294-304)DNSDeleteAction(81-88)DNSDeleteAction(291-291)DNSCertAction(92-95)DNSCertAction(292-292)dns/grpc_adapter.go (3)
FromProtoAddResponse(34-39)FromProtoDeleteResponse(41-46)FromProtoCertResponse(48-58)
dns/aether_test.go (1)
dns/aether.go (10)
DNSAction(183-192)DNSCertAction(92-95)DNSCertAction(292-292)DNSCert(104-110)DNSResponse(97-102)DNSAddAction(24-44)DNSAddAction(290-290)DNSRecord(112-114)SendDNSAction(307-343)WithTimeout(247-251)
🪛 Buf (1.57.2)
dns/proto/dns.proto
3-3: Files with package "dns" must be within a directory "dns" relative to root but were in directory "dns/proto".
(PACKAGE_DIRECTORY_MATCH)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
🔇 Additional comments (4)
dns/proto/dns.proto (2)
19-33: LGTM: AddRequest message is well-structured.The message design appropriately handles DNS record fields including TTL, expiration, and MX/SRV-specific fields (priority, weight, port). Converting durations to seconds for wire format is the correct approach.
55-62: LGTM: CertResponse uses appropriate timestamp type.The use of
google.protobuf.Timestampfor the expires field is the correct protobuf approach for representing timestamps across languages.dns/grpc_adapter.go (1)
60-66: LGTM: DNSError type is simple and correct.The error type properly implements the
errorinterface and provides a clean way to propagate protobuf response errors.dns/aether_test.go (1)
13-17: LGTM: Interface assertion correctly targets private transport.The change from
var _ Transporttovar _ transportproperly aligns with the refactoring to a private transport interface, ensuring the test mock implements the correct interface.
| func (a *DNSAddAction) ToProto() *pb.AddRequest { | ||
| return &pb.AddRequest{ | ||
| Name: a.Name, | ||
| Type: a.Type, | ||
| Value: a.Value, | ||
| TtlSeconds: int64(a.TTL.Seconds()), | ||
| ExpiresSeconds: int64(a.Expires.Seconds()), | ||
| Priority: int32(a.Priority), | ||
| Weight: int32(a.Weight), | ||
| Port: int32(a.Port), | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
🧩 Analysis chain
Verify behavior for zero or negative duration values.
The conversion from time.Duration to seconds using .Seconds() may produce unexpected results:
- Zero durations will result in 0 seconds (which may be valid)
- Negative durations (if allowed) will produce negative values
Ensure that the calling code validates duration values, or add validation here if needed.
Run the following script to check if there's validation logic for TTL and Expires fields:
🏁 Script executed:
#!/bin/bash
# Description: Search for validation of TTL and Expires duration fields
# Search for validation patterns in DNSAddAction usage
rg -A 5 -B 5 'DNSAddAction' --type go | rg -i 'ttl|expires|validate|check'Length of output: 629
Validate non-negative TTL and Expires durations
The .Seconds() conversion can produce negative values which will be sent in the protobuf request. Add checks in WithTTL/WithExpires (or in ToProto) to reject or clamp negative durations before serialization.
🤖 Prompt for AI Agents
dns/grpc_adapter.go around lines 8 to 19: the current ToProto serializes TTL and
Expires using a.TTL.Seconds() / a.Expires.Seconds() which can be negative;
update the code to validate and normalize these durations before converting to
int64 — either in the WithTTL/WithExpires setters (preferred) or immediately in
ToProto: if a.TTL < 0 or a.Expires < 0 clamp them to zero (or return an error
from the setters), then convert the non-negative duration seconds to int64 for
TtlSeconds and ExpiresSeconds so no negative values are sent in the protobuf.
There was a problem hiding this comment.
the server will do this
There was a problem hiding this comment.
@robindiddams, understood! I've noted that the server will handle validation for these duration values.
✏️ Learnings added
Learnt from: robindiddams
PR: agentuity/go-common#114
File: dns/grpc_adapter.go:10-21
Timestamp: 2025-10-09T14:17:54.245Z
Learning: In the DNS gRPC adapter (`dns/grpc_adapter.go`), TTL and Expires duration validation (including checking for negative values) is performed server-side, not client-side in the ToProto() methods.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
| sub := t.redis.Subscribe(ctx, replyChannel) | ||
| defer sub.Close() | ||
|
|
||
| if err := t.redis.Publish(ctx, "aether:request:"+action.GetAction()+":"+id, cstr.JSONStringify(action)).Err(); err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| select { | ||
| case <-ctx.Done(): | ||
| return nil, ctx.Err() | ||
| case msg := <-sub.Channel(): | ||
| if msg == nil { |
There was a problem hiding this comment.
Wait for subscription ACK before publishing
Without waiting for the SUBSCRIBE acknowledgement, Publish can win the race and the reply gets lost, leaving the caller blocked until timeout. Call Receive after Subscribe so the subscription is active before publishing.
sub := t.redis.Subscribe(ctx, replyChannel)
defer sub.Close()
+ if _, err := sub.Receive(ctx); err != nil {
+ return nil, err
+ }
+
if err := t.redis.Publish(ctx, "aether:request:"+action.GetAction()+":"+id, cstr.JSONStringify(action)).Err(); err != nil {
return nil, err
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| sub := t.redis.Subscribe(ctx, replyChannel) | |
| defer sub.Close() | |
| if err := t.redis.Publish(ctx, "aether:request:"+action.GetAction()+":"+id, cstr.JSONStringify(action)).Err(); err != nil { | |
| return nil, err | |
| } | |
| select { | |
| case <-ctx.Done(): | |
| return nil, ctx.Err() | |
| case msg := <-sub.Channel(): | |
| if msg == nil { | |
| sub := t.redis.Subscribe(ctx, replyChannel) | |
| defer sub.Close() | |
| if _, err := sub.Receive(ctx); err != nil { | |
| return nil, err | |
| } | |
| if err := t.redis.Publish(ctx, "aether:request:"+action.GetAction()+":"+id, cstr.JSONStringify(action)).Err(); err != nil { | |
| return nil, err | |
| } | |
| select { | |
| case <-ctx.Done(): | |
| return nil, ctx.Err() | |
| case msg := <-sub.Channel(): | |
| if msg == nil { |
🤖 Prompt for AI Agents
In dns/redis_transport.go around lines 26 to 37, the code subscribes then
immediately publishes which can race and drop replies; after calling
t.redis.Subscribe(ctx, replyChannel) call sub.Receive(ctx) (or otherwise wait
for the SUBSCRIBE acknowledgement) and check for errors before proceeding to
t.redis.Publish so the subscription is active and cannot miss the reply; keep
the existing defer sub.Close() and preserve context cancellation handling.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
dns/aether.go (1)
238-240: Clarify the comment.The comment "for Testing" is vague. Consider being more specific about why this is unexported and test-only.
-// WithTransport sets a custom transport for the DNS action -// -// for Testing +// withTransport sets a custom transport for the DNS action. +// This is unexported and intended for test mocks/stubs only. func withTransport(t transport) OptionHandler {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
dns/aether.go(7 hunks)dns/grpc_transport.go(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-10-09T01:41:38.404Z
Learnt from: robindiddams
PR: agentuity/go-common#114
File: dns/grpc_transport.go:73-86
Timestamp: 2025-10-09T01:41:38.404Z
Learning: `grpc.NewClient` is a valid function in grpc-go, introduced in version v1.63.0. It is the modern API for creating gRPC clients and should be preferred over older methods like `grpc.Dial` or `grpc.DialContext`.
Applied to files:
dns/grpc_transport.go
🧬 Code graph analysis (2)
dns/grpc_transport.go (3)
dns/proto/dns_grpc.pb.go (2)
DNSServiceClient(32-39)NewDNSServiceClient(45-47)dns/aether.go (8)
DNSAction(183-192)DNSAddAction(24-44)DNSAddAction(290-290)NewDNSResponse(294-304)DNSDeleteAction(81-88)DNSDeleteAction(291-291)DNSCertAction(92-95)DNSCertAction(292-292)dns/grpc_adapter.go (3)
FromProtoAddResponse(36-41)FromProtoDeleteResponse(43-48)FromProtoCertResponse(50-64)
dns/aether.go (1)
dns/proto/dns_grpc.pb.go (1)
DNSServiceClient(32-39)
🔇 Additional comments (10)
dns/grpc_transport.go (4)
16-20: LGTM!Clean struct definition with proper interface compliance verification.
22-55: LGTM!The implementation correctly handles all three DNS action types, properly converting between protobuf responses and domain types using the adapter functions from
dns/grpc_adapter.go, and marshaling the responses to JSON.
57-74: LGTM!Correct implementation for asynchronous publishing without waiting for responses.
85-85: Verify handling of caller-provided dial options with TLS.When
tlsCertis provided, line 85 replaces the entireoptsslice with TLS credentials, discarding any other dial options the caller may have provided (e.g., keepalive settings, interceptors, max message size). This may be intentional to avoid conflicts with transport credentials, but it prevents callers from configuring other aspects of the connection when using TLS.Consider whether non-transport dial options should be preserved when TLS is enabled.
dns/aether.go (6)
11-11: LGTM!Good practice adding deprecation notices to guide users toward the protobuf-generated types. The import alias
pbis clear and idiomatic.Also applies to: 22-23, 79-80, 90-91
214-220: LGTM!The refactored private
transportinterface with simplified method signatures (Publishreturning[]bytedirectly) is cleaner than the previous channel-based approach.
261-267: LGTM!The new
WithGRPCfunction correctly wires up the gRPC transport for use with DNS actions.
307-318: LGTM!The function signature correctly uses the now-public
OptionHandlertype, and the option initialization logic properly sets defaults and validates transport availability.
320-328: LGTM!The timeout handling with
context.WithTimeoutis correct, and the async publishing path properly routes fire-and-forget requests.
330-342: LGTM!The response handling correctly unmarshals the transport response, validates the
Successfield, and extracts the typed data. Error propagation is appropriate.
| u, err := url.Parse(address) | ||
| if err != nil { | ||
| return nil, nil, fmt.Errorf("failed to parse address: %w", err) | ||
| } | ||
| target := u.Host |
There was a problem hiding this comment.
Plain "host:port" addresses won't work.
The code parses address as a URL and extracts u.Host as the gRPC target. For plain addresses like "localhost:50051" (without a scheme), url.Parse will succeed but u.Host will be empty (the address ends up in u.Path instead). This will cause grpc.NewClient to be called with an empty target, resulting in an error.
Consider handling plain addresses explicitly:
u, err := url.Parse(address)
if err != nil {
return nil, nil, fmt.Errorf("failed to parse address: %w", err)
}
target := u.Host
+if target == "" {
+ // Handle plain "host:port" format
+ target = address
+}Or document that address must be a full URL (e.g., "dns://localhost:50051" or "grpc://localhost:50051").
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| u, err := url.Parse(address) | |
| if err != nil { | |
| return nil, nil, fmt.Errorf("failed to parse address: %w", err) | |
| } | |
| target := u.Host | |
| u, err := url.Parse(address) | |
| if err != nil { | |
| return nil, nil, fmt.Errorf("failed to parse address: %w", err) | |
| } | |
| target := u.Host | |
| if target == "" { | |
| // Handle plain "host:port" format | |
| target = address | |
| } |
🤖 Prompt for AI Agents
In dns/grpc_transport.go around lines 91 to 95, parsing address with url.Parse
can leave host empty for plain "host:port" strings (they land in u.Path); update
the code to detect that case and set target accordingly — if u.Host is empty use
u.Path (or fallback to the original address) so plain "localhost:50051" works,
otherwise use u.Host; alternatively, explicitly require/normalize a scheme by
prepending a default (e.g., "dns://") before parsing — implement one of these
fixes so grpc.NewClient receives a non-empty target.
1 participant
Summary by CodeRabbit
New Features
Documentation
Chores