fix(dns): resolve DNS record state consistency and deletion issues - #21
Conversation
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📝 WalkthroughWalkthroughBumps provider version to 0.1.22 and adds a DNSEntry model plus enhanced DNS record handling: create uses a 30s retry, read uses normalized name/TXT comparisons and skips disabled records, and delete may fetch, delete, and selectively recreate records to preserve non-deleted entries. Changes
Sequence Diagram(s)sequenceDiagram
participant Provider as Provider (Terraform)
participant Client as Hostinger API Client
participant API as Hostinger API
Provider->>Client: Create DNS record request
Client->>API: POST /dns/records (create)
API-->>Client: 202/Created
Client->>Provider: Retry-read loop (wait up to 30s)
loop retry until visible or timeout
Provider->>Client: List/Get DNS records
Client->>API: GET /dns/records
API-->>Client: records list
Client-->>Provider: records (normalized comparison)
end
Provider->>Client: Read DNS record (normalization, TXT quote handling)
Client->>API: GET /dns/records
API-->>Client: records
Client-->>Provider: matched record (sets fields & ID)
Provider->>Client: Delete DNS record
Client->>API: GET /dns/records (fetch current)
API-->>Client: records
alt other records with same name/type exist
Client->>API: DELETE multiple /dns/records (delete then recreate kept ones)
API-->>Client: 200/204
Client-->>Provider: deletion+recreate result
else no other records
Client->>API: DELETE /dns/records/{id}
API-->>Client: 200/204
Client-->>Provider: deletion result
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~30 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 |
1fc8f9a to
2708cb0
Compare
![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
🤖 Fix all issues with AI agents
In `@hostinger/dns_record.go`:
- Around line 282-294: The delete logic in the loop that sets hasOtherRecords
uses normalize on record content which lowercases TXT values; change the content
comparison so that for recordType == "TXT" you compare rec.Content to
valueToDelete without calling normalize (preserving case), and for other types
continue using normalize(rec.Content) != normalize(valueToDelete); keep the
existing normalize(name) and strings.EqualFold(entry.Type, recordType) checks
and the rec.IsDisabled guard (symbols: hasOtherRecords, entries loop,
entry.Name, entry.Type, rec.IsDisabled, rec.Content, normalize, name,
recordType, valueToDelete).
- Around line 192-200: The current comparison lowercases values via normalize
which breaks case-sensitive TXT records; update the logic in the block
referencing normalize, recordType, contentNorm, valueNorm (and inputs
rec.Content and value) so that when recordType == "TXT" you do not call
normalize (preserve original case) and only trim surrounding quotes for
comparison, while for non-TXT types continue to use normalize as before; ensure
the downstream comparison uses the adjusted contentNorm/valueNorm variables.
- Around line 10-13: Replace the deprecated helper/resource usage with
helper/retry: update the imports to remove
"github.com/hashicorp/terraform-plugin-sdk/v2/helper/resource" and add
"github.com/hashicorp/terraform-plugin-sdk/v2/helper/retry", then in the retry
block around the DNS record wait logic (the code using resource.Retry,
resource.RetryableError, resource.NonRetryableError — lines ~107-122) change
those calls to retry.Retry, retry.RetryableError, and retry.NonRetryableError
respectively so the retry logic uses the new package and types.
- Around line 107-124: Replace the deprecated resource.* retry usage with the
helper/retry package: import
"github.com/hashicorp/terraform-plugin-sdk/v2/helper/retry", change
resource.Retry(...) to retry.Retry(...), and replace
resource.RetryError/resource.RetryableError/resource.NonRetryableError with
retry.RetryError/retry.RetryableError/retry.NonRetryableError while keeping the
same callback logic that calls resourceHostingerDNSRecordRead(d, meta) and
checks d.Id(); preserve the outer error wrap ("error waiting for DNS record to
be created: %w").
- Around line 131-137: The empty-zone guard in the Read function must not
silently return nil; when the local variable zone (obtained from d.Get("zone"))
is empty during Read (which can be invoked via retry.Retry after Create), treat
this as a configuration error and return a proper error/diagnostic instead of
preserving state. Replace the if zone == "" { return nil } with logic that
returns an explicit error (or Terraform diagnostic) indicating "zone is
required" (so callers like Create and retry.Retry surface the failure), or
remove the guard entirely if you confirm zone is always set; locate this change
in the Read method where zone is read and in the Create->Read interaction to
ensure failures are propagated.
🧹 Nitpick comments (3)
hostinger/dns_record.go (3)
243-246: Extract duplicatenormalizehelper to package level.The
normalizefunction is defined identically in bothresourceHostingerDNSRecordRead(lines 177-179) andresourceHostingerDNSRecordDelete(lines 244-246). Extract it to a package-level function.♻️ Suggested refactor
Add at package level:
// normalizeDNSName normalizes a DNS name for comparison by lowercasing and removing trailing dots. func normalizeDNSName(s string) string { return strings.TrimSuffix(strings.ToLower(s), ".") }Then replace inline definitions in both Read and Delete functions.
356-359: Consider using retry logic instead of fixed sleep.A fixed 2-second sleep may be insufficient for propagation in some cases or unnecessarily slow in others. Consider using retry logic similar to the Create function for more robust handling.
268-276: Extract duplicated DNS entry struct to package level.The same anonymous struct is defined in both
resourceHostingerDNSRecordRead(lines 162-170) andresourceHostingerDNSRecordDelete(lines 268-276). Extract to a named type for reuse.♻️ Suggested refactor
// dnsZoneEntry represents a DNS record entry from the Hostinger API. type dnsZoneEntry struct { Name string `json:"name"` Type string `json:"type"` TTL int `json:"ttl"` Records []struct { Content string `json:"content"` IsDisabled bool `json:"is_disabled"` } `json:"records"` }Then use
var entries []dnsZoneEntryin both functions.
2708cb0 to
12fd99c
Compare
12fd99c to
596e7ba
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@hostinger/dns_record.go`:
- Around line 190-211: The non-TXT record content comparisons must normalize
trailing dots and case before comparing: add a helper normalizeDNSValue (similar
to normalizeDNSName) that trims trailing dots and lowercases the value, then
replace the non-TXT branch in the entries loop (where
strings.EqualFold(rec.Content, value) is used and compareTXTContent is used for
TXT) to compare normalizeDNSValue(rec.Content) == normalizeDNSValue(value); also
update the delete logic where valueToDelete is matched against record content to
use normalizeDNSValue so CNAME/NS/MX/SRV values with/without trailing dots are
treated equal.
- Around line 128-146: resourceHostingerDNSRecordRead clears the Terraform
resource ID (via d.SetId("")) causing subsequent retry iterations to see an
empty ID and fail when parsing it; before entering the retry loop capture the
synthetic ID value created by d.SetId(fmt.Sprintf("%s|%s|%s", name, recordType,
value)) into a local variable (e.g., syntheticID) and inside the retry.Retry
closure restore it with d.SetId(syntheticID) before calling
resourceHostingerDNSRecordRead so the read logic always has the expected ID
until the record is truly absent or the operation completes.
| // Use synthetic ID to track record uniquely | ||
| d.SetId(fmt.Sprintf("%s|%s|%s", name, recordType, value)) | ||
|
|
||
| return resourceHostingerDNSRecordRead(d, meta) | ||
| // Use retry logic to handle eventual consistency | ||
| err = retry.Retry(30*time.Second, func() *retry.RetryError { | ||
| err := resourceHostingerDNSRecordRead(d, meta) | ||
| if err != nil { | ||
| return retry.NonRetryableError(err) | ||
| } | ||
| // Check if the record was found | ||
| if d.Id() == "" { | ||
| return retry.RetryableError(fmt.Errorf("waiting for DNS record to be available")) | ||
| } | ||
| return nil | ||
| }) | ||
|
|
||
| if err != nil { | ||
| return fmt.Errorf("error waiting for DNS record to be created: %w", err) | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -name "dns_record.go" -type fRepository: hostinger/terraform-provider-hostinger
Length of output: 107
🏁 Script executed:
head -150 hostinger/dns_record.go | tail -50Repository: hostinger/terraform-provider-hostinger
Length of output: 1185
🏁 Script executed:
grep -n "resourceHostingerDNSRecordRead" hostinger/dns_record.goRepository: hostinger/terraform-provider-hostinger
Length of output: 269
🏁 Script executed:
rg "func resourceHostingerDNSRecordRead" -A 30Repository: hostinger/terraform-provider-hostinger
Length of output: 1716
🏁 Script executed:
rg "retry\.Retry" --type goRepository: hostinger/terraform-provider-hostinger
Length of output: 305
🏁 Script executed:
rg "retry\.RetryContext" --type goRepository: hostinger/terraform-provider-hostinger
Length of output: 196
🏁 Script executed:
head -20 hostinger/dns_record.goRepository: hostinger/terraform-provider-hostinger
Length of output: 458
🏁 Script executed:
sed -n '152,220p' hostinger/dns_record.goRepository: hostinger/terraform-provider-hostinger
Length of output: 2211
🏁 Script executed:
rg "d\.SetId" hostinger/dns_record.goRepository: hostinger/terraform-provider-hostinger
Length of output: 243
🏁 Script executed:
rg "d\.SetId\(\"\"\)" hostinger/dns_record.go -B 3 -A 1Repository: hostinger/terraform-provider-hostinger
Length of output: 270
🏁 Script executed:
sed -n '128,150p' hostinger/dns_record.goRepository: hostinger/terraform-provider-hostinger
Length of output: 714
🏁 Script executed:
sed -n '180,230p' hostinger/dns_record.goRepository: hostinger/terraform-provider-hostinger
Length of output: 1822
🏁 Script executed:
sed -n '152,165p' hostinger/dns_record.goRepository: hostinger/terraform-provider-hostinger
Length of output: 533
Preserve the synthetic ID across retry attempts to prevent "unexpected ID format" errors.
When resourceHostingerDNSRecordRead doesn't find the record, it clears the ID with d.SetId(""). On the next retry iteration, the function tries to parse the empty ID and fails with fmt.Errorf("unexpected ID format: %s", d.Id()). Store the ID before the retry loop and restore it when retrying.
Proposed fix
// Use synthetic ID to track record uniquely
- d.SetId(fmt.Sprintf("%s|%s|%s", name, recordType, value))
+ id := fmt.Sprintf("%s|%s|%s", name, recordType, value)
+ d.SetId(id)
// Use retry logic to handle eventual consistency
err = retry.RetryContext(context.Background(), 30*time.Second, func() *retry.RetryError {
err := resourceHostingerDNSRecordRead(d, meta)
if err != nil {
return retry.NonRetryableError(err)
}
// Check if the record was found
if d.Id() == "" {
+ d.SetId(id)
return retry.RetryableError(fmt.Errorf("waiting for DNS record to be available"))
}
return nil
})🧰 Tools
🪛 GitHub Actions: lint
[error] 132-132: SA1019: retry.Retry is deprecated: Please use RetryContext to ensure proper plugin shutdown (staticcheck). Command: golangci-lint run --timeout 5m --verbose
🪛 GitHub Check: golangci-lint
[failure] 132-132:
SA1019: retry.Retry is deprecated: Please use RetryContext to ensure proper plugin shutdown (staticcheck)
🤖 Prompt for AI Agents
In `@hostinger/dns_record.go` around lines 128 - 146,
resourceHostingerDNSRecordRead clears the Terraform resource ID (via
d.SetId("")) causing subsequent retry iterations to see an empty ID and fail
when parsing it; before entering the retry loop capture the synthetic ID value
created by d.SetId(fmt.Sprintf("%s|%s|%s", name, recordType, value)) into a
local variable (e.g., syntheticID) and inside the retry.Retry closure restore it
with d.SetId(syntheticID) before calling resourceHostingerDNSRecordRead so the
read logic always has the expected ID until the record is truly absent or the
operation completes.
| for _, entry := range entries { | ||
| if normalize(entry.Name) == normalize(name) && strings.EqualFold(entry.Type, recordType) { | ||
| // Normalize names for comparison (case-insensitive, no trailing dots) | ||
| entryNameNorm := normalizeDNSName(entry.Name) | ||
| searchNameNorm := normalizeDNSName(name) | ||
|
|
||
| if entryNameNorm == searchNameNorm && strings.EqualFold(entry.Type, recordType) { | ||
| for _, rec := range entry.Records { | ||
| if !rec.IsDisabled && normalize(rec.Content) == normalize(value) { | ||
| if rec.IsDisabled { | ||
| continue | ||
| } | ||
|
|
||
| // Compare content based on record type | ||
| var contentMatch bool | ||
| if recordType == "TXT" { | ||
| // TXT records: case-sensitive content, handle quotes | ||
| contentMatch = compareTXTContent(rec.Content, value) | ||
| } else { | ||
| // Other records: case-insensitive content comparison | ||
| contentMatch = strings.EqualFold(rec.Content, value) | ||
| } | ||
|
|
||
| if contentMatch { |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Locate and examine the DNS record file
find . -name "dns_record.go" -path "*/hostinger/*" | head -5Repository: hostinger/terraform-provider-hostinger
Length of output: 107
🏁 Script executed:
# Get context around the specified lines
cat -n hostinger/dns_record.go | sed -n '180,220p'Repository: hostinger/terraform-provider-hostinger
Length of output: 1740
🏁 Script executed:
# Check for the normalizeDNSName function and any related normalization helpers
rg "normalizeDNSName|normalizeDNSValue|compareTXTContent" hostinger/dns_record.go -A 5Repository: hostinger/terraform-provider-hostinger
Length of output: 2074
🏁 Script executed:
# Look for delete/remove logic that handles DNS records
rg "Delete|Remove|delete" hostinger/dns_record.go -B 2 -A 5Repository: hostinger/terraform-provider-hostinger
Length of output: 3547
🏁 Script executed:
# Search for any tests related to DNS record comparison
fd "test" hostinger/ -name "*test.go" | head -5Repository: hostinger/terraform-provider-hostinger
Length of output: 312
🏁 Script executed:
# Check if trailing dots are mentioned anywhere in the codebase
rg "trailing.*dot|TrimSuffix.*\\\." hostinger/ -iRepository: hostinger/terraform-provider-hostinger
Length of output: 312
Normalize trailing dots in non-TXT record content comparisons.
For CNAME/NS/MX/SRV records, the API returns values with trailing dots while user input may omit them. Using strings.EqualFold() alone (lines 209 and delete logic) will miss these matches, causing perpetual diffs and failed deletes. Add a normalizeDNSValue() helper similar to the existing normalizeDNSName() that trims trailing dots and lowercases before comparison, and apply it consistently across both read and delete logic.
🔧 Suggested normalization
func compareTXTContent(content1, content2 string) bool {
// Remove surrounding quotes for comparison, but preserve case
clean1 := strings.Trim(content1, "\"")
clean2 := strings.Trim(content2, "\"")
return clean1 == clean2
}
+
+// normalizeDNSValue normalizes record values for comparison (non-TXT)
+func normalizeDNSValue(value string) string {
+ return strings.TrimSuffix(strings.ToLower(strings.TrimSpace(value)), ".")
+}
@@
if recordType == "TXT" {
// TXT records: case-sensitive content, handle quotes
contentMatch = compareTXTContent(rec.Content, value)
} else {
// Other records: case-insensitive content comparison
- contentMatch = strings.EqualFold(rec.Content, value)
+ contentMatch = normalizeDNSValue(rec.Content) == normalizeDNSValue(value)
}Apply the same normalization in the delete logic where non-TXT records are matched against valueToDelete.
🤖 Prompt for AI Agents
In `@hostinger/dns_record.go` around lines 190 - 211, The non-TXT record content
comparisons must normalize trailing dots and case before comparing: add a helper
normalizeDNSValue (similar to normalizeDNSName) that trims trailing dots and
lowercases the value, then replace the non-TXT branch in the entries loop (where
strings.EqualFold(rec.Content, value) is used and compareTXTContent is used for
TXT) to compare normalizeDNSValue(rec.Content) == normalizeDNSValue(value); also
update the delete logic where valueToDelete is matched against record content to
use normalizeDNSValue so CNAME/NS/MX/SRV values with/without trailing dots are
treated equal.
2 participants
Resolves:
#19
#17
Summary by CodeRabbit
Chores
Bug Fixes
✏️ Tip: You can customize this high-level summary in your review settings.