diff --git a/.github/workflows/release.yml b/.github/workflows/release.yml new file mode 100644 index 0000000..eb9e4c2 --- /dev/null +++ b/.github/workflows/release.yml @@ -0,0 +1,43 @@ +name: Release +on: + push: + tags: + - 'v*' +permissions: + contents: write +jobs: + release: + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@v5 + with: + fetch-depth: 0 + - uses: actions/setup-go@v6 + with: + go-version-file: go.mod + - name: Configure Git for private GoCodeAlone repos + env: + RELEASES_TOKEN: ${{ secrets.RELEASES_TOKEN }} + run: | + if [ -z "$RELEASES_TOKEN" ]; then + echo "::error::RELEASES_TOKEN secret is not set. The goreleaser step needs it to resolve private GoCodeAlone module deps." >&2 + exit 1 + fi + git config --global url."https://x-access-token:${RELEASES_TOKEN}@github.com/GoCodeAlone/".insteadOf "https://github.com/GoCodeAlone/" + - uses: goreleaser/goreleaser-action@v7 + with: + distribution: goreleaser + version: '~> v2' + args: release --clean + env: + GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }} + - name: Unset credentialed git config + if: always() + env: + RELEASES_TOKEN: ${{ secrets.RELEASES_TOKEN }} + run: | + git config --global --unset-all "url.https://x-access-token:${RELEASES_TOKEN}@github.com/GoCodeAlone/.insteadOf" || true + - name: Publish release (was draft during asset upload) + env: + GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }} + run: gh release edit ${{ github.ref_name }} --draft=false --repo ${{ github.repository }} diff --git a/.gitignore b/.gitignore index 4c5f206..c9b2b08 100644 --- a/.gitignore +++ b/.gitignore @@ -1 +1,3 @@ .claude/ +.release/ +dist/ diff --git a/.goreleaser.yaml b/.goreleaser.yaml new file mode 100644 index 0000000..69b1c6f --- /dev/null +++ b/.goreleaser.yaml @@ -0,0 +1,46 @@ +version: 2 + +project_name: workflow-plugin-hover + +before: + hooks: + - "sh -c \"rm -rf .release && mkdir -p .release && jq --arg v '{{ .Version }}' '.version = $v' plugin.json > .release/plugin.json\"" + - "sh -c \"jq -e --arg v '{{ .Version }}' '.version == $v' .release/plugin.json > /dev/null || (echo 'plugin.json version rewrite failed' >&2 && exit 1)\"" + +builds: + - id: workflow-plugin-hover + main: ./cmd/workflow-plugin-hover + binary: workflow-plugin-hover + env: + - CGO_ENABLED=0 + - GOPRIVATE=github.com/GoCodeAlone/* + goos: + - linux + - darwin + goarch: + - amd64 + - arm64 + ldflags: + - -s -w -X github.com/GoCodeAlone/workflow-plugin-hover/internal.Version={{.Version}} + +archives: + - id: workflow-plugin-hover + ids: + - workflow-plugin-hover + formats: [tar.gz] + name_template: "{{ .ProjectName }}-{{ .Os }}-{{ .Arch }}" + files: + - src: .release/plugin.json + dst: plugin.json + +checksum: + name_template: "checksums.txt" + +changelog: + sort: asc + +release: + github: + owner: GoCodeAlone + name: workflow-plugin-hover + draft: true diff --git a/README.md b/README.md index 2c23738..8a92651 100644 --- a/README.md +++ b/README.md @@ -43,6 +43,15 @@ resources: - { type: CNAME, name: 'www', content: example.com., ttl: 900 } ``` +The `records` key is **required**. The plugin treats your declared +list as authoritative: on apply, records present upstream but +absent from `records` are deleted, records present in `records` +but absent upstream are created, and records that differ are +updated. To deliberately drop every record from a zone, set +`records: []` — that explicit empty list is the only way to ask +for a wipe. Omitting `records` entirely is rejected at Plan time +to avoid the "I forgot the key and lost my zone" failure mode. + ## Required secrets | Name | Sensitive | Source | @@ -74,15 +83,10 @@ against [RFC 6238 Appendix B vectors](https://datatracker.ietf.org/doc/html/rfc6 ## Limitations -- **No prune on apply**: `upsertRecords` only adds/updates. Records - that exist upstream but are not in the desired config are NOT - deleted on `apply`. `Diff` does flag them (so Plan reports drift), - but converging the actual record set requires manually deleting - the orphan records via Hover's UI or via a future explicit prune - path. Track follow-up via the project issue list. - **No zone delete**: Hover exposes no API to drop a DNS zone. Resource `Delete` is a no-op — the IaC state is cleared but - upstream records remain. + upstream records remain. Operators who want to drop the zone + must do so manually via Hover's UI. ## Development diff --git a/internal/drivers/dns.go b/internal/drivers/dns.go index b3d1b7f..3d9831c 100644 --- a/internal/drivers/dns.go +++ b/internal/drivers/dns.go @@ -105,6 +105,20 @@ func (d *DNSDriver) Delete(_ context.Context, _ interfaces.ResourceRef) error { } func (d *DNSDriver) Diff(_ context.Context, desired interfaces.ResourceSpec, current *interfaces.ResourceOutput) (*interfaces.DiffResult, error) { + // Validate the desired spec up front so config errors surface at + // Plan time, even for brand-new resources (current == nil) where + // the engine would otherwise just see NeedsUpdate=true and let + // Apply discover the same problem one stage later. domainFromSpec + // rejects missing/empty domain; declaredRecords rejects missing or + // wrong-type `records`. + if _, err := domainFromSpec(desired); err != nil { + return nil, err + } + desiredRecs, err := declaredRecords(desired.Config) + if err != nil { + return nil, err + } + if current == nil { return &interfaces.DiffResult{NeedsUpdate: true}, nil } @@ -123,10 +137,6 @@ func (d *DNSDriver) Diff(_ context.Context, desired interfaces.ResourceSpec, cur }, nil } - desiredRecs, err := declaredRecords(desired.Config) - if err != nil { - return nil, err - } currentRecs, err := dnsRecordsFromOutput(current) if err != nil { return nil, err @@ -134,13 +144,7 @@ func (d *DNSDriver) Diff(_ context.Context, desired interfaces.ResourceSpec, cur // Empty desired record set with no current records → in sync. // Empty desired with leftover current records → drift (everything - // extra needs deletion). Note: upsertRecords today does NOT delete - // extras (it only adds/updates), so this Diff signal currently - // produces a NeedsUpdate the engine cannot fully satisfy. The - // alternative — silently letting extras persist — is worse: the - // declared spec never matches reality. Operators who want strict - // pruning need to either explicitly add `Delete` plumbing or - // document the gap; flagging it is the right Plan signal. + // extra needs deletion); upsertRecords prunes them during apply. if len(desiredRecs) == 0 { if len(currentRecs) == 0 { return &interfaces.DiffResult{NeedsUpdate: false}, nil @@ -185,10 +189,9 @@ func (d *DNSDriver) Diff(_ context.Context, desired interfaces.ResourceSpec, cur currentByKey[key] = append(candidates[:idx], candidates[idx+1:]...) } // Any remaining candidates are records that exist upstream but - // are not in the desired set. Treat that as drift so the engine - // surfaces it during Plan, even though upsertRecords does not - // currently prune the extras (an explicit prune path is a - // separate follow-up; see the "Limitations" section of README.md). + // are not in the desired set. Treat that as drift so Plan + // surfaces the change to the operator; upsertRecords prunes + // them during apply. for _, leftover := range currentByKey { if len(leftover) > 0 { return &interfaces.DiffResult{NeedsUpdate: true}, nil @@ -229,9 +232,9 @@ func (d *DNSDriver) readOutput(ctx context.Context, domain, name string) (*inter } func (d *DNSDriver) upsertRecords(ctx context.Context, domain string, desired []hover.DNSRecord) error { - if len(desired) == 0 { - return nil - } + // An empty desired set means "drop everything" — fall through into + // the prune step rather than short-circuiting. Without this, an + // explicit `records: []` would still leave upstream records intact. // Hover's POST /api/dns endpoint requires `domain_id` (hover-assigned // numeric ID), NOT the apex name. Resolve the domain up front via @@ -293,12 +296,26 @@ func (d *DNSDriver) upsertRecords(ctx context.Context, domain string, desired [] return fmt.Errorf("hover dns update %s/%s %q: %w", dr.Type, dr.Name, domain, err) } } else { - created, err := d.client.CreateRecord(ctx, dom.ID, dr) - if err != nil { + if _, err := d.client.CreateRecord(ctx, dom.ID, dr); err != nil { return fmt.Errorf("hover dns create %s/%s %q: %w", dr.Type, dr.Name, domain, err) } - if created != nil { - existingByKey[key] = append(existingByKey[key], *created) + // Do NOT add the created record back into existingByKey. + // existingByKey is the upstream set we're reconciling + // against; the create is a NEW record, not a leftover. + // Adding it would trip the prune sweep below and delete + // the record we just created. + } + } + + // Prune: any candidates still in existingByKey after both passes + // are upstream records that no longer appear in the desired config. + // Delete them so the upstream zone converges to the declared set. + // (Hover has no whole-zone replace API; this per-record delete is + // the only path to convergence.) + for _, leftovers := range existingByKey { + for _, orphan := range leftovers { + if err := d.client.DeleteRecord(ctx, orphan.ID); err != nil { + return fmt.Errorf("hover dns prune %s/%s %q (id=%s): %w", orphan.Type, orphan.Name, domain, orphan.ID, err) } } } @@ -335,10 +352,17 @@ func domainFromConfigIfPresent(config map[string]any) (string, bool, error) { } // declaredRecords parses config["records"] into a []hover.DNSRecord slice. +// +// `records` is REQUIRED. A missing key errors out — silently coercing +// to an empty slice would let upsertRecords prune every upstream +// record, which is rarely what an operator intends when they forgot +// to set the key. An explicitly empty `records: []` IS allowed (and +// does deliberately prune everything); only the missing-key / +// wrong-type cases are rejected. func declaredRecords(config map[string]any) ([]hover.DNSRecord, error) { - raw, ok := config["records"] - if !ok { - return nil, nil + raw, present := config["records"] + if !present { + return nil, fmt.Errorf("hover dns: config.records is required (use an explicit 'records: []' to drop every record)") } items, err := toSliceOfMaps(raw) if err != nil { diff --git a/internal/drivers/dns_test.go b/internal/drivers/dns_test.go index 7ccb829..54c3ea2 100644 --- a/internal/drivers/dns_test.go +++ b/internal/drivers/dns_test.go @@ -92,8 +92,11 @@ func newDriver(records ...hover.DNSRecord) (*DNSDriver, *fakeClient) { } func TestDNSDriver_Create_Empty(t *testing.T) { + // Explicitly-empty records list is the supported way to declare + // "no DNS records on this zone". A missing records key is now an + // error (would otherwise silently prune everything upstream). d, _ := newDriver() - spec := interfaces.ResourceSpec{Name: "example.com", Type: "infra.dns", Config: map[string]any{}} + spec := interfaces.ResourceSpec{Name: "example.com", Type: "infra.dns", Config: map[string]any{"records": []any{}}} out, err := d.Create(context.Background(), spec) if err != nil { t.Fatalf("Create: %v", err) @@ -103,6 +106,14 @@ func TestDNSDriver_Create_Empty(t *testing.T) { } } +func TestDNSDriver_Create_MissingRecordsKey_Rejected(t *testing.T) { + d, _ := newDriver() + spec := interfaces.ResourceSpec{Name: "example.com", Type: "infra.dns", Config: map[string]any{}} + if _, err := d.Create(context.Background(), spec); err == nil { + t.Fatal("expected error for missing config.records key") + } +} + func TestDNSDriver_Create_WithRecords(t *testing.T) { d, fc := newDriver() spec := interfaces.ResourceSpec{ @@ -148,8 +159,11 @@ func TestDNSDriver_Create_UpdatesExistingRecord(t *testing.T) { } func TestDNSDriver_Diff_NilCurrent(t *testing.T) { + // Diff now validates config.records up front so config errors + // surface at Plan time even for new resources. Use an explicit + // empty records list to exercise the nil-current early-return path. d, _ := newDriver() - spec := interfaces.ResourceSpec{Name: "example.com", Type: "infra.dns", Config: map[string]any{}} + spec := interfaces.ResourceSpec{Name: "example.com", Type: "infra.dns", Config: map[string]any{"records": []any{}}} diff, err := d.Diff(context.Background(), spec, nil) if err != nil { t.Fatalf("Diff: %v", err) @@ -159,6 +173,14 @@ func TestDNSDriver_Diff_NilCurrent(t *testing.T) { } } +func TestDNSDriver_Diff_MissingRecordsKey_ErrorsAtPlanTime(t *testing.T) { + d, _ := newDriver() + spec := interfaces.ResourceSpec{Name: "example.com", Type: "infra.dns", Config: map[string]any{}} + if _, err := d.Diff(context.Background(), spec, nil); err == nil { + t.Fatal("expected error for missing config.records at Plan time") + } +} + func TestDNSDriver_Diff_UpToDate(t *testing.T) { d, _ := newDriver() spec := interfaces.ResourceSpec{ @@ -218,7 +240,7 @@ func TestDNSDriver_Diff_DomainChange_ForceReplace(t *testing.T) { spec := interfaces.ResourceSpec{ Name: "new.com", Type: "infra.dns", - Config: map[string]any{"domain": "new.com"}, + Config: map[string]any{"domain": "new.com", "records": []any{}}, } current := &interfaces.ResourceOutput{ProviderID: "old.com"} diff, err := d.Diff(context.Background(), spec, current) @@ -246,7 +268,7 @@ func TestDNSDriver_Update_DomainRenameRejected(t *testing.T) { d, _ := newDriver() spec := interfaces.ResourceSpec{ Name: "new.com", Type: "infra.dns", - Config: map[string]any{"domain": "new.com"}, + Config: map[string]any{"domain": "new.com", "records": []any{}}, } ref := interfaces.ResourceRef{Name: "old.com", ProviderID: "old.com"} _, err := d.Update(context.Background(), ref, spec) @@ -531,3 +553,67 @@ func TestDiff_EmptyDesired_WithCurrentRecords_NeedsUpdate(t *testing.T) { t.Error("expected NeedsUpdate=true when desired is empty but current has records") } } + +// TestUpsertRecords_PrunesExtraRecords verifies that records in the +// upstream zone that don't appear in the desired config are deleted +// during apply. Regresses the "no prune on apply" gap that left +// removed records as orphans upstream. +func TestUpsertRecords_PrunesExtraRecords(t *testing.T) { + fc := &fakeClient{ + records: []hover.DNSRecord{ + {ID: "r1", Type: "A", Name: "@", Content: "1.1.1.1"}, + {ID: "r2", Type: "A", Name: "www", Content: "1.1.1.1"}, // orphan + }, + } + d := NewDNSDriverWithClient(fc) + spec := interfaces.ResourceSpec{ + Name: "example.com", Type: "infra.dns", + Config: map[string]any{ + "records": []any{ + map[string]any{"type": "A", "name": "@", "content": "1.1.1.1"}, + }, + }, + } + if _, err := d.Update(context.Background(), interfaces.ResourceRef{Name: "example.com", ProviderID: "example.com"}, spec); err != nil { + t.Fatalf("Update: %v", err) + } + if len(fc.records) != 1 { + t.Fatalf("expected upstream to converge to 1 record after prune; got %d: %+v", len(fc.records), fc.records) + } + if fc.records[0].Name != "@" { + t.Errorf("expected the apex record to remain; got %+v", fc.records[0]) + } +} + +func TestUpsertRecords_EmptyDesiredDeletesAll(t *testing.T) { + fc := &fakeClient{ + records: []hover.DNSRecord{ + {ID: "r1", Type: "A", Name: "@", Content: "1.1.1.1"}, + {ID: "r2", Type: "A", Name: "www", Content: "1.1.1.1"}, + }, + } + d := NewDNSDriverWithClient(fc) + spec := interfaces.ResourceSpec{ + Name: "example.com", Type: "infra.dns", + Config: map[string]any{"records": []any{}}, + } + if _, err := d.Update(context.Background(), interfaces.ResourceRef{Name: "example.com", ProviderID: "example.com"}, spec); err != nil { + t.Fatalf("Update: %v", err) + } + if len(fc.records) != 0 { + t.Errorf("expected all upstream records pruned; got %d: %+v", len(fc.records), fc.records) + } +} + +func TestDNSDriver_Diff_MissingDomain_ErrorsAtPlanTime(t *testing.T) { + // No name + no config.domain → domainFromSpec returns error. + // Diff must surface that before short-circuiting on nil current. + d, _ := newDriver() + spec := interfaces.ResourceSpec{ + Type: "infra.dns", + Config: map[string]any{"records": []any{}}, + } + if _, err := d.Diff(context.Background(), spec, nil); err == nil { + t.Fatal("expected error for missing domain at Plan time") + } +}