fix: address code review findings for RFC 9432 Catalog Zones - #290
Open
poyrazK wants to merge 43 commits into
Open
fix: address code review findings for RFC 9432 Catalog Zones#290poyrazK wants to merge 43 commits into
poyrazK wants to merge 43 commits into
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds RFC 9432 catalog-zone support across domain models, packet types, DB schema, Postgres repository (PTR-backed catalog membership), service orchestration, admin HTTP handlers, server poller with AXFR provisioning, and comprehensive tests/mocks. ChangesCatalog Zones Feature
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
poyrazK
force-pushed
the
release/catalog-zones
branch
from
June 10, 2026 16:28
a8efc10 to
2ad3713
Compare
poyrazK
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 19
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
internal/dns/packet/packet.go (2)
153-179:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAdd missing cases for CATZ and CZTR in
QueryType.String().The
String()method returns human-readable names forQueryTypevalues, but it's missing cases for the newly addedCATZandCZTRtypes. Without these, logging and debugging will show"TYPE53"and"TYPE54"instead of the more readable"CATZ"and"CZTR".🔧 Proposed fix
func (t QueryType) String() string { switch t { case A: return "A" case NS: return "NS" case CNAME: return "CNAME" case SOA: return "SOA" case MX: return "MX" case TXT: return "TXT" case AAAA: return "AAAA" case SRV: return "SRV" case DS: return "DS" case RRSIG: return "RRSIG" case NSEC: return "NSEC" case DNSKEY: return "DNSKEY" case NSEC3: return "NSEC3" case NSEC3PARAM: return "NSEC3PARAM" case AXFR: return "AXFR" case IXFR: return "IXFR" case ANY: return "ANY" case OPT: return "OPT" case TSIG: return "TSIG" case PTR: return "PTR" case CAA: return "CAA" case HTTPS: return "HTTPS" + case CATZ: return "CATZ" + case CZTR: return "CZTR" default: return fmt.Sprintf("TYPE%d", t) } }🤖 Prompt for 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. In `@internal/dns/packet/packet.go` around lines 153 - 179, The QueryType.String() switch is missing cases for the new CATZ and CZTR constants, causing those types to fall back to "TYPEN"; update the switch in the QueryType.String() method to add case CATZ: return "CATZ" and case CZTR: return "CZTR" (keeping the existing default fmt.Sprintf behavior for unknown types) so logging and debugging show the human-readable names; ensure you reference the CATZ and CZTR symbols and modify the switch inside the String() function.
135-150:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAdd missing cases for CATZ and CZTR in
RecordTypeToQueryType.The function converts
domain.RecordTypetopacket.QueryType, but it's missing cases for the newly addeddomain.TypeCATZanddomain.TypeCZTR. Without these, catalog zone records cannot be converted from the domain model to wire format, breaking the catalog zone feature.🔧 Proposed fix
func RecordTypeToQueryType(t domain.RecordType) QueryType { switch t { case domain.TypeA: return A case domain.TypeNS: return NS case domain.TypeCNAME: return CNAME case domain.TypeSOA: return SOA case domain.TypeMX: return MX case domain.TypeTXT: return TXT case domain.TypeAAAA: return AAAA case domain.TypePTR: return PTR case domain.TypeSRV: return SRV case domain.TypeCAA: return CAA case domain.TypeHTTPS: return HTTPS + case domain.TypeCATZ: return CATZ + case domain.TypeCZTR: return CZTR default: return UNKNOWN } }🤖 Prompt for 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. In `@internal/dns/packet/packet.go` around lines 135 - 150, The RecordTypeToQueryType function is missing mappings for the new catalog zone record types; update RecordTypeToQueryType to add case branches for domain.TypeCATZ and domain.TypeCZTR that return the corresponding packet QueryType values (CATZ and CZTR) so domain catalog zone records convert to wire format; modify the switch in RecordTypeToQueryType to include "case domain.TypeCATZ: return CATZ" and "case domain.TypeCZTR: return CZTR" alongside the existing cases.
🤖 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 `@internal/adapters/api/handler_test.go`:
- Around line 109-119: The catalog mock methods on mockDNSService
(CreateCatalogZone, GetCatalogZone, ListCatalogZones, DeleteCatalogZone,
AddZoneToCatalog, RemoveZoneFromCatalog, ListZoneCatalogEntries,
PollCatalogZone, SyncZonesFromCatalog) currently always return nil errors;
update each method to honor the mock's error injection by checking and returning
m.err when set (e.g., if m.err != nil { return nil, m.err } or appropriate
zero-value plus m.err), matching the pattern used in existing mocks like
CreateZone and ListZones so tests can exercise error paths.
In `@internal/adapters/api/handler.go`:
- Around line 358-368: The handler must enforce content-type, body size, and
zone-name format: before decoding, call validateContentType(r) (same pattern as
CreateZone/CreateRecord), replace direct r.Body with http.MaxBytesReader(w,
r.Body, maxBodySize) when creating the JSON decoder, and after decoding check
the parsed req.ZoneName with domain.ValidateZoneName and return a 400 via
h.writeJSONError on validation failure; apply these changes around the existing
req struct decode block so it mirrors CreateZone/CreateRecord behavior.
- Around line 483-495: Wrap the request body with http.MaxBytesReader before
decoding (use http.MaxBytesReader(w, r.Body, maxBodySize)) to limit payload
size, call validateContentType(r) and return a 415/400 via h.writeJSONError if
it fails, then decode into the existing req struct; after decoding, validate
req.ZoneName with domain.ValidateZoneName and return a bad-request error via
h.writeJSONError if the validation fails (keep existing checks for
ZoneID/ZoneName and reuse the same error handling pattern as
CreateZone/CreateRecord).
In `@internal/adapters/repository/postgres.go`:
- Around line 1815-1828: CreateZoneFromCatalog currently calls CreateZone and
BatchCreateRecords separately which neither writes catalog_id/catalog_zone_name
(so GetZoneByCatalogName/DeleteZoneByCatalogName can't find them) and can leave
an orphaned zone if record insertion fails; change CreateZoneFromCatalog to
perform a single transactional operation that inserts the zone including
catalog_id and catalog_zone_name and then inserts all records within the same
transaction (use the repository's DB transaction handling or a new internal
helper), replacing the CreateZone call with a transactional zone insert that
persists catalog linkage, then call BatchCreateRecords (or a transactional
variant) using the same tx, and rollback on any error so the whole creation is
atomic; refer to CreateZoneFromCatalog, CreateZone, BatchCreateRecords,
GetZoneByCatalogName, and DeleteZoneByCatalogName when locating and updating the
logic.
- Around line 1748-1781: In AddZoneToCatalog, you currently insert a PTR for
entry.ZoneName/entry.ZoneID without verifying ownership; query dns_zones for the
target zone using entry.ZoneID (or entry.ZoneName) and ensure the returned
tenant_id equals the incoming tenantID (and that id/name match), returning an
error if not; only after that verification proceed to build ptrContent and
insert the dns_records row (use the same checked zone id/value), keeping
reverseDomainName(entry.ZoneName) for name generation and preserving the
existing conflict behavior.
- Around line 1624-1854: The repository currently uses database/sql (db *sql.DB,
NewPostgresRepository(db *sql.DB) and ExecContext/QueryContext/QueryRowContext)
but must be ported to pgx/v5; update the PostgresRepository to hold a
pgxpool.Pool (or pgx.Conn) instead of *sql.DB, change the constructor
NewPostgresRepository to accept that pgx type, and replace all
ExecContext/QueryContext/QueryRowContext calls in methods like
CreateCatalogZone, GetCatalogZone, ListCatalogZones, UpdateCatalogZoneVersion,
DeleteCatalogZone, ListZoneCatalogEntries, AddZoneToCatalog,
RemoveZoneFromCatalog, CreateZoneFromCatalog, DeleteZoneByCatalogName, and
GetZoneByCatalogName with the corresponding pgx/v5 methods (Exec, Query,
QueryRow with ctx) and swap sql.ErrNoRows checks for pgx.ErrNoRows; ensure
parameter placeholders remain $n and scanning semantics match pgx (use row.Scan
/ rows.Next with pgx), and update imports to github.com/jackc/pgx/v5/pgxpool and
github.com/jackc/pgx/v5.
- Around line 1775-1779: The INSERT in postgres.go uses an ON CONFLICT arbiter
of (zone_id, LOWER(name), type, content) which may not match the actual unique
index/constraint (schema.sql migrates name to citext so the real arbiter is
likely (zone_id, name, type, content)); verify the actual unique
constraint/index for dns_records (or the migrations in
internal/adapters/repository/schema.sql) and either update the ON CONFLICT
clause in the INSERT to use the exact arbiter the DB enforces (e.g., (zone_id,
name, type, content)) or add a matching unique index/constraint to the
migrations (e.g., unique on (zone_id, LOWER(name), type, content)) so the
conflict target in the INSERT matches the DB.
In `@internal/adapters/repository/schema.sql`:
- Around line 19-31: The catalog name columns are currently TEXT and must be
case-insensitive: change catalog_zones.zone_name and dns_zones.catalog_zone_name
to use citext (or equivalent LOWER-based uniqueness) and ensure the citext
extension exists; update the CREATE TABLE definition for catalog_zones to define
zone_name as CITEXT NOT NULL UNIQUE, add/alter the dns_zones.catalog_zone_name
column to CITEXT (ALTER TABLE ... ALTER COLUMN ... TYPE citext USING
zone_name::citext), and ensure any indexes/unique constraints (e.g.,
idx_catalog_zones_tenant_id or a new index for catalog lookups) reflect the
citext type so GetZoneByCatalogName/DeleteZoneByCatalogName lookups are
case-insensitive.
In `@internal/core/services/dns_service_test.go`:
- Around line 294-308: The catalog mock methods currently always return nil and
ignore m.err; update each method so they honor m.err for error injection: for
error-only methods (CreateCatalogZone, UpdateCatalogZoneVersion,
DeleteCatalogZone, AddZoneToCatalog, RemoveZoneFromCatalog,
CreateZoneFromCatalog, DeleteZoneByCatalogName) return m.err instead of nil, and
for data-returning methods (GetCatalogZone, ListCatalogZones,
ListZoneCatalogEntries, GetZoneByCatalogName) check if m.err != nil and return
zero-value + m.err (e.g., nil, m.err) otherwise return the normal zero-value
response as before. Ensure the behavior matches the pattern used in GetZone
where m.err is checked first.
In `@internal/core/services/dns_service.go`:
- Around line 341-353: The inventory mutation (s.repo.AddZoneToCatalog / delete)
and the serial bump (s.repo.UpdateCatalogZoneVersion after GetCatalogZone) must
be performed inside a single repository transaction to ensure atomicity and
avoid lost concurrent bumps; add a new transactional repo method (e.g.,
AddZoneToCatalogWithSerialBump or MutateCatalogZoneWithSerialIncrement) that
accepts the catalogID, tenantID and the inventory change (entry or delete flag)
and performs the insert/delete and version increment atomically, update the
callers in dns_service.go to call that new method instead of calling
AddZoneToCatalog, GetCatalogZone, and UpdateCatalogZoneVersion separately (apply
the same change for the other occurrence around the 360-372 block).
- Around line 303-312: Normalize DNS names to a canonical FQDN form (lowercase,
with a trailing dot) at the service boundary so repository lookups for exact
name and prefix deletes (zoneName + ":") are consistent; implement or use a
helper like canonicalizeDNSName(name string) string and call it in
CreateCatalogZone (normalize zoneName before building CatalogZone), in the
methods around lines 335-340 that create catalog members (normalize member/zone
names before persisting), and in the delete paths (e.g.,
DeleteCatalogZone/DeleteCatalogMember) to normalize the provided names before
issuing repository exact-match or prefix delete operations. Ensure the same
canonicalization is applied everywhere a zone or member name enters the service.
In `@internal/core/services/dnssec_service_test.go`:
- Around line 82-96: The catalog mock methods currently always return nil and
must honor the mock error field; update each method (CreateCatalogZone,
GetCatalogZone, ListCatalogZones, UpdateCatalogZoneVersion, DeleteCatalogZone,
ListZoneCatalogEntries, AddZoneToCatalog, RemoveZoneFromCatalog,
CreateZoneFromCatalog, DeleteZoneByCatalogName, GetZoneByCatalogName) to check
m.err and return it when non-nil, otherwise return the appropriate nil/zero
result (e.g., return m.err for error-only methods, or return nil, m.err /
[]domain.CatalogZone(nil), m.err for methods that return values plus error).
Ensure the signature return types remain unchanged and only the returned
error/value logic is modified.
In `@internal/dns/packet/packet.go`:
- Around line 1043-1047: The CATZ RDATA write uses a non-existent
buffer.WriteRange(buffer.Position(), []byte(r.Txt)) in the CATZ case; replace
that call by writing the TXT bytes one-by-one like the TXT case does: after
writing the length with buffer.Writeu16 and the single-byte length with
buffer.Write, iterate over []byte(r.Txt) and call buffer.Write(b) for each byte,
handling and returning any errors; modify the block in the CATZ case
(referencing r.Txt, buffer.Writeu16, buffer.Write) to mirror the TXT write loop
rather than calling WriteRange.
In `@internal/dns/server/client.go`:
- Around line 423-437: The code updates s.catalogState.lastSeenSerial and
lastSeenAt immediately after reading currentSerial; change this so the update
happens only after the AXFR fetch and the subsequent zone sync loop complete
successfully: remove the early assignment to
s.catalogState.lastSeenSerial[catalogZoneName] and
s.catalogState.lastSeenAt[catalogZoneName], and instead acquire
s.catalogState.mu.Lock() and set those fields (using catalogZoneName and
currentSerial) right after the AXFR and sync succeed; mirror the same change for
the other occurrence referenced in the comment (the block around the later
441-458 region) and ensure you hold and release the mutex around the write and
keep the initial RLock check to decide if work is needed.
- Around line 619-624: When BatchCreateRecords fails after CreateZone succeeded,
roll back the partially-created zone to avoid leaving an empty catalog-managed
slave. In the error path after s.Repo.BatchCreateRecords(ctx, records) check,
call the repository delete/remove method for the created zone (e.g.,
s.Repo.DeleteZone(ctx, zone.ID) or s.Repo.DeleteZoneByName(ctx, zone.Name)),
handle/log any deletion error, and then return the original BatchCreateRecords
error (wrapping both if helpful); update the error block around
Repo.BatchCreateRecords to perform this cleanup using the created zone
identifier (zone.ID or zone.Name).
- Around line 507-547: The read loop currently treats io.EOF as success and
returns whatever packets were read; change the error handling so any unexpected
EOF/short read is surfaced as an error: when io.ReadFull(conn, lenBuf) or
io.ReadFull(conn, pData) returns io.EOF or io.ErrUnexpectedEOF, return a
descriptive error (e.g. "truncated AXFR: unexpected EOF") instead of
breaking/continuing; additionally, after the loop if soaCount < 2 return an
error indicating an incomplete AXFR so callers
(fetchCatalogEntries/fetchZoneRecords) do not consume partial data, and keep
existing checks for resp.Header.ResCode to return DNS errors as before.
- Around line 638-647: The loop in client.go is appending both SOA copies from
AXFR; before adding a converted record (ConvertPacketRecordToDomain -> dRec)
check for SOA and skip duplicates: if the DNS/type field indicates SOA (use
ans.Type or dRec.Type) and there is already an SOA in the in-memory records
slice for this zone/tenant, do not append it so BatchCreateRecords won't receive
duplicate SOA rows. Implement the check inside the existing loop that builds
records (referencing ConvertPacketRecordToDomain, dRec, records) and only append
non-SOA or the first-seen SOA.
In `@internal/dns/server/server.go`:
- Around line 519-533: The catalog poller is started even when s.Repo is nil
which causes pollCatalogZone/syncZoneFromCatalog to dereference s.Repo and
panic; before starting the goroutine in the block that checks
s.CatalogPollingEnabled && len(s.CatalogZones) > 0, also verify s.Repo != nil
(and log a warning similar to the CATALOG_MASTER_ADDR check) and skip starting
StartCatalogPoller if Repo is nil so that identity-only servers do not spawn the
poller that will call pollCatalogZone/syncZoneFromCatalog on a nil Repo.
In `@internal/testutil/mock_repo.go`:
- Around line 416-474: The catalog-related MockDNSService methods
(CreateCatalogZone, GetCatalogZone, ListCatalogZones, DeleteCatalogZone,
AddZoneToCatalog, RemoveZoneFromCatalog, ListZoneCatalogEntries,
PollCatalogZone, SyncZonesFromCatalog) are calling m.Called() with no arguments
which prevents testify from matching inputs; update each method to forward the
received parameters into m.Called(...) in the same order they appear in the
signature (e.g., CreateCatalogZone should call m.Called(ctx, arg1, arg2) or
m.Called(arg1, arg2) consistent with other mocks), keeping the existing return
handling (args.Get(0), args.Error(...)) unchanged so tests can assert and stub
by arguments.
---
Outside diff comments:
In `@internal/dns/packet/packet.go`:
- Around line 153-179: The QueryType.String() switch is missing cases for the
new CATZ and CZTR constants, causing those types to fall back to "TYPEN"; update
the switch in the QueryType.String() method to add case CATZ: return "CATZ" and
case CZTR: return "CZTR" (keeping the existing default fmt.Sprintf behavior for
unknown types) so logging and debugging show the human-readable names; ensure
you reference the CATZ and CZTR symbols and modify the switch inside the
String() function.
- Around line 135-150: The RecordTypeToQueryType function is missing mappings
for the new catalog zone record types; update RecordTypeToQueryType to add case
branches for domain.TypeCATZ and domain.TypeCZTR that return the corresponding
packet QueryType values (CATZ and CZTR) so domain catalog zone records convert
to wire format; modify the switch in RecordTypeToQueryType to include "case
domain.TypeCATZ: return CATZ" and "case domain.TypeCZTR: return CZTR" alongside
the existing cases.
🪄 Autofix (Beta)
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: defaults
Review profile: CHILL
Plan: Pro
Run ID: a232fd53-4e2b-458b-b46b-5f0caf834fda
📒 Files selected for processing (15)
internal/adapters/api/handler.gointernal/adapters/api/handler_test.gointernal/adapters/repository/postgres.gointernal/adapters/repository/schema.sqlinternal/core/domain/dns.gointernal/core/ports/ports.gointernal/core/services/anycast_manager_test.gointernal/core/services/dns_service.gointernal/core/services/dns_service_test.gointernal/core/services/dnssec_service_test.gointernal/dns/packet/packet.gointernal/dns/server/client.gointernal/dns/server/server.gointernal/dns/server/server_test.gointernal/testutil/mock_repo.go
Comment on lines
+358
to
+368
| var req struct { | ||
| ZoneName string `json:"zone_name"` | ||
| } | ||
| if err := json.NewDecoder(r.Body).Decode(&req); err != nil { | ||
| h.writeJSONError(w, http.StatusBadRequest, "Invalid request body", err) | ||
| return | ||
| } | ||
| if req.ZoneName == "" { | ||
| h.writeJSONError(w, http.StatusBadRequest, "zone_name is required", nil) | ||
| return | ||
| } |
There was a problem hiding this comment.
Add input validation for DoS protection, consistency, and correctness.
Three validation gaps compared to the established handler pattern (CreateZone, CreateRecord):
-
Missing
MaxBytesReader(major security issue): Line 361 decodes the request body without a size limit, allowing an attacker to send an unbounded payload and exhaust server memory. Compare withCreateZoneline 188 andCreateRecordline 279, which wrap the body withhttp.MaxBytesReader(w, r.Body, maxBodySize). -
Missing Content-Type validation: Lines 358-364 don't call
validateContentType(r)before decoding JSON. This is inconsistent withCreateZone(line 184) andCreateRecord(line 275). -
Missing DNS zone name validation: Lines 365-368 check that
zone_nameis non-empty but don't validate that it's a valid DNS name.CreateZonecallsdomain.ValidateZoneNameat line 193. Invalid catalog zone names will cause failures downstream during catalog polling and AXFR operations.
🔒 Proposed fix to add all three validations
func (h *Handler) CreateCatalogZone(w http.ResponseWriter, r *http.Request) {
tenantID, ok := r.Context().Value(CtxTenantID).(string)
if !ok || tenantID == "" {
h.logger.Warn("CreateCatalogZone: missing or invalid tenant ID in context")
h.writeJSONError(w, http.StatusUnauthorized, "Unauthorized: missing tenant context", nil)
return
}
+ if !validateContentType(r) {
+ http.Error(w, "Content-Type must be application/json", http.StatusUnsupportedMediaType)
+ return
+ }
+
var req struct {
ZoneName string `json:"zone_name"`
}
- if err := json.NewDecoder(r.Body).Decode(&req); err != nil {
+ if err := json.NewDecoder(http.MaxBytesReader(w, r.Body, maxBodySize)).Decode(&req); err != nil {
h.writeJSONError(w, http.StatusBadRequest, "Invalid request body", err)
return
}
if req.ZoneName == "" {
h.writeJSONError(w, http.StatusBadRequest, "zone_name is required", nil)
return
}
+ if err := domain.ValidateZoneName(req.ZoneName); err != nil {
+ h.writeJSONError(w, http.StatusBadRequest, "Invalid zone name", err)
+ return
+ }
catz, err := h.svc.CreateCatalogZone(r.Context(), tenantID, req.ZoneName)📝 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.
Suggested change
| var req struct { | |
| ZoneName string `json:"zone_name"` | |
| } | |
| if err := json.NewDecoder(r.Body).Decode(&req); err != nil { | |
| h.writeJSONError(w, http.StatusBadRequest, "Invalid request body", err) | |
| return | |
| } | |
| if req.ZoneName == "" { | |
| h.writeJSONError(w, http.StatusBadRequest, "zone_name is required", nil) | |
| return | |
| } | |
| if !validateContentType(r) { | |
| http.Error(w, "Content-Type must be application/json", http.StatusUnsupportedMediaType) | |
| return | |
| } | |
| var req struct { | |
| ZoneName string `json:"zone_name"` | |
| } | |
| if err := json.NewDecoder(http.MaxBytesReader(w, r.Body, maxBodySize)).Decode(&req); err != nil { | |
| h.writeJSONError(w, http.StatusBadRequest, "Invalid request body", err) | |
| return | |
| } | |
| if req.ZoneName == "" { | |
| h.writeJSONError(w, http.StatusBadRequest, "zone_name is required", nil) | |
| return | |
| } | |
| if err := domain.ValidateZoneName(req.ZoneName); err != nil { | |
| h.writeJSONError(w, http.StatusBadRequest, "Invalid zone name", err) | |
| return | |
| } |
🤖 Prompt for 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.
In `@internal/adapters/api/handler.go` around lines 358 - 368, The handler must
enforce content-type, body size, and zone-name format: before decoding, call
validateContentType(r) (same pattern as CreateZone/CreateRecord), replace direct
r.Body with http.MaxBytesReader(w, r.Body, maxBodySize) when creating the JSON
decoder, and after decoding check the parsed req.ZoneName with
domain.ValidateZoneName and return a 400 via h.writeJSONError on validation
failure; apply these changes around the existing req struct decode block so it
mirrors CreateZone/CreateRecord behavior.
Comment on lines
+483
to
+495
| var req struct { | ||
| ZoneName string `json:"zone_name"` | ||
| ZoneID string `json:"zone_id"` | ||
| GroupID string `json:"group_id,omitempty"` | ||
| } | ||
| if err := json.NewDecoder(r.Body).Decode(&req); err != nil { | ||
| h.writeJSONError(w, http.StatusBadRequest, "Invalid request body", err) | ||
| return | ||
| } | ||
| if req.ZoneName == "" || req.ZoneID == "" { | ||
| h.writeJSONError(w, http.StatusBadRequest, "zone_name and zone_id are required", nil) | ||
| return | ||
| } |
There was a problem hiding this comment.
Add input validation for DoS protection, consistency, and correctness.
Three validation gaps compared to the established handler pattern (CreateZone, CreateRecord):
-
Missing
MaxBytesReader(major security issue): Line 488 decodes the request body without a size limit, allowing an attacker to send an unbounded payload and exhaust server memory. Compare withCreateZoneline 188 andCreateRecordline 279, which wrap the body withhttp.MaxBytesReader(w, r.Body, maxBodySize). -
Missing Content-Type validation: Lines 483-491 don't call
validateContentType(r)before decoding JSON. This is inconsistent withCreateZone(line 184) andCreateRecord(line 275). -
Missing DNS zone name validation: Lines 492-495 check that
zone_nameis non-empty but don't validate that it's a valid DNS name. Thezone_nameparameter represents a zone being added to the catalog and should be validated usingdomain.ValidateZoneName, consistent withCreateZoneline 193.
🔒 Proposed fix to add all three validations
func (h *Handler) AddCatalogEntry(w http.ResponseWriter, r *http.Request) {
id := r.PathValue("id")
tenantID, ok := r.Context().Value(CtxTenantID).(string)
if !ok || tenantID == "" {
h.logger.Warn("AddCatalogEntry: missing or invalid tenant ID in context")
h.writeJSONError(w, http.StatusUnauthorized, "Unauthorized: missing tenant context", nil)
return
}
+ if !validateContentType(r) {
+ http.Error(w, "Content-Type must be application/json", http.StatusUnsupportedMediaType)
+ return
+ }
+
var req struct {
ZoneName string `json:"zone_name"`
ZoneID string `json:"zone_id"`
GroupID string `json:"group_id,omitempty"`
}
- if err := json.NewDecoder(r.Body).Decode(&req); err != nil {
+ if err := json.NewDecoder(http.MaxBytesReader(w, r.Body, maxBodySize)).Decode(&req); err != nil {
h.writeJSONError(w, http.StatusBadRequest, "Invalid request body", err)
return
}
if req.ZoneName == "" || req.ZoneID == "" {
h.writeJSONError(w, http.StatusBadRequest, "zone_name and zone_id are required", nil)
return
}
+ if err := domain.ValidateZoneName(req.ZoneName); err != nil {
+ h.writeJSONError(w, http.StatusBadRequest, "Invalid zone name", err)
+ return
+ }
if err := h.svc.AddZoneToCatalog(r.Context(), id, tenantID, req.ZoneName, req.ZoneID, req.GroupID); err != nil {🤖 Prompt for 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.
In `@internal/adapters/api/handler.go` around lines 483 - 495, Wrap the request
body with http.MaxBytesReader before decoding (use http.MaxBytesReader(w,
r.Body, maxBodySize)) to limit payload size, call validateContentType(r) and
return a 415/400 via h.writeJSONError if it fails, then decode into the existing
req struct; after decoding, validate req.ZoneName with domain.ValidateZoneName
and return a bad-request error via h.writeJSONError if the validation fails
(keep existing checks for ZoneID/ZoneName and reuse the same error handling
pattern as CreateZone/CreateRecord).
Comment on lines
+1624
to
+1854
| // CreateCatalogZone creates a new catalog zone. | ||
| func (r *PostgresRepository) CreateCatalogZone(ctx context.Context, catz *domain.CatalogZone) error { | ||
| query := ` | ||
| INSERT INTO catalog_zones (id, tenant_id, zone_name, version, serial, created_at, updated_at) | ||
| VALUES ($1, $2, $3, $4, $5, $6, $7) | ||
| ` | ||
| _, err := r.db.ExecContext(ctx, query, | ||
| catz.ID, catz.TenantID, catz.ZoneName, catz.Version, catz.Serial, catz.CreatedAt, catz.UpdatedAt) | ||
| return err | ||
| } | ||
|
|
||
| // GetCatalogZone retrieves a catalog zone by ID. | ||
| func (r *PostgresRepository) GetCatalogZone(ctx context.Context, catalogID string, tenantID string) (*domain.CatalogZone, error) { | ||
| query := ` | ||
| SELECT id, tenant_id, zone_name, version, serial, created_at, updated_at | ||
| FROM catalog_zones WHERE id = $1 AND tenant_id = $2 | ||
| ` | ||
| row := r.db.QueryRowContext(ctx, query, catalogID, tenantID) | ||
| var catz domain.CatalogZone | ||
| err := row.Scan(&catz.ID, &catz.TenantID, &catz.ZoneName, &catz.Version, &catz.Serial, &catz.CreatedAt, &catz.UpdatedAt) | ||
| if errors.Is(err, sql.ErrNoRows) { | ||
| return nil, nil | ||
| } | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| return &catz, nil | ||
| } | ||
|
|
||
| // ListCatalogZones lists all catalog zones for a tenant. | ||
| func (r *PostgresRepository) ListCatalogZones(ctx context.Context, tenantID string) ([]domain.CatalogZone, error) { | ||
| query := ` | ||
| SELECT id, tenant_id, zone_name, version, serial, created_at, updated_at | ||
| FROM catalog_zones WHERE tenant_id = $1 ORDER BY created_at DESC | ||
| ` | ||
| rows, err := r.db.QueryContext(ctx, query, tenantID) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| defer rows.Close() | ||
|
|
||
| var catalogZones []domain.CatalogZone | ||
| for rows.Next() { | ||
| var catz domain.CatalogZone | ||
| if err := rows.Scan(&catz.ID, &catz.TenantID, &catz.ZoneName, &catz.Version, &catz.Serial, &catz.CreatedAt, &catz.UpdatedAt); err != nil { | ||
| return nil, err | ||
| } | ||
| catalogZones = append(catalogZones, catz) | ||
| } | ||
| return catalogZones, rows.Err() | ||
| } | ||
|
|
||
| // UpdateCatalogZoneVersion updates the version and serial of a catalog zone. | ||
| func (r *PostgresRepository) UpdateCatalogZoneVersion(ctx context.Context, catalogID string, version string, serial uint32) error { | ||
| query := ` | ||
| UPDATE catalog_zones SET version = $1, serial = $2, updated_at = CURRENT_TIMESTAMP | ||
| WHERE id = $3 | ||
| ` | ||
| _, err := r.db.ExecContext(ctx, query, version, serial, catalogID) | ||
| return err | ||
| } | ||
|
|
||
| // DeleteCatalogZone deletes a catalog zone. | ||
| func (r *PostgresRepository) DeleteCatalogZone(ctx context.Context, catalogID string, tenantID string) error { | ||
| query := `DELETE FROM catalog_zones WHERE id = $1 AND tenant_id = $2` | ||
| _, err := r.db.ExecContext(ctx, query, catalogID, tenantID) | ||
| return err | ||
| } | ||
|
|
||
| // ListZoneCatalogEntries lists all zone entries in a catalog zone. | ||
| // Zone entries are stored as PTR records in the catalog zone. | ||
| func (r *PostgresRepository) ListZoneCatalogEntries(ctx context.Context, catalogID string, tenantID string) ([]domain.ZoneCatalogEntry, error) { | ||
| // First get the catalog zone to find its name (tenant-scoped) | ||
| catz, err := r.GetCatalogZone(ctx, catalogID, tenantID) | ||
| if err != nil || catz == nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| // Get the zone ID for the catalog zone | ||
| zoneQuery := `SELECT id FROM dns_zones WHERE name = $1 AND tenant_id = (SELECT tenant_id FROM catalog_zones WHERE id = $2)` | ||
| var zoneID string | ||
| err = r.db.QueryRowContext(ctx, zoneQuery, catz.ZoneName, catalogID).Scan(&zoneID) | ||
| if err == sql.ErrNoRows { | ||
| return nil, nil | ||
| } | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| // Query catalog entries - stored as PTR records with content format "zone_name:zone_id[:group_id]" | ||
| ptrQuery := ` | ||
| SELECT name, content FROM dns_records | ||
| WHERE zone_id = $1 AND type = 'PTR' | ||
| ` | ||
| rows, err := r.db.QueryContext(ctx, ptrQuery, zoneID) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| defer rows.Close() | ||
|
|
||
| var entries []domain.ZoneCatalogEntry | ||
| for rows.Next() { | ||
| var name, content string | ||
| if err := rows.Scan(&name, &content); err != nil { | ||
| return nil, err | ||
| } | ||
| // PTR name is the reversed zone name for catalog lookup | ||
| // Content format: "zone_name:zone_id[:group_id]" | ||
| parts := strings.SplitN(content, ":", 3) | ||
| if len(parts) >= 2 { | ||
| entry := domain.ZoneCatalogEntry{ | ||
| ZoneName: parts[0], | ||
| ZoneID: parts[1], | ||
| } | ||
| if len(parts) == 3 { | ||
| entry.GroupID = parts[2] | ||
| } | ||
| entries = append(entries, entry) | ||
| } | ||
| } | ||
| return entries, rows.Err() | ||
| } | ||
|
|
||
| // AddZoneToCatalog adds a zone to a catalog zone's inventory. | ||
| func (r *PostgresRepository) AddZoneToCatalog(ctx context.Context, catalogID string, tenantID string, entry *domain.ZoneCatalogEntry) error { | ||
| // Get the catalog zone (tenant-scoped) | ||
| catz, err := r.GetCatalogZone(ctx, catalogID, tenantID) | ||
| if err != nil || catz == nil { | ||
| return fmt.Errorf("catalog zone not found") | ||
| } | ||
|
|
||
| // Get the zone ID for the catalog zone | ||
| zoneQuery := `SELECT id FROM dns_zones WHERE name = $1` | ||
| var zoneID string | ||
| err = r.db.QueryRowContext(ctx, zoneQuery, catz.ZoneName).Scan(&zoneID) | ||
| if err != nil { | ||
| return fmt.Errorf("catalog zone not provisioned: %w", err) | ||
| } | ||
|
|
||
| // Create PTR record for the zone entry | ||
| // Name: reversed zone name, Content: zone_name:zone_id[:group_id] | ||
| ptrContent := entry.ZoneName + ":" + entry.ZoneID | ||
| if entry.GroupID != "" { | ||
| ptrContent += ":" + entry.GroupID | ||
| } | ||
|
|
||
| // Generate unique name for PTR (reversed zone name for uniqueness) | ||
| // e.g., "example.com." -> "com.example." | ||
| reversedName := reverseDomainName(entry.ZoneName) | ||
|
|
||
| recordID := uuid.New().String() | ||
| query := ` | ||
| INSERT INTO dns_records (id, zone_id, name, type, content, ttl, created_at, updated_at) | ||
| VALUES ($1, $2, $3, 'PTR', $4, 3600, CURRENT_TIMESTAMP, CURRENT_TIMESTAMP) | ||
| ON CONFLICT (zone_id, LOWER(name), type, content) DO NOTHING | ||
| ` | ||
| _, err = r.db.ExecContext(ctx, query, recordID, zoneID, reversedName, ptrContent) | ||
| return err | ||
| } | ||
|
|
||
| // RemoveZoneFromCatalog removes a zone from a catalog zone's inventory. | ||
| func (r *PostgresRepository) RemoveZoneFromCatalog(ctx context.Context, catalogID string, tenantID string, zoneName string) error { | ||
| // Get the catalog zone (tenant-scoped) | ||
| catz, err := r.GetCatalogZone(ctx, catalogID, tenantID) | ||
| if err != nil || catz == nil { | ||
| return fmt.Errorf("catalog zone not found") | ||
| } | ||
|
|
||
| // Get the zone ID for the catalog zone | ||
| zoneQuery := `SELECT id FROM dns_zones WHERE name = $1` | ||
| var zoneID string | ||
| err = r.db.QueryRowContext(ctx, zoneQuery, catz.ZoneName).Scan(&zoneID) | ||
| if err != nil { | ||
| return fmt.Errorf("catalog zone not provisioned: %w", err) | ||
| } | ||
|
|
||
| // Find and delete the PTR record matching this zone | ||
| // Content format: "zone_name:zone_id[:group_id]" - use lexicographic range to avoid LIKE issues | ||
| // Match content that starts with "zoneName:" (exact prefix match, not substring) | ||
| query := ` | ||
| DELETE FROM dns_records | ||
| WHERE zone_id = $1 AND type = 'PTR' AND content >= $2 AND content < $3 | ||
| ` | ||
| // prefix is "zoneName:" and nextPrefix is the lexicographically next string | ||
| prefix := zoneName + ":" | ||
| nextPrefix := prefix + "\xff" | ||
| _, err = r.db.ExecContext(ctx, query, zoneID, prefix, nextPrefix) | ||
| return err | ||
| } | ||
|
|
||
| // CreateZoneFromCatalog creates a zone on the slave from catalog entry data. | ||
| func (r *PostgresRepository) CreateZoneFromCatalog(ctx context.Context, zone *domain.Zone, records []domain.Record) error { | ||
| // Create the zone | ||
| if err := r.CreateZone(ctx, zone); err != nil { | ||
| return err | ||
| } | ||
|
|
||
| // Create the zone's records | ||
| if len(records) > 0 { | ||
| if err := r.BatchCreateRecords(ctx, records); err != nil { | ||
| return err | ||
| } | ||
| } | ||
|
|
||
| return nil | ||
| } | ||
|
|
||
| // DeleteZoneByCatalogName deletes a zone by its catalog zone name. | ||
| func (r *PostgresRepository) DeleteZoneByCatalogName(ctx context.Context, catalogZoneName string, tenantID string) error { | ||
| query := `DELETE FROM dns_zones WHERE catalog_zone_name = $1 AND tenant_id = $2` | ||
| _, err := r.db.ExecContext(ctx, query, catalogZoneName, tenantID) | ||
| return err | ||
| } | ||
|
|
||
| // GetZoneByCatalogName retrieves a zone by its catalog zone name. | ||
| func (r *PostgresRepository) GetZoneByCatalogName(ctx context.Context, catalogZoneName string, tenantID string) (*domain.Zone, error) { | ||
| query := ` | ||
| SELECT id, tenant_id, name, role, master_server, catalog_zone_name, created_at, updated_at | ||
| FROM dns_zones WHERE catalog_zone_name = $1 AND tenant_id = $2 | ||
| ` | ||
| row := r.db.QueryRowContext(ctx, query, catalogZoneName, tenantID) | ||
| var z domain.Zone | ||
| err := row.Scan(&z.ID, &z.TenantID, &z.Name, &z.Role, &z.MasterServer, &z.CatalogZoneName, &z.CreatedAt, &z.UpdatedAt) | ||
| if errors.Is(err, sql.ErrNoRows) { | ||
| return nil, nil | ||
| } | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| return &z, nil | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="internal/adapters/repository/postgres.go"
echo "== imports (top of file) =="
sed -n '1,120p' "$FILE" | nl -ba | sed -n '1,120p'
echo
echo "== check for database/sql usage =="
rg -n "database/sql|sql\.ErrNoRows|errors\.Is\(err, sql\.ErrNoRows\)|ExecContext\(|QueryRowContext\(|QueryContext\(" "$FILE" || true
echo
echo "== check for pgx/v5 usage =="
rg -n "pgx|pgconn|pgtype|pgxpool|github.com/jackc/pgx/v5" "$FILE" || true
echo
echo "== inspect PostgresRepository struct / r.db type (near definition) =="
rg -n "type PostgresRepository" -n "$FILE"
# print around first match
line=$(rg -n "type PostgresRepository" "$FILE" | head -n1 | cut -d: -f1)
start=$((line-30)); end=$((line+60))
sed -n "${start},${end}p" "$FILE" | nl -ba | sed -n "1,120p"
echo
echo "== check r.db field definition (search for '.db ' type) =="
rg -n "db\s+[^=]*" "$FILE" | head -n 50 || true
echo
echo "== repo-wide: any other repository files using pgx/v5 =="
fd -e go internal/adapters/repository | head -n 50
fd -e go internal/adapters/repository | xargs -I{} rg -n "github.com/jackc/pgx/v5|pgxpool|pgx\.ErrNoRows|pgx/v5" "{}" 2>/dev/null | head -n 80 || trueRepository: poyrazK/cloudDNS
Length of output: 149
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="internal/adapters/repository/postgres.go"
echo "== first 160 lines =="
head -n 160 "$FILE" | sed -n '1,160p'
echo
echo "== database/sql usage checks =="
rg -n "database/sql|sql\.ErrNoRows|errors\.Is\(err, sql\.ErrNoRows\)|ExecContext\(|QueryRowContext\(|QueryContext\(" "$FILE" || true
echo
echo "== pgx/v5 usage checks =="
rg -n "pgx|pgconn|pgtype|pgxpool|github.com/jackc/pgx/v5" "$FILE" || true
echo
echo "== PostgresRepository struct definition block =="
line=$(rg -n "type PostgresRepository" "$FILE" | head -n1 | cut -d: -f1 || true)
if [[ -n "${line:-}" ]]; then
start=$((line-30))
end=$((line+80))
if (( start < 1 )); then start=1; fi
sed -n "${start},${end}p" "$FILE" | sed -n '1,120p'
else
echo "type PostgresRepository not found"
fi
echo
echo "== r.db field definition hints =="
rg -n "r\.db\s*[:=]|type .*db|db\s+\*" "$FILE" || true
rg -n "db\s+\w+\s*[\*]?sql\.|sql\.DB|sql\.Tx|pgx|pgxpool" "$FILE" || true
echo
echo "== repo-wide: other repository files using pgx/v5 =="
# list go files under internal/adapters/repository
fd -e go internal/adapters/repository | wc -l
fd -e go internal/adapters/repository | xargs -I{} rg -n "github.com/jackc/pgx/v5|pgxpool|pgconn" "{}" 2>/dev/null | head -n 120 || trueRepository: poyrazK/cloudDNS
Length of output: 14488
Switch internal/adapters/repository/postgres.go to pgx/v5 (don’t extend the database/sql adapter)
internal/adapters/repository/postgres.go is implemented on database/sql (db *sql.DB, NewPostgresRepository(db *sql.DB), and QueryContext/ExecContext/QueryRowContext calls), with no pgx/v5 usage. This violates the guideline for internal/adapters/repository/*.go to use pgx/v5 for PostgreSQL access, so the new catalog methods should be implemented on the repo-standard pgx/v5 stack.
🤖 Prompt for 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.
In `@internal/adapters/repository/postgres.go` around lines 1624 - 1854, The
repository currently uses database/sql (db *sql.DB, NewPostgresRepository(db
*sql.DB) and ExecContext/QueryContext/QueryRowContext) but must be ported to
pgx/v5; update the PostgresRepository to hold a pgxpool.Pool (or pgx.Conn)
instead of *sql.DB, change the constructor NewPostgresRepository to accept that
pgx type, and replace all ExecContext/QueryContext/QueryRowContext calls in
methods like CreateCatalogZone, GetCatalogZone, ListCatalogZones,
UpdateCatalogZoneVersion, DeleteCatalogZone, ListZoneCatalogEntries,
AddZoneToCatalog, RemoveZoneFromCatalog, CreateZoneFromCatalog,
DeleteZoneByCatalogName, and GetZoneByCatalogName with the corresponding pgx/v5
methods (Exec, Query, QueryRow with ctx) and swap sql.ErrNoRows checks for
pgx.ErrNoRows; ensure parameter placeholders remain $n and scanning semantics
match pgx (use row.Scan / rows.Next with pgx), and update imports to
github.com/jackc/pgx/v5/pgxpool and github.com/jackc/pgx/v5.
Source: Coding guidelines
Comment on lines
+1748
to
+1781
| func (r *PostgresRepository) AddZoneToCatalog(ctx context.Context, catalogID string, tenantID string, entry *domain.ZoneCatalogEntry) error { | ||
| // Get the catalog zone (tenant-scoped) | ||
| catz, err := r.GetCatalogZone(ctx, catalogID, tenantID) | ||
| if err != nil || catz == nil { | ||
| return fmt.Errorf("catalog zone not found") | ||
| } | ||
|
|
||
| // Get the zone ID for the catalog zone | ||
| zoneQuery := `SELECT id FROM dns_zones WHERE name = $1` | ||
| var zoneID string | ||
| err = r.db.QueryRowContext(ctx, zoneQuery, catz.ZoneName).Scan(&zoneID) | ||
| if err != nil { | ||
| return fmt.Errorf("catalog zone not provisioned: %w", err) | ||
| } | ||
|
|
||
| // Create PTR record for the zone entry | ||
| // Name: reversed zone name, Content: zone_name:zone_id[:group_id] | ||
| ptrContent := entry.ZoneName + ":" + entry.ZoneID | ||
| if entry.GroupID != "" { | ||
| ptrContent += ":" + entry.GroupID | ||
| } | ||
|
|
||
| // Generate unique name for PTR (reversed zone name for uniqueness) | ||
| // e.g., "example.com." -> "com.example." | ||
| reversedName := reverseDomainName(entry.ZoneName) | ||
|
|
||
| recordID := uuid.New().String() | ||
| query := ` | ||
| INSERT INTO dns_records (id, zone_id, name, type, content, ttl, created_at, updated_at) | ||
| VALUES ($1, $2, $3, 'PTR', $4, 3600, CURRENT_TIMESTAMP, CURRENT_TIMESTAMP) | ||
| ON CONFLICT (zone_id, LOWER(name), type, content) DO NOTHING | ||
| ` | ||
| _, err = r.db.ExecContext(ctx, query, recordID, zoneID, reversedName, ptrContent) | ||
| return err |
There was a problem hiding this comment.
Validate that the published zone actually belongs to tenantID.
This path writes entry.ZoneName / entry.ZoneID straight into the catalog PTR without proving that the referenced zone is owned by the caller's tenant. A caller who knows another zone's ID/name can therefore advertise it from their own catalog. Resolve the zone from dns_zones with id, name, and tenant_id first, and reject mismatches.
🤖 Prompt for 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.
In `@internal/adapters/repository/postgres.go` around lines 1748 - 1781, In
AddZoneToCatalog, you currently insert a PTR for entry.ZoneName/entry.ZoneID
without verifying ownership; query dns_zones for the target zone using
entry.ZoneID (or entry.ZoneName) and ensure the returned tenant_id equals the
incoming tenantID (and that id/name match), returning an error if not; only
after that verification proceed to build ptrContent and insert the dns_records
row (use the same checked zone id/value), keeping
reverseDomainName(entry.ZoneName) for name generation and preserving the
existing conflict behavior.
Comment on lines
+619
to
+624
| // 4. Batch create records | ||
| if len(records) > 0 { | ||
| if err := s.Repo.BatchCreateRecords(ctx, records); err != nil { | ||
| return fmt.Errorf("failed to create zone records: %w", err) | ||
| } | ||
| } |
There was a problem hiding this comment.
Rollback the zone when record import fails.
CreateZone is reverted on AXFR fetch failure, but not when BatchCreateRecords fails. That leaves a catalog-managed slave zone behind with no records, and the next poll will treat it as already provisioned.
Suggested fix
// 4. Batch create records
if len(records) > 0 {
if err := s.Repo.BatchCreateRecords(ctx, records); err != nil {
+ if delErr := s.Repo.DeleteZone(ctx, entry.ZoneID, tenantID); delErr != nil {
+ s.Logger.Error("failed to rollback zone creation after record import failure", "zone", entry.ZoneName, "delete_error", delErr)
+ }
return fmt.Errorf("failed to create zone records: %w", 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.
Suggested change
| // 4. Batch create records | |
| if len(records) > 0 { | |
| if err := s.Repo.BatchCreateRecords(ctx, records); err != nil { | |
| return fmt.Errorf("failed to create zone records: %w", err) | |
| } | |
| } | |
| // 4. Batch create records | |
| if len(records) > 0 { | |
| if err := s.Repo.BatchCreateRecords(ctx, records); err != nil { | |
| if delErr := s.Repo.DeleteZone(ctx, entry.ZoneID, tenantID); delErr != nil { | |
| s.Logger.Error("failed to rollback zone creation after record import failure", "zone", entry.ZoneName, "delete_error", delErr) | |
| } | |
| return fmt.Errorf("failed to create zone records: %w", err) | |
| } | |
| } |
🤖 Prompt for 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.
In `@internal/dns/server/client.go` around lines 619 - 624, When
BatchCreateRecords fails after CreateZone succeeded, roll back the
partially-created zone to avoid leaving an empty catalog-managed slave. In the
error path after s.Repo.BatchCreateRecords(ctx, records) check, call the
repository delete/remove method for the created zone (e.g.,
s.Repo.DeleteZone(ctx, zone.ID) or s.Repo.DeleteZoneByName(ctx, zone.Name)),
handle/log any deletion error, and then return the original BatchCreateRecords
error (wrapping both if helpful); update the error block around
Repo.BatchCreateRecords to perform this cleanup using the created zone
identifier (zone.ID or zone.Name).
Comment on lines
+638
to
+647
| for _, resp := range packets { | ||
| for _, ans := range resp.Answers { | ||
| dRec, err := repository.ConvertPacketRecordToDomain(ans, zoneID) | ||
| if err != nil { | ||
| s.Logger.Warn("failed to convert packet record", "error", err) | ||
| continue | ||
| } | ||
| dRec.TenantID = tenantID | ||
| records = append(records, dRec) | ||
| } |
There was a problem hiding this comment.
Drop the closing SOA before persisting AXFR results.
AXFR streams contain the SOA twice: once at the start and once at the end. This loop stores both copies, which either creates a duplicate SOA row or makes BatchCreateRecords fail under a uniqueness constraint.
Suggested fix
var records []domain.Record
+ soaCount := 0
for _, resp := range packets {
for _, ans := range resp.Answers {
+ if ans.Type == packet.SOA {
+ soaCount++
+ if soaCount > 1 {
+ continue
+ }
+ }
dRec, err := repository.ConvertPacketRecordToDomain(ans, zoneID)
if err != nil {
s.Logger.Warn("failed to convert packet record", "error", err)
continue
}📝 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.
Suggested change
| for _, resp := range packets { | |
| for _, ans := range resp.Answers { | |
| dRec, err := repository.ConvertPacketRecordToDomain(ans, zoneID) | |
| if err != nil { | |
| s.Logger.Warn("failed to convert packet record", "error", err) | |
| continue | |
| } | |
| dRec.TenantID = tenantID | |
| records = append(records, dRec) | |
| } | |
| var records []domain.Record | |
| soaCount := 0 | |
| for _, resp := range packets { | |
| for _, ans := range resp.Answers { | |
| if ans.Type == packet.SOA { | |
| soaCount++ | |
| if soaCount > 1 { | |
| continue | |
| } | |
| } | |
| dRec, err := repository.ConvertPacketRecordToDomain(ans, zoneID) | |
| if err != nil { | |
| s.Logger.Warn("failed to convert packet record", "error", err) | |
| continue | |
| } | |
| dRec.TenantID = tenantID | |
| records = append(records, dRec) | |
| } | |
| } |
🤖 Prompt for 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.
In `@internal/dns/server/client.go` around lines 638 - 647, The loop in client.go
is appending both SOA copies from AXFR; before adding a converted record
(ConvertPacketRecordToDomain -> dRec) check for SOA and skip duplicates: if the
DNS/type field indicates SOA (use ans.Type or dRec.Type) and there is already an
SOA in the in-memory records slice for this zone/tenant, do not append it so
BatchCreateRecords won't receive duplicate SOA rows. Implement the check inside
the existing loop that builds records (referencing ConvertPacketRecordToDomain,
dRec, records) and only append non-SOA or the first-seen SOA.
Comment on lines
+519
to
+533
| if s.CatalogPollingEnabled && len(s.CatalogZones) > 0 { | ||
| if s.CatalogMasterAddr == "" { | ||
| s.Logger.Warn("catalog polling enabled but CATALOG_MASTER_ADDR not set, skipping") | ||
| } else { | ||
| s.Logger.Info("starting catalog zone poller", | ||
| "zones", s.CatalogZones, | ||
| "master", s.CatalogMasterAddr, | ||
| "interval", s.CatalogPollInterval, | ||
| "tenant", s.CatalogTenantID, | ||
| ) | ||
| s.wg.Add(1) | ||
| go func() { | ||
| defer s.wg.Done() | ||
| s.StartCatalogPoller(s.lifecycleCtx, s.CatalogZones, s.CatalogMasterAddr, s.CatalogPollInterval) | ||
| }() |
There was a problem hiding this comment.
Skip catalog polling when Repo is nil.
handlePacket already supports repo-less servers, but this path starts the poller unconditionally. pollCatalogZone/syncZoneFromCatalog dereference s.Repo, so enabling catalog polling on an identity-only node will panic the background goroutine on its first sync.
Suggested fix
// Start catalog zone poller if configured (RFC 9432 slave-side)
if s.CatalogPollingEnabled && len(s.CatalogZones) > 0 {
- if s.CatalogMasterAddr == "" {
+ if s.Repo == nil {
+ s.Logger.Warn("catalog polling enabled but repository is nil, skipping")
+ } else if s.CatalogMasterAddr == "" {
s.Logger.Warn("catalog polling enabled but CATALOG_MASTER_ADDR not set, skipping")
} else {
s.Logger.Info("starting catalog zone poller",
"zones", s.CatalogZones,📝 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.
Suggested change
| if s.CatalogPollingEnabled && len(s.CatalogZones) > 0 { | |
| if s.CatalogMasterAddr == "" { | |
| s.Logger.Warn("catalog polling enabled but CATALOG_MASTER_ADDR not set, skipping") | |
| } else { | |
| s.Logger.Info("starting catalog zone poller", | |
| "zones", s.CatalogZones, | |
| "master", s.CatalogMasterAddr, | |
| "interval", s.CatalogPollInterval, | |
| "tenant", s.CatalogTenantID, | |
| ) | |
| s.wg.Add(1) | |
| go func() { | |
| defer s.wg.Done() | |
| s.StartCatalogPoller(s.lifecycleCtx, s.CatalogZones, s.CatalogMasterAddr, s.CatalogPollInterval) | |
| }() | |
| if s.CatalogPollingEnabled && len(s.CatalogZones) > 0 { | |
| if s.Repo == nil { | |
| s.Logger.Warn("catalog polling enabled but repository is nil, skipping") | |
| } else if s.CatalogMasterAddr == "" { | |
| s.Logger.Warn("catalog polling enabled but CATALOG_MASTER_ADDR not set, skipping") | |
| } else { | |
| s.Logger.Info("starting catalog zone poller", | |
| "zones", s.CatalogZones, | |
| "master", s.CatalogMasterAddr, | |
| "interval", s.CatalogPollInterval, | |
| "tenant", s.CatalogTenantID, | |
| ) | |
| s.wg.Add(1) | |
| go func() { | |
| defer s.wg.Done() | |
| s.StartCatalogPoller(s.lifecycleCtx, s.CatalogZones, s.CatalogMasterAddr, s.CatalogPollInterval) | |
| }() |
🤖 Prompt for 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.
In `@internal/dns/server/server.go` around lines 519 - 533, The catalog poller is
started even when s.Repo is nil which causes pollCatalogZone/syncZoneFromCatalog
to dereference s.Repo and panic; before starting the goroutine in the block that
checks s.CatalogPollingEnabled && len(s.CatalogZones) > 0, also verify s.Repo !=
nil (and log a warning similar to the CATALOG_MASTER_ADDR check) and skip
starting StartCatalogPoller if Repo is nil so that identity-only servers do not
spawn the poller that will call pollCatalogZone/syncZoneFromCatalog on a nil
Repo.
Comment on lines
+416
to
+474
| // CreateCatalogZone implements ports.DNSService for testing. | ||
| func (m *MockDNSService) CreateCatalogZone(_ context.Context, _ string, _ string) (*domain.CatalogZone, error) { | ||
| args := m.Called() | ||
| if args.Get(0) == nil { | ||
| return nil, args.Error(1) | ||
| } | ||
| return args.Get(0).(*domain.CatalogZone), args.Error(1) | ||
| } | ||
|
|
||
| // GetCatalogZone implements ports.DNSService for testing. | ||
| func (m *MockDNSService) GetCatalogZone(_ context.Context, _ string, _ string) (*domain.CatalogZone, error) { | ||
| args := m.Called() | ||
| if args.Get(0) == nil { | ||
| return nil, args.Error(1) | ||
| } | ||
| return args.Get(0).(*domain.CatalogZone), args.Error(1) | ||
| } | ||
|
|
||
| // ListCatalogZones implements ports.DNSService for testing. | ||
| func (m *MockDNSService) ListCatalogZones(_ context.Context, _ string) ([]domain.CatalogZone, error) { | ||
| args := m.Called() | ||
| return args.Get(0).([]domain.CatalogZone), args.Error(1) | ||
| } | ||
|
|
||
| // DeleteCatalogZone implements ports.DNSService for testing. | ||
| func (m *MockDNSService) DeleteCatalogZone(_ context.Context, _ string, _ string) error { | ||
| args := m.Called() | ||
| return args.Error(0) | ||
| } | ||
|
|
||
| // AddZoneToCatalog implements ports.DNSService for testing. | ||
| func (m *MockDNSService) AddZoneToCatalog(_ context.Context, _ string, _ string, _ string, _ string, _ string) error { | ||
| args := m.Called() | ||
| return args.Error(0) | ||
| } | ||
|
|
||
| // RemoveZoneFromCatalog implements ports.DNSService for testing. | ||
| func (m *MockDNSService) RemoveZoneFromCatalog(_ context.Context, _ string, _ string, _ string) error { | ||
| args := m.Called() | ||
| return args.Error(0) | ||
| } | ||
|
|
||
| // ListZoneCatalogEntries implements ports.DNSService for testing. | ||
| func (m *MockDNSService) ListZoneCatalogEntries(_ context.Context, _ string, _ string) ([]domain.ZoneCatalogEntry, error) { | ||
| args := m.Called() | ||
| return args.Get(0).([]domain.ZoneCatalogEntry), args.Error(1) | ||
| } | ||
|
|
||
| // PollCatalogZone implements ports.DNSService for testing. | ||
| func (m *MockDNSService) PollCatalogZone(_ context.Context, _ string, _ string) ([]domain.ZoneCatalogEntry, error) { | ||
| args := m.Called() | ||
| return args.Get(0).([]domain.ZoneCatalogEntry), args.Error(1) | ||
| } | ||
|
|
||
| // SyncZonesFromCatalog implements ports.DNSService for testing. | ||
| func (m *MockDNSService) SyncZonesFromCatalog(_ context.Context, _ string, _ string) error { | ||
| args := m.Called() | ||
| return args.Error(0) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
MockDNSService catalog methods don't forward parameters to m.Called().
Existing MockDNSService methods pass their parameters to m.Called() to enable testify argument matching (e.g., CreateZone at line 337 calls m.Called(zone)). The new catalog methods call m.Called() with no arguments, preventing tests from verifying that the correct parameters were passed or configuring behavior based on specific inputs.
♻️ Recommended fix: forward parameters to m.Called()
// CreateCatalogZone implements ports.DNSService for testing.
-func (m *MockDNSService) CreateCatalogZone(_ context.Context, _ string, _ string) (*domain.CatalogZone, error) {
- args := m.Called()
+func (m *MockDNSService) CreateCatalogZone(_ context.Context, name string, tenantID string) (*domain.CatalogZone, error) {
+ args := m.Called(name, tenantID)
if args.Get(0) == nil {
return nil, args.Error(1)
}
return args.Get(0).(*domain.CatalogZone), args.Error(1)
}
// GetCatalogZone implements ports.DNSService for testing.
-func (m *MockDNSService) GetCatalogZone(_ context.Context, _ string, _ string) (*domain.CatalogZone, error) {
- args := m.Called()
+func (m *MockDNSService) GetCatalogZone(_ context.Context, catalogID string, tenantID string) (*domain.CatalogZone, error) {
+ args := m.Called(catalogID, tenantID)
if args.Get(0) == nil {
return nil, args.Error(1)
}
return args.Get(0).(*domain.CatalogZone), args.Error(1)
}
// ListCatalogZones implements ports.DNSService for testing.
-func (m *MockDNSService) ListCatalogZones(_ context.Context, _ string) ([]domain.CatalogZone, error) {
- args := m.Called()
+func (m *MockDNSService) ListCatalogZones(_ context.Context, tenantID string) ([]domain.CatalogZone, error) {
+ args := m.Called(tenantID)
return args.Get(0).([]domain.CatalogZone), args.Error(1)
}
// DeleteCatalogZone implements ports.DNSService for testing.
-func (m *MockDNSService) DeleteCatalogZone(_ context.Context, _ string, _ string) error {
- args := m.Called()
+func (m *MockDNSService) DeleteCatalogZone(_ context.Context, catalogID string, tenantID string) error {
+ args := m.Called(catalogID, tenantID)
return args.Error(0)
}
// AddZoneToCatalog implements ports.DNSService for testing.
-func (m *MockDNSService) AddZoneToCatalog(_ context.Context, _ string, _ string, _ string, _ string, _ string) error {
- args := m.Called()
+func (m *MockDNSService) AddZoneToCatalog(_ context.Context, catalogID string, tenantID string, zoneName string, group string, masters string) error {
+ args := m.Called(catalogID, tenantID, zoneName, group, masters)
return args.Error(0)
}
// RemoveZoneFromCatalog implements ports.DNSService for testing.
-func (m *MockDNSService) RemoveZoneFromCatalog(_ context.Context, _ string, _ string, _ string) error {
- args := m.Called()
+func (m *MockDNSService) RemoveZoneFromCatalog(_ context.Context, catalogID string, tenantID string, zoneName string) error {
+ args := m.Called(catalogID, tenantID, zoneName)
return args.Error(0)
}
// ListZoneCatalogEntries implements ports.DNSService for testing.
-func (m *MockDNSService) ListZoneCatalogEntries(_ context.Context, _ string, _ string) ([]domain.ZoneCatalogEntry, error) {
- args := m.Called()
+func (m *MockDNSService) ListZoneCatalogEntries(_ context.Context, catalogID string, tenantID string) ([]domain.ZoneCatalogEntry, error) {
+ args := m.Called(catalogID, tenantID)
return args.Get(0).([]domain.ZoneCatalogEntry), args.Error(1)
}
// PollCatalogZone implements ports.DNSService for testing.
-func (m *MockDNSService) PollCatalogZone(_ context.Context, _ string, _ string) ([]domain.ZoneCatalogEntry, error) {
- args := m.Called()
+func (m *MockDNSService) PollCatalogZone(_ context.Context, masterAddr string, catalogZoneName string) ([]domain.ZoneCatalogEntry, error) {
+ args := m.Called(masterAddr, catalogZoneName)
return args.Get(0).([]domain.ZoneCatalogEntry), args.Error(1)
}
// SyncZonesFromCatalog implements ports.DNSService for testing.
-func (m *MockDNSService) SyncZonesFromCatalog(_ context.Context, _ string, _ string) error {
- args := m.Called()
+func (m *MockDNSService) SyncZonesFromCatalog(_ context.Context, masterAddr string, catalogZoneName string) error {
+ args := m.Called(masterAddr, catalogZoneName)
return args.Error(0)
}🤖 Prompt for 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.
In `@internal/testutil/mock_repo.go` around lines 416 - 474, The catalog-related
MockDNSService methods (CreateCatalogZone, GetCatalogZone, ListCatalogZones,
DeleteCatalogZone, AddZoneToCatalog, RemoveZoneFromCatalog,
ListZoneCatalogEntries, PollCatalogZone, SyncZonesFromCatalog) are calling
m.Called() with no arguments which prevents testify from matching inputs; update
each method to forward the received parameters into m.Called(...) in the same
order they appear in the signature (e.g., CreateCatalogZone should call
m.Called(ctx, arg1, arg2) or m.Called(arg1, arg2) consistent with other mocks),
keeping the existing return handling (args.Get(0), args.Error(...)) unchanged so
tests can assert and stub by arguments.
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 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 `@internal/adapters/api/handler_test.go`:
- Around line 113-120: The mock CreateCatalogZone in mockDNSService currently
ignores its input parameters and returns hardcoded values; update
mockDNSService.CreateCatalogZone to use the passed tenant and zone name
parameters (the function's second and third args) when constructing the returned
*domain.CatalogZone and when appending to m.catalogs so tests validate
request-to-service mapping; preserve existing fields like Version/Serial or
generate ID deterministically from tenant/zone (e.g., fmt.Sprintf) so tests
remain stable and reflective of inputs.
- Around line 821-829: The test currently makes the auth lookup optional by
using repo.On("GetAPIKeyByHash", ...).Return(...).Maybe(), which allows the
protected-route assertion to pass without exercising the auth path; remove the
.Maybe() so the repo mock requires the GetAPIKeyByHash call (i.e., use
repo.On("GetAPIKeyByHash", mock.Anything, mock.Anything).Return(...)) and, if
needed, tighten arguments to the expected hash or context values and/or add
repo.AssertExpectations(t) after handler invocation to ensure the auth lookup is
actually invoked for New/handler tests.
In `@internal/adapters/repository/postgres_test.go`:
- Around line 546-548: Several test calls ignore errors from repository methods
(e.g., repo.CreateCatalogZone) which can hide failures; update each ignored call
(repo.CreateCatalogZone at the locations noted and any other repo.* calls in the
same test: those at lines referenced) to assert the error is nil so tests fail
fast—replace the discard of the returned error with a test assertion (e.g.,
require.NoError(t, err) or t.Fatalf on err) immediately after calling the
functions (targeting repo.CreateCatalogZone and any other repo.* calls in the
same test functions).
In `@internal/adapters/repository/postgres_unit_test.go`:
- Around line 869-871: The test's mock.ExpectQuery for "SELECT id FROM dns_zones
WHERE name = \$1" is missing tenant scoping; update the SQL expectation to
include tenant_id (e.g., "SELECT id FROM dns_zones WHERE name = \$1 AND
tenant_id = \$2") and adjust the corresponding WithArgs calls (replace
WithArgs("catalog.example.com.") with WithArgs("catalog.example.com.", tenantID)
or the test's tenant variable) so the mock enforces tenant-scoped zone lookup;
make this change for both occurrences where mock.ExpectQuery + WithArgs are used
in the test.
- Around line 995-999: The test currently calls repo.CreateZoneFromCatalog(ctx,
zone, recs) and discards its returned error; change it to capture the error
(e.g., err := repo.CreateZoneFromCatalog(ctx, zone, recs)) and assert the
expected failure before checking mock.ExpectationsWereMet — for example use if
err == nil { t.Fatalf("expected error from CreateZoneFromCatalog, got nil") } or
compare against the specific expected error value/message, then proceed to call
mock.ExpectationsWereMet() and report any unmet expectations.
In `@internal/core/services/dns_service_test.go`:
- Around line 346-370: The mockRepo currently stores a single global
catalogEntries slice and its ListZoneCatalogEntries, AddZoneToCatalog and
RemoveZoneFromCatalog ignore catalogID/tenantID, breaking isolation; change the
mockRepo to index entries by catalogID+tenantID (e.g. a
map[string][]domain.ZoneCatalogEntry) and update ListZoneCatalogEntries,
AddZoneToCatalog and RemoveZoneFromCatalog to look up the correct map bucket
using the provided catalogID and tenantID, validate those keys before
reading/mutating, and only append/remove entries in that specific bucket so
tests exercise tenant/catalog routing correctly.
🪄 Autofix (Beta)
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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 06437f56-2f78-4637-8573-f0e379a8d405
📒 Files selected for processing (5)
internal/adapters/api/handler_test.gointernal/adapters/repository/postgres_test.gointernal/adapters/repository/postgres_unit_test.gointernal/adapters/repository/schema.sqlinternal/core/services/dns_service_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- internal/adapters/repository/schema.sql
Comment on lines
+113
to
+120
| func (m *mockDNSService) CreateCatalogZone(_ context.Context, _, _ string) (*domain.CatalogZone, error) { | ||
| if m.err != nil { | ||
| return nil, m.err | ||
| } | ||
| catz := &domain.CatalogZone{ID: "catz-new", TenantID: "t1", ZoneName: "catalog.example.com.", Version: "1", Serial: 1} | ||
| m.catalogs = append(m.catalogs, *catz) | ||
| return catz, nil | ||
| } |
There was a problem hiding this comment.
Mock CreateCatalogZone ignores inputs, masking handler bugs.
On Line 113 and Line 117, the mock does not use the provided tenant/zone name and always returns hardcoded values. That allows handler tests to pass even if request-to-service mapping is broken.
Suggested fix
-func (m *mockDNSService) CreateCatalogZone(_ context.Context, _, _ string) (*domain.CatalogZone, error) {
+func (m *mockDNSService) CreateCatalogZone(_ context.Context, tenantID, zoneName string) (*domain.CatalogZone, error) {
if m.err != nil {
return nil, m.err
}
- catz := &domain.CatalogZone{ID: "catz-new", TenantID: "t1", ZoneName: "catalog.example.com.", Version: "1", Serial: 1}
+ catz := &domain.CatalogZone{ID: "catz-new", TenantID: tenantID, ZoneName: zoneName, Version: "1", Serial: 1}
m.catalogs = append(m.catalogs, *catz)
return catz, nil
}📝 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.
Suggested change
| func (m *mockDNSService) CreateCatalogZone(_ context.Context, _, _ string) (*domain.CatalogZone, error) { | |
| if m.err != nil { | |
| return nil, m.err | |
| } | |
| catz := &domain.CatalogZone{ID: "catz-new", TenantID: "t1", ZoneName: "catalog.example.com.", Version: "1", Serial: 1} | |
| m.catalogs = append(m.catalogs, *catz) | |
| return catz, nil | |
| } | |
| func (m *mockDNSService) CreateCatalogZone(_ context.Context, tenantID, zoneName string) (*domain.CatalogZone, error) { | |
| if m.err != nil { | |
| return nil, m.err | |
| } | |
| catz := &domain.CatalogZone{ID: "catz-new", TenantID: tenantID, ZoneName: zoneName, Version: "1", Serial: 1} | |
| m.catalogs = append(m.catalogs, *catz) | |
| return catz, nil | |
| } |
🤖 Prompt for 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.
In `@internal/adapters/api/handler_test.go` around lines 113 - 120, The mock
CreateCatalogZone in mockDNSService currently ignores its input parameters and
returns hardcoded values; update mockDNSService.CreateCatalogZone to use the
passed tenant and zone name parameters (the function's second and third args)
when constructing the returned *domain.CatalogZone and when appending to
m.catalogs so tests validate request-to-service mapping; preserve existing
fields like Version/Serial or generate ID deterministically from tenant/zone
(e.g., fmt.Sprintf) so tests remain stable and reflective of inputs.
Comment on lines
+821
to
+829
| repo.On("GetAPIKeyByHash", mock.Anything, mock.Anything).Return(&domain.APIKey{ | ||
| ID: "key-1", | ||
| TenantID: "t1", | ||
| Role: domain.RoleAdmin, | ||
| Active: true, | ||
| ExpiresAt: nil, | ||
| CreatedAt: time.Now(), | ||
| }, nil).Maybe() | ||
| handler := New(svc, repo, slog.Default()) |
There was a problem hiding this comment.
Auth-path assertion is optional, so this test can pass without validating auth lookup.
At Line 828, .Maybe() makes GetAPIKeyByHash optional. This weakens security-path coverage for the protected route test.
Suggested fix
-repo.On("GetAPIKeyByHash", mock.Anything, mock.Anything).Return(&domain.APIKey{
+repo.On("GetAPIKeyByHash", mock.Anything, mock.Anything).Return(&domain.APIKey{
ID: "key-1",
TenantID: "t1",
Role: domain.RoleAdmin,
Active: true,
ExpiresAt: nil,
CreatedAt: time.Now(),
- }, nil).Maybe()
+ }, nil).Once()
...
mux.ServeHTTP(w, req)
+repo.AssertExpectations(t)🤖 Prompt for 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.
In `@internal/adapters/api/handler_test.go` around lines 821 - 829, The test
currently makes the auth lookup optional by using repo.On("GetAPIKeyByHash",
...).Return(...).Maybe(), which allows the protected-route assertion to pass
without exercising the auth path; remove the .Maybe() so the repo mock requires
the GetAPIKeyByHash call (i.e., use repo.On("GetAPIKeyByHash", mock.Anything,
mock.Anything).Return(...)) and, if needed, tighten arguments to the expected
hash or context values and/or add repo.AssertExpectations(t) after handler
invocation to ensure the auth lookup is actually invoked for New/handler tests.
Comment on lines
+995
to
+999
| _ = repo.CreateZoneFromCatalog(ctx, zone, recs) | ||
| // Verify CreateZone was called (Exec expectation met) | ||
| if err := mock.ExpectationsWereMet(); err != nil { | ||
| t.Errorf("unfulfilled expectations: %s", err) | ||
| } |
There was a problem hiding this comment.
CreateZoneFromCatalog subtest doesn’t assert its expected error.
Line 995 discards the return value even though the comment says failure is expected. This allows the test to pass without validating behavior.
Suggested fix
- _ = repo.CreateZoneFromCatalog(ctx, zone, recs)
+ err = repo.CreateZoneFromCatalog(ctx, zone, recs)
+ if err == nil {
+ t.Fatal("expected CreateZoneFromCatalog to return error in sqlmock UNNEST path")
+ }📝 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.
Suggested change
| _ = repo.CreateZoneFromCatalog(ctx, zone, recs) | |
| // Verify CreateZone was called (Exec expectation met) | |
| if err := mock.ExpectationsWereMet(); err != nil { | |
| t.Errorf("unfulfilled expectations: %s", err) | |
| } | |
| err = repo.CreateZoneFromCatalog(ctx, zone, recs) | |
| if err == nil { | |
| t.Fatal("expected CreateZoneFromCatalog to return error in sqlmock UNNEST path") | |
| } | |
| // Verify CreateZone was called (Exec expectation met) | |
| if err := mock.ExpectationsWereMet(); err != nil { | |
| t.Errorf("unfulfilled expectations: %s", err) | |
| } |
🤖 Prompt for 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.
In `@internal/adapters/repository/postgres_unit_test.go` around lines 995 - 999,
The test currently calls repo.CreateZoneFromCatalog(ctx, zone, recs) and
discards its returned error; change it to capture the error (e.g., err :=
repo.CreateZoneFromCatalog(ctx, zone, recs)) and assert the expected failure
before checking mock.ExpectationsWereMet — for example use if err == nil {
t.Fatalf("expected error from CreateZoneFromCatalog, got nil") } or compare
against the specific expected error value/message, then proceed to call
mock.ExpectationsWereMet() and report any unmet expectations.
Comment on lines
+346
to
+370
| func (m *mockRepo) ListZoneCatalogEntries(_ context.Context, catalogID string, tenantID string) ([]domain.ZoneCatalogEntry, error) { | ||
| if m.err != nil { | ||
| return nil, m.err | ||
| } | ||
| return m.catalogEntries, nil | ||
| } | ||
| func (m *mockRepo) AddZoneToCatalog(_ context.Context, catalogID string, tenantID string, entry *domain.ZoneCatalogEntry) error { | ||
| if m.err != nil { | ||
| return m.err | ||
| } | ||
| m.catalogEntries = append(m.catalogEntries, *entry) | ||
| return nil | ||
| } | ||
| func (m *mockRepo) RemoveZoneFromCatalog(_ context.Context, catalogID string, tenantID string, zoneName string) error { | ||
| if m.err != nil { | ||
| return m.err | ||
| } | ||
| var remaining []domain.ZoneCatalogEntry | ||
| for _, e := range m.catalogEntries { | ||
| if e.ZoneName != zoneName { | ||
| remaining = append(remaining, e) | ||
| } | ||
| } | ||
| m.catalogEntries = remaining | ||
| return nil |
There was a problem hiding this comment.
Catalog-entry mock behavior ignores catalog/tenant identity, weakening isolation tests.
From Line 346-370, list/add/remove operations mutate/read a global slice without validating catalogID and tenantID. This can let service tests pass even if tenant/catalog routing is wrong.
Suggested direction
func (m *mockRepo) AddZoneToCatalog(_ context.Context, catalogID string, tenantID string, entry *domain.ZoneCatalogEntry) error {
if m.err != nil {
return m.err
}
+ found := false
+ for _, c := range m.catalogs {
+ if c.ID == catalogID && c.TenantID == tenantID {
+ found = true
+ break
+ }
+ }
+ if !found {
+ return errors.New("catalog not found for tenant")
+ }
m.catalogEntries = append(m.catalogEntries, *entry)
return nil
}(Apply similar catalog+tenant validation to RemoveZoneFromCatalog and ListZoneCatalogEntries.)
🤖 Prompt for 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.
In `@internal/core/services/dns_service_test.go` around lines 346 - 370, The
mockRepo currently stores a single global catalogEntries slice and its
ListZoneCatalogEntries, AddZoneToCatalog and RemoveZoneFromCatalog ignore
catalogID/tenantID, breaking isolation; change the mockRepo to index entries by
catalogID+tenantID (e.g. a map[string][]domain.ZoneCatalogEntry) and update
ListZoneCatalogEntries, AddZoneToCatalog and RemoveZoneFromCatalog to look up
the correct map bucket using the provided catalogID and tenantID, validate those
keys before reading/mutating, and only append/remove entries in that specific
bucket so tests exercise tenant/catalog routing correctly.
poyrazK
added a commit
that referenced
this pull request
Jun 11, 2026
1. Implement PollCatalogZone: resolve catalog by zone name via new GetCatalogZoneByName repo method, then call ListZoneCatalogEntries. SyncZonesFromCatalog: add comment explaining it's handled by server poller in client.go. 2. Add warning log when CATALOG_TENANT_ID is empty and falling back to NodeID for catalog-provisioned zones. 3. ListZoneCatalogEntries: remove redundant subquery, use catz.TenantID directly from GetCatalogZone result. 4. AddZoneToCatalog: check RowsAffected to detect duplicate entries from ON CONFLICT DO NOTHING. 5. Add partial index on dns_zones(catalog_zone_name) for GetZoneByCatalogName queries. Also update mocks (mockDNSSECRepo, mockRepo, auditMockRepo) to implement the new GetCatalogZoneByName interface method.
…e stubbed features - Add tenant authorization to ListCatalogEntries endpoint - Fix RemoveZoneFromCatalog LIKE deletion with lexicographic range - Complete syncZoneFromCatalog to actually provision zones via AXFR - Add catalog version tracking to skip unnecessary re-syncs
…ry handlers Previously these handlers extracted the catalog ID from the path but did not validate that the authenticated user's tenant owns that catalog zone. Any admin could modify ANY tenant's catalog by guessing a catalog ID. Now both handlers extract tenantID from context and pass it through the service layer to the repository, which enforces WHERE tenant_id = on all catalog operations. GetCatalogZone, AddZoneToCatalog, RemoveZoneFromCatalog, and ListZoneCatalogEntries all now require tenantID for authorization.
…rns error Previously the function returned nil after AXFR failed, silently leaving a zone with zero records. Now it logs an error, deletes the partially-created zone, and returns the error to the caller. Also refactors fetchCatalogEntries and fetchZoneRecords to share AXFR boilerplate via a new fetchAXFRPackets helper, eliminating ~75 lines of duplicated TCP/dial code.
Adds CATALOG_POLLING_ENABLED, CATALOG_ZONES, CATALOG_MASTER_ADDR, CATALOG_POLL_INTERVAL, and CATALOG_TENANT_ID env vars. The poller is now wired into Run() and starts automatically when enabled. Also adds TTL-based cleanup to catalogState.lastSeenSerial: lastSeenAt map tracks entry ages, cleanup runs every half-ttl (default 12h, ttl 24h) to prevent unbounded memory growth from stale entries.
…uery syncZoneFromCatalog was calling ListZones(ctx, tenantID) and iterating all zones to check if a zone with matching catalogZoneName exists. This is O(n) per entry. New GetZoneByCatalogName(ctx, catalogZoneName, tenantID) does a single WHERE catalog_zone_name = AND tenant_id = query - O(1). Interface and postgres implementation added; all test mocks updated.
…holesale replacement
Add test coverage across repository (integration + unit), service, and handler layers for catalog zone functionality: - Repository integration tests: CreateCatalogZone, GetCatalogZone, ListCatalogZones, UpdateCatalogZoneVersion, DeleteCatalogZone, AddZoneToCatalog, ListZoneCatalogEntries, lexicographic range edge case, GetZoneByCatalogName, CreateZoneFromCatalog - Repository unit tests: SQL query verification with sqlmock for all catalog SQL operations - Service layer tests: mockRepo behavioral methods for catalog operations, tenant isolation, serial increment - Handler layer tests: all 7 catalog endpoints with success/error/unauthorized variants Fix TestGetCatalogZone_Success: use mux.ServeHTTP with auth middleware instead of calling handler directly, which bypasses tenant context setup.
1. Implement PollCatalogZone: resolve catalog by zone name via new GetCatalogZoneByName repo method, then call ListZoneCatalogEntries. SyncZonesFromCatalog: add comment explaining it's handled by server poller in client.go. 2. Add warning log when CATALOG_TENANT_ID is empty and falling back to NodeID for catalog-provisioned zones. 3. ListZoneCatalogEntries: remove redundant subquery, use catz.TenantID directly from GetCatalogZone result. 4. AddZoneToCatalog: check RowsAffected to detect duplicate entries from ON CONFLICT DO NOTHING. 5. Add partial index on dns_zones(catalog_zone_name) for GetZoneByCatalogName queries. Also update mocks (mockDNSSECRepo, mockRepo, auditMockRepo) to implement the new GetCatalogZoneByName interface method.
- Add catalog zones feature description to README.md under Advanced DNS Standards - Add catalog zones feature to features.md - Create ADR 0012 documenting the RFC 9432 implementation decisions: - Schema design (catalog_zones table, dns_zones columns) - API endpoints (7 REST endpoints) - Storage format (PTR records with zone_name:zone_id[:group_id] content) - Lexicographic range deletion pattern - Polling architecture and lifecycle - Tenant isolation strategy
poyrazK
force-pushed
the
release/catalog-zones
branch
from
June 11, 2026 18:22
5242563 to
b19c4a9
Compare
- Add GetCatalogZoneByName and GetCatalogZoneByName_NotFound unit tests in postgres_unit_test.go (sqlmock) - Add CreateZoneFromCatalog_ThenDeleteByCatalogName integration test in postgres_test.go (simulates rollback path) - Add TestCatalogPollerState_Cleanup unit test in server_test.go (verifies TTL eviction removes stale entries) - Add TestMockRepo_GetCatalogZoneByName and _NotFound tests in mock_repo_test.go (testify assertions)
poyrazK
force-pushed
the
release/catalog-zones
branch
from
June 11, 2026 18:28
b19c4a9 to
82d4c27
Compare
…evive, empty-block)
…g methods, and MockRepo catalog methods
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@cmd/clouddns/main_test.go`:
- Around line 475-479: The test mutates process-wide env vars by iterating
os.Environ and calling os.Unsetenv for keys with strings.HasPrefix(e,
"TRUST_ANCHOR_") without restoring them; change this to capture the prior state
(map of key->value or a set of keys that existed), perform the Unsetenv only for
the test, and register t.Cleanup to restore each prior value (or re-unset keys
that didn't exist) — you can wrap this logic in a helper like
clearTrustAnchorsForTest(t) and use the same approach where Unsetenv currently
appears (the loop using os.Environ, strings.HasPrefix and strings.SplitN).
🪄 Autofix (Beta)
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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 38a03db5-8cd4-4ca2-8a4d-5ce6128d6dc4
📒 Files selected for processing (24)
README.mdcmd/clouddns/main_test.godocs/decisions/0012-catalog-zones-rfc-9432.mdfeatures.mdinternal/adapters/api/handler.gointernal/adapters/api/handler_test.gointernal/adapters/repository/postgres.gointernal/adapters/repository/postgres_test.gointernal/adapters/repository/postgres_unit_test.gointernal/adapters/repository/schema.sqlinternal/core/config/dnssec_test.gointernal/core/domain/dns.gointernal/core/ports/ports.gointernal/core/services/anycast_manager_test.gointernal/core/services/dns_service.gointernal/core/services/dns_service_test.gointernal/core/services/dnssec_service_test.gointernal/core/utils/time_test.gointernal/dns/packet/packet.gointernal/dns/server/client.gointernal/dns/server/server.gointernal/dns/server/server_test.gointernal/testutil/mock_repo.gointernal/testutil/mock_repo_test.go
💤 Files with no reviewable changes (1)
- internal/dns/server/server.go
✅ Files skipped from review due to trivial changes (4)
- internal/core/utils/time_test.go
- features.md
- README.md
- docs/decisions/0012-catalog-zones-rfc-9432.md
🚧 Files skipped from review as they are similar to previous changes (11)
- internal/core/services/anycast_manager_test.go
- internal/core/services/dnssec_service_test.go
- internal/adapters/repository/schema.sql
- internal/dns/packet/packet.go
- internal/dns/server/server_test.go
- internal/core/ports/ports.go
- internal/core/domain/dns.go
- internal/adapters/api/handler.go
- internal/dns/server/client.go
- internal/testutil/mock_repo.go
- internal/adapters/api/handler_test.go
👮 Files not reviewed due to content moderation or server errors (5)
- internal/adapters/repository/postgres.go
- internal/adapters/repository/postgres_test.go
- internal/adapters/repository/postgres_unit_test.go
- internal/core/services/dns_service.go
- internal/core/services/dns_service_test.go
Comment on lines
+475
to
+479
| for _, e := range os.Environ() { | ||
| if strings.HasPrefix(e, "TRUST_ANCHOR_") { | ||
| os.Unsetenv(strings.SplitN(e, "=", 2)[0]) | ||
| } | ||
| } |
There was a problem hiding this comment.
Avoid process-wide env mutation without restoration in subtests.
Line 475, Line 488, Line 506, and Line 526 unset TRUST_ANCHOR_* variables via os.Unsetenv but never restore prior values. That can leak state across tests and cause order-dependent flakes. Prefer capturing prior values and restoring in t.Cleanup (or isolate via helper that does this once per subtest).
Also applies to: 488-492, 506-510, 526-530
🤖 Prompt for 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.
In `@cmd/clouddns/main_test.go` around lines 475 - 479, The test mutates
process-wide env vars by iterating os.Environ and calling os.Unsetenv for keys
with strings.HasPrefix(e, "TRUST_ANCHOR_") without restoring them; change this
to capture the prior state (map of key->value or a set of keys that existed),
perform the Unsetenv only for the test, and register t.Cleanup to restore each
prior value (or re-unset keys that didn't exist) — you can wrap this logic in a
helper like clearTrustAnchorsForTest(t) and use the same approach where Unsetenv
currently appears (the loop using os.Environ, strings.HasPrefix and
strings.SplitN).
…o improve testability - Add TCPConnFactory interface to client.go for mocking net.DialTimeout in performIXFR, performAXFR, and fetchAXFRPackets - Add TickerFactory and Ticker interfaces to client.go for mocking time.Ticker in StartCatalogPoller - Inject sleepFn and readFileFn package variables in cmd/bench/main.go to allow mocking in tests - Add mockTCPFactory and mockTicker test doubles in client_test.go - Add tests for dial error paths and catalog poller with mock ticker This refactoring enables testing of previously untestable code paths, contributing to coverage improvement toward the 80% CI threshold.
Add tests for: - fetchCatalogEntries dial error path - fetchZoneRecords dial error path - pollCatalogZone CZTR error and empty response paths - StartCatalogPoller with mock ticker These tests increase coverage of the previously untestable catalog poller chain, contributing to coverage improvement.
Add tests for: - PollCatalogZone with valid catalog zone returns entries - PollCatalogZone when catalog zone not found returns nil - PollCatalogZone propagates repo errors - SyncZonesFromCatalog is a no-op stub Also fix mockRepo.GetCatalogZoneByName to handle empty tenantID as a wildcard match, which matches the actual service behavior. This pushes coverage to 80.0%.
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
1 participant
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
Security fixes
Robustness fixes
Code quality fixes
New features (slave-side RFC 9432)
Test coverage
Commits (5 since last review)
Summary by CodeRabbit
New Features
Tests
Documentation