diff --git a/CHANGELOG.md b/CHANGELOG.md index 67863e2..753defa 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -13,7 +13,9 @@ Format based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/), using ### Fixed -- `dns record-create` prints one row for the value it created. `dns record-delete` prints a pre-delete snapshot of the selected record set in table, JSON, and YAML output, marked by a `SOURCE` column in tables and `source: pre_delete_snapshot` in structured output. The snapshot may differ from the values deleted if the record set changes concurrently, and the CLI warns when it prints one. Both commands support `--show-zone` to print the resulting zone. CAA matching accepts equivalent numeric flags, tag case, and quoted values. MX output includes its preference, including `0`. `dns show` remains the full-zone read command. If a successful delete cannot identify the previous values, the CLI prints an empty result and a warning. +- `dns record-delete` now deletes each value in the finite pre-delete snapshot with the raw content required by the DNS API, fixing multi-value record-set deletion. Values added after the read are left alone. A failure after partial deletion returns an error and identifies the partial progress instead of reporting a successful snapshot. An already absent DNS record is handled idempotently when the DNS API returns its verified `DNS record not found.` response. +- `dns record-create` now rejects unquoted CAA values before sending a request; use `--content '0 issue "letsencrypt.org"'`. +- `dns record-create` prints one row for the value it created. `dns record-delete` prints a pre-delete snapshot of the selected record set in table, JSON, and YAML output, marked by a `SOURCE` column in tables and `source: pre_delete_snapshot` in structured output. Both commands support `--show-zone` to print the resulting zone. MX output includes its preference, including `0`. `dns show` remains the full-zone read command. ## [v1.0.1] - 2026-10-11 diff --git a/docs/commands.md b/docs/commands.md index b21b423..444eed6 100644 --- a/docs/commands.md +++ b/docs/commands.md @@ -442,9 +442,11 @@ zcp dns record-create --domain --name @ --type MX --content mail.e # Record creation shows the affected value. By default, deletion prints a pre-delete # snapshot, marked as `Pre-delete snapshot` in table output and # `source: pre_delete_snapshot` in JSON/YAML, and warns on stderr that concurrent -# changes may make it differ from values actually removed. With --show-zone, deletion +# changes may make it differ from values actually removed. When the selected record set +# has several values, deletion sends one request for each value in that initial snapshot; +# values added concurrently are left alone. With --show-zone, deletion # prints the resulting zone without the snapshot label or warning. -# CAA create matching accepts equivalent numeric flags, tag case, and quoted values. +# CAA values must be quoted, for example: --content '0 issue "letsencrypt.org"'. # If a successful delete cannot identify previous values, it prints an empty result and a warning. zcp dns record-create --domain --name www --type A --content 192.0.2.1 --show-zone diff --git a/internal/commands/dns.go b/internal/commands/dns.go index 6b3819a..6a3ced9 100644 --- a/internal/commands/dns.go +++ b/internal/commands/dns.go @@ -3,6 +3,7 @@ package commands import ( "bufio" "context" + "errors" "fmt" "net/netip" "os" @@ -315,6 +316,11 @@ func newDNSRecordCreateCmd() *cobra.Command { if content == "" { return fmt.Errorf("--content is required") } + if strings.EqualFold(recordType, "CAA") { + if err := validateCAARecordContent(content); err != nil { + return err + } + } if ttl <= 0 { ttl = 14400 } @@ -484,14 +490,36 @@ func runDNSRecordDeleteByName(cmd *cobra.Command, domainSlug, name, recordType s ctx, cancel := context.WithTimeout(context.Background(), time.Duration(getTimeout(cmd))*time.Second) defer cancel() - if err := svc.DeleteRecordByName(ctx, domainSlug, fqdn, recordType); err != nil { - if apierrors.IsResourceNotFound(err) { - fmt.Fprintf(os.Stderr, "DNS record %s %q not found (already deleted).\n", recordType, fqdn) - if !showZone { - return printDNSRecordRows(printer, nil) + if showErr == nil && len(snapshot) == 0 { + if showZone { + return printDNSRecordRows(printer, domain.Records) + } + return printDNSRecordRows(printer, nil) + } + + if len(snapshot) == 0 { + if err := svc.DeleteRecordByName(ctx, domainSlug, fqdn, recordType); err != nil { + if isDNSRecordNotFound(err) { + fmt.Fprintf(os.Stderr, "DNS record %s %q not found (already deleted).\n", recordType, fqdn) + if !showZone { + return printDNSRecordRows(printer, nil) + } + } else { + return fmt.Errorf("dns record-delete: %w", err) } - } else { - return fmt.Errorf("dns record-delete: %w", err) + } + } else { + values := dnsRecordValues(snapshot) + deleted := 0 + for _, value := range values { + if err := svc.DeleteRecordValueByName(ctx, domainSlug, fqdn, recordType, value); err != nil { + if isDNSRecordNotFound(err) { + continue + } + fmt.Fprintf(os.Stderr, "DNS record-delete stopped after deleting %d of %d values from the pre-delete snapshot; remaining snapshot values may still exist.\n", deleted, len(values)) + return fmt.Errorf("dns record-delete: %w", err) + } + deleted++ } } @@ -510,6 +538,18 @@ func runDNSRecordDeleteByName(cmd *cobra.Command, domainSlug, name, recordType s return printDNSRecordSnapshotRows(printer, snapshot) } +// isDNSRecordNotFound handles the DNS endpoint's verified idempotency +// response. Unlike the normal API not-found status, it reports an absent DNS +// record as HTTP 500 with this exact message. Do not broaden this exception: +// other 500 responses remain failures. +func isDNSRecordNotFound(err error) bool { + if apierrors.IsResourceNotFound(err) { + return true + } + var apiErr *apierrors.APIError + return errors.As(err, &apiErr) && apiErr.StatusCode == 500 && strings.TrimSpace(apiErr.Message) == "DNS record not found." +} + // suppliedDNSRecordFQDN returns a normalized FQDN only when the caller made // its absolute form explicit with a trailing dot. It never derives a name from // a domain slug, because slugs are not DNS zone names. @@ -659,6 +699,14 @@ func recordContents(record *dns.Record) []string { return []string{record.Content} } +func dnsRecordValues(records []dns.Record) []string { + values := make([]string, 0) + for i := range records { + values = append(values, recordContents(&records[i])...) + } + return values +} + func dnsRecordMatchesRequest(record dns.Record, content string, req dns.CreateRecordRequest) bool { if strings.EqualFold(record.Type, "CAA") { return caaRecordMatches(content, req.Content) @@ -719,6 +767,27 @@ func parseCAAContent(content string) (uint8, string, string, bool) { return uint8(flags), tag, value, true } +func validateCAARecordContent(content string) error { + original := strings.TrimSpace(content) + flagsEnd := strings.IndexAny(original, " \t\n\r") + if flagsEnd <= 0 { + return fmt.Errorf(`CAA --content must use a quoted value (e.g. 0 issue "letsencrypt.org")`) + } + remainder := strings.TrimLeft(original[flagsEnd:], " \t\n\r") + tagEnd := strings.IndexAny(remainder, " \t\n\r") + if tagEnd <= 0 { + return fmt.Errorf(`CAA --content must use a quoted value (e.g. 0 issue "letsencrypt.org")`) + } + value := strings.TrimSpace(remainder[tagEnd:]) + if _, ok := decodeQuotedTXTContent(value); !ok { + return fmt.Errorf(`CAA --content value must be quoted (e.g. 0 issue "letsencrypt.org")`) + } + if _, _, _, ok := parseCAAContent(original); !ok { + return fmt.Errorf(`CAA --content must use the form "value" (e.g. 0 issue "letsencrypt.org")`) + } + return nil +} + func isCAATag(tag string) bool { for i := range tag { if (tag[i] < 'a' || tag[i] > 'z') && (tag[i] < 'A' || tag[i] > 'Z') && (tag[i] < '0' || tag[i] > '9') { diff --git a/internal/commands/dns_record_output_test.go b/internal/commands/dns_record_output_test.go index aca2250..87f5298 100644 --- a/internal/commands/dns_record_output_test.go +++ b/internal/commands/dns_record_output_test.go @@ -182,7 +182,7 @@ func TestDNSRecordCreateNormalizesCAAOutput(t *testing.T) { defer srv.Close() t.Setenv("ZCP_BEARER_TOKEN", "test-token") - out, _, err := execCmd(t, NewDNSCmd(), "record-create", "--domain", "example-com-1", "--name", "caa", "--type", "CAA", "--content", "000 issue letsencrypt.org", "--api-url", srv.URL, "--output", format) + out, _, err := execCmd(t, NewDNSCmd(), "record-create", "--domain", "example-com-1", "--name", "caa", "--type", "CAA", "--content", `000 issue "letsencrypt.org"`, "--api-url", srv.URL, "--output", format) if err != nil { t.Fatalf("record-create error = %v", err) } @@ -360,7 +360,7 @@ func TestDNSRecordDeleteReportsSnapshotAndShowZone(t *testing.T) { if err := json.Unmarshal([]byte(out), &snapshot); err != nil { t.Fatalf("JSON output = %q: %v", out, err) } - if deleteCalls != 1 || getCalls != 1 || len(snapshot) != 2 || snapshot[1]["content"] != "192.0.2.2" || snapshot[0]["source"] != "pre_delete_snapshot" || snapshot[1]["source"] != "pre_delete_snapshot" || !strings.Contains(stderr, "snapshot read before deletion") { + if deleteCalls != 2 || getCalls != 1 || len(snapshot) != 2 || snapshot[1]["content"] != "192.0.2.2" || snapshot[0]["source"] != "pre_delete_snapshot" || snapshot[1]["source"] != "pre_delete_snapshot" || !strings.Contains(stderr, "snapshot read before deletion") { t.Errorf("get=%d delete=%d snapshot=%#v, want marked pre-delete values", getCalls, deleteCalls, snapshot) } @@ -397,8 +397,8 @@ func TestDNSRecordDeleteMissingAndFailureDoNotFabricateRows(t *testing.T) { t.Setenv("ZCP_BEARER_TOKEN", "test-token") out, stderr, err := execCaptureStdio(t, NewDNSCmd(), "record-delete", "--domain", "example-com-1", "--name", "www", "--type", "A", "--yes", "--api-url", srv.URL, "--output", "json") - if err != nil || strings.TrimSpace(out) != "[]" || deleteCalls != 1 || !strings.Contains(stderr, "previous values are unavailable") || strings.Contains(stderr, "snapshot read before deletion") { - t.Errorf("error=%v output=%q stderr=%q deleteCalls=%d, want empty result and warning after delete", err, out, stderr, deleteCalls) + if err != nil || strings.TrimSpace(out) != "[]" || deleteCalls != 0 || stderr != "" { + t.Errorf("error=%v output=%q stderr=%q deleteCalls=%d, want empty result without a delete request", err, out, stderr, deleteCalls) } }) @@ -415,8 +415,8 @@ func TestDNSRecordDeleteMissingAndFailureDoNotFabricateRows(t *testing.T) { t.Setenv("ZCP_BEARER_TOKEN", "test-token") out, stderr, err := execCaptureStdio(t, NewDNSCmd(), "record-delete", "--domain", "example-com-1", "--name", "www", "--type", "A", "--yes", "--api-url", srv.URL, "--output", "json") - if err != nil || strings.TrimSpace(out) != "[]" || strings.Contains(stderr, "previous values are unavailable") || strings.Contains(stderr, "snapshot read before deletion") { - t.Errorf("error=%v output=%q stderr=%q, want empty result without missing-details warning for an already deleted record", err, out, stderr) + if err != nil || !strings.Contains(out, "192.0.2.1") || !strings.Contains(stderr, "snapshot read before deletion") { + t.Errorf("error=%v output=%q stderr=%q, want the pre-delete snapshot when the value disappears after the read", err, out, stderr) } }) @@ -599,10 +599,12 @@ func TestDNSRecordDeleteAlreadyDeletedShowZone(t *testing.T) { args []string showZone bool postShowError bool + wantSnapshot bool }{ { - name: "name and type without show zone", - args: []string{"--name", "www", "--type", "A"}, + name: "name and type without show zone", + args: []string{"--name", "www", "--type", "A"}, + wantSnapshot: true, }, { name: "name and type with show zone", @@ -680,6 +682,13 @@ func TestDNSRecordDeleteAlreadyDeletedShowZone(t *testing.T) { } return } + if tt.wantSnapshot { + var rows []map[string]string + if json.Unmarshal([]byte(out), &rows) != nil || showCalls != 1 || len(rows) != 1 || rows[0]["content"] != "192.0.2.1" || rows[0]["source"] != "pre_delete_snapshot" || strings.Contains(stderr, "previous values are unavailable") || !strings.Contains(stderr, "snapshot read before deletion") { + t.Errorf("show=%d output=%q stderr=%q, want pre-delete snapshot and warning when the value was already deleted", showCalls, out, stderr) + } + return + } if showCalls != 1 || strings.TrimSpace(out) != "[]" || strings.Contains(stderr, "previous values are unavailable") || strings.Contains(stderr, "snapshot read before deletion") { t.Errorf("show=%d output=%q stderr=%q, want empty output without missing-details warning", showCalls, out, stderr) } @@ -805,3 +814,175 @@ func TestDNSRecordDeleteLegacyIDGetsFreshTimeoutAfterPreRead(t *testing.T) { t.Errorf("error=%v delete=%d output=%q stderr=%q, want successful delete with empty rows and warning", err, deleteCalls, out, stderr) } } + +func TestDNSRecordDeleteUsesRawInitialSnapshotValues(t *testing.T) { + tests := []struct { + name string + recordType string + contents []string + }{ + {"A", "A", []string{"192.0.2.10", "192.0.2.11"}}, + {"MX", "MX", []string{"0 mail.example.com.", "10 backup.example.com."}}, + {"TXT", "TXT", []string{`"one"`, `"two"`}}, + {"CAA", "CAA", []string{`0 issue "letsencrypt.org"`, `0 iodef "mailto:security@example.com"`}}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + var contents []string + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Content-Type", "application/json") + switch r.Method { + case http.MethodGet: + payload, err := json.Marshal(tt.contents) + if err != nil { + t.Fatal(err) + } + fmt.Fprintf(w, `{"status":"Success","data":{"name":"example.com","records":[{"name":"value.example.com.","type":%q,"ttl":60,"contents":%s}]}}`, tt.recordType, payload) + case http.MethodDelete: + content := r.URL.Query().Get("content") + if content == "" { + w.WriteHeader(http.StatusInternalServerError) + fmt.Fprint(w, `{"status":"Error","message":"Content is required to delete a record from a multi-value record set."}`) + return + } + contents = append(contents, content) + fmt.Fprint(w, `{"status":"Success"}`) + default: + http.NotFound(w, r) + } + })) + defer srv.Close() + t.Setenv("ZCP_BEARER_TOKEN", "test-token") + + out, _, err := execCaptureStdio(t, NewDNSCmd(), "record-delete", "--domain", "example-com-1", "--name", "value", "--type", tt.recordType, "--yes", "--api-url", srv.URL, "--output", "json") + if err != nil { + t.Fatalf("record-delete error = %v", err) + } + if got, want := strings.Join(contents, "|"), strings.Join(tt.contents, "|"); got != want { + t.Errorf("delete content = %q, want %q", got, want) + } + if !strings.Contains(out, "pre_delete_snapshot") { + t.Errorf("output = %q, want snapshot rows", out) + } + }) + } +} + +func TestDNSRecordDeleteStopsOnPartialFailureWithoutSnapshotOutput(t *testing.T) { + var contents []string + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Content-Type", "application/json") + switch r.Method { + case http.MethodGet: + fmt.Fprint(w, `{"status":"Success","data":{"name":"example.com","records":[{"name":"www.example.com.","type":"A","ttl":60,"contents":["192.0.2.10","192.0.2.11"]}]}}`) + case http.MethodDelete: + contents = append(contents, r.URL.Query().Get("content")) + if len(contents) == 2 { + http.Error(w, "backend unavailable", http.StatusServiceUnavailable) + return + } + fmt.Fprint(w, `{"status":"Success"}`) + default: + http.NotFound(w, r) + } + })) + defer srv.Close() + t.Setenv("ZCP_BEARER_TOKEN", "test-token") + + out, stderr, err := execCaptureStdio(t, NewDNSCmd(), "record-delete", "--domain", "example-com-1", "--name", "www", "--type", "A", "--yes", "--api-url", srv.URL, "--output", "json") + if err == nil || !strings.Contains(stderr, "stopped after deleting 1 of 2 values") || strings.Contains(out, "pre_delete_snapshot") { + t.Errorf("error=%v output=%q stderr=%q, want partial failure without success output", err, out, stderr) + } + if got := strings.Join(contents, "|"); got != "192.0.2.10|192.0.2.11" { + t.Errorf("delete content = %q, want both initial values in order", got) + } +} + +func TestDNSRecordDeleteDoesNotDrainConcurrentValues(t *testing.T) { + var deleteCalls, showCalls int + remaining := []string{"192.0.2.10"} + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Content-Type", "application/json") + switch r.Method { + case http.MethodGet: + showCalls++ + payload, err := json.Marshal(remaining) + if err != nil { + t.Fatal(err) + } + fmt.Fprintf(w, `{"status":"Success","data":{"name":"example.com","records":[{"name":"www.example.com.","type":"A","ttl":60,"contents":%s}]}}`, payload) + case http.MethodDelete: + deleteCalls++ + if r.URL.Query().Get("content") != "192.0.2.10" { + http.Error(w, "unexpected value", http.StatusBadRequest) + return + } + remaining = []string{"192.0.2.99"} + fmt.Fprint(w, `{"status":"Success"}`) + default: + http.NotFound(w, r) + } + })) + defer srv.Close() + t.Setenv("ZCP_BEARER_TOKEN", "test-token") + + out, _, err := execCaptureStdio(t, NewDNSCmd(), "record-delete", "--domain", "example-com-1", "--name", "www", "--type", "A", "--yes", "--show-zone", "--api-url", srv.URL, "--output", "json") + if err != nil || deleteCalls != 1 || showCalls != 2 || !strings.Contains(out, "192.0.2.99") { + t.Errorf("error=%v deleteCalls=%d showCalls=%d output=%q, want only the initial value deleted and the concurrent value preserved", err, deleteCalls, showCalls, out) + } +} + +func TestDNSRecordDeleteTreatsOnlyExactDNS500AsAbsent(t *testing.T) { + for _, tt := range []struct { + name string + message string + wantError bool + }{ + {"verified absent response", "DNS record not found.", false}, + {"unrelated server failure", "database unavailable", true}, + } { + t.Run(tt.name, func(t *testing.T) { + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + switch r.Method { + case http.MethodGet: + http.Error(w, "pre-read unavailable", http.StatusServiceUnavailable) + case http.MethodDelete: + w.Header().Set("Content-Type", "application/json") + w.WriteHeader(http.StatusInternalServerError) + fmt.Fprintf(w, `{"status":"Error","message":%q}`, tt.message) + default: + http.NotFound(w, r) + } + })) + defer srv.Close() + t.Setenv("ZCP_BEARER_TOKEN", "test-token") + + out, stderr, err := execCaptureStdio(t, NewDNSCmd(), "record-delete", "--domain", "example-com-1", "--name", "www.example.com.", "--type", "A", "--yes", "--api-url", srv.URL, "--output", "json") + if tt.wantError { + if err == nil || strings.Contains(out, "pre_delete_snapshot") { + t.Errorf("error=%v output=%q, want unrelated 500 to fail", err, out) + } + return + } + if err != nil || strings.TrimSpace(out) != "[]" || !strings.Contains(stderr, "not found (already deleted)") { + t.Errorf("error=%v output=%q stderr=%q, want idempotent absent result", err, out, stderr) + } + }) + } +} + +func TestDNSRecordCreateRejectsUnquotedCAABeforeHTTP(t *testing.T) { + var calls int + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + calls++ + http.Error(w, "must not be called", http.StatusInternalServerError) + })) + defer srv.Close() + t.Setenv("ZCP_BEARER_TOKEN", "test-token") + + _, _, err := execCaptureStdio(t, NewDNSCmd(), "record-create", "--domain", "example-com-1", "--name", "caa", "--type", "CAA", "--content", "0 issue letsencrypt.org", "--api-url", srv.URL) + if err == nil || !strings.Contains(err.Error(), `0 issue "letsencrypt.org"`) || calls != 0 { + t.Errorf("error=%v calls=%d, want local quoted-CAA validation without HTTP", err, calls) + } +} diff --git a/pkg/api/dns/dns.go b/pkg/api/dns/dns.go index 1e184a4..db0f309 100644 --- a/pkg/api/dns/dns.go +++ b/pkg/api/dns/dns.go @@ -294,6 +294,20 @@ func (s *Service) DeleteRecordByName(ctx context.Context, domainSlug, name, reco return nil } +// DeleteRecordValueByName removes one value from a DNS record set identified +// by its fully qualified name, type, and backend-returned raw content. The +// PowerDNS-backed API requires content when deleting a multi-value record set. +func (s *Service) DeleteRecordValueByName(ctx context.Context, domainSlug, name, recordType, content string) error { + q := url.Values{} + q.Set("name", name) + q.Set("type", recordType) + q.Set("content", content) + if err := s.client.Delete(ctx, "/dns/domains/"+domainSlug+"/records", q); err != nil { + return fmt.Errorf("deleting DNS record value %s %s on domain %s: %w", recordType, name, domainSlug, err) + } + return nil +} + // CanonicalRecordFQDN builds the backend's stored record name for a record in // the given zone: relative labels get the zone appended, absolute names are // normalized, and the result always carries a trailing dot. diff --git a/pkg/api/dns/dns_test.go b/pkg/api/dns/dns_test.go index f3b7dd8..389f347 100644 --- a/pkg/api/dns/dns_test.go +++ b/pkg/api/dns/dns_test.go @@ -627,6 +627,30 @@ func TestDNSDeleteRecordByName(t *testing.T) { } } +func TestDNSDeleteRecordValueByName(t *testing.T) { + var gotPath, gotName, gotType, gotContent string + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + gotPath = r.URL.Path + gotName = r.URL.Query().Get("name") + gotType = r.URL.Query().Get("type") + gotContent = r.URL.Query().Get("content") + w.Header().Set("Content-Type", "application/json") + json.NewEncoder(w).Encode(map[string]interface{}{"status": "Success", "message": "deleted"}) + })) + defer srv.Close() + + svc := dns.NewService(newClient(srv.URL)) + if err := svc.DeleteRecordValueByName(context.Background(), "example-com-1", "mail.example.com.", "MX", "0 mail.example.com."); err != nil { + t.Fatalf("DeleteRecordValueByName() error = %v", err) + } + if gotPath != "/dns/domains/example-com-1/records" { + t.Errorf("path = %q", gotPath) + } + if gotName != "mail.example.com." || gotType != "MX" || gotContent != "0 mail.example.com." { + t.Errorf("query = name %q type %q content %q, want mail.example.com. / MX / 0 mail.example.com.", gotName, gotType, gotContent) + } +} + func TestCanonicalRecordFQDN(t *testing.T) { cases := []struct{ name, zone, want string }{ {"www", "example.com", "www.example.com."},