fix: atomic bulk apply - #24
Open
sshine wants to merge 9 commits into
Open
Conversation
When deSEC returns 429 Too Many Requests, the response carries a Retry-After header indicating how long the throttle window will remain active. Until now the webhook ignored that header at the client level: on the next reconcile (1m) external-dns would POST /records again and we'd immediately hit deSEC, extending the throttle window. This change adds: - rateLimitTransport: an http.RoundTripper that wraps the http.Client passed to nrdcg/desec. On any 429 response it parses Retry-After (delta-seconds or HTTP-date per RFC 7231) and records the deadline. - rateLimitTracker: a goroutine-safe holder for the next-allowed-at timestamp. The window only ever extends, never shrinks, so a later 429 with a smaller Retry-After can't accidentally release us early. - ApplyChanges short-circuit: at the top of ApplyChanges, if the tracker reports we're still inside the window, return a typed RateLimitError without hitting the API. external-dns still gets a 500 and will keep retrying, but our daily quota is preserved until the window legitimately expires. The error type is modeled after the one proposed in the upstream draft PR michelangelomo#17 so the API stays consistent if that work eventually replaces nrdcg/desec.
Adds a single debug-gated body dump at the top of both /records and /adjustendpoints handlers. The dump fires only when WEBHOOK_LOGLEVEL=debug is set; at info level (the default) the helper short-circuits before reading the body, so production deployments pay no cost. Motivation: The existing summary log line "applying changes: N creates, M updates, K deletes" and the per-domain "updating ... records: <payload>" line both show only the UpdateNew side of the plan. Once a diff loop appears with N creates = K deletes = 0 and M updates > 0, the visible payload does not reveal what external-dns thought was different: UpdateOld, Labels, ProviderSpecific, and ownerID are nowhere in the current logs.
external-dns sources emit Endpoint.DNSName without a trailing dot. convertRRSetToEndpoint was appending one unconditionally, so /records returned the dotted form. The external-dns plan calculator compares dotted current state against dotless desired state, and the TXT registry treats that string mismatch as a stale companion record. It then set providerSpecific txt/force-update=true on every UpdateOld entry, which plan.providerSpecificChanged() promoted into an Update. Tests: - existing convertRRSetToEndpoint cases updated to assert dotless DNSName (including the "Apex with dotted domain" case that proves inbound domain strings with a trailing dot still get normalized) - new TestConvertRRSetToEndpoint_DNSNameHasNoTrailingDot pins the invariant across subdomain, apex, deep subdomain, and dotted-domain input so a future refactor cannot silently reintroduce the loop
v0.11.1 tags `RRSet.SubName` with `json:"subname,omitempty"`.
This drops the field for apex RRsets (where the value must be `""`).
The deSEC API requires the field be present even at the apex.
So apex record creates land with 400 {"subname":["This field is required."]}.
Upstream commit nrdcg/desec@fd7f5e0 ("fix: subname as empty string") removed the omitempty modifier, shipped in v0.11.2.
Bumping picks up the fix. TestApexRRSetJSONIncludesEmptySubname confirms that this works and guards against regressions.
ApplyChanges: - Merge Create + UpdateNew + Delete for each domain into a single BulkUpdate(FullResource) PUT instead of three ordered, non-atomic calls. - Encode deletions as records:[] so deSEC validates the resulting zone state. Why: - A record-type change (A->CNAME at one subname) reaches external-dns as Delete(old A) + Create(new CNAME). The old code POSTed the CNAME first, while the A still existed; deSEC rejects a CNAME coexisting with another type, the POST 400s, and the Delete never runs -- a permanent wedge under policy=sync. Sending both in one atomic bulk request lets deSEC validate the final state (A gone, CNAME present) and accept it. - BulkUpdate(FullResource) only touches the rrsets in the request (BulkDelete already relies on this), so unrelated records are left intact. tests: - desecMock: in-memory zone enforcing deSEC's create-only POST and CNAME-coexistence rules, validating resulting state atomically. - Pin the CNAME-over-A rejection; repro the A->CNAME retype (red before fix); assert update+create collapse to a single bulk request; dry-run hits no API.
📝 WalkthroughWalkthroughAdds client-side HTTP rate limiting to the deSEC provider via a new ChangesdeSEC Rate Limiting Infrastructure
Bulk Apply Rewrite and DNSName Fix
Server Debug Request Body Logging
CI and Release Action Version Upgrades
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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 |
Author
|
Note: The change lives in 4128852, but is obscured slightly by main branch being so much ahead. There are some fixes to other things that I didn't publish PRs on yet. This PR mostly exists as a reminder to eventually merge. The updated package (including other fixes) can be fetched from here: |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
internal/provider/desec_apply_test.go (1)
249-260: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert the retype is applied as one bulk request.
This test only checks the final zone state. A future delete-then-create implementation would still pass here while reintroducing the non-atomic behavior this PR is meant to prevent. Please also assert
m.requestCount == 1so the regression guard pins the atomic contract.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 303db28f-1938-446f-b5e9-d83c2d6adc92
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (10)
.github/workflows/ci.yml.github/workflows/release.ymlgo.modinternal/provider/desec.gointernal/provider/desec_apply_test.gointernal/provider/desec_test.gointernal/provider/ratelimit.gointernal/provider/ratelimit_test.gointernal/server/server.gointernal/server/server_test.go
Comment on lines
87
to
+91
| func (d *DesecClient) ApplyChanges(changes plan.Changes) error { | ||
| if remaining := d.rateLimit.wait(); remaining > 0 { | ||
| log.Warnf("deSEC rate limit active; skipping ApplyChanges for %s to preserve daily quota", remaining) | ||
| return &RateLimitError{RetryAfter: remaining} | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Don't rate-limit the dry-run path.
Lines 88-90 run before the dry-run branch, so a prior 429 makes ApplyChanges return RateLimitError even when d.dryRun is true and no API call would be made. That turns --dry-run into a false failure during a throttle window.
Suggested fix
func (d *DesecClient) ApplyChanges(changes plan.Changes) error {
- if remaining := d.rateLimit.wait(); remaining > 0 {
- log.Warnf("deSEC rate limit active; skipping ApplyChanges for %s to preserve daily quota", remaining)
- return &RateLimitError{RetryAfter: remaining}
+ if !d.dryRun {
+ if remaining := d.rateLimit.wait(); remaining > 0 {
+ log.Warnf("deSEC rate limit active; skipping ApplyChanges for %s to preserve daily quota", remaining)
+ return &RateLimitError{RetryAfter: remaining}
+ }
}📝 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 (d *DesecClient) ApplyChanges(changes plan.Changes) error { | |
| if remaining := d.rateLimit.wait(); remaining > 0 { | |
| log.Warnf("deSEC rate limit active; skipping ApplyChanges for %s to preserve daily quota", remaining) | |
| return &RateLimitError{RetryAfter: remaining} | |
| } | |
| func (d *DesecClient) ApplyChanges(changes plan.Changes) error { | |
| if !d.dryRun { | |
| if remaining := d.rateLimit.wait(); remaining > 0 { | |
| log.Warnf("deSEC rate limit active; skipping ApplyChanges for %s to preserve daily quota", remaining) | |
| return &RateLimitError{RetryAfter: remaining} | |
| } | |
| } |
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.
fix(provider): apply DNS changes as one atomic bulk request per domain
ApplyChanges:
BulkUpdate(FullResource) PUT instead of three ordered, non-atomic calls.
Why:
Delete(old A) + Create(new CNAME). The old code POSTed the CNAME first,
while the A still existed; deSEC rejects a CNAME coexisting with another
type, the POST 400s, and the Delete never runs -- a permanent wedge under
policy=sync. Sending both in one atomic bulk request lets deSEC validate
the final state (A gone, CNAME present) and accept it.
already relies on this), so unrelated records are left intact.
tests:
CNAME-coexistence rules, validating resulting state atomically.
assert update+create collapse to a single bulk request; dry-run hits no API.