diff --git a/docs/cli/datumctl-dns.md b/docs/cli/datumctl-dns.md index 51b731c4..23cf02ad 100644 --- a/docs/cli/datumctl-dns.md +++ b/docs/cli/datumctl-dns.md @@ -292,6 +292,19 @@ Drop `--dry-run` to apply the changes. Re-running an unchanged file reports `No Datum manages some records for you — the zone's SOA and apex NS records, and records created by a Gateway. `apply` never prunes or modifies them, and reports what it skipped. +## Private zones that share a domain + +Private zones in different VPCs can use the same domain. Every VPC's default zone is `datum.internal`, for example. When more than one zone matches what you typed, the plugin refuses rather than guess, and lists each zone's object name and the VPCs that use it. + +Pick one with `--vpc `, which uses the zone that VPC resolves, or pass the zone's object name in place of the domain: + +```sh +datumctl dns record create datum.internal db A 10.0.0.5 --vpc app +datumctl dns record list managed-3f9c2a +``` + +`--vpc` works on every `record` command and on `zone describe`, `delete`, `nameservers`, `import`, and `export`. It applies only to private zones. Suggested next steps name a private zone by its object name, and deleting a private zone asks you to confirm with its object name. + ## Output formats and scripting Every command accepts `-o`: @@ -352,6 +365,7 @@ Add `--verbose` to see the underlying cause. | DNS is not enabled for the project | Run `datumctl services enable dns.networking.miloapis.com --wait`. | | The domain doesn't resolve | Run `datumctl dns zone nameservers --check`. If delegation is `Incomplete` or `Partial`, fix it at your registrar. | | A record is stuck at `Pending` | This is normal right after a write. If it lasts, run `datumctl dns record describe ` for the server's message. | +| `N zones in this project match ""` | Several zones share the domain. See [Private zones that share a domain](#private-zones-that-share-a-domain). | | `Conflict` | Another record occupies that name. Usually the name isn't inside the zone — check for `www.example.com` where `www` was meant. Otherwise see [DNS record conflict troubleshooting](../troubleshooting/dnsrecordset-downstream-orphan.md). | | `Not owner` | Another record set owns that name; `describe` names it. Edit the record through that set, or delete the set first. Records Datum manages, such as a Gateway's, revert if you edit them. | diff --git a/internal/cmd/dns/record/apply.go b/internal/cmd/dns/record/apply.go index 47b1c262..1f7c6fd1 100644 --- a/internal/cmd/dns/record/apply.go +++ b/internal/cmd/dns/record/apply.go @@ -123,7 +123,7 @@ func runApply(cmd *cobra.Command, domain, file string, prune, dryRun bool) error return err } - zone, err := resolveZone(ctx, c, domain) + zone, err := resolveZone(ctx, c, domain, util.VPCFromCmd(cmd)) if err != nil { return err } @@ -171,7 +171,7 @@ func confirmApply(cmd *cobra.Command, zone *dnsv1alpha1.DNSZone, plan *plan, err } ok, err := util.ConfirmYesNo(cmd.InOrStdin(), errOut, fmt.Sprintf("Apply %d %s to %s?", - len(plan.changes), pluralize(len(plan.changes), "change"), zone.Spec.DomainName), + len(plan.changes), pluralize(len(plan.changes), "change"), util.ZoneLabel(zone)), false) if err != nil { return err diff --git a/internal/cmd/dns/record/client.go b/internal/cmd/dns/record/client.go index 24e8d8c3..626b1da5 100644 --- a/internal/cmd/dns/record/client.go +++ b/internal/cmd/dns/record/client.go @@ -30,10 +30,9 @@ const ( // can inject a fake client without a control plane behind it. var clientFactory = util.NewClient -// resolveZone finds the DNSZone serving a domain. Users type the domain name, -// which is a selectable field; the object name is accepted as a fallback so a -// name copied out of `kubectl get dnszones` also works. -func resolveZone(ctx context.Context, c client.Client, domain string) (*dnsv1alpha1.DNSZone, error) { +// resolveZone resolves what a user typed, a domain or an object name, to one +// DNSZone. +func resolveZone(ctx context.Context, c client.Client, domain, vpc string) (*dnsv1alpha1.DNSZone, error) { name := normalizeDomain(domain) if name == "" { return nil, util.UsageErrorf("a zone domain is required"). @@ -48,23 +47,54 @@ func resolveZone(ctx context.Context, c client.Client, domain string) (*dnsv1alp return nil, util.ClassifyError(fmt.Errorf("listing zones: %w", err)) } - if len(list.Items) > 0 { - sort.Slice(list.Items, func(i, j int) bool { return list.Items[i].Name < list.Items[j].Name }) - return &list.Items[0], nil - } - var byName dnsv1alpha1.DNSZone err := c.Get(ctx, types.NamespacedName{Namespace: util.ResourceNamespace, Name: name}, &byName) switch { case err == nil: - return &byName, nil - case apierrors.IsNotFound(err): + if !containsZone(list.Items, byName.Name) { + list.Items = append(list.Items, byName) + } + case !apierrors.IsNotFound(err): + return nil, util.ClassifyError(fmt.Errorf("getting zone: %w", err)) + } + + if len(list.Items) == 0 { return nil, util.NewCLIError(util.ExitNotFound, fmt.Sprintf("zone %q not found", name)). WithFix("run `datumctl dns zone list` to see the zones in this project."). WithCause(err) - default: - return nil, util.ClassifyError(fmt.Errorf("getting zone: %w", err)) } + zone, err := util.PickZone(ctx, c, list.Items, name, vpc) + if err != nil { + return nil, err + } + sharing := 0 + for i := range list.Items { + if list.Items[i].Spec.DomainName == zone.Spec.DomainName { + sharing++ + } + } + if zone.Spec.DomainName != name { + var same dnsv1alpha1.DNSZoneList + if err := c.List(ctx, &same, + client.InNamespace(util.ResourceNamespace), + client.MatchingFields{fieldDomainName: zone.Spec.DomainName}, + ); err == nil { + sharing = len(same.Items) + } + } + if sharing > 1 { + util.MarkSharedDomain(zone) + } + return zone, nil +} + +func containsZone(zones []dnsv1alpha1.DNSZone, name string) bool { + for i := range zones { + if zones[i].Name == name { + return true + } + } + return false } // normalizeDomain folds a domain to the spelling spec.domainName uses: lowercase diff --git a/internal/cmd/dns/record/create.go b/internal/cmd/dns/record/create.go index 4cedf622..7cbd7482 100644 --- a/internal/cmd/dns/record/create.go +++ b/internal/cmd/dns/record/create.go @@ -131,7 +131,7 @@ func runMutate(cmd *cobra.Command, args []string, opts *mutateOptions) error { if err != nil { return err } - zone, err := resolveZone(ctx, c, args[0]) + zone, err := resolveZone(ctx, c, args[0], util.VPCFromCmd(cmd)) if err != nil { return err } diff --git a/internal/cmd/dns/record/delete.go b/internal/cmd/dns/record/delete.go index 179ff98b..eab31b79 100644 --- a/internal/cmd/dns/record/delete.go +++ b/internal/cmd/dns/record/delete.go @@ -84,7 +84,7 @@ func runDelete(cmd *cobra.Command, args []string, opts *deleteOptions) error { if err != nil { return err } - zone, err := resolveZone(ctx, c, args[0]) + zone, err := resolveZone(ctx, c, args[0], util.VPCFromCmd(cmd)) if err != nil { return err } @@ -103,7 +103,7 @@ func runDelete(cmd *cobra.Command, args []string, opts *deleteOptions) error { return err } if set == nil { - return notFoundError(t, ownerName, zoneDomain) + return notFoundError(t, ownerName, zoneDomain, util.ZoneArg(zone)) } // What is being deleted is established before the managed-record guard, so // a name or value that is simply not there says so rather than being told @@ -114,9 +114,9 @@ func runDelete(cmd *cobra.Command, args []string, opts *deleteOptions) error { return util.NewCLIError(util.ExitNotFound, fmt.Sprintf("%s has no %s value %q", ownerDisplay(ownerName, zoneDomain), t, rdata.Render(t, *target))). WithFix(fmt.Sprintf("list what is there: datumctl dns record list %s --name %s --type %s", - zoneDomain, displayName(ownerName), t)) + util.ZoneArg(zone), displayName(ownerName), t)) } - return notFoundError(t, ownerName, zoneDomain) + return notFoundError(t, ownerName, zoneDomain, util.ZoneArg(zone)) } if err := guardMutation(errOut, set, zone, t, ownerName, opts.force); err != nil { @@ -198,8 +198,8 @@ func deletePrompt(doomed []dnsv1alpha1.RecordEntry, t dnsv1alpha1.RRType, ownerN return fmt.Sprintf("Delete all %d %s records for %s?", len(doomed), t, who) } -func notFoundError(t dnsv1alpha1.RRType, ownerName, zoneDomain string) error { +func notFoundError(t dnsv1alpha1.RRType, ownerName, zoneDomain, zoneArg string) error { return util.NewCLIError(util.ExitNotFound, fmt.Sprintf("no %s records for %s", t, ownerDisplay(ownerName, zoneDomain))). - WithFix(fmt.Sprintf("list what is there: datumctl dns record list %s", zoneDomain)) + WithFix(fmt.Sprintf("list what is there: datumctl dns record list %s", zoneArg)) } diff --git a/internal/cmd/dns/record/describe.go b/internal/cmd/dns/record/describe.go index b6b82311..1457cc4c 100644 --- a/internal/cmd/dns/record/describe.go +++ b/internal/cmd/dns/record/describe.go @@ -70,7 +70,7 @@ func runDescribe(cmd *cobra.Command, args []string) error { if err != nil { return err } - zone, err := resolveZone(ctx, c, args[0]) + zone, err := resolveZone(ctx, c, args[0], util.VPCFromCmd(cmd)) if err != nil { return err } @@ -96,7 +96,7 @@ func runDescribe(cmd *cobra.Command, args []string) error { } } if len(owning) == 0 { - return describeNotFound(ownerName, zoneDomain, rrTypes) + return describeNotFound(ownerName, zoneDomain, util.ZoneArg(zone), rrTypes) } switch format { @@ -190,7 +190,7 @@ func managedBy(p provenance, rs *dnsv1alpha1.DNSRecordSet) string { } func printNextSteps(out io.Writer, zone *dnsv1alpha1.DNSZone, ownerName string, owning []dnsv1alpha1.DNSRecordSet) { - domain := zone.Spec.DomainName + domain := util.ZoneArg(zone) name := displayName(ownerName) t := string(owning[0].Spec.RecordType) @@ -201,11 +201,11 @@ func printNextSteps(out io.Writer, zone *dnsv1alpha1.DNSZone, ownerName string, _, _ = fmt.Fprintf(out, " See the whole zone: datumctl dns record list %s\n", domain) } -func describeNotFound(ownerName, zoneDomain string, rrTypes []dnsv1alpha1.RRType) error { +func describeNotFound(ownerName, zoneDomain, zoneArg string, rrTypes []dnsv1alpha1.RRType) error { if len(rrTypes) == 1 { - return notFoundError(rrTypes[0], ownerName, zoneDomain) + return notFoundError(rrTypes[0], ownerName, zoneDomain, zoneArg) } return util.NewCLIError(util.ExitNotFound, fmt.Sprintf("no records for %s", ownerDisplay(ownerName, zoneDomain))). - WithFix(fmt.Sprintf("list what is there: datumctl dns record list %s", zoneDomain)) + WithFix(fmt.Sprintf("list what is there: datumctl dns record list %s", zoneArg)) } diff --git a/internal/cmd/dns/record/list.go b/internal/cmd/dns/record/list.go index d8c716b7..f856032c 100644 --- a/internal/cmd/dns/record/list.go +++ b/internal/cmd/dns/record/list.go @@ -144,7 +144,7 @@ func runList(cmd *cobra.Command, domain string, opts *listOptions) error { if err != nil { return err } - zone, err := resolveZone(ctx, c, domain) + zone, err := resolveZone(ctx, c, domain, util.VPCFromCmd(cmd)) if err != nil { return err } @@ -202,7 +202,7 @@ func runList(cmd *cobra.Command, domain string, opts *listOptions) error { } if len(rows) == 0 { - printEmpty(out, zone.Spec.DomainName, opts, rrTypes, beforeRowFilters > 0) + printEmpty(out, zone.Spec.DomainName, util.ZoneArg(zone), opts, rrTypes, beforeRowFilters > 0) return nil } @@ -468,7 +468,7 @@ func printNames(out io.Writer, rows []row) { // different situation from a zone with nothing to show and wants the opposite // advice: offering to populate a zone that already holds a thousand records // reads as though they have been lost. -func printEmpty(out io.Writer, domain string, opts *listOptions, rrTypes []dnsv1alpha1.RRType, excludedByFilters bool) { +func printEmpty(out io.Writer, domain, zoneArg string, opts *listOptions, rrTypes []dnsv1alpha1.RRType, excludedByFilters bool) { // The headline reports what was asked; the advice below reports what to do // about it, and the two turn on different questions. if listIsFiltered(opts) { @@ -479,8 +479,8 @@ func printEmpty(out io.Writer, domain string, opts *listOptions, rrTypes []dnsv1 if excludedByFilters { _, _ = fmt.Fprintf(out, "\nNext steps:\n") - _, _ = fmt.Fprintf(out, " Show every record: datumctl dns record list %s\n", domain) - _, _ = fmt.Fprintf(out, " Widen the filter: datumctl dns record list %s --help\n", domain) + _, _ = fmt.Fprintf(out, " Show every record: datumctl dns record list %s\n", zoneArg) + _, _ = fmt.Fprintf(out, " Widen the filter: datumctl dns record list %s --help\n", zoneArg) return } @@ -491,8 +491,8 @@ func printEmpty(out io.Writer, domain string, opts *listOptions, rrTypes []dnsv1 } } _, _ = fmt.Fprintf(out, "\nGet started:\n") - _, _ = fmt.Fprintf(out, " Add a record: datumctl dns record create %s %s\n", domain, example) - _, _ = fmt.Fprintf(out, " Import a zone: datumctl dns zone import %s --file zone.txt\n", domain) + _, _ = fmt.Fprintf(out, " Add a record: datumctl dns record create %s %s\n", zoneArg, example) + _, _ = fmt.Fprintf(out, " Import a zone: datumctl dns zone import %s --file zone.txt\n", zoneArg) } // warnUnfilterable says so when a row filter was given alongside raw-object diff --git a/internal/cmd/dns/record/record.go b/internal/cmd/dns/record/record.go index 5f34bc64..0c8e09de 100644 --- a/internal/cmd/dns/record/record.go +++ b/internal/cmd/dns/record/record.go @@ -12,6 +12,8 @@ package record import ( "github.com/spf13/cobra" + + "go.miloapis.com/dns-operator/internal/cmd/dns/util" ) // Command builds the `dns record` noun and its verbs. @@ -50,6 +52,8 @@ so a value pasted out of a provider export needs no translation.`, }, } + util.AddVPCFlag(cmd.PersistentFlags()) + cmd.AddCommand( listCommand(), createCommand(), diff --git a/internal/cmd/dns/record/vpc_test.go b/internal/cmd/dns/record/vpc_test.go new file mode 100644 index 00000000..602f7b1c --- /dev/null +++ b/internal/cmd/dns/record/vpc_test.go @@ -0,0 +1,247 @@ +// SPDX-License-Identifier: AGPL-3.0-only + +package record + +import ( + "context" + "strings" + "testing" + + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/types" + "sigs.k8s.io/controller-runtime/pkg/client" + + networkingv1alpha "go.datum.net/network-services-operator/api/v1alpha" + + dnsv1alpha1 "go.miloapis.com/dns-operator/api/v1alpha1" + "go.miloapis.com/dns-operator/internal/cmd/dns/util" +) + +func privateZone(name, domain string) *dnsv1alpha1.DNSZone { + return &dnsv1alpha1.DNSZone{ + ObjectMeta: metav1.ObjectMeta{Name: name, Namespace: util.ResourceNamespace}, + Spec: dnsv1alpha1.DNSZoneSpec{ + DomainName: domain, + DNSZoneClassName: "private", + Visibility: dnsv1alpha1.DNSZoneVisibilityPrivate, + }, + } +} + +func vpc(name string) *networkingv1alpha.Network { + return &networkingv1alpha.Network{ObjectMeta: metav1.ObjectMeta{ + Name: name, Namespace: util.ResourceNamespace, UID: types.UID("uid-" + name), + }} +} + +func vpcContext(name, vpcName, managedZone string) *dnsv1alpha1.DNSResolverContext { + rc := &dnsv1alpha1.DNSResolverContext{ + ObjectMeta: metav1.ObjectMeta{Name: name, Namespace: util.ResourceNamespace}, + Spec: dnsv1alpha1.DNSResolverContextSpec{ConsumerID: "project-uid/uid-" + vpcName}, + } + rc.Status.ManagedNamespace.DNSZoneRef.Name = managedZone + return rc +} + +func association(name, zone, rc string) *dnsv1alpha1.DNSZoneAssociation { + a := pendingAssociation(name, zone, rc) + a.Status.ResolvedDNSZoneRef.Name = zone + a.Status.ResolvedResolverContextRef.Name = rc + return a +} + +func pendingAssociation(name, zone, rc string) *dnsv1alpha1.DNSZoneAssociation { + return &dnsv1alpha1.DNSZoneAssociation{ + ObjectMeta: metav1.ObjectMeta{Name: name, Namespace: util.ResourceNamespace}, + Spec: dnsv1alpha1.DNSZoneAssociationSpec{ + DNSZoneRef: dnsv1alpha1.DNSNameReference{Name: zone}, + ResolverContextRef: dnsv1alpha1.DNSNameReference{Name: rc}, + }, + } +} + +func twoVPCFixture() []client.Object { + return []client.Object{ + vpc("app-a"), + vpc("app-b"), + privateZone("managed-a", "datum.internal"), + privateZone("managed-b", "datum.internal"), + privateZone("prod-a", "prod.internal"), + privateZone("prod-b", "prod.internal"), + vpcContext("ctx-a", "app-a", "managed-a"), + vpcContext("ctx-b", "app-b", "managed-b"), + association("prod-a", "prod-a", "ctx-a"), + association("prod-b", "prod-b", "ctx-b"), + } +} + +func deleting(z *dnsv1alpha1.DNSZone) *dnsv1alpha1.DNSZone { + now := metav1.Now() + z.DeletionTimestamp = &now + z.Finalizers = []string{"test"} + return z +} + +func requireZones(t *testing.T, h *harness, want ...string) { + t.Helper() + got := recordSetZones(t, h) + if strings.Join(got, ",") != strings.Join(want, ",") { + t.Fatalf("record sets landed in %v, want %v", got, want) + } +} + +func recordSetZones(t *testing.T, h *harness) []string { + t.Helper() + var list dnsv1alpha1.DNSRecordSetList + if err := h.client.List(context.Background(), &list); err != nil { + t.Fatalf("listing record sets: %v", err) + } + zones := make([]string, 0, len(list.Items)) + for i := range list.Items { + zones = append(zones, list.Items[i].Spec.DNSZoneRef.Name) + } + return zones +} + +func TestSharedDomainRequiresVPC(t *testing.T) { + h := newHarness(t, twoVPCFixture()...) + + ce := requireExit(t, h.run("record", "create", "datum.internal", "db", "A", "10.0.0.5"), util.ExitUsage) + for _, want := range []string{"managed-a", "managed-b", "VPC app-a", "VPC app-b", "--vpc"} { + if !strings.Contains(ce.Error()+ce.Fix(), want) { + t.Errorf("error does not mention %q:\n%s\n%s", want, ce.Error(), ce.Fix()) + } + } + requireZones(t, h) +} + +func TestVPCPicksItsOwnZone(t *testing.T) { + h := newHarness(t, twoVPCFixture()...) + + requireNoError(t, h.run("record", "create", "datum.internal", "db", "A", "10.0.0.5", "--vpc", "app-b")) + requireZones(t, h, "managed-b") +} + +func TestVPCFollowsAcceptedAssociations(t *testing.T) { + h := newHarness(t, twoVPCFixture()...) + + requireNoError(t, h.run("record", "create", "prod.internal", "db", "A", "10.0.0.5", "--vpc", "app-b")) + requireZones(t, h, "prod-b") +} + +func TestPendingAssociationDoesNotCount(t *testing.T) { + h := newHarness(t, + vpc("app-a"), + privateZone("managed-a", "datum.internal"), + privateZone("managed-b", "datum.internal"), + vpcContext("ctx-a", "app-a", ""), + pendingAssociation("b-to-a", "managed-b", "ctx-a"), + ) + + _ = requireExit(t, h.run("record", "create", "datum.internal", "db", "A", "10.0.0.5", "--vpc", "app-a"), util.ExitNotFound) + requireZones(t, h) +} + +func TestContextLabelsDoNotDecideTheVPC(t *testing.T) { + ctxB := vpcContext("ctx-b", "app-b", "managed-b") + ctxB.Labels = map[string]string{"networking.datumapis.com/network": "app-a"} + h := newHarness(t, + vpc("app-a"), + vpc("app-b"), + privateZone("managed-a", "datum.internal"), + privateZone("managed-b", "datum.internal"), + ctxB, + ) + + _ = requireExit(t, h.run("record", "create", "datum.internal", "db", "A", "10.0.0.5", "--vpc", "app-a"), util.ExitNotFound) + requireZones(t, h) +} + +func TestUnknownVPC(t *testing.T) { + h := newHarness(t, twoVPCFixture()...) + + ce := requireExit(t, h.run("record", "list", "datum.internal", "--vpc", "nope"), util.ExitNotFound) + if !strings.Contains(ce.Error(), `"nope"`) { + t.Fatalf("error does not name the VPC: %v", ce) + } +} + +func TestObjectNameStillSelectsAZone(t *testing.T) { + h := newHarness(t, twoVPCFixture()...) + + requireNoError(t, h.run("record", "create", "managed-a", "db", "A", "10.0.0.5")) + requireZones(t, h, "managed-a") +} + +func TestObjectNameWithAnotherVPCIsRefused(t *testing.T) { + h := newHarness(t, twoVPCFixture()...) + + _ = requireExit(t, h.run("record", "create", "managed-a", "db", "A", "10.0.0.5", "--vpc", "app-b"), util.ExitNotFound) + requireZones(t, h) +} + +func TestDeletingPrivateZoneStillMakesADomainAmbiguous(t *testing.T) { + objs := twoVPCFixture() + objs[2] = deleting(privateZone("managed-a", "datum.internal")) + h := newHarness(t, objs...) + + ce := requireExit(t, h.run("record", "create", "datum.internal", "db", "A", "10.0.0.5"), util.ExitUsage) + if !strings.Contains(ce.Fix(), "being deleted") { + t.Errorf("listing does not flag the zone being deleted:\n%s", ce.Fix()) + } + requireZones(t, h) + + requireNoError(t, h.run("record", "create", "datum.internal", "db", "A", "10.0.0.5", "--vpc", "app-b")) + requireZones(t, h, "managed-b") +} + +func TestDeletingPublicZoneDoesNotMakeADomainAmbiguous(t *testing.T) { + live := testZone() + gone := deleting(live.DeepCopy()) + gone.Name = "old-" + live.Name + h := newHarness(t, live, gone) + + requireNoError(t, h.run("record", "create", testDomain, "db", "A", "10.0.0.5")) + requireZones(t, h, live.Name) +} + +func TestDomainMatchingAnotherZonesObjectNameIsAmbiguous(t *testing.T) { + h := newHarness(t, + privateZone("corp.example", "datum.internal"), + privateZone("managed-c", "corp.example"), + ) + + _ = requireExit(t, h.run("record", "create", "corp.example", "db", "A", "10.0.0.5"), util.ExitUsage) + requireZones(t, h) +} + +func TestPublicZoneSharingADomainIsNamedByObject(t *testing.T) { + public := testZone() + h := newHarness(t, public, vpc("app-a"), privateZone("managed-a", testDomain), vpcContext("ctx-a", "app-a", "managed-a")) + + _ = requireExit(t, h.run("record", "list", testDomain), util.ExitUsage) + + requireNoError(t, h.run("record", "list", public.Name)) + if strings.Contains(h.stdout(), "record create "+testDomain+" ") || + !strings.Contains(h.stdout(), "record create "+public.Name+" ") { + t.Fatalf("next steps do not name the public zone by object:\n%s", h.stdout()) + } +} + +func TestVPCOnAPublicZone(t *testing.T) { + h := newHarness(t, zoneFixture()...) + + ce := requireExit(t, h.run("record", "list", testDomain, "--vpc", "typo"), util.ExitUsage) + if !strings.Contains(ce.Error(), "public zone") { + t.Fatalf("error does not say the zone is public: %v", ce) + } +} + +func TestHintsNameAPrivateZoneByObject(t *testing.T) { + h := newHarness(t, twoVPCFixture()...) + + requireNoError(t, h.run("record", "list", "datum.internal", "--vpc", "app-b")) + if !strings.Contains(h.stdout(), "datumctl dns record create managed-b ") { + t.Fatalf("next steps do not name the zone by object:\n%s", h.stdout()) + } +} diff --git a/internal/cmd/dns/record/wait.go b/internal/cmd/dns/record/wait.go index bda06377..a8d255c4 100644 --- a/internal/cmd/dns/record/wait.go +++ b/internal/cmd/dns/record/wait.go @@ -64,12 +64,12 @@ func waitForProgrammed( _, _ = fmt.Fprintf(out, " %s %s %s\n", who, t, word) return util.NewCLIError(util.ExitConflict, detail). WithFix(fmt.Sprintf("inspect it with: datumctl dns record describe %s %s %s", - zone.Spec.DomainName, displayName(ownerName), t)) + util.ZoneArg(zone), displayName(ownerName), t)) case util.StatusError, util.StatusRejected: _, _ = fmt.Fprintf(out, " %s %s %s\n", who, t, word) return util.NewCLIError(util.ExitError, detail). WithFix(fmt.Sprintf("inspect it with: datumctl dns record describe %s %s %s", - zone.Spec.DomainName, displayName(ownerName), t)) + util.ZoneArg(zone), displayName(ownerName), t)) } } @@ -89,7 +89,7 @@ func waitForProgrammed( msg = fmt.Sprintf("%s — last status was %s", msg, lastWord) } fix := fmt.Sprintf("the record was written; check on it with: datumctl dns record describe %s %s %s", - zone.Spec.DomainName, displayName(ownerName), t) + util.ZoneArg(zone), displayName(ownerName), t) if lastDetail != "" { fix = lastDetail + "\n " + fix } diff --git a/internal/cmd/dns/util/zonepick.go b/internal/cmd/dns/util/zonepick.go new file mode 100644 index 00000000..4b4cca27 --- /dev/null +++ b/internal/cmd/dns/util/zonepick.go @@ -0,0 +1,267 @@ +// SPDX-License-Identifier: AGPL-3.0-only + +package util + +import ( + "context" + "fmt" + "sort" + "strings" + "sync" + + "github.com/spf13/cobra" + "github.com/spf13/pflag" + apierrors "k8s.io/apimachinery/pkg/api/errors" + "k8s.io/apimachinery/pkg/types" + "sigs.k8s.io/controller-runtime/pkg/client" + + networkingv1alpha "go.datum.net/network-services-operator/api/v1alpha" + + dnsv1alpha1 "go.miloapis.com/dns-operator/api/v1alpha1" +) + +const VPCFlag = "vpc" + +func AddVPCFlag(flags *pflag.FlagSet) { + flags.String(VPCFlag, "", "Use the zone this VPC resolves, when several zones in the project share the domain") +} + +func VPCFromCmd(cmd *cobra.Command) string { + f := cmd.Flags().Lookup(VPCFlag) + if f == nil { + return "" + } + return strings.TrimSpace(f.Value.String()) +} + +var sharedDomainZones sync.Map + +// ZoneArg is how to name a zone on a later command line: by domain only when +// no other zone could answer to the same argument. +func ZoneArg(z *dnsv1alpha1.DNSZone) string { + if _, shared := sharedDomainZones.Load(z); shared || z.Spec.Visibility == dnsv1alpha1.DNSZoneVisibilityPrivate { + return z.Name + } + return z.Spec.DomainName +} + +// ZoneLabel names a zone for prompts and receipts. +func ZoneLabel(z *dnsv1alpha1.DNSZone) string { + if z.Spec.Visibility != dnsv1alpha1.DNSZoneVisibilityPrivate { + return z.Spec.DomainName + } + return fmt.Sprintf("%s (zone %s)", z.Spec.DomainName, z.Name) +} + +// MarkSharedDomain records that other zones in the project carry z's domain, +// so ZoneArg names it by object. +func MarkSharedDomain(z *dnsv1alpha1.DNSZone) { + sharedDomainZones.Store(z, true) +} + +// PickZone chooses among every zone whose domain or object name equals typed. +// Without --vpc it refuses whenever more than one could be meant, including a +// private zone that is being deleted: skipping it could hand the write to +// another VPC's zone. +func PickZone(ctx context.Context, c client.Client, candidates []dnsv1alpha1.DNSZone, typed, vpc string) (*dnsv1alpha1.DNSZone, error) { + if vpc == "" { + if !anyPrivate(candidates) { + candidates = preferLive(candidates) + } + if len(candidates) == 1 { + return &candidates[0], nil + } + return nil, ambiguousZoneError(ctx, c, candidates, typed, "") + } + + if !anyPrivate(candidates) { + return nil, UsageErrorf("%q is a public zone, and --vpc applies only to private zones", typed). + WithFix("drop --vpc.") + } + + used, err := zonesUsedByVPC(ctx, c, vpc) + if err != nil { + return nil, err + } + var matched []dnsv1alpha1.DNSZone + for i := range candidates { + if used[candidates[i].Name] { + matched = append(matched, candidates[i]) + } + } + matched = preferLive(matched) + + switch len(matched) { + case 1: + return &matched[0], nil + case 0: + return nil, NewCLIError(ExitNotFound, + fmt.Sprintf("VPC %q does not use a zone for %q", vpc, typed)). + WithFix("run the command without --vpc to see which VPCs use each zone.") + default: + return nil, ambiguousZoneError(ctx, c, matched, typed, vpc) + } +} + +func anyPrivate(zones []dnsv1alpha1.DNSZone) bool { + for i := range zones { + if zones[i].Spec.Visibility == dnsv1alpha1.DNSZoneVisibilityPrivate { + return true + } + } + return false +} + +func preferLive(zones []dnsv1alpha1.DNSZone) []dnsv1alpha1.DNSZone { + var live []dnsv1alpha1.DNSZone + for i := range zones { + if zones[i].DeletionTimestamp.IsZero() { + live = append(live, zones[i]) + } + } + if len(live) == 0 { + return zones + } + return live +} + +func zonesUsedByVPC(ctx context.Context, c client.Client, vpc string) (map[string]bool, error) { + var network networkingv1alpha.Network + err := c.Get(ctx, types.NamespacedName{Namespace: ResourceNamespace, Name: vpc}, &network) + switch { + case apierrors.IsNotFound(err): + return nil, NewCLIError(ExitNotFound, fmt.Sprintf("no VPC named %q in this project", vpc)). + WithFix("check the VPC name:\n datumctl network list") + case err != nil: + return nil, ClassifyError(fmt.Errorf("getting VPC %q: %w", vpc, err)) + } + + byZone, err := zoneVPCs(ctx, c, map[string]string{string(network.UID): vpc}) + if err != nil { + return nil, ClassifyError(err) + } + used := map[string]bool{} + for zone, vpcs := range byZone { + if contains(vpcs, vpc) { + used[zone] = true + } + } + return used, nil +} + +// contextNetworkUID reads the network UID out of a resolver context's +// immutable consumerID, which network services writes as +// "/". +func contextNetworkUID(rc *dnsv1alpha1.DNSResolverContext) string { + i := strings.LastIndex(rc.Spec.ConsumerID, "/") + if i < 0 { + return "" + } + return rc.Spec.ConsumerID[i+1:] +} + +// zoneVPCs maps zone names to the VPCs that resolve them, counting a VPC's +// managed zone and its accepted associations. vpcByUID maps network UIDs to +// VPC names. +func zoneVPCs(ctx context.Context, c client.Client, vpcByUID map[string]string) (map[string][]string, error) { + out := map[string][]string{} + + var rcs dnsv1alpha1.DNSResolverContextList + if err := c.List(ctx, &rcs, client.InNamespace(ResourceNamespace)); err != nil { + return nil, fmt.Errorf("listing DNS contexts: %w", err) + } + vpcOf := map[string]string{} + for i := range rcs.Items { + rc := &rcs.Items[i] + vpc := vpcByUID[contextNetworkUID(rc)] + if vpc == "" || !rc.DeletionTimestamp.IsZero() { + continue + } + vpcOf[rc.Name] = vpc + if z := rc.Status.ManagedNamespace.DNSZoneRef.Name; z != "" && !contains(out[z], vpc) { + out[z] = append(out[z], vpc) + } + } + + var assocs dnsv1alpha1.DNSZoneAssociationList + if err := c.List(ctx, &assocs, client.InNamespace(ResourceNamespace)); err != nil { + return nil, fmt.Errorf("listing zone associations: %w", err) + } + for i := range assocs.Items { + a := &assocs.Items[i] + vpc := vpcOf[a.Spec.ResolverContextRef.Name] + if vpc == "" || !a.DeletionTimestamp.IsZero() || + a.Status.ResolvedDNSZoneRef.Name != a.Spec.DNSZoneRef.Name || + a.Status.ResolvedResolverContextRef.Name != a.Spec.ResolverContextRef.Name { + continue + } + if z := a.Spec.DNSZoneRef.Name; !contains(out[z], vpc) { + out[z] = append(out[z], vpc) + } + } + return out, nil +} + +func allVPCs(ctx context.Context, c client.Client) (map[string]string, error) { + var networks networkingv1alpha.NetworkList + if err := c.List(ctx, &networks, client.InNamespace(ResourceNamespace)); err != nil { + return nil, err + } + out := map[string]string{} + for i := range networks.Items { + out[string(networks.Items[i].UID)] = networks.Items[i].Name + } + return out, nil +} + +func ambiguousZoneError(ctx context.Context, c client.Client, zones []dnsv1alpha1.DNSZone, typed, vpc string) *CLIError { + var vpcs map[string][]string + known := false + if byUID, err := allVPCs(ctx, c); err == nil { + if m, err := zoneVPCs(ctx, c, byUID); err == nil { + vpcs, known = m, true + } + } + + sorted := append([]dnsv1alpha1.DNSZone(nil), zones...) + sort.Slice(sorted, func(i, j int) bool { return sorted[i].Name < sorted[j].Name }) + + var b strings.Builder + for i := range sorted { + z := &sorted[i] + names := append([]string(nil), vpcs[z.Name]...) + sort.Strings(names) + var where string + switch { + case z.Spec.Visibility != dnsv1alpha1.DNSZoneVisibilityPrivate: + where = "public" + case len(names) > 0: + where = "VPC " + strings.Join(names, ", ") + case known: + where = "used by no VPC" + default: + where = "VPC unknown" + } + if !z.DeletionTimestamp.IsZero() { + where += ", being deleted" + } + fmt.Fprintf(&b, "\n %s %s (%s)", z.Name, z.Spec.DomainName, where) + } + + msg := fmt.Sprintf("%d zones in this project match %q", len(sorted), typed) + fix := "pass --vpc to choose the VPC's zone, or the zone's object name instead of the domain:" + b.String() + if vpc != "" { + msg = fmt.Sprintf("VPC %q uses %d zones for %q", vpc, len(sorted), typed) + fix = "pass the zone's object name instead of the domain:" + b.String() + } + return NewCLIError(ExitUsage, msg).WithFix(fix) +} + +func contains(list []string, s string) bool { + for _, v := range list { + if v == s { + return true + } + } + return false +} diff --git a/internal/cmd/dns/zone/create.go b/internal/cmd/dns/zone/create.go index a6b75c49..826df218 100644 --- a/internal/cmd/dns/zone/create.go +++ b/internal/cmd/dns/zone/create.go @@ -165,7 +165,8 @@ func checkDomainFree(ctx context.Context, c client.Client, project, domain strin return util.ClassifyError(fmt.Errorf("listing zones: %w", err)) } for i := range list.Items { - if normalizeDomain(list.Items[i].Spec.DomainName) != domain { + if normalizeDomain(list.Items[i].Spec.DomainName) != domain || + list.Items[i].Spec.Visibility == dnsv1alpha1.DNSZoneVisibilityPrivate { continue } return util.NewCLIError(util.ExitConflict, diff --git a/internal/cmd/dns/zone/delete.go b/internal/cmd/dns/zone/delete.go index ab2cdbfb..12798aaf 100644 --- a/internal/cmd/dns/zone/delete.go +++ b/internal/cmd/dns/zone/delete.go @@ -9,6 +9,7 @@ import ( "github.com/spf13/cobra" "sigs.k8s.io/controller-runtime/pkg/client" + dnsv1alpha1 "go.miloapis.com/dns-operator/api/v1alpha1" "go.miloapis.com/dns-operator/internal/cmd/dns/util" ) @@ -39,6 +40,7 @@ to run non-interactively without --yes.`, cmd.Flags().Bool("dry-run", false, "Submit the deletion for server-side validation without performing it") + util.AddVPCFlag(cmd.Flags()) return cmd } @@ -55,7 +57,7 @@ func runDelete(cmd *cobra.Command, args []string) error { return err } - z, err := getZone(ctx, c, project, args[0]) + z, err := getZone(ctx, c, project, args[0], util.VPCFromCmd(cmd)) if err != nil { return util.ClassifyError(err) } @@ -80,7 +82,12 @@ func runDelete(cmd *cobra.Command, args []string) error { if !dryRun && !assumeYes { prompt := cascadeWarning(domain, records, countErr) + objectNameNote(args[0], domain, z.Name) - confirmed, err := util.ConfirmTyped(cmd.InOrStdin(), cmd.ErrOrStderr(), prompt, domain) + confirmAs := domain + if z.Spec.Visibility == dnsv1alpha1.DNSZoneVisibilityPrivate { + prompt = cascadeWarning(util.ZoneLabel(z), records, countErr) + confirmAs = z.Name + } + confirmed, err := util.ConfirmTyped(cmd.InOrStdin(), cmd.ErrOrStderr(), prompt, confirmAs) if err != nil { return err } diff --git a/internal/cmd/dns/zone/describe.go b/internal/cmd/dns/zone/describe.go index 49c2fb77..60dbd1a6 100644 --- a/internal/cmd/dns/zone/describe.go +++ b/internal/cmd/dns/zone/describe.go @@ -37,6 +37,7 @@ the assigned nameservers, and what it contains.`, cmd.Flags().StringP("output", "o", "wide", "Output format: wide, json, yaml, name") _ = cmd.RegisterFlagCompletionFunc("output", util.CompleteEnum("wide", "json", "yaml", "name")) + util.AddVPCFlag(cmd.Flags()) return cmd } @@ -56,7 +57,7 @@ func runDescribe(cmd *cobra.Command, args []string) error { return err } - z, err := getZone(ctx, c, project, args[0]) + z, err := getZone(ctx, c, project, args[0], util.VPCFromCmd(cmd)) if err != nil { return util.ClassifyError(err) } @@ -124,7 +125,7 @@ func printZoneDetail(out io.Writer, z *dnsv1alpha1.DNSZone, project string, sets printDelegationInstructions(out, domain, d) } - printZoneNextSteps(out, domain, word) + printZoneNextSteps(out, util.ZoneArg(z), word) } // printZoneNextSteps offers what is worth doing next, which depends on whether diff --git a/internal/cmd/dns/zone/export.go b/internal/cmd/dns/zone/export.go index 739a557a..6b9b7513 100644 --- a/internal/cmd/dns/zone/export.go +++ b/internal/cmd/dns/zone/export.go @@ -44,6 +44,7 @@ func exportCommand() *cobra.Command { } cmd.Flags().StringVarP(&file, "file", "f", "", "Write to this path instead of standard output") + util.AddVPCFlag(cmd.Flags()) return cmd } @@ -55,7 +56,7 @@ func runExport(cmd *cobra.Command, domain, file string) error { return err } - zone, err := getZone(ctx, c, util.ProjectFromCmd(cmd), domain) + zone, err := getZone(ctx, c, util.ProjectFromCmd(cmd), domain, util.VPCFromCmd(cmd)) if err != nil { return err } diff --git a/internal/cmd/dns/zone/import.go b/internal/cmd/dns/zone/import.go index 8cdd63f6..630c0a44 100644 --- a/internal/cmd/dns/zone/import.go +++ b/internal/cmd/dns/zone/import.go @@ -103,6 +103,7 @@ func importCommand() *cobra.Command { "Validate against the API server without writing anything") cmd.Flags().DurationVar(&timeout, "timeout", 2*time.Minute, "How long to wait for --discover to finish") + util.AddVPCFlag(cmd.Flags()) return cmd } @@ -149,7 +150,7 @@ func runImport(cmd *cobra.Command, domain string, opts importOptions) error { return err } - zone, err := getZone(ctx, c, util.ProjectFromCmd(cmd), domain) + zone, err := getZone(ctx, c, util.ProjectFromCmd(cmd), domain, util.VPCFromCmd(cmd)) if err != nil { return err } @@ -183,7 +184,7 @@ func runImport(cmd *cobra.Command, domain string, opts importOptions) error { if !util.AssumeYes(cmd) { prompt := fmt.Sprintf( "--replace will discard the existing records of every type in the input for %s. Continue?", - zone.Spec.DomainName) + util.ZoneLabel(zone)) ok, cerr := util.ConfirmYesNo(cmd.InOrStdin(), errOut, prompt, false) if cerr != nil { return cerr diff --git a/internal/cmd/dns/zone/nameservers.go b/internal/cmd/dns/zone/nameservers.go index dce665ad..6ed105ff 100644 --- a/internal/cmd/dns/zone/nameservers.go +++ b/internal/cmd/dns/zone/nameservers.go @@ -31,6 +31,7 @@ actually working" rather than "is the control plane happy".`, cmd.Flags().Bool("check", false, "Resolve the zone live against its assigned nameservers") cmd.Flags().Duration("timeout", defaultCheckTimeout, "Per-query timeout for --check") + util.AddVPCFlag(cmd.Flags()) return cmd } @@ -46,7 +47,7 @@ func runNameservers(cmd *cobra.Command, args []string) error { return err } - z, err := getZone(ctx, c, project, args[0]) + z, err := getZone(ctx, c, project, args[0], util.VPCFromCmd(cmd)) if err != nil { return util.ClassifyError(err) } diff --git a/internal/cmd/dns/zone/zone.go b/internal/cmd/dns/zone/zone.go index 15e8a83c..aa5907f2 100644 --- a/internal/cmd/dns/zone/zone.go +++ b/internal/cmd/dns/zone/zone.go @@ -144,11 +144,9 @@ func validateDomain(domain, raw string) error { return nil } -// getZone resolves the domain a user typed to the DNSZone object that carries -// it. The object name is generated at creation and nobody memorises it, so the -// lookup is by spec.domainName with an object-name fallback for the user who -// pasted one out of `kubectl get`. -func getZone(ctx context.Context, c client.Client, project, domain string) (*dnsv1alpha1.DNSZone, error) { +// getZone resolves what a user typed, a domain or an object name, to one +// DNSZone. +func getZone(ctx context.Context, c client.Client, project, domain, vpc string) (*dnsv1alpha1.DNSZone, error) { want := normalizeDomain(domain) var list dnsv1alpha1.DNSZoneList @@ -156,15 +154,27 @@ func getZone(ctx context.Context, c client.Client, project, domain string) (*dns return nil, fmt.Errorf("listing zones: %w", err) } + var matched []dnsv1alpha1.DNSZone for i := range list.Items { - if normalizeDomain(list.Items[i].Spec.DomainName) == want { - return &list.Items[i], nil + if normalizeDomain(list.Items[i].Spec.DomainName) == want || list.Items[i].Name == domain { + matched = append(matched, list.Items[i]) } } - for i := range list.Items { - if list.Items[i].Name == domain { - return &list.Items[i], nil + if len(matched) > 0 { + z, err := util.PickZone(ctx, c, matched, want, vpc) + if err != nil { + return nil, err + } + sharing := 0 + for i := range list.Items { + if list.Items[i].Spec.DomainName == z.Spec.DomainName { + sharing++ + } + } + if sharing > 1 { + util.MarkSharedDomain(z) } + return z, nil } return nil, util.NewCLIError(util.ExitNotFound, diff --git a/internal/cmd/dns/zone/zone_test.go b/internal/cmd/dns/zone/zone_test.go index 39260940..b9800707 100644 --- a/internal/cmd/dns/zone/zone_test.go +++ b/internal/cmd/dns/zone/zone_test.go @@ -12,6 +12,7 @@ import ( "github.com/spf13/cobra" corev1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/types" "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/client/fake" "sigs.k8s.io/controller-runtime/pkg/client/interceptor" @@ -318,7 +319,7 @@ func TestValidateDomain(t *testing.T) { func TestGetZoneNotFound(t *testing.T) { c := newFakeClient(t, newZone("example-com-abc123", "example.com")) - _, err := getZone(t.Context(), c, testProject, "missing.com") + _, err := getZone(t.Context(), c, testProject, "missing.com", "") if err == nil { t.Fatal("expected an error for a zone that does not exist") } @@ -336,7 +337,7 @@ func TestGetZoneNotFound(t *testing.T) { func TestGetZoneMatchesCaseInsensitively(t *testing.T) { c := newFakeClient(t, newZone("example-com-abc123", "example.com")) - z, err := getZone(t.Context(), c, testProject, "Example.COM.") + z, err := getZone(t.Context(), c, testProject, "Example.COM.", "") if err != nil { t.Fatalf("getZone: %v", err) } @@ -372,3 +373,72 @@ func equalStrings(a, b []string) bool { } return true } + +func private(z *dnsv1alpha1.DNSZone) { + z.Spec.Visibility = dnsv1alpha1.DNSZoneVisibilityPrivate +} + +func twoVPCZones() []client.Object { + ctxB := &dnsv1alpha1.DNSResolverContext{ + ObjectMeta: metav1.ObjectMeta{Name: "ctx-b", Namespace: util.ResourceNamespace}, + Spec: dnsv1alpha1.DNSResolverContextSpec{ConsumerID: "project-uid/uid-app-b"}, + } + ctxB.Status.ManagedNamespace.DNSZoneRef.Name = "managed-b" + return []client.Object{ + &networkingv1alpha.Network{ObjectMeta: metav1.ObjectMeta{ + Name: "app-b", Namespace: util.ResourceNamespace, UID: types.UID("uid-app-b"), + }}, + newZone("managed-a", "datum.internal", private), + newZone("managed-b", "datum.internal", private), + ctxB, + } +} + +func TestGetZoneRefusesSharedDomainWithoutVPC(t *testing.T) { + c := newFakeClient(t, twoVPCZones()...) + + _, err := getZone(t.Context(), c, testProject, "datum.internal", "") + assertExitCode(t, err, util.ExitUsage) + + z, err := getZone(t.Context(), c, testProject, "datum.internal", "app-b") + if err != nil { + t.Fatalf("getZone --vpc app-b: %v", err) + } + if z.Name != "managed-b" { + t.Errorf("zone = %q, want managed-b", z.Name) + } +} + +func TestDeletePrivateZoneConfirmsByObjectName(t *testing.T) { + interactive(t) + c := newFakeClient(t, twoVPCZones()...) + h := newHarness(t, c) + h.in.WriteString("managed-b\n") + + if err := h.run("zone", "delete", "datum.internal", "--vpc", "app-b"); err != nil { + t.Fatalf("zone delete: %v", err) + } + prompt := h.err.String() + if !strings.Contains(prompt, "datum.internal (zone managed-b)") || !strings.Contains(prompt, `Type "managed-b" to confirm:`) { + t.Errorf("prompt does not name the zone object:\n%s", prompt) + } + if err := c.Get(t.Context(), client.ObjectKey{Namespace: util.ResourceNamespace, Name: "managed-a"}, &dnsv1alpha1.DNSZone{}); err != nil { + t.Errorf("the other VPC's zone is gone: %v", err) + } + if err := c.Get(t.Context(), client.ObjectKey{Namespace: util.ResourceNamespace, Name: "managed-b"}, &dnsv1alpha1.DNSZone{}); err == nil { + t.Error("managed-b still exists after a confirmed delete") + } +} + +func TestZoneLookupCommandsTakeVPC(t *testing.T) { + root := Command() + for _, name := range []string{"describe", "delete", "nameservers", "import", "export"} { + sub, _, err := root.Find([]string{name}) + if err != nil || sub.Name() != name { + t.Fatalf("finding %s: %v", name, err) + } + if sub.Flags().Lookup(util.VPCFlag) == nil { + t.Errorf("zone %s has no --%s flag", name, util.VPCFlag) + } + } +}