From c26f688392462da2e26a1f6dd79cad0fc99ae777 Mon Sep 17 00:00:00 2001 From: Matt Jenkinson <75292329+mattdjenkinson@users.noreply.github.com> Date: Wed, 29 Jul 2026 13:27:46 +0100 Subject: [PATCH 1/4] fix: Show add/delete for hostname edits Portal keeps one DNSRecordSet per zone per type, so adding or removing a hostname is a Kubernetes update. Stamp activity-* annotations from the admission OldObject diff and prefer them in ActivityPolicy so the timeline matches the UI (issue #72). --- .../policies/dnsrecordset-policy.yaml | 88 ++++++ docs/enhancements/activity-integration.md | 10 +- .../dnsrecordset_policy_test.go | 102 +++++++ internal/display/activity.go | 251 ++++++++++++++++++ internal/display/activity_test.go | 136 ++++++++++ internal/webhook/dnsrecordset_mutating.go | 23 +- .../webhook/dnsrecordset_mutating_test.go | 69 +++++ 7 files changed, 676 insertions(+), 3 deletions(-) create mode 100644 internal/display/activity.go create mode 100644 internal/display/activity_test.go diff --git a/config/milo/activity/policies/dnsrecordset-policy.yaml b/config/milo/activity/policies/dnsrecordset-policy.yaml index 130a113..0f615b9 100644 --- a/config/milo/activity/policies/dnsrecordset-policy.yaml +++ b/config/milo/activity/policies/dnsrecordset-policy.yaml @@ -13,6 +13,8 @@ # - Show record values (IPs, CNAME targets, MX hosts, TXT content) where available # - Accurate create / update / delete language ("added", "updated", "deleted") # - Prefer display-name / display-value annotations (stamped at admission) +# - Prefer activity-change / activity-name / activity-value on updates when a +# hostname was added/removed on a multi-name DNSRecordSet (issue #72) # - Fallback to relative owner name from spec.records when annotations are absent # - Exclude system components (system:*) from human audit rules # - Update rules read recordType from responseObject (portal PATCHes omit it on request) @@ -83,8 +85,94 @@ spec: summary: "{{ actor }} deleted {{ link('a DNS record', audit.objectRef) }}" # ----- UPDATE RULES ----- + # Prefer activity-* annotations when present: hostname-level add/remove on a + # multi-name DNSRecordSet is still a K8s update/patch (issue #72). # recordType is read from responseObject: portal PATCHes often omit it on requestObject (#36). # All update rules require requestObject.spec so metadata-only patches are ignored. + - name: update-activity-added-a-aaaa + match: "!audit.user.username.startsWith('system:') && audit.verb in ['update', 'patch'] && !has(audit.objectRef.subresource) && has(audit.requestObject.spec) && has(audit.responseObject.spec) && audit.responseObject.spec.recordType in ['A', 'AAAA'] && has(audit.responseObject.metadata.annotations) && 'dns.networking.miloapis.com/activity-change' in audit.responseObject.metadata.annotations && audit.responseObject.metadata.annotations['dns.networking.miloapis.com/activity-change'] == 'added' && 'dns.networking.miloapis.com/activity-name' in audit.responseObject.metadata.annotations" + summary: "{{ actor }} added {{ audit.responseObject.spec.recordType }} record {{ link(audit.responseObject.metadata.annotations['dns.networking.miloapis.com/activity-name'], audit.objectRef) }} pointing to {{ audit.responseObject.metadata.annotations['dns.networking.miloapis.com/activity-value'] }}" + + - name: update-activity-removed-a-aaaa + match: "!audit.user.username.startsWith('system:') && audit.verb in ['update', 'patch'] && !has(audit.objectRef.subresource) && has(audit.requestObject.spec) && has(audit.responseObject.spec) && audit.responseObject.spec.recordType in ['A', 'AAAA'] && has(audit.responseObject.metadata.annotations) && 'dns.networking.miloapis.com/activity-change' in audit.responseObject.metadata.annotations && audit.responseObject.metadata.annotations['dns.networking.miloapis.com/activity-change'] == 'removed' && 'dns.networking.miloapis.com/activity-name' in audit.responseObject.metadata.annotations" + summary: "{{ actor }} deleted {{ audit.responseObject.spec.recordType }} record {{ link(audit.responseObject.metadata.annotations['dns.networking.miloapis.com/activity-name'], audit.objectRef) }}" + + - name: update-activity-updated-a-aaaa + match: "!audit.user.username.startsWith('system:') && audit.verb in ['update', 'patch'] && !has(audit.objectRef.subresource) && has(audit.requestObject.spec) && has(audit.responseObject.spec) && audit.responseObject.spec.recordType in ['A', 'AAAA'] && has(audit.responseObject.metadata.annotations) && 'dns.networking.miloapis.com/activity-change' in audit.responseObject.metadata.annotations && audit.responseObject.metadata.annotations['dns.networking.miloapis.com/activity-change'] == 'updated' && 'dns.networking.miloapis.com/activity-name' in audit.responseObject.metadata.annotations" + summary: "{{ actor }} updated {{ audit.responseObject.spec.recordType }} record {{ link(audit.responseObject.metadata.annotations['dns.networking.miloapis.com/activity-name'], audit.objectRef) }} to point to {{ audit.responseObject.metadata.annotations['dns.networking.miloapis.com/activity-value'] }}" + + - name: update-activity-added-cname + match: "!audit.user.username.startsWith('system:') && audit.verb in ['update', 'patch'] && !has(audit.objectRef.subresource) && has(audit.requestObject.spec) && has(audit.responseObject.spec) && audit.responseObject.spec.recordType == 'CNAME' && has(audit.responseObject.metadata.annotations) && 'dns.networking.miloapis.com/activity-change' in audit.responseObject.metadata.annotations && audit.responseObject.metadata.annotations['dns.networking.miloapis.com/activity-change'] == 'added' && 'dns.networking.miloapis.com/activity-name' in audit.responseObject.metadata.annotations" + summary: "{{ actor }} added CNAME record {{ link(audit.responseObject.metadata.annotations['dns.networking.miloapis.com/activity-name'], audit.objectRef) }} as an alias for {{ audit.responseObject.metadata.annotations['dns.networking.miloapis.com/activity-value'] }}" + + - name: update-activity-removed-cname + match: "!audit.user.username.startsWith('system:') && audit.verb in ['update', 'patch'] && !has(audit.objectRef.subresource) && has(audit.requestObject.spec) && has(audit.responseObject.spec) && audit.responseObject.spec.recordType == 'CNAME' && has(audit.responseObject.metadata.annotations) && 'dns.networking.miloapis.com/activity-change' in audit.responseObject.metadata.annotations && audit.responseObject.metadata.annotations['dns.networking.miloapis.com/activity-change'] == 'removed' && 'dns.networking.miloapis.com/activity-name' in audit.responseObject.metadata.annotations" + summary: "{{ actor }} deleted CNAME record {{ link(audit.responseObject.metadata.annotations['dns.networking.miloapis.com/activity-name'], audit.objectRef) }}" + + - name: update-activity-updated-cname + match: "!audit.user.username.startsWith('system:') && audit.verb in ['update', 'patch'] && !has(audit.objectRef.subresource) && has(audit.requestObject.spec) && has(audit.responseObject.spec) && audit.responseObject.spec.recordType == 'CNAME' && has(audit.responseObject.metadata.annotations) && 'dns.networking.miloapis.com/activity-change' in audit.responseObject.metadata.annotations && audit.responseObject.metadata.annotations['dns.networking.miloapis.com/activity-change'] == 'updated' && 'dns.networking.miloapis.com/activity-name' in audit.responseObject.metadata.annotations" + summary: "{{ actor }} updated CNAME record {{ link(audit.responseObject.metadata.annotations['dns.networking.miloapis.com/activity-name'], audit.objectRef) }} as an alias for {{ audit.responseObject.metadata.annotations['dns.networking.miloapis.com/activity-value'] }}" + + - name: update-activity-added-alias + match: "!audit.user.username.startsWith('system:') && audit.verb in ['update', 'patch'] && !has(audit.objectRef.subresource) && has(audit.requestObject.spec) && has(audit.responseObject.spec) && audit.responseObject.spec.recordType == 'ALIAS' && has(audit.responseObject.metadata.annotations) && 'dns.networking.miloapis.com/activity-change' in audit.responseObject.metadata.annotations && audit.responseObject.metadata.annotations['dns.networking.miloapis.com/activity-change'] == 'added' && 'dns.networking.miloapis.com/activity-name' in audit.responseObject.metadata.annotations" + summary: "{{ actor }} added ALIAS record {{ link(audit.responseObject.metadata.annotations['dns.networking.miloapis.com/activity-name'], audit.objectRef) }} as an alias for {{ audit.responseObject.metadata.annotations['dns.networking.miloapis.com/activity-value'] }}" + + - name: update-activity-removed-alias + match: "!audit.user.username.startsWith('system:') && audit.verb in ['update', 'patch'] && !has(audit.objectRef.subresource) && has(audit.requestObject.spec) && has(audit.responseObject.spec) && audit.responseObject.spec.recordType == 'ALIAS' && has(audit.responseObject.metadata.annotations) && 'dns.networking.miloapis.com/activity-change' in audit.responseObject.metadata.annotations && audit.responseObject.metadata.annotations['dns.networking.miloapis.com/activity-change'] == 'removed' && 'dns.networking.miloapis.com/activity-name' in audit.responseObject.metadata.annotations" + summary: "{{ actor }} deleted ALIAS record {{ link(audit.responseObject.metadata.annotations['dns.networking.miloapis.com/activity-name'], audit.objectRef) }}" + + - name: update-activity-updated-alias + match: "!audit.user.username.startsWith('system:') && audit.verb in ['update', 'patch'] && !has(audit.objectRef.subresource) && has(audit.requestObject.spec) && has(audit.responseObject.spec) && audit.responseObject.spec.recordType == 'ALIAS' && has(audit.responseObject.metadata.annotations) && 'dns.networking.miloapis.com/activity-change' in audit.responseObject.metadata.annotations && audit.responseObject.metadata.annotations['dns.networking.miloapis.com/activity-change'] == 'updated' && 'dns.networking.miloapis.com/activity-name' in audit.responseObject.metadata.annotations" + summary: "{{ actor }} updated ALIAS record {{ link(audit.responseObject.metadata.annotations['dns.networking.miloapis.com/activity-name'], audit.objectRef) }} as an alias for {{ audit.responseObject.metadata.annotations['dns.networking.miloapis.com/activity-value'] }}" + + - name: update-activity-added-mx + match: "!audit.user.username.startsWith('system:') && audit.verb in ['update', 'patch'] && !has(audit.objectRef.subresource) && has(audit.requestObject.spec) && has(audit.responseObject.spec) && audit.responseObject.spec.recordType == 'MX' && has(audit.responseObject.metadata.annotations) && 'dns.networking.miloapis.com/activity-change' in audit.responseObject.metadata.annotations && audit.responseObject.metadata.annotations['dns.networking.miloapis.com/activity-change'] == 'added' && 'dns.networking.miloapis.com/activity-name' in audit.responseObject.metadata.annotations" + summary: "{{ actor }} added MX record {{ link(audit.responseObject.metadata.annotations['dns.networking.miloapis.com/activity-name'], audit.objectRef) }} using {{ audit.responseObject.metadata.annotations['dns.networking.miloapis.com/activity-value'] }}" + + - name: update-activity-removed-mx + match: "!audit.user.username.startsWith('system:') && audit.verb in ['update', 'patch'] && !has(audit.objectRef.subresource) && has(audit.requestObject.spec) && has(audit.responseObject.spec) && audit.responseObject.spec.recordType == 'MX' && has(audit.responseObject.metadata.annotations) && 'dns.networking.miloapis.com/activity-change' in audit.responseObject.metadata.annotations && audit.responseObject.metadata.annotations['dns.networking.miloapis.com/activity-change'] == 'removed' && 'dns.networking.miloapis.com/activity-name' in audit.responseObject.metadata.annotations" + summary: "{{ actor }} deleted MX record {{ link(audit.responseObject.metadata.annotations['dns.networking.miloapis.com/activity-name'], audit.objectRef) }}" + + - name: update-activity-updated-mx + match: "!audit.user.username.startsWith('system:') && audit.verb in ['update', 'patch'] && !has(audit.objectRef.subresource) && has(audit.requestObject.spec) && has(audit.responseObject.spec) && audit.responseObject.spec.recordType == 'MX' && has(audit.responseObject.metadata.annotations) && 'dns.networking.miloapis.com/activity-change' in audit.responseObject.metadata.annotations && audit.responseObject.metadata.annotations['dns.networking.miloapis.com/activity-change'] == 'updated' && 'dns.networking.miloapis.com/activity-name' in audit.responseObject.metadata.annotations" + summary: "{{ actor }} updated MX record {{ link(audit.responseObject.metadata.annotations['dns.networking.miloapis.com/activity-name'], audit.objectRef) }} to use {{ audit.responseObject.metadata.annotations['dns.networking.miloapis.com/activity-value'] }}" + + - name: update-activity-added-txt + match: "!audit.user.username.startsWith('system:') && audit.verb in ['update', 'patch'] && !has(audit.objectRef.subresource) && has(audit.requestObject.spec) && has(audit.responseObject.spec) && audit.responseObject.spec.recordType == 'TXT' && has(audit.responseObject.metadata.annotations) && 'dns.networking.miloapis.com/activity-change' in audit.responseObject.metadata.annotations && audit.responseObject.metadata.annotations['dns.networking.miloapis.com/activity-change'] == 'added' && 'dns.networking.miloapis.com/activity-name' in audit.responseObject.metadata.annotations" + summary: "{{ actor }} added TXT record {{ link(audit.responseObject.metadata.annotations['dns.networking.miloapis.com/activity-name'], audit.objectRef) }} with {{ audit.responseObject.metadata.annotations['dns.networking.miloapis.com/activity-value'] }}" + + - name: update-activity-removed-txt + match: "!audit.user.username.startsWith('system:') && audit.verb in ['update', 'patch'] && !has(audit.objectRef.subresource) && has(audit.requestObject.spec) && has(audit.responseObject.spec) && audit.responseObject.spec.recordType == 'TXT' && has(audit.responseObject.metadata.annotations) && 'dns.networking.miloapis.com/activity-change' in audit.responseObject.metadata.annotations && audit.responseObject.metadata.annotations['dns.networking.miloapis.com/activity-change'] == 'removed' && 'dns.networking.miloapis.com/activity-name' in audit.responseObject.metadata.annotations" + summary: "{{ actor }} deleted TXT record {{ link(audit.responseObject.metadata.annotations['dns.networking.miloapis.com/activity-name'], audit.objectRef) }}" + + - name: update-activity-updated-txt + match: "!audit.user.username.startsWith('system:') && audit.verb in ['update', 'patch'] && !has(audit.objectRef.subresource) && has(audit.requestObject.spec) && has(audit.responseObject.spec) && audit.responseObject.spec.recordType == 'TXT' && has(audit.responseObject.metadata.annotations) && 'dns.networking.miloapis.com/activity-change' in audit.responseObject.metadata.annotations && audit.responseObject.metadata.annotations['dns.networking.miloapis.com/activity-change'] == 'updated' && 'dns.networking.miloapis.com/activity-name' in audit.responseObject.metadata.annotations" + summary: "{{ actor }} updated TXT record {{ link(audit.responseObject.metadata.annotations['dns.networking.miloapis.com/activity-name'], audit.objectRef) }} to {{ audit.responseObject.metadata.annotations['dns.networking.miloapis.com/activity-value'] }}" + + - name: update-activity-added-ns + match: "!audit.user.username.startsWith('system:') && audit.verb in ['update', 'patch'] && !has(audit.objectRef.subresource) && has(audit.requestObject.spec) && has(audit.responseObject.spec) && audit.responseObject.spec.recordType == 'NS' && has(audit.responseObject.metadata.annotations) && 'dns.networking.miloapis.com/activity-change' in audit.responseObject.metadata.annotations && audit.responseObject.metadata.annotations['dns.networking.miloapis.com/activity-change'] == 'added' && 'dns.networking.miloapis.com/activity-name' in audit.responseObject.metadata.annotations" + summary: "{{ actor }} added NS record {{ link(audit.responseObject.metadata.annotations['dns.networking.miloapis.com/activity-name'], audit.objectRef) }} with nameservers {{ audit.responseObject.metadata.annotations['dns.networking.miloapis.com/activity-value'] }}" + + - name: update-activity-removed-ns + match: "!audit.user.username.startsWith('system:') && audit.verb in ['update', 'patch'] && !has(audit.objectRef.subresource) && has(audit.requestObject.spec) && has(audit.responseObject.spec) && audit.responseObject.spec.recordType == 'NS' && has(audit.responseObject.metadata.annotations) && 'dns.networking.miloapis.com/activity-change' in audit.responseObject.metadata.annotations && audit.responseObject.metadata.annotations['dns.networking.miloapis.com/activity-change'] == 'removed' && 'dns.networking.miloapis.com/activity-name' in audit.responseObject.metadata.annotations" + summary: "{{ actor }} deleted NS record {{ link(audit.responseObject.metadata.annotations['dns.networking.miloapis.com/activity-name'], audit.objectRef) }}" + + - name: update-activity-updated-ns + match: "!audit.user.username.startsWith('system:') && audit.verb in ['update', 'patch'] && !has(audit.objectRef.subresource) && has(audit.requestObject.spec) && has(audit.responseObject.spec) && audit.responseObject.spec.recordType == 'NS' && has(audit.responseObject.metadata.annotations) && 'dns.networking.miloapis.com/activity-change' in audit.responseObject.metadata.annotations && audit.responseObject.metadata.annotations['dns.networking.miloapis.com/activity-change'] == 'updated' && 'dns.networking.miloapis.com/activity-name' in audit.responseObject.metadata.annotations" + summary: "{{ actor }} updated NS record {{ link(audit.responseObject.metadata.annotations['dns.networking.miloapis.com/activity-name'], audit.objectRef) }} to nameservers {{ audit.responseObject.metadata.annotations['dns.networking.miloapis.com/activity-value'] }}" + + - name: update-activity-added-other + match: "!audit.user.username.startsWith('system:') && audit.verb in ['update', 'patch'] && !has(audit.objectRef.subresource) && has(audit.requestObject.spec) && has(audit.responseObject.spec) && has(audit.responseObject.metadata.annotations) && 'dns.networking.miloapis.com/activity-change' in audit.responseObject.metadata.annotations && audit.responseObject.metadata.annotations['dns.networking.miloapis.com/activity-change'] == 'added' && 'dns.networking.miloapis.com/activity-name' in audit.responseObject.metadata.annotations" + summary: "{{ actor }} added {{ audit.responseObject.spec.recordType }} record {{ link(audit.responseObject.metadata.annotations['dns.networking.miloapis.com/activity-name'], audit.objectRef) }} with {{ audit.responseObject.metadata.annotations['dns.networking.miloapis.com/activity-value'] }}" + + - name: update-activity-removed-other + match: "!audit.user.username.startsWith('system:') && audit.verb in ['update', 'patch'] && !has(audit.objectRef.subresource) && has(audit.requestObject.spec) && has(audit.responseObject.spec) && has(audit.responseObject.metadata.annotations) && 'dns.networking.miloapis.com/activity-change' in audit.responseObject.metadata.annotations && audit.responseObject.metadata.annotations['dns.networking.miloapis.com/activity-change'] == 'removed' && 'dns.networking.miloapis.com/activity-name' in audit.responseObject.metadata.annotations" + summary: "{{ actor }} deleted {{ audit.responseObject.spec.recordType }} record {{ link(audit.responseObject.metadata.annotations['dns.networking.miloapis.com/activity-name'], audit.objectRef) }}" + + - name: update-activity-updated-other + match: "!audit.user.username.startsWith('system:') && audit.verb in ['update', 'patch'] && !has(audit.objectRef.subresource) && has(audit.requestObject.spec) && has(audit.responseObject.spec) && has(audit.responseObject.metadata.annotations) && 'dns.networking.miloapis.com/activity-change' in audit.responseObject.metadata.annotations && audit.responseObject.metadata.annotations['dns.networking.miloapis.com/activity-change'] == 'updated' && 'dns.networking.miloapis.com/activity-name' in audit.responseObject.metadata.annotations" + summary: "{{ actor }} updated {{ audit.responseObject.spec.recordType }} record {{ link(audit.responseObject.metadata.annotations['dns.networking.miloapis.com/activity-name'], audit.objectRef) }} to {{ audit.responseObject.metadata.annotations['dns.networking.miloapis.com/activity-value'] }}" + - name: update-a-aaaa match: "!audit.user.username.startsWith('system:') && audit.verb in ['update', 'patch'] && !has(audit.objectRef.subresource) && has(audit.requestObject.spec) && has(audit.responseObject.spec) && audit.responseObject.spec.recordType in ['A', 'AAAA'] && has(audit.responseObject.metadata.annotations) && 'dns.networking.miloapis.com/display-name' in audit.responseObject.metadata.annotations" summary: "{{ actor }} updated {{ audit.responseObject.spec.recordType }} record {{ link(audit.responseObject.metadata.annotations['dns.networking.miloapis.com/display-name'], audit.objectRef) }} to point to {{ audit.responseObject.metadata.annotations['dns.networking.miloapis.com/display-value'] }}" diff --git a/docs/enhancements/activity-integration.md b/docs/enhancements/activity-integration.md index ed8ca47..7510f36 100644 --- a/docs/enhancements/activity-integration.md +++ b/docs/enhancements/activity-integration.md @@ -3,7 +3,7 @@ **Status**: Implemented (Phase 1–2; UX polish for operator-readable summaries in issue #62) **Author**: Engineering **Created**: 2026-02-12 -**Updated**: 2026-07-23 +**Updated**: 2026-07-29 ## Summary @@ -101,9 +101,14 @@ The Activity Service translates audit logs and Kubernetes events into human-read |------------|---------|--------| | `dns.networking.miloapis.com/display-name` | `www.example.com` | Mutating webhook at create/update; replicator safety net | | `dns.networking.miloapis.com/display-value` | `192.0.2.10` | Same | +| `dns.networking.miloapis.com/activity-change` | `added` / `removed` / `updated` | Mutating webhook on update when `records[]` differs from OldObject (issue #72) | +| `dns.networking.miloapis.com/activity-name` | `app.example.com` | Hostname(s) that changed (FQDN) | +| `dns.networking.miloapis.com/activity-value` | `192.0.2.10` | Value(s) for the changed hostname(s) | Helpers live in `internal/display`. The mutating webhook resolves the parent `DNSZone` on the project control plane (cluster-aware admission context); if the zone is missing or cluster resolution fails, annotations are left unset and the replicator acts as a safety net. Policy fallbacks use `spec.records[0].name`. +Portal UX often keeps one `DNSRecordSet` per zone per type and edits hostnames inside `spec.records[]`. Adding or removing a hostname is therefore a Kubernetes update. Activity update rules prefer `activity-*` annotations so the timeline says **added** / **deleted** for that hostname instead of **updated** with a joined sibling list. When `activity-change` is absent (older images), rules fall back to `display-name` / `display-value`. + Admission uses `failurePolicy: Ignore` so a missing webhook server degrades activity FQDNs instead of blocking DNSRecordSet writes. Cross-cluster registration for Datum control planes ships in `config/components/admission-webhooks` (OCI path `components/admission-webhooks`), versioned with the manager image (see that directory's README). Same-cluster kind/e2e packaging stays under `config/webhook/`. ### Data Sources @@ -236,7 +241,8 @@ We use ActivityPolicy + Events for activity generation. A **mutating** webhook i ## Open Questions -1. How should we handle bulk operations (e.g., many records updated at once)? +1. How should we handle bulk operations (e.g., many records updated at once)? Mixed add+remove in one write is summarized as `updated` with affected names; pure adds/removes use `added`/`removed` via `activity-*` annotations. 2. Should activities include namespace information for multi-tenant visibility? 3. How should we format multiple record values (e.g., multiple A records for the same name)? 4. Portal activity *detail* views may still emphasize resource ids; summary link text is the FQDN — follow up in the portal if detail prominence is still weak. +5. Longer-term: portal may choose one DNSRecordSet per hostname so create/delete map 1:1; activity-* annotations remain useful until then. diff --git a/internal/activitypolicy/dnsrecordset_policy_test.go b/internal/activitypolicy/dnsrecordset_policy_test.go index 16304c5..b2500ed 100644 --- a/internal/activitypolicy/dnsrecordset_policy_test.go +++ b/internal/activitypolicy/dnsrecordset_policy_test.go @@ -133,6 +133,44 @@ func TestDNSRecordSetPolicy_Structure(t *testing.T) { t.Errorf("update-a-aaaa must include A/AAAA recordType in summary, got: %s", updateA) } + for _, name := range []string{ + "update-activity-added-a-aaaa", + "update-activity-removed-a-aaaa", + "update-activity-updated-a-aaaa", + } { + m, ok := byName[name] + if !ok { + t.Fatalf("missing rule %q", name) + } + if !strings.Contains(m, "activity-change") { + t.Errorf("%s must match activity-change annotation, got: %s", name, m) + } + if !strings.Contains(m, "has(audit.requestObject.spec)") { + t.Errorf("%s must require requestObject.spec", name) + } + } + addedSummary := summaries["update-activity-added-a-aaaa"] + if !strings.Contains(addedSummary, "added") || !strings.Contains(addedSummary, "activity-name") { + t.Errorf("update-activity-added-a-aaaa must say added with activity-name, got: %s", addedSummary) + } + removedSummary := summaries["update-activity-removed-a-aaaa"] + if !strings.Contains(removedSummary, "deleted") || !strings.Contains(removedSummary, "activity-name") { + t.Errorf("update-activity-removed-a-aaaa must say deleted with activity-name, got: %s", removedSummary) + } + // Activity rules must appear before display-name update fallbacks. + addedIdx, updateAIdx := -1, -1 + for i, r := range pol.Spec.AuditRules { + switch r.Name { + case "update-activity-added-a-aaaa": + addedIdx = i + case "update-a-aaaa": + updateAIdx = i + } + } + if addedIdx < 0 || updateAIdx < 0 || addedIdx >= updateAIdx { + t.Errorf("update-activity-added-a-aaaa (idx %d) must precede update-a-aaaa (idx %d)", addedIdx, updateAIdx) + } + createCNAME := summaries["create-cname"] if !strings.Contains(createCNAME, "CNAME record") { t.Errorf("create-cname must name the record type, got: %s", createCNAME) @@ -251,6 +289,70 @@ func TestDNSRecordSetPolicy_CELMatchFixtures(t *testing.T) { }, }, }, + { + name: "add hostname on multi-name A object (#72)", + wantRule: "update-activity-added-a-aaaa", + audit: map[string]any{ + "user": map[string]any{"username": "dgaghan@datum.net"}, + "verb": "patch", + "objectRef": map[string]any{}, + "requestObject": map[string]any{ + "spec": map[string]any{ + "records": []any{ + map[string]any{"name": "www", "a": map[string]any{"content": "192.168.1.1"}}, + map[string]any{"name": "app", "a": map[string]any{"content": "192.168.1.1"}}, + }, + }, + }, + "responseObject": map[string]any{ + "metadata": map[string]any{"annotations": map[string]any{ + "dns.networking.miloapis.com/display-name": "www.dodik.me, app.dodik.me", + "dns.networking.miloapis.com/display-value": "192.168.1.1, 192.168.1.1", + "dns.networking.miloapis.com/activity-change": "added", + "dns.networking.miloapis.com/activity-name": "app.dodik.me", + "dns.networking.miloapis.com/activity-value": "192.168.1.1", + }}, + "spec": map[string]any{ + "recordType": "A", + "records": []any{ + map[string]any{"name": "www", "a": map[string]any{"content": "192.168.1.1"}}, + map[string]any{"name": "app", "a": map[string]any{"content": "192.168.1.1"}}, + }, + }, + }, + }, + }, + { + name: "remove hostname on multi-name A object (#72)", + wantRule: "update-activity-removed-a-aaaa", + audit: map[string]any{ + "user": map[string]any{"username": "dgaghan@datum.net"}, + "verb": "patch", + "objectRef": map[string]any{}, + "requestObject": map[string]any{ + "spec": map[string]any{ + "records": []any{ + map[string]any{"name": "www", "a": map[string]any{"content": "192.168.1.1"}}, + }, + }, + }, + "responseObject": map[string]any{ + "metadata": map[string]any{"annotations": map[string]any{ + "dns.networking.miloapis.com/display-name": "www.dodik.me", + "dns.networking.miloapis.com/display-value": "192.168.1.1", + "dns.networking.miloapis.com/activity-change": "removed", + "dns.networking.miloapis.com/activity-name": "app.dodik.me", + "dns.networking.miloapis.com/activity-value": "192.168.1.1", + }}, + "spec": map[string]any{ + "recordType": "A", + "records": []any{ + map[string]any{"name": "www", "a": map[string]any{"content": "192.168.1.1"}}, + }, + }, + }, + }, + }, { name: "human delete with annotations", wantRule: "delete-annotated", diff --git a/internal/display/activity.go b/internal/display/activity.go new file mode 100644 index 0000000..7e006c1 --- /dev/null +++ b/internal/display/activity.go @@ -0,0 +1,251 @@ +// SPDX-License-Identifier: AGPL-3.0-only + +package display + +import ( + "strings" + + dnsv1alpha1 "go.miloapis.com/dns-operator/api/v1alpha1" +) + +// Activity annotation keys describe the logical hostname-level change on a +// multi-name DNSRecordSet update (issue #72). ActivityPolicy prefers these +// over display-name / display-value for update summaries. +const ( + AnnotationActivityChange = "dns.networking.miloapis.com/activity-change" + AnnotationActivityName = "dns.networking.miloapis.com/activity-name" + AnnotationActivityValue = "dns.networking.miloapis.com/activity-value" +) + +// ActivityChange values stamped on AnnotationActivityChange. +const ( + ActivityChangeAdded = "added" + ActivityChangeRemoved = "removed" + ActivityChangeUpdated = "updated" +) + +// ActivityDiff is the hostname-level change between two DNSRecordSet specs. +type ActivityDiff struct { + Change string + Name string + Value string +} + +// ComputeActivityDiff compares old and new record sets and returns a single +// logical change suitable for Activity summaries. Pure adds → "added", pure +// removes → "removed", pure value changes → "updated". Mixed edits fall back +// to "updated" with all affected names. +func ComputeActivityDiff(oldRS, newRS *dnsv1alpha1.DNSRecordSet, zoneDomainName string) ActivityDiff { + if newRS == nil { + return ActivityDiff{} + } + if oldRS == nil { + return ActivityDiff{} + } + + oldByName := entriesByName(oldRS) + newByName := entriesByName(newRS) + + var added, removed, updated []string + for name := range newByName { + if _, ok := oldByName[name]; !ok { + added = append(added, name) + continue + } + if signatureForName(oldRS, name) != signatureForName(newRS, name) { + updated = append(updated, name) + } + } + for name := range oldByName { + if _, ok := newByName[name]; !ok { + removed = append(removed, name) + } + } + + // Stable order matching UniqueRecordNames / first-occurrence in new, then old. + added = orderNames(UniqueRecordNames(newRS), added) + updated = orderNames(UniqueRecordNames(newRS), updated) + removed = orderNames(UniqueRecordNames(oldRS), removed) + + switch { + case len(added) > 0 && len(removed) == 0 && len(updated) == 0: + return ActivityDiff{ + Change: ActivityChangeAdded, + Name: fqdnsForNames(added, zoneDomainName), + Value: displayValueForNames(newRS, added), + } + case len(removed) > 0 && len(added) == 0 && len(updated) == 0: + return ActivityDiff{ + Change: ActivityChangeRemoved, + Name: fqdnsForNames(removed, zoneDomainName), + Value: displayValueForNames(oldRS, removed), + } + case len(updated) > 0 && len(added) == 0 && len(removed) == 0: + return ActivityDiff{ + Change: ActivityChangeUpdated, + Name: fqdnsForNames(updated, zoneDomainName), + Value: displayValueForNames(newRS, updated), + } + case len(added) == 0 && len(removed) == 0 && len(updated) == 0: + return ActivityDiff{} + default: + // Mixed add/remove/update in one write — summarize as updated. + affected := append(append(append([]string{}, added...), removed...), updated...) + affected = dedupePreserveOrder(affected) + src := newRS + if len(newByName) == 0 { + src = oldRS + } + return ActivityDiff{ + Change: ActivityChangeUpdated, + Name: fqdnsForNames(affected, zoneDomainName), + Value: displayValueForNames(src, intersectNames(affected, UniqueRecordNames(src))), + } + } +} + +// EnsureActivityAnnotations stamps or clears activity-* annotations based on +// the diff between oldRS and rs. Returns true if annotations changed. +func EnsureActivityAnnotations(rs, oldRS *dnsv1alpha1.DNSRecordSet, zoneDomainName string) bool { + if rs == nil { + return false + } + diff := ComputeActivityDiff(oldRS, rs, zoneDomainName) + if diff.Change == "" { + return ClearActivityAnnotations(rs) + } + + if rs.Annotations == nil { + rs.Annotations = make(map[string]string) + } + + changed := false + if rs.Annotations[AnnotationActivityChange] != diff.Change { + rs.Annotations[AnnotationActivityChange] = diff.Change + changed = true + } + if rs.Annotations[AnnotationActivityName] != diff.Name { + rs.Annotations[AnnotationActivityName] = diff.Name + changed = true + } + if rs.Annotations[AnnotationActivityValue] != diff.Value { + rs.Annotations[AnnotationActivityValue] = diff.Value + changed = true + } + return changed +} + +// ClearActivityAnnotations removes activity-* annotations. Returns true if any +// were present. +func ClearActivityAnnotations(rs *dnsv1alpha1.DNSRecordSet) bool { + if rs == nil || rs.Annotations == nil { + return false + } + changed := false + for _, key := range []string{AnnotationActivityChange, AnnotationActivityName, AnnotationActivityValue} { + if _, ok := rs.Annotations[key]; ok { + delete(rs.Annotations, key) + changed = true + } + } + return changed +} + +func entriesByName(rs *dnsv1alpha1.DNSRecordSet) map[string][]dnsv1alpha1.RecordEntry { + out := make(map[string][]dnsv1alpha1.RecordEntry) + if rs == nil { + return out + } + for _, r := range rs.Spec.Records { + out[r.Name] = append(out[r.Name], r) + } + return out +} + +func subsetForNames(rs *dnsv1alpha1.DNSRecordSet, names []string) *dnsv1alpha1.DNSRecordSet { + want := make(map[string]struct{}, len(names)) + for _, n := range names { + want[n] = struct{}{} + } + sub := &dnsv1alpha1.DNSRecordSet{ + Spec: dnsv1alpha1.DNSRecordSetSpec{ + RecordType: rs.Spec.RecordType, + }, + } + for _, r := range rs.Spec.Records { + if _, ok := want[r.Name]; ok { + sub.Spec.Records = append(sub.Spec.Records, r) + } + } + return sub +} + +func signatureForName(rs *dnsv1alpha1.DNSRecordSet, name string) string { + return ComputeDisplayValue(subsetForNames(rs, []string{name})) +} + +func displayValueForNames(rs *dnsv1alpha1.DNSRecordSet, names []string) string { + if len(names) == 0 { + return "" + } + return ComputeDisplayValue(subsetForNames(rs, names)) +} + +func fqdnsForNames(names []string, zoneDomainName string) string { + fqdns := make([]string, 0, len(names)) + for _, name := range names { + fqdns = append(fqdns, BuildFQDN(name, zoneDomainName)) + } + return strings.Join(fqdns, ", ") +} + +func orderNames(preferredOrder, selected []string) []string { + if len(selected) == 0 { + return nil + } + want := make(map[string]struct{}, len(selected)) + for _, n := range selected { + want[n] = struct{}{} + } + var out []string + for _, n := range preferredOrder { + if _, ok := want[n]; ok { + out = append(out, n) + delete(want, n) + } + } + for _, n := range selected { + if _, ok := want[n]; ok { + out = append(out, n) + delete(want, n) + } + } + return out +} + +func dedupePreserveOrder(names []string) []string { + seen := make(map[string]struct{}, len(names)) + var out []string + for _, n := range names { + if _, ok := seen[n]; ok { + continue + } + seen[n] = struct{}{} + out = append(out, n) + } + return out +} + +func intersectNames(names, available []string) []string { + avail := make(map[string]struct{}, len(available)) + for _, n := range available { + avail[n] = struct{}{} + } + var out []string + for _, n := range names { + if _, ok := avail[n]; ok { + out = append(out, n) + } + } + return out +} diff --git a/internal/display/activity_test.go b/internal/display/activity_test.go new file mode 100644 index 0000000..1b1e415 --- /dev/null +++ b/internal/display/activity_test.go @@ -0,0 +1,136 @@ +// SPDX-License-Identifier: AGPL-3.0-only + +package display + +import ( + "testing" + + dnsv1alpha1 "go.miloapis.com/dns-operator/api/v1alpha1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" +) + +func aRecord(name, ip string) dnsv1alpha1.RecordEntry { + return dnsv1alpha1.RecordEntry{Name: name, A: &dnsv1alpha1.ARecordSpec{Content: ip}} +} + +func aRS(records ...dnsv1alpha1.RecordEntry) *dnsv1alpha1.DNSRecordSet { + return &dnsv1alpha1.DNSRecordSet{ + ObjectMeta: metav1.ObjectMeta{Name: "zone-a"}, + Spec: dnsv1alpha1.DNSRecordSetSpec{ + RecordType: dnsv1alpha1.RRTypeA, + Records: records, + }, + } +} + +func TestComputeActivityDiff(t *testing.T) { + t.Parallel() + + zone := "dodik.me" + www := aRecord("www", "192.168.1.1") + app := aRecord("app", "192.168.1.1") + app2 := aRecord("app", "10.0.0.1") + + tests := []struct { + name string + old *dnsv1alpha1.DNSRecordSet + new *dnsv1alpha1.DNSRecordSet + want ActivityDiff + }{ + { + name: "add second hostname", + old: aRS(www), + new: aRS(www, app), + want: ActivityDiff{ + Change: ActivityChangeAdded, + Name: "app.dodik.me", + Value: "192.168.1.1", + }, + }, + { + name: "remove hostname leaving sibling", + old: aRS(www, app), + new: aRS(www), + want: ActivityDiff{ + Change: ActivityChangeRemoved, + Name: "app.dodik.me", + Value: "192.168.1.1", + }, + }, + { + name: "update value on existing hostname", + old: aRS(www, app), + new: aRS(www, app2), + want: ActivityDiff{ + Change: ActivityChangeUpdated, + Name: "app.dodik.me", + Value: "10.0.0.1", + }, + }, + { + name: "no records change", + old: aRS(www), + new: aRS(www), + want: ActivityDiff{}, + }, + { + name: "nil old", + old: nil, + new: aRS(www), + want: ActivityDiff{}, + }, + { + name: "mixed add and remove", + old: aRS(www), + new: aRS(app), + want: ActivityDiff{ + Change: ActivityChangeUpdated, + Name: "app.dodik.me, www.dodik.me", + Value: "192.168.1.1", + }, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + got := ComputeActivityDiff(tt.old, tt.new, zone) + if got != tt.want { + t.Errorf("ComputeActivityDiff() = %+v, want %+v", got, tt.want) + } + }) + } +} + +func TestEnsureActivityAnnotations(t *testing.T) { + t.Parallel() + + old := aRS(aRecord("www", "192.168.1.1")) + rs := aRS(aRecord("www", "192.168.1.1"), aRecord("app", "192.168.1.1")) + if !EnsureActivityAnnotations(rs, old, "dodik.me") { + t.Fatal("expected annotations to change") + } + if got := rs.Annotations[AnnotationActivityChange]; got != ActivityChangeAdded { + t.Errorf("activity-change = %q, want %q", got, ActivityChangeAdded) + } + if got := rs.Annotations[AnnotationActivityName]; got != "app.dodik.me" { + t.Errorf("activity-name = %q, want app.dodik.me", got) + } + if got := rs.Annotations[AnnotationActivityValue]; got != "192.168.1.1" { + t.Errorf("activity-value = %q, want 192.168.1.1", got) + } + + // Identical records clear stale activity annotations. + same := aRS(aRecord("www", "192.168.1.1")) + same.Annotations = map[string]string{ + AnnotationActivityChange: ActivityChangeAdded, + AnnotationActivityName: "app.dodik.me", + AnnotationActivityValue: "192.168.1.1", + } + if !EnsureActivityAnnotations(same, same.DeepCopy(), "dodik.me") { + t.Fatal("expected clear to report change") + } + if _, ok := same.Annotations[AnnotationActivityChange]; ok { + t.Fatalf("expected activity annotations cleared, got %v", same.Annotations) + } +} diff --git a/internal/webhook/dnsrecordset_mutating.go b/internal/webhook/dnsrecordset_mutating.go index 1706531..b4280f9 100644 --- a/internal/webhook/dnsrecordset_mutating.go +++ b/internal/webhook/dnsrecordset_mutating.go @@ -7,6 +7,7 @@ package webhook import ( "context" + "encoding/json" "fmt" "strings" @@ -25,7 +26,8 @@ import ( ) // DNSRecordSetMutator stamps display-name / display-value annotations at -// admission so ActivityPolicy create audits can include the FQDN. +// admission so ActivityPolicy create audits can include the FQDN. On update it +// also stamps activity-* annotations from a records[] diff (issue #72). type DNSRecordSetMutator struct { // Manager is optional; when set and cluster context is present, DNSZone // lookups use the project control plane client. @@ -59,9 +61,28 @@ func (m *DNSRecordSetMutator) Default(ctx context.Context, obj runtime.Object) e } _ = display.EnsureAnnotations(rs, zone.Spec.DomainName) + m.stampActivityAnnotations(ctx, rs, zone.Spec.DomainName) return nil } +// stampActivityAnnotations compares against admission OldObject when present +// (updates) and stamps or clears activity-change / activity-name / activity-value. +func (m *DNSRecordSetMutator) stampActivityAnnotations(ctx context.Context, rs *dnsv1alpha1.DNSRecordSet, zoneDomainName string) { + req, err := admission.RequestFromContext(ctx) + if err != nil || len(req.OldObject.Raw) == 0 { + _ = display.ClearActivityAnnotations(rs) + return + } + + var oldRS dnsv1alpha1.DNSRecordSet + if err := json.Unmarshal(req.OldObject.Raw, &oldRS); err != nil { + logf.FromContext(ctx).V(1).Info("skipping activity annotations; failed to decode OldObject", "error", err) + _ = display.ClearActivityAnnotations(rs) + return + } + _ = display.EnsureActivityAnnotations(rs, &oldRS, zoneDomainName) +} + // clientForZoneLookup returns the project control plane client when cluster // context is available, otherwise the local Client. // diff --git a/internal/webhook/dnsrecordset_mutating_test.go b/internal/webhook/dnsrecordset_mutating_test.go index a898817..8d1e7a9 100644 --- a/internal/webhook/dnsrecordset_mutating_test.go +++ b/internal/webhook/dnsrecordset_mutating_test.go @@ -4,9 +4,11 @@ package webhook import ( "context" + "encoding/json" "fmt" "testing" + admissionv1 "k8s.io/api/admission/v1" authv1 "k8s.io/api/authentication/v1" corev1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" @@ -14,6 +16,7 @@ import ( "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/client/fake" "sigs.k8s.io/controller-runtime/pkg/cluster" + "sigs.k8s.io/controller-runtime/pkg/webhook/admission" mccontext "sigs.k8s.io/multicluster-runtime/pkg/context" mcmanager "sigs.k8s.io/multicluster-runtime/pkg/manager" @@ -151,6 +154,72 @@ func TestDNSRecordSetMutator_Default(t *testing.T) { } } +func TestDNSRecordSetMutator_Default_activityDiff(t *testing.T) { + t.Parallel() + + scheme := runtime.NewScheme() + if err := dnsv1alpha1.AddToScheme(scheme); err != nil { + t.Fatalf("AddToScheme: %v", err) + } + + zone := &dnsv1alpha1.DNSZone{ + ObjectMeta: metav1.ObjectMeta{Name: "my-zone", Namespace: "default"}, + Spec: dnsv1alpha1.DNSZoneSpec{DomainName: "dodik.me"}, + } + cl := fake.NewClientBuilder().WithScheme(scheme).WithRuntimeObjects(zone).Build() + m := &DNSRecordSetMutator{Client: cl} + + oldRS := &dnsv1alpha1.DNSRecordSet{ + ObjectMeta: metav1.ObjectMeta{Name: "zone-a", Namespace: "default"}, + Spec: dnsv1alpha1.DNSRecordSetSpec{ + DNSZoneRef: corev1.LocalObjectReference{Name: "my-zone"}, + RecordType: dnsv1alpha1.RRTypeA, + Records: []dnsv1alpha1.RecordEntry{{Name: "www", A: &dnsv1alpha1.ARecordSpec{Content: "192.168.1.1"}}}, + }, + } + oldRaw, err := json.Marshal(oldRS) + if err != nil { + t.Fatalf("marshal old: %v", err) + } + + newRS := oldRS.DeepCopy() + newRS.Spec.Records = append(newRS.Spec.Records, dnsv1alpha1.RecordEntry{ + Name: "app", A: &dnsv1alpha1.ARecordSpec{Content: "192.168.1.1"}, + }) + + ctx := admission.NewContextWithRequest(context.Background(), admission.Request{ + AdmissionRequest: admissionv1.AdmissionRequest{ + Operation: admissionv1.Update, + OldObject: runtime.RawExtension{Raw: oldRaw}, + }, + }) + if err := m.Default(ctx, newRS); err != nil { + t.Fatalf("Default: %v", err) + } + if got := newRS.Annotations[display.AnnotationActivityChange]; got != display.ActivityChangeAdded { + t.Errorf("activity-change = %q, want %q", got, display.ActivityChangeAdded) + } + if got := newRS.Annotations[display.AnnotationActivityName]; got != "app.dodik.me" { + t.Errorf("activity-name = %q, want app.dodik.me", got) + } + if got := newRS.Annotations[display.AnnotationDisplayName]; got != "www.dodik.me, app.dodik.me" { + t.Errorf("display-name = %q, want joined FQDNs", got) + } + + // Create (no OldObject) clears activity annotations. + createRS := oldRS.DeepCopy() + createRS.Annotations = map[string]string{ + display.AnnotationActivityChange: display.ActivityChangeAdded, + display.AnnotationActivityName: "stale.dodik.me", + } + if err := m.Default(context.Background(), createRS); err != nil { + t.Fatalf("Default create: %v", err) + } + if _, ok := createRS.Annotations[display.AnnotationActivityChange]; ok { + t.Fatalf("create should clear activity annotations, got %v", createRS.Annotations) + } +} + func TestDNSRecordSetMutator_Default_projectClusterClient(t *testing.T) { t.Parallel() From e289b7e2dcd2702b6718ec8b82f7243ca4e1d4e2 Mon Sep 17 00:00:00 2001 From: Matt Jenkinson <75292329+mattdjenkinson@users.noreply.github.com> Date: Wed, 29 Jul 2026 13:31:58 +0100 Subject: [PATCH 2/4] fix: Pre-allocate slices for golangci prealloc --- internal/display/activity.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/internal/display/activity.go b/internal/display/activity.go index 7e006c1..8e43dbb 100644 --- a/internal/display/activity.go +++ b/internal/display/activity.go @@ -207,7 +207,7 @@ func orderNames(preferredOrder, selected []string) []string { for _, n := range selected { want[n] = struct{}{} } - var out []string + out := make([]string, 0, len(selected)) for _, n := range preferredOrder { if _, ok := want[n]; ok { out = append(out, n) @@ -225,7 +225,7 @@ func orderNames(preferredOrder, selected []string) []string { func dedupePreserveOrder(names []string) []string { seen := make(map[string]struct{}, len(names)) - var out []string + out := make([]string, 0, len(names)) for _, n := range names { if _, ok := seen[n]; ok { continue @@ -241,7 +241,7 @@ func intersectNames(names, available []string) []string { for _, n := range available { avail[n] = struct{}{} } - var out []string + out := make([]string, 0, len(names)) for _, n := range names { if _, ok := avail[n]; ok { out = append(out, n) From 56e52c85c0af25fb47920b1eb351c32a42fb55e4 Mon Sep 17 00:00:00 2001 From: Matt Jenkinson <75292329+mattdjenkinson@users.noreply.github.com> Date: Wed, 29 Jul 2026 14:40:26 +0100 Subject: [PATCH 3/4] fix: Require webhook success for DNSRecordSet admits failurePolicy Ignore lets writes succeed without activity annotations and those audits never self-heal. Fail is safe now that the MWC ships with the same OCI tag as the manager (follow-up to #70). --- config/components/admission-webhooks/README.md | 8 +++++++- .../mutating-webhook-configuration.yaml | 7 ++++--- config/webhook/manifests.yaml | 2 +- docs/enhancements/activity-integration.md | 2 +- internal/webhook/dnsrecordset_mutating.go | 2 +- 5 files changed, 14 insertions(+), 7 deletions(-) diff --git a/config/components/admission-webhooks/README.md b/config/components/admission-webhooks/README.md index 94ac279..0f3753f 100644 --- a/config/components/admission-webhooks/README.md +++ b/config/components/admission-webhooks/README.md @@ -14,6 +14,12 @@ apply. Hand-authoring the MWC in infra decoupled those versions and caused a production write outage when `failurePolicy: Fail` hit a build with nothing listening on `:9443` (see [#69](https://github.com/datum-cloud/dns-operator/issues/69)). +`failurePolicy` is `Fail`. With co-versioned registration, a missing webhook +should block DNSRecordSet writes rather than admit without activity +annotations (Ignore has no self-recovery for the audit event that already +fired). Always apply this path from the same OCI tag as the manager +Deployment. + This directory is **not** included in `config/default` or the replicator overlay. Same-cluster packaging for kind/e2e stays under `config/webhook/`. @@ -21,7 +27,7 @@ overlay. Same-cluster packaging for kind/e2e stays under `config/webhook/`. | Kind | Name | Notes | | ---- | ---- | ----- | -| `MutatingWebhookConfiguration` | `dns-operator-mutating-webhook-configuration` | `failurePolicy: Ignore`; Service ref is `dns-operator-webhook-service` / `datum-dns-system` | +| `MutatingWebhookConfiguration` | `dns-operator-mutating-webhook-configuration` | `failurePolicy: Fail`; Service ref is `dns-operator-webhook-service` / `datum-dns-system` | No Service and no kustomize `namespace` transformer: Flux `targetNamespace` must not rewrite `clientConfig.service.namespace`. diff --git a/config/components/admission-webhooks/mutating-webhook-configuration.yaml b/config/components/admission-webhooks/mutating-webhook-configuration.yaml index f1e186e..932c5e4 100644 --- a/config/components/admission-webhooks/mutating-webhook-configuration.yaml +++ b/config/components/admission-webhooks/mutating-webhook-configuration.yaml @@ -5,8 +5,9 @@ # clientConfig points at the webhook Service on the infra/deployment cluster. # Do not add kustomize nameReference/namespace transformers here: Flux # targetNamespace would rewrite the Service namespace and break cross-cluster -# lookup. failurePolicy is Ignore so a missing webhook server degrades -# activity FQDNs instead of blocking all DNSRecordSet writes. +# lookup. failurePolicy is Fail so a missing webhook does not silently +# skip activity annotations (no self-recovery for the audit event). The +# MWC must ship with the same OCI tag as the manager that serves it. apiVersion: admissionregistration.k8s.io/v1 kind: MutatingWebhookConfiguration metadata: @@ -21,7 +22,7 @@ webhooks: namespace: datum-dns-system path: /mutate-dns-networking-miloapis-com-v1alpha1-dnsrecordset port: 443 - failurePolicy: Ignore + failurePolicy: Fail matchPolicy: Equivalent sideEffects: None timeoutSeconds: 10 diff --git a/config/webhook/manifests.yaml b/config/webhook/manifests.yaml index 8daadb2..bb40572 100644 --- a/config/webhook/manifests.yaml +++ b/config/webhook/manifests.yaml @@ -11,7 +11,7 @@ webhooks: name: webhook-service namespace: system path: /mutate-dns-networking-miloapis-com-v1alpha1-dnsrecordset - failurePolicy: Ignore + failurePolicy: Fail name: mdnsrecordset.kb.io rules: - apiGroups: diff --git a/docs/enhancements/activity-integration.md b/docs/enhancements/activity-integration.md index 7510f36..7fa058f 100644 --- a/docs/enhancements/activity-integration.md +++ b/docs/enhancements/activity-integration.md @@ -109,7 +109,7 @@ Helpers live in `internal/display`. The mutating webhook resolves the parent `DN Portal UX often keeps one `DNSRecordSet` per zone per type and edits hostnames inside `spec.records[]`. Adding or removing a hostname is therefore a Kubernetes update. Activity update rules prefer `activity-*` annotations so the timeline says **added** / **deleted** for that hostname instead of **updated** with a joined sibling list. When `activity-change` is absent (older images), rules fall back to `display-name` / `display-value`. -Admission uses `failurePolicy: Ignore` so a missing webhook server degrades activity FQDNs instead of blocking DNSRecordSet writes. Cross-cluster registration for Datum control planes ships in `config/components/admission-webhooks` (OCI path `components/admission-webhooks`), versioned with the manager image (see that directory's README). Same-cluster kind/e2e packaging stays under `config/webhook/`. +Admission uses `failurePolicy: Fail` so a missing webhook blocks DNSRecordSet writes instead of admitting without activity annotations (those audits do not self-heal). Cross-cluster registration for Datum control planes ships in `config/components/admission-webhooks` (OCI path `components/admission-webhooks`), versioned with the same manager image tag (see that directory's README). Same-cluster kind/e2e packaging stays under `config/webhook/`. ### Data Sources diff --git a/internal/webhook/dnsrecordset_mutating.go b/internal/webhook/dnsrecordset_mutating.go index b4280f9..650f163 100644 --- a/internal/webhook/dnsrecordset_mutating.go +++ b/internal/webhook/dnsrecordset_mutating.go @@ -2,7 +2,7 @@ // Package webhook provides admission webhooks for DNS resources. // -// +kubebuilder:webhook:path=/mutate-dns-networking-miloapis-com-v1alpha1-dnsrecordset,mutating=true,failurePolicy=ignore,sideEffects=None,groups=dns.networking.miloapis.com,resources=dnsrecordsets,verbs=create;update,versions=v1alpha1,name=mdnsrecordset.kb.io,admissionReviewVersions=v1 +// +kubebuilder:webhook:path=/mutate-dns-networking-miloapis-com-v1alpha1-dnsrecordset,mutating=true,failurePolicy=fail,sideEffects=None,groups=dns.networking.miloapis.com,resources=dnsrecordsets,verbs=create;update,versions=v1alpha1,name=mdnsrecordset.kb.io,admissionReviewVersions=v1 package webhook import ( From 9a616ff989eaa9afd6a7eb70e06992419b7133ec Mon Sep 17 00:00:00 2001 From: Matt Jenkinson <75292329+mattdjenkinson@users.noreply.github.com> Date: Wed, 29 Jul 2026 14:46:40 +0100 Subject: [PATCH 4/4] fix: Extract repeated test host constant for goconst --- internal/pdns/pdns_test.go | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/internal/pdns/pdns_test.go b/internal/pdns/pdns_test.go index ac0500f..90108e5 100644 --- a/internal/pdns/pdns_test.go +++ b/internal/pdns/pdns_test.go @@ -15,8 +15,9 @@ import ( ) const ( - ns1ExampleNet = "ns1.example.net." - exampleCom = "example.com." + ns1ExampleNet = "ns1.example.net." + exampleCom = "example.com." + targetExampleNet = "target.example.net." ) func TestCreateGetDeleteZoneAndRRSets(t *testing.T) { @@ -196,7 +197,7 @@ func TestBuildRRSets_NormalizationAndFormats(t *testing.T) { }, } rr = buildRRSets("example.com", rsALIAS) - if rr[0].Type != "ALIAS" || rr[0].Records[0].Content != "target.example.net." { + if rr[0].Type != "ALIAS" || rr[0].Records[0].Content != targetExampleNet { t.Fatalf("ALIAS rrset unexpected: %#v", rr) } @@ -718,13 +719,13 @@ func TestBuildRRSets_DeduplicatesRecords(t *testing.T) { Spec: dnsv1alpha1.DNSRecordSetSpec{ RecordType: dnsv1alpha1.RRTypeCNAME, Records: []dnsv1alpha1.RecordEntry{ - {Name: "www", CNAME: &dnsv1alpha1.CNAMERecordSpec{Content: "target.example.net."}}, - {Name: "www", CNAME: &dnsv1alpha1.CNAMERecordSpec{Content: "target.example.net."}}, + {Name: "www", CNAME: &dnsv1alpha1.CNAMERecordSpec{Content: targetExampleNet}}, + {Name: "www", CNAME: &dnsv1alpha1.CNAMERecordSpec{Content: targetExampleNet}}, }, }, } rr = buildRRSets("example.com", rsCNAME) - if len(rr) != 1 || len(rr[0].Records) != 1 || rr[0].Records[0].Content != "target.example.net." { + if len(rr) != 1 || len(rr[0].Records) != 1 || rr[0].Records[0].Content != targetExampleNet { t.Fatalf("expected CNAME deduped to single record, got %#v", rr) } }