diff --git a/docs/enhancements/datumctl-compute-urls.md b/docs/enhancements/datumctl-compute-urls.md new file mode 100644 index 00000000..687884d7 --- /dev/null +++ b/docs/enhancements/datumctl-compute-urls.md @@ -0,0 +1,177 @@ +# `datumctl compute` — Workload URLs + +**Status:** Draft +**Companion:** [`datumctl-compute-dx.md`](./datumctl-compute-dx.md) (the DX arc this extends) + +--- + +## Summary + +`datumctl compute deploy --port=8080` ran a container across multiple cities and gave the developer nothing to point a browser at. This closes that gap with the smallest surface that does it: **declaring an HTTP port publishes the workload on a Datum-managed HTTPS URL.** + +One breaking change — `--port` becomes `--http-port` — and no new commands. + +--- + +## Scope: this plugin publishes a URL, it does not configure a proxy + +Advanced proxy configuration belongs to dedicated ALB tooling: custom hostnames and their DNS verification, path and header routing, multiple backends, certificates, header rewriting, timeouts. This plugin deliberately owns none of it. + +What it owns is the zero-config path, because that is the part a *compute* user needs and the part that was missing: run a container, get a working URL. Anything beyond that is a proxy configuration question, and answering it here would mean this plugin growing a second product inside it. + +That boundary is why there is no `compute domains` command group and no `compute open`. It also sets one hard requirement in the other direction: **because hostnames are configured out of band, publishing must never clobber them.** `deploy` reads the custom hostnames already on the proxy and carries them forward on every redeploy, and fails closed if it cannot read them — a redeploy that silently detached someone's production hostname would be far worse than a redeploy that stops and says why. + +--- + +## Product principles + +**1. Declaring an HTTP port is declaring a web service.** One flag, one meaning. `--port` said *what is listening*, not *who can reach it*; in Kubernetes, which this platform is, `containerPort` exposes nothing at all. Every comparable platform makes the declared role decide exposure instead — Heroku routes `web:`, Render makes you pick Web Service or Private Service, Fly's port field lives inside `[http_service]`. `--http-port` carries that contract in the name, so no second confirmation flag is needed. + +**2. The URL is free, so it is automatic.** A managed URL costs the developer nothing, which is what licenses issuing one without asking. + +**3. The URL is the deliverable.** Last line of output, on its own, copy-pasteable. + +**4. Never name the machinery.** The developer sees "URL", "backends", "certificate" — never `NetworkService`, `HTTPProxy`, or a raw condition reason. Blocking text is routed through `url.HumanBlock`, which shows the server's message and never its reason. + +**5. Never invent resources that don't exist.** No `endpoint/api created` for a kind nobody can `datumctl get`. + +--- + +## The experience + +### Deploy and get a URL + +``` +$ datumctl compute deploy api --image=ghcr.io/acme/api:1.4.2 --location=us-east-1,eu-west-1 --min=2 --http-port=8080 + +Resolving workload "api" in project acme-prod... + Placement "default": locations=[us-east-1, eu-west-1], min=2 + HTTP service: port 8080 → Datum-managed URL + +Apply? (Y/n): y + workload/api created + +Waiting for rollout. Ctrl-C to detach (rollout continues in background). + + PLACEMENT LOCATION UPDATED READY OLD PHASE + default us-east-1 2 2 0 Done + default eu-west-1 2 2 0 Done + +Rollout complete in 47s. + +Publishing... + Backends 4 healthy across us-east-1, eu-west-1 + Edge programmed + Certificate issued + + https://a1b2c3d4.datumproxy.net +``` + +The URL objects are written alongside the workload, not after the rollout, so backends register as instances come up and the URL answers moments after the last city reaches `Done`. + +A workload with no `--http-port` deploys as it always did, and says so rather than leaving the developer guessing: + +``` +Rollout complete in 47s. + + No HTTP port declared — this workload is not reachable from the internet. + To publish it: datumctl compute deploy api --http-port 8080 +``` + +### Find the URL again + +There is no `status` command in this CLI (the DX doc proposes one that was never built), so the URL surfaces where a developer already looks. In the list: + +``` +$ datumctl compute workloads + + NAME LOCATIONS READY IMAGE URL + api us-east-1, eu-west-1 4/4 ghcr.io/acme/api:1.4.2 https://api.example.com + worker us-east-1 1/1 ghcr.io/acme/worker:2.0 — +``` + +As a field, so `| jq -r .url` works: `datumctl compute workloads -o json`. + +And in `describe`, with per-location backend health — the view that makes multi-location serving visible, which it previously was not: + +``` +$ datumctl compute workloads describe api + +Workload api project: acme-prod +Type sandbox/datumcloud/d1-standard-2 +Updated 4m ago + +Health Available + +URL https://api.example.com +Backend port 8080/tcp + +Serving Degraded — 2 of 4 backends healthy + + LOCATION BACKENDS HEALTHY SERVING + us-east-1 2 2 yes + eu-west-1 2 0 no + + eu-west-1: no healthy backends — instances are running but not passing health checks. + Traffic is being served from us-east-1 only. + + Next steps: + Check instances: datumctl compute instances --workload=api --location=eu-west-1 +``` + +### Stop serving + +`--no-http` removes the HTTP service and the URL with it, naming what will stop answering before the prompt. `destroy` does the same as part of its summary: + +``` +$ datumctl compute destroy api + +Workload: api +Placements: 1 Locations: us-east-1, eu-west-1 +Min replicas: 2 +URLs: https://api.example.com, https://a1b2c3d4.datumproxy.net + +This will delete the workload, all its instances, and its URLs. Continue? (y/N): +``` + +The URL resources are deleted explicitly rather than by owner-reference GC, which is unverified in project virtual control planes. Domain objects are left alone — a verified domain is a project asset that outlives any one workload. + +--- + +## Command surface + +Changed, and nothing added: + +``` +datumctl compute deploy --port → --http-port (breaking); --no-http removes it +datumctl compute workloads URL column, and a `url` field in -o json/yaml +datumctl compute workloads describe URL, backend port, and per-location backend health +datumctl compute destroy lists URLs in its summary and deletes them +``` + +### On breaking `--port` + +`--port` is removed, not silently aliased. Aliasing would publish every existing workload on the next plugin upgrade — precisely the surprise this design exists to avoid. For one release it stays registered and hard-errors: + +``` +Error: --port has been replaced by --http-port, which publishes the workload on a public +HTTPS URL. Use --http-port 8080 to publish, or --no-http to keep it internal +``` + +Redeploys are idempotent: an unchanged redeploy writes nothing, omitting `--http-port` inherits the port the workload already declares, and the managed hostname is never reissued — anything already pointing at that URL keeps working. + +--- + +## What this deliberately does not do + +- **No proxy configuration.** Custom hostnames, path and header routing, multiple backends, certificates, rewrites, timeouts. All ALB tooling's job; see the scope section. +- **No TCP or UDP exposure.** HTTP/HTTPS only, matching the platform's proxy. +- **No plaintext HTTP.** Always `https://`. No `--insecure`. +- **No backend TLS.** The edge reaches instances over plaintext inside the network, and the platform rejects backend TLS for this backend form. A container terminating TLS itself will not work, and the CLI says so on the publishing path. +- **One workload, one URL.** + +--- + +## Open question + +**What should the managed hostname look like?** `.datumproxy.net` is stable and collision-free but unmemorable and awkward to share. Heroku moved off `.herokuapp.com` for squatting reasons; Fly uses `.fly.dev` and accepts the collision namespace. This is a platform decision the CLI inherits, but it shapes the first-run experience more than anything else here. diff --git a/docs/scoping/alb-workload-exposure.md b/docs/scoping/alb-workload-exposure.md new file mode 100644 index 00000000..55406e9a --- /dev/null +++ b/docs/scoping/alb-workload-exposure.md @@ -0,0 +1,199 @@ +# Scoping: Auto-creating an ALB / HTTP proxy to expose a Workload + +## 1. The NetworkService API (PR 411) + +**Status: `datum-cloud/network-services-operator#411` is OPEN and a DRAFT.** Branch `proto/network-service` → `main`. The body says verbatim: *"Draft: this is a working prototype to prove the design, not a merge candidate."* Design doc is `datum-cloud/enhancements#870`. Everything below can change. + +**GVK:** `networking.datumapis.com/v1alpha`, `Kind: NetworkService`, **namespaced**. Defined in `api/v1alpha/networkservice_types.go` (new, 305 lines); CRD at `config/crd/bases/networking.datumapis.com_networkservices.yaml`; registered as an IAM `ProtectedResource` parented to `resourcemanager.miloapis.com/Project` in `config/iam/protected-resources/networkservices.yaml`, so it is a **user-facing, project-scoped** resource. + +```yaml +apiVersion: networking.datumapis.com/v1alpha +kind: NetworkService +metadata: {name: storefront, namespace: default} +spec: + networkInterfaces: # REQUIRED + selector: # REQUIRED metav1.LabelSelector, CEL-validated non-empty + matchLabels: {compute.datumapis.com/workload-name: storefront} + ports: # REQUIRED, 1..16, unique name + unique number (CEL) + - name: http # DNS label, <=63, ^[a-z0-9]([-a-z0-9]*[a-z0-9])?$ + port: 8080 # 1..65535 + protocol: TCP # optional, default TCP, enum{TCP} only + trafficDistribution: # optional, defaulted + strategy: Nearest # enum{Nearest} only +status: + summary: {locations: 2, members: 6, healthy: 5} + locations: # listType=map on name, MaxItems=64 + - {name: us-central-1, members: 3, healthy: 2, serving: true} + conditions: [MembersResolved, Ready] # both default-seeded Unknown/Pending +``` + +Key semantics: +- **The user gets no hostname or IP from a NetworkService.** Its status is membership/health only. The URL comes from the HTTPProxy in front of it. +- **Backends are selected by label, not referenced.** Nothing in the type names a Workload. Membership = `NetworkInterface` objects **in the same namespace** matching the selector (`internal/controller/networkservice_controller.go`, `matchingInterfaces(ctx, cl, service.Namespace, selector)`). +- Member health is read from the **new `HolderAvailable` condition on `NetworkInterface`** (`api/v1alpha/networkinterface_types.go`, +38 lines). NSO only ever writes it `Unknown`; True/False is the holder's (compute's) to write. +- Conditions: `MembersResolved` (reasons `NoMatchingInterfaces`, `MultipleNetworks`, `InvalidSelector`) and **`Ready` — "wait on this one rather than on what it summarizes."** + +**Label plumbing (already working in this repo):** compute stamps `compute.datumapis.com/{workload-name,placement-name,city-code,instance-index}` on the `NetworkInterfaceClaim` it creates — `internal/controller/networkinterfaceclaim.go:220-238` (`desiredNetworkInterfaceClaimLabels`). PR 411's new `internal/controller/networkinterface_labels.go` allow-lists the `compute.datumapis.com/` prefix and copies those keys claim→interface, and stamps `networking.datumapis.com/location`. So **`matchLabels: {compute.datumapis.com/workload-name: }` is the intended selector and needs no new labelling work in compute.** + +**HTTPProxy gains a fourth backend form** (`api/v1alpha/httpproxy_types.go`, +48/-1): + +```go +type NetworkServiceBackendRef struct { + Name string `json:"name"` // NetworkService in the same namespace + Port string `json:"port"` // names a spec.ports[].name, not a number +} +``` +```yaml +rules: + - backends: + - networkService: {name: storefront, port: http} +``` +Mutually exclusive with `endpoint`/`connector`/`instance` (CEL). **Backend TLS is rejected for `networkService` backends** — the edge always reaches members over plaintext HTTP. New reasons: `NetworkServiceBackendNotFound`, `NetworkServiceMembersUnreferenced` (>100 members: the proxy serves the first shard and says so). + +**Gateway API relation:** indirect. HTTPProxy reuses `gatewayv1` types for `Hostname`, `HTTPRouteMatch`, `HTTPRouteFilter`, `GatewayStatusAddress`, and NSO translates it into downstream Gateway/EnvoyProxy resources. The CLI never touches Gateway API objects. + +**Where the URL comes from** (already in the pinned dep, `api/v1alpha/httpproxy_types.go:240-280`): +- `status.canonicalHostname` — platform-managed stable `.datumproxy.net`. **This is the zero-config URL.** +- `status.addresses`, `status.hostnameStatuses[]` (per-hostname `Verified`, `DNSRecordProgrammed`, `Available`, `CertificateReady`). +- Conditions: `Accepted`, `Programmed`, `HostnamesVerified`, `CertificatesReady`, `DNSRecordsProgrammed`. +- `spec.hostnames` is **optional**; a custom hostname needs a verified `Domain` in the same namespace (auto-created if absent, but still requires user verification). + +## 2. The CLI today + +Entry point `cmd/datumctl-compute/main.go` → `internal/cmd/compute/root.go` (50 lines): `plugin.NewRootCmd("compute", …)` from `go.datum.net/datumctl/plugin`, which supplies persistent `--org`, `--project`, `-o/--output`. A `PersistentPreRunE` runs `util.RunActivationGate`. Subcommands are plain cobra `Command()` constructors registered at `root.go:35-46`. + +- **Client:** `internal/cmd/compute/util/client.go:34-69`. `client.New` (controller-runtime), bearer token from `plugin.Token()`, host = `https:///apis/resourcemanager.miloapis.com/v1alpha1/projects//control-plane`. Scheme registers `computev1alpha`, **`networkingv1alpha` (already!)**, `locationsv1alpha1`, `quotav1alpha1`. Everything lives in namespace `"default"` (`util.ResourceNamespace`, `client.go:24`). +- **Workload creation:** `internal/cmd/compute/deploy/deploy.go:131-263`. Typed structs, `c.Get` → `c.Create`/`c.Update`. `--port` produces exactly one `NamedPort{Name: "http", Port: n, Protocol: TCP}` (`deploy.go:183-187`). One interface on network `"default"` (`deploy.go:210-215`). +- **Precedent for auto-creating a dependent networking resource:** `ensureNetwork` (`deploy.go:384-428`) checks for the `Network`, prompts `"Create it now? (Y/n)"`, creates a minimal auto-IPAM `Network`, refuses in non-interactive mode without `--yes`. **The exposure flow should mirror this exactly.** +- **Readiness waiting:** `internal/cmd/compute/watch/watch.go:39-92` — 2s `time.Ticker` poll of `WorkloadDeploymentList` selected by `compute.datumapis.com/workload-uid`, tabwriter rows, `signal.NotifyContext` for Ctrl-C detach. +- **Status/conditions helpers:** `internal/cmd/compute/util/conditions.go` — `FindCondition`, `ReadinessBlock` (with an explicit rule: *"Callers must not branch on specific reason values — display whatever the server emits"*), `InstanceStatus`/`InstanceStatusDetail`. +- **Output:** `util/printer.go` (`PrintJSON`/`PrintYAML`), `util/table.go` (`NewTabWriter`). `-o yaml/json` exists **only on read commands** (`workloads`, `instances`, `quota`, `access`). +- **No `--dry-run` anywhere in the plugin.** A grep over `internal/cmd/` and `cmd/` returns nothing. +- **Delete:** `internal/cmd/compute/destroy/destroy.go:82-84` deletes only the `Workload`. +- **Completions:** `util/completion.go` — `CompleteWorkloadNames`, `CompleteCityCodes`, `CompleteOutputFormats`. + +**Dependency status (the key finding):** `go.mod:14-17` already pins `go.datum.net/network-services-operator v0.26.1-0.20260821014231-aceb24b1b569` as a **direct** dependency, with a comment saying it is pinned to main for the `Prepared` condition. That pin **has `httpproxy_types.go` but NOT `networkservice_types.go`**, and its `HTTPProxyRuleBackend` has no `NetworkService` field. Re-pinning is required and is the hard blocker. Once re-pinned, **no scheme change is needed** — PR 411 adds `NetworkService`/`NetworkServiceList` to the existing `SchemeBuilder` in `api/v1alpha/groupversion_info.go` (+4), which `util/client.go:53` already calls. + +Compute also already writes the condition NSO reads: `internal/controller/networkinterface_holder.go:27` declares `networkInterfaceHolderAvailable = "HolderAvailable"` (as a local literal; `datum-cloud/compute#254`, still open, swaps it for the NSO constant). + +## 3. Proposed UX + +### Alternative A — flag on `deploy` +``` +datumctl compute deploy api --image=… --city=DFW --port=8080 --expose-http +datumctl compute deploy api … --expose-http --hostname=api.example.com +``` +Pros: single command from source to URL; matches the DX arc in `docs/enhancements/datumctl-compute-dx.md` (whose interactive mock at line 91 already prompts `? Expose port (optional): 8080`). Cons: no way to expose an existing workload; no way to unexpose without editing a manifest; couples exposure lifecycle to deploy lifecycle; `deploy -f workload.yaml` has nowhere sensible to put the flag. + +### Alternative B — dedicated `expose` verb group +``` +datumctl compute expose [--port=8080|--port-name=http] [--hostname=…] [--wait] [-o yaml] +datumctl compute expose status +datumctl compute unexpose +``` +Pros: exposure has its own lifecycle (hostnames, DNS, certs, delete semantics) and deserves its own verbs; works on existing workloads; `unexpose` is discoverable; `expose -o yaml` gives a manifest-first path without inventing `--dry-run`. Cons: two commands to a URL. + +### Recommendation: **B as the foundation, A as a thin caller.** + +Build `internal/cmd/compute/expose/` with an exported `Ensure(ctx, c, out, opts)` that owns all resource construction and readiness. Then `deploy --expose-http` is ~10 lines calling `Ensure` after `watch.Rollout` returns. Rationale: + +1. Exposure state must be inspectable and removable independent of the workload — `expose status` / `unexpose` are not optional, so B's surface has to exist regardless. A alone cannot get there. +2. The custom-hostname path (Domain verification, TXT records, cert issuance) is inherently multi-step and interactive; it needs a command that can be re-run to poll. Bolting that onto `deploy` makes `deploy` unpredictable. +3. `deploy` already sets the precedent for auto-creating a dependency with a prompt (`ensureNetwork`), so `--expose-http` fits naturally as sugar without owning the logic. +4. `expose -o yaml` printing the two objects gives the manifest-driven users (`deploy -f`) what they need, and is cheaper than retrofitting `--dry-run` across the plugin. + +**Recommended phase-1 scope: no `--hostname`.** Ship the zero-config path only — `status.canonicalHostname` gives a working `https://.datumproxy.net` with a platform-managed cert and no user DNS. Defer custom hostnames to phase 2. + +Proposed output: +``` +$ datumctl compute expose api --port=8080 + networkservice/api created (selector: compute.datumapis.com/workload-name=api) + httpproxy/api created +Waiting for endpoints and edge programming. Ctrl-C to detach. + + RESOURCE STATE + networkservice/api Ready (2 locations, 4 members, 4 healthy) + httpproxy/api Programmed + + https://a1b2c3d4.datumproxy.net +``` + +## 4. Resources created, order, ownership, deletion + +Order (each `Get` → `Create`-or-`Update`, matching `deploy.go:238-249`): + +1. **Preflight.** Workload exists; resolve the port. Prefer an existing `NamedPort` from `workload.Spec.Template.Spec.Runtime.Sandbox.Containers[*].Ports` (or `VirtualMachine.Ports`); require `--port`/`--port-name` only when ambiguous. Fail early with a clear message if the workload declares no ports. +2. **`NetworkService/`** in `default`: + - `spec.networkInterfaces.selector.matchLabels = {compute.datumapis.com/workload-name: }` (constant `computev1alpha.WorkloadNameLabel`, `api/v1alpha/labels.go:23`). + - `spec.ports = [{name: , port: , protocol: TCP}]`. + - Leave `trafficDistribution` unset (the type comment explicitly says *"Leave it unset"*). +3. **`HTTPProxy/`** in `default`: `spec.rules[0].backends[0].networkService = {name: , port: }`. Omit `spec.hostnames` in phase 1. Omit `matches` (CRD defaults to `PathPrefix: /`). Never set backend `tls`. +4. **Wait** (opt-out with `--no-wait`): poll `NetworkService` for `Ready=True`, then `HTTPProxy` for `Programmed=True` and non-empty `status.canonicalHostname`. Surface blocking reason+message verbatim via `util.ReadinessBlock`, per the existing house rule. + +**Ownership and labels** — set on both objects: +- `ownerReferences: [{apiVersion: compute.datumapis.com/v1alpha, kind: Workload, name, uid, controller: false, blockOwnerDeletion: false}]`. Same namespace, so this is legal; cross-group is fine. +- `labels: {compute.datumapis.com/workload-name: , compute.datumapis.com/workload-uid: }` — existing constants `WorkloadNameLabel` / `WorkloadUIDLabel` (`api/v1alpha/labels.go:19-23`). The labels, not the ownerRef, are what `expose status` and `unexpose` list on; they also survive a project control plane whose GC behaviour is unverified. + +**On delete:** +- `unexpose `: delete `HTTPProxy` first, then `NetworkService` (proxy-first avoids a window where the proxy reports `NetworkServiceBackendNotFound`), selected by the UID label. Warn that any `Domain` created for a custom hostname is deliberately left behind (it is a project-level ownership record, not per-workload). +- `destroy ` (`destroy.go`): list the labelled `HTTPProxy`/`NetworkService`, include them in the confirmation summary, and delete them explicitly rather than trusting owner-reference GC. Do not silently rely on GC until it is confirmed to run in project virtual control planes. +- The `Network` is never deleted (consistent with `ensureNetwork` never cleaning up). + +## 5. Implementation plan + +**Dependencies** +- `go.mod:14-17` — re-pin `go.datum.net/network-services-operator` to a commit carrying `NetworkService` + the `networkService` backend field. **Blocked on PR 411 merging** (it is an explicit non-merge-candidate today). Update the existing pin comment, which currently explains the `Prepared` condition rationale. +- No new modules. `sigs.k8s.io/gateway-api v1.5.1` is already required (`go.mod:29`) for the `gatewayv1.Hostname` types. + +**Scheme registration:** none. `networkingv1alpha.AddToScheme` at `internal/cmd/compute/util/client.go:53` picks up the new kinds automatically. Add a defensive `meta.IsNoMatchError` check so an older control plane yields *"HTTP exposure is not available in this project"* rather than a raw REST mapper error. + +**Files to touch** + +| File | Change | +|---|---| +| `go.mod` / `go.sum` | Re-pin NSO | +| `internal/cmd/compute/expose/expose.go` *(new)* | `Command()`, `Ensure()`, `Remove()`, `Status()` | +| `internal/cmd/compute/expose/resources.go` *(new)* | Pure builders: workload → `NetworkService` + `HTTPProxy`. Unit-testable, no client. | +| `internal/cmd/compute/expose/wait.go` *(new)* | Ticker-based readiness, modelled on `watch/watch.go:39-92` | +| `internal/cmd/compute/expose/*_test.go` *(new)* | Builder table tests + fake-client flow tests | +| `internal/cmd/compute/root.go:35-46` | Register `expose.Command()`, `unexpose.Command()` | +| `internal/cmd/compute/deploy/deploy.go` | `--expose-http` flag (`~line 85`), validation in `runDeploy`, call `expose.Ensure` after `watch.Rollout` (`:262`) | +| `internal/cmd/compute/destroy/destroy.go:55-84` | List + summarize + delete exposure resources | +| `internal/cmd/compute/util/conditions.go` | `NetworkServiceStatus()` / `HTTPProxyStatus()` summarizers, same shape as `InstanceStatus` | +| `internal/cmd/compute/util/completion.go` | `CompleteExposedWorkloads` for `unexpose` | +| `docs/enhancements/datumctl-compute-dx.md` | Update the interactive mock (line 91) and add the exposure flow | + +**RBAC:** the CLI acts as the end user via `plugin.Token()`; there is no service account to grant. The user needs `networking.datumapis.com/networkservices.{create,get,list,watch,update,patch,delete}` and the equivalent `httpproxies` permissions. PR 411 adds `networkservices.{create,update,patch,delete}` **only to `config/iam/roles/networking-admin.yaml`** and read verbs to `networking-viewer.yaml`. **A project member holding only compute roles will get a 403.** This is a cross-repo prerequisite: either the compute roles need these permissions, or the docs must state that `networking-admin` is required to expose a workload. Worth raising with the networking team before build starts. + +## 6. Open questions and risks + +1. **PR 411 is a draft prototype, not a merge candidate.** Everything is blocked on it. Treat all field names as provisional. +2. **The PR body contradicts the code.** The body's YAML example uses `spec.networkInterfaceClaims:`, but the Go type is `NetworkInterfaces NetworkServiceInterfaceSelector` with json tag `networkInterfaces`, and the CRD/chainsaw test both use `networkInterfaces`. Confirm which survives before writing builders. +3. **Biggest functional risk — membership may not resolve at all.** The controller lists `NetworkInterface` objects **in the NetworkService's own namespace**. Compute creates claims in the cell control plane: *"the claim is served by the control plane the instance runs in"* (`internal/controller/networkinterfaceclaim.go:69-72`). PR 411 states plainly: *"claims are not published to the consumer's project, so membership currently resolves where the claims already are."* Until interfaces are projected into the project control plane, a CLI-written NetworkService in `default` will sit at `MembersResolved=False/NoMatchingInterfaces` forever. **Verify this against a real staging project before committing to the design.** +4. **Multi-city is the default and is currently broken.** PR 411: *"a service with members in two locations binds no VRF and fails every request"* until `datum-cloud/cloud#16` lands. `deploy --city=DFW,IAD` is the documented happy path (`deploy.go:60`). The CLI must detect >1 city and warn loudly, or refuse, rather than producing a silently non-serving URL. +5. **No location coordinates exist yet**, so `Nearest` ranking is not actually computable — cross-location behaviour is untested end to end (single-cell environment). +6. **Single network per service.** A workload with interfaces on two networks yields `MultipleNetworks`. Today `deploy` hardcodes one interface on `"default"` (`deploy.go:210-215`), so this is safe now but fragile; consider adding the network to the selector. +7. **100-member cap.** Past it the proxy serves the first shard and reports `NetworkServiceMembersUnreferenced`. `expose status` must surface it. +8. **Port-name mismatch.** `computev1alpha.NamedPort.Name` (`api/v1alpha/instance_types.go:254-258`) has no pattern constraint; `NetworkServicePort.Name` requires `^[a-z0-9]([-a-z0-9]*[a-z0-9])?$`, <=63. Needs sanitization with a clear error. `deploy` currently hardcodes `"http"` (`deploy.go:185`), which is safe. +9. **Plaintext to the backend, always.** `networkService` backends reject `tls`. Users terminating TLS in their container will break. Must be documented. +10. **DNS/certs.** Phase 1 is free — `.datumproxy.net` with platform-managed A/AAAA and certs. Phase 2 (`--hostname`) drags in `Domain` creation, TXT/HTTP verification, `HostnamesVerified`/`CertificatesReady`/`DNSRecordsProgrammed`, and hostname-uniqueness conflicts across the whole platform. Substantially more work than phase 1. +11. **`HolderAvailable` string coupling.** Compute writes the literal `"HolderAvailable"` (`internal/controller/networkinterface_holder.go:27`). If NSO renames it, compute keeps compiling and every member silently reads unhealthy. `datum-cloud/compute#254` fixes this and should land first. +12. **Multi-tenancy.** All resources go to namespace `default` in the project's virtual control plane, so naming collides on the workload name — acceptable, and it makes exposure idempotent per workload, but it means one workload cannot have two proxies. +13. **Alpha API, no conversion guarantees.** Handle `NoKindMatch` gracefully. + +## 7. Effort estimate + +| Chunk | Est. | +|---|---| +| Re-pin NSO, verify build + scheme, `NoKindMatch` guard | 0.5 d *(gated on PR 411)* | +| Resource builders + table tests (`resources.go`) | 1.5 d | +| `expose` / `unexpose` / `expose status` commands, fake-client tests | 2 d | +| Readiness watcher + status summarizers + condition messaging | 2 d | +| `deploy --expose-http` wiring + interactive prompt | 1 d | +| `destroy` cascade, ownership/labels, completions | 1 d | +| Docs (`datumctl-compute-dx.md`) | 0.5 d | +| **Phase 1 total** | **~8.5 dev-days** | +| Phase 2: `--hostname`, Domain creation + verification UX, cert/DNS status | +4 d | +| Cross-repo prerequisites (IAM roles, interface projection, `#254`) | not estimated — external | + +Add ~1 d of slack for API churn while PR 411 is a draft. diff --git a/go.mod b/go.mod index c95bfd7c..3e1095c7 100644 --- a/go.mod +++ b/go.mod @@ -10,10 +10,14 @@ require ( github.com/onsi/gomega v1.42.1 github.com/prometheus/client_golang v1.23.2 github.com/stretchr/testify v1.11.1 - // Pinned to network-services-operator main: the latest tag (v0.26.0) - // predates the Prepared condition this gate reads. Re-pin to a tagged - // release once one carries it. - go.datum.net/network-services-operator v0.26.1-0.20260821014231-aceb24b1b569 + // UNMERGED: pinned to the head of network-services-operator#411 + // (branch proto/network-service), which adds the NetworkService API and the + // networkService HTTPProxy backend that `datumctl compute --http-port` needs. + // That PR is a draft and its branch may be force-pushed or deleted, which + // would break `go mod download` here. Re-pin to main as soon as it merges. + // The pre-411 pin was chosen for the Prepared condition, which this commit + // also carries. + go.datum.net/network-services-operator v0.26.5-0.20260909170421-bcd1965a821c // Pinned by pseudo-version to the commit deployed to staging, which is the // same one network-services-operator pins. The module publishes no tag yet. go.miloapis.com/locations v0.0.0-20260825185141-507ac2cbd48c @@ -116,7 +120,7 @@ require ( github.com/go-openapi/jsonreference v0.21.6 // indirect github.com/go-openapi/swag v0.25.4 // indirect github.com/go-task/slim-sprig/v3 v3.0.0 // indirect - github.com/google/cel-go v0.26.0 // indirect + github.com/google/cel-go v0.27.0 // indirect github.com/google/gnostic-models v0.7.1 // indirect github.com/google/pprof v0.0.0-20260115054156-294ebfa9ad83 // indirect github.com/google/uuid v1.6.0 // indirect diff --git a/go.sum b/go.sum index d63b9ff2..799a9c28 100644 --- a/go.sum +++ b/go.sum @@ -165,6 +165,8 @@ github.com/golang/protobuf v1.5.4 h1:i7eJL8qZTpSEXOPTxNKhASYpMn+8e5Q6AdndVa1dWek github.com/golang/protobuf v1.5.4/go.mod h1:lnTiLA8Wa4RWRcIUkrtSVa5nRhsEGBg48fD6rSs7xps= github.com/google/cel-go v0.26.0 h1:DPGjXackMpJWH680oGY4lZhYjIameYmR+/6RBdDGmaI= github.com/google/cel-go v0.26.0/go.mod h1:A9O8OU9rdvrK5MQyrqfIxo1a0u4g3sF8KB6PUIaryMM= +github.com/google/cel-go v0.27.0 h1:e7ih85+4qVrBuqQWTW4FKSqZYokVuc3HnhH5keboFTo= +github.com/google/cel-go v0.27.0/go.mod h1:tTJ11FWqnhw5KKpnWpvW9CJC3Y9GK4EIS0WXnBbebzw= github.com/google/gnostic-models v0.7.1 h1:SisTfuFKJSKM5CPZkffwi6coztzzeYUhc3v4yxLWH8c= github.com/google/gnostic-models v0.7.1/go.mod h1:whL5G0m6dmc5cPxKc5bdKdEN3UjI7OUGxBlw57miDrQ= github.com/google/go-cmp v0.7.0 h1:wk8382ETsv4JYUZwIsn6YpYiWiBsYLSJiTsyBybVuN8= @@ -350,6 +352,8 @@ go.datum.net/datumctl v0.17.1-0.20260710003126-296c2fcbbd67 h1:Mhgt688CeTh2hX7ql go.datum.net/datumctl v0.17.1-0.20260710003126-296c2fcbbd67/go.mod h1:6skEjcE7aT8VPf/HVamA/BB6Dc9IISA6c/DdYKhqWNc= go.datum.net/network-services-operator v0.26.1-0.20260821014231-aceb24b1b569 h1:14vajo15fGGEAmdystQEE261rFH4AqER9s1Vg7POUKo= go.datum.net/network-services-operator v0.26.1-0.20260821014231-aceb24b1b569/go.mod h1:A7JNOuc+e6j/KkUVCcZ7Z2odvf6JFMQlX0Zo1Awj2TY= +go.datum.net/network-services-operator v0.26.5-0.20260909170421-bcd1965a821c h1:nHFLUbR5xi2hvYuV7DtH00SPuve/uyL95Fi0ASSjHY8= +go.datum.net/network-services-operator v0.26.5-0.20260909170421-bcd1965a821c/go.mod h1:9nuuBWdrkdnIBMaWJsWM3j4CcbKJIF1GDuCmAjdpIHo= go.miloapis.com/locations v0.0.0-20260825185141-507ac2cbd48c h1:+BQirT3wYCgv7H2lEZtAH+dpMiWu9j/Wa+c8s4jJUIA= go.miloapis.com/locations v0.0.0-20260825185141-507ac2cbd48c/go.mod h1:gzfAfHhSMwl/N68k/uSNYXOKK3IOBJCdXYaBgCE3gdE= go.miloapis.com/milo v0.32.0 h1:TkNIQu/37d+SEquLJ5+GmdisSl+K2RT7eEC4idg6RIs= diff --git a/internal/cmd/compute/deploy/deploy.go b/internal/cmd/compute/deploy/deploy.go index 2c3925d4..6d08af3e 100644 --- a/internal/cmd/compute/deploy/deploy.go +++ b/internal/cmd/compute/deploy/deploy.go @@ -4,7 +4,9 @@ import ( "bufio" "bytes" "context" + "errors" "fmt" + "io" "os" "os/signal" "strings" @@ -20,6 +22,7 @@ import ( computev1alpha "go.datum.net/compute/api/v1alpha" "go.datum.net/compute/internal/cmd/compute/build" + "go.datum.net/compute/internal/cmd/compute/url" "go.datum.net/compute/internal/cmd/compute/util" "go.datum.net/compute/internal/cmd/compute/watch" networkingv1alpha "go.datum.net/network-services-operator/api/v1alpha" @@ -27,6 +30,24 @@ import ( "sigs.k8s.io/controller-runtime/pkg/client" ) +const ( + // httpPortName is the name given to the container port --http-port + // declares, and the name the URL's backend reference points at. + httpPortName = "http" + + // planLabelWidth is the label column of the plan summary printed before + // the Apply prompt, so every line in it starts its value at the same + // column. + planLabelWidth = 20 +) + +// errPortRenamed is the one-release migration for --port. It is an error and +// not an alias on purpose: silently mapping it to --http-port would publish +// every existing workload on the internet at the next plugin upgrade. +var errPortRenamed = errors.New( + "--port has been replaced by --http-port, which publishes the workload on a public HTTPS URL. " + + "Use --http-port 8080 to publish, or --no-http to keep it internal") + type options struct { image string build string @@ -35,12 +56,22 @@ type options struct { locationSelector string cities []string min int32 + httpPort int32 + noHTTP bool port int32 file string yes bool } +// Command returns the deploy command. func Command() *cobra.Command { + cmd, _ := command() + return cmd +} + +// command builds the deploy command and hands back the options it writes into, +// so flag validation can be exercised without a control plane. +func command() (*cobra.Command, *options) { opts := &options{} cmd := &cobra.Command{ @@ -57,10 +88,21 @@ Dockerfile discovery, no build-arg/target overrides — use 'datumctl compute bu directly if you need those) and pushes to --image, which the deployed workload then pins by digest rather than the tag you gave it. It also analyzes and auto-fixes common compatibility issues, rewriting the Dockerfile in place when -a fix is applied — same as 'datumctl compute build --fix'.`, +a fix is applied — same as 'datumctl compute build --fix'. + +Use --http-port to declare that the workload is an HTTP service. Declaring one +publishes the workload on a Datum-managed HTTPS URL, printed as the last line +of a successful deploy. Omitting --http-port on an existing workload leaves its +HTTP service as it is; --no-http removes it and stops serving.`, Args: cobra.MaximumNArgs(1), Example: ` # Deploy with flags - datumctl compute deploy api --image=ghcr.io/acme/api:1.4.2 --location=us-east-1,eu-west-1 --min=2 --port=8080 + datumctl compute deploy api --image=ghcr.io/acme/api:1.4.2 --location=us-east-1,eu-west-1 --min=2 --http-port=8080 + + # Deploy an internal workload (no URL) + datumctl compute deploy worker --image=ghcr.io/acme/worker:2.0 --location=us-east-1 + + # Stop serving: remove the HTTP service and its URL + datumctl compute deploy api --image=ghcr.io/acme/api:1.4.2 --location=us-east-1,eu-west-1 --no-http # Build and deploy in one step (builds ., pushes to --image, deploys that digest) datumctl compute deploy api --build --image=ghcr.io/acme/api:1.4.2 --location=us-east-1,eu-west-1 @@ -93,17 +135,62 @@ a fix is applied — same as 'datumctl compute build --fix'.`, cmd.Flags().StringVar(&opts.locationSelector, "location-selector", "", "Select every location whose topology matches a label selector (e.g. 'topology.datum.net/city-code=DFW' or 'topology.datum.net/region in (us-east-1,eu-west-1)')") cmd.Flags().StringSliceVar(&opts.cities, "city", nil, "Deploy to every location in these cities (e.g. DFW,IAD); shorthand for a --location-selector on topology.datum.net/city-code") cmd.Flags().Int32Var(&opts.min, "min", 1, "Minimum number of instances per location") - cmd.Flags().Int32Var(&opts.port, "port", 0, "Port to expose on the workload (optional)") + cmd.Flags().Int32Var(&opts.httpPort, "http-port", 0, "Port the container serves HTTP on; publishes the workload on a Datum-managed HTTPS URL") + cmd.Flags().BoolVar(&opts.noHTTP, "no-http", false, "Remove the workload's HTTP service, and with it its URL") cmd.Flags().StringVarP(&opts.file, "file", "f", "", "Path to a workload manifest file") cmd.Flags().BoolVarP(&opts.yes, "yes", "y", false, "Skip confirmation prompts") _ = cmd.RegisterFlagCompletionFunc("location", util.CompletePlacementLocations) _ = cmd.RegisterFlagCompletionFunc("location-selector", util.CompleteLocationSelector) _ = cmd.RegisterFlagCompletionFunc("city", util.CompleteCityCodes) - return cmd + // --port stays registered for one release so that using it produces the + // migration error rather than "unknown flag". It is hidden rather than + // deprecated: cobra's deprecation only warns and proceeds, and printing a + // warning above the error that follows says the same thing twice. + cmd.Flags().Int32Var(&opts.port, "port", 0, "Removed: use --http-port") + _ = cmd.Flags().MarkHidden("port") + + return cmd, opts +} + +// validateFlags rejects flag combinations before the command builds a client +// or creates anything, so an upgrade that trips the --port break costs a +// message and not a workload. +func validateFlags(cmd *cobra.Command, opts *options) error { + if cmd.Flags().Changed("port") { + return errPortRenamed + } + + httpPortSet := cmd.Flags().Changed("http-port") + + if httpPortSet && opts.noHTTP { + return fmt.Errorf("--http-port and --no-http cannot be combined — pass --http-port to publish the workload, or --no-http to stop serving it") + } + + if opts.file != "" { + switch { + case httpPortSet: + // TODO: a manifest has no way to declare an HTTP service yet. + // Resolving that is an API conversation (a field on the workload + // spec), not CLI sugar layered on top of -f. + return fmt.Errorf("--http-port cannot be combined with -f: a manifest declares its own ports, and declaring an HTTP service in a manifest is not supported yet") + case opts.noHTTP: + return fmt.Errorf("--no-http cannot be combined with -f: remove the URL with 'datumctl compute destroy', or deploy with flags") + } + } + + if httpPortSet && (opts.httpPort < 1 || opts.httpPort > 65535) { + return fmt.Errorf("--http-port must be between 1 and 65535, got %d", opts.httpPort) + } + + return nil } func runDeploy(cmd *cobra.Command, args []string, opts *options) error { + if err := validateFlags(cmd, opts); err != nil { + return err + } + // Determine path. if opts.file != "" { if opts.build != "" { @@ -142,36 +229,48 @@ func runDeploy(cmd *cobra.Command, args []string, opts *options) error { } // deployFromFlags implements Path A: deploy a workload using CLI flags. -func deployFromFlags(cmd *cobra.Command, workloadName string, opts *options) error { - project := util.ProjectFromCmd(cmd) - if project == "" { - return fmt.Errorf("no project set — pass --project or run 'datumctl config set project '") - } - if opts.image == "" { - return fmt.Errorf("--image is required") - } - placementFlags := 0 - for _, set := range []bool{len(opts.locations) > 0, len(opts.cities) > 0, opts.locationSelector != ""} { - if set { - placementFlags++ +// resolveLocationSelector validates the three mutually exclusive ways a deploy +// can say where to run, and returns the selector they resolve to. --location +// names its locations outright and needs no selector, so a nil return with a +// nil error means "the locations were named". +func resolveLocationSelector(opts *options) (*metav1.LabelSelector, error) { + set := 0 + for _, given := range []bool{len(opts.locations) > 0, len(opts.cities) > 0, opts.locationSelector != ""} { + if given { + set++ } } - if placementFlags == 0 { - return fmt.Errorf("--location is required (e.g. --location=us-east-1,eu-west-1); or use --city to deploy to every location in a city, or --location-selector to select locations by topology") + switch { + case set == 0: + return nil, fmt.Errorf("--location is required (e.g. --location=us-east-1,eu-west-1); or use --city to deploy to every location in a city, or --location-selector to select locations by topology") + case set > 1: + return nil, fmt.Errorf("--location, --city, and --location-selector are mutually exclusive") } - if placementFlags > 1 { - return fmt.Errorf("--location, --city, and --location-selector are mutually exclusive") - } - var locationSelector *metav1.LabelSelector + if opts.locationSelector != "" { parsed, err := metav1.ParseToLabelSelector(opts.locationSelector) if err != nil { - return fmt.Errorf("invalid --location-selector %q: %w", opts.locationSelector, err) + return nil, fmt.Errorf("invalid --location-selector %q: %w", opts.locationSelector, err) } - locationSelector = parsed + return parsed, nil } if len(opts.cities) > 0 { - locationSelector = computev1alpha.CityCodeSelector(opts.cities) + return computev1alpha.CityCodeSelector(opts.cities), nil + } + return nil, nil +} + +func deployFromFlags(cmd *cobra.Command, workloadName string, opts *options) error { + project := util.ProjectFromCmd(cmd) + if project == "" { + return fmt.Errorf("no project set — pass --project or run 'datumctl config set project '") + } + if opts.image == "" { + return fmt.Errorf("--image is required") + } + locationSelector, err := resolveLocationSelector(opts) + if err != nil { + return err } instanceType := opts.instanceType if instanceType == "" { @@ -209,16 +308,34 @@ func deployFromFlags(cmd *cobra.Command, workloadName string, opts *options) err } } + // Resolve the HTTP service this deploy declares: + // + // --http-port N declares (or changes) it + // --no-http removes it, and the URL with it + // neither keeps what the workload already declares + // + // Carrying the existing port forward matters: without it, a routine image + // bump would drop the port from the spec and take a live URL down without + // anyone saying so. --no-http is the only way to stop serving. + httpPort := opts.httpPort + if httpPort == 0 && !opts.noHTTP { + httpPort = declaredHTTPPort(&workload) + } + // Build spec. tcp := corev1.ProtocolTCP container := computev1alpha.SandboxContainer{ Name: "app", Image: opts.image, } - if opts.port > 0 { - container.Ports = []computev1alpha.NamedPort{ - {Name: "http", Port: opts.port, Protocol: &tcp}, + portName := "" + if httpPort > 0 { + httpNamedPort := computev1alpha.NamedPort{Name: httpPortName, Port: httpPort, Protocol: &tcp} + portName, err = url.PortName(httpNamedPort) + if err != nil { + return err } + container.Ports = []computev1alpha.NamedPort{httpNamedPort} } locationRefs := make([]locationsv1alpha1.LocationReference, 0, len(locations)) @@ -258,7 +375,10 @@ func deployFromFlags(cmd *cobra.Command, workloadName string, opts *options) err Placements: []computev1alpha.WorkloadPlacement{placement}, } - fmt.Fprintf(out, " Placement \"default\": %s, min=%d\n", describePlacementLocations(placement), opts.min) + fmt.Fprintln(out, planLine(`Placement "default"`, + fmt.Sprintf("%s, min=%d", describePlacementLocations(placement), opts.min))) + + removedURL := planHTTPService(ctx, out, c, workloadName, httpPort, opts, creating) // Prompt unless --yes or non-interactive. if !opts.yes && term.IsTerminal(int(os.Stdin.Fd())) { @@ -287,6 +407,21 @@ func deployFromFlags(cmd *cobra.Command, workloadName string, opts *options) err fmt.Fprintf(out, " workload/%s updated\n", workloadName) } + if opts.noHTTP { + if err := removeHTTPService(ctx, out, c, workloadName, removedURL, creating); err != nil { + return err + } + } + + // The URL goes in alongside the workload, not after the rollout: backends + // register as instances come up, so the URL answers moments after the last + // city is Done rather than starting from scratch once it is. + // + // A failure is carried to publish rather than returned here. The workload + // is applied and rolling out, and a user is owed that table before being + // told the URL did not go up. + publishErr := declareURL(ctx, c, &workload, portName, httpPort) + // Save workload.yaml. if err := saveWorkloadYAML(workloadName, &workload); err != nil { fmt.Fprintf(out, " warning: could not save workload.yaml: %v\n", err) @@ -298,7 +433,198 @@ func deployFromFlags(cmd *cobra.Command, workloadName string, opts *options) err watchCtx, cancel := signal.NotifyContext(cmd.Context(), os.Interrupt) defer cancel() - return watch.Rollout(watchCtx, c, out, project, workload.UID) + if err := watch.Rollout(watchCtx, c, out, project, workload.UID); err != nil { + return err + } + + return publish(watchCtx, out, c, &workload, httpPort, opts, publishErr) +} + +// planHTTPService prints the HTTP line of the plan summary and returns the URL +// that --no-http is about to take down, if there is one. +// +// The HTTP service is part of the plan, not a surprise after the fact: what +// gets published — or what stops answering — is stated before the prompt that +// creates it. +func planHTTPService(ctx context.Context, out io.Writer, c client.Client, workloadName string, httpPort int32, opts *options, creating bool) string { + if httpPort > 0 { + fmt.Fprintln(out, planLine("HTTP service", fmt.Sprintf("port %d → Datum-managed URL", httpPort))) + // Said here, once, because a container that terminates TLS itself + // answers nothing and the only symptom is a URL that does not work. + fmt.Fprintln(out, planNote("Datum terminates TLS; serve plain HTTP on this port.")) + return "" + } + if !opts.noHTTP || creating { + return "" + } + + // A lookup failure here is not worth failing a deploy over: the line just + // loses the hostname it would have named, and Unpublish reports any real + // problem with the control plane a moment later. + removedURL := "" + if info, err := url.ForWorkload(ctx, c, workloadName); err == nil && info != nil { + removedURL = info.URL + } + + if removedURL == "" { + fmt.Fprintln(out, planLine("HTTP service", "removed")) + return "" + } + fmt.Fprintln(out, planLine("HTTP service", fmt.Sprintf("removed — %s will stop responding", removedURL))) + return removedURL +} + +// removeHTTPService takes the URL down. It runs as soon as the workload stops +// declaring the port rather than at the end of the rollout: the workload and +// what answers for it have to agree. +func removeHTTPService(ctx context.Context, out io.Writer, c client.Client, workloadName, removedURL string, creating bool) error { + if err := url.Unpublish(ctx, c, workloadName); err != nil { + return err + } + switch { + case removedURL != "": + fmt.Fprintf(out, " HTTP service removed — %s no longer responds\n", removedURL) + case !creating: + _, _ = fmt.Fprintln(out, " HTTP service removed") + } + return nil +} + +// declareURL writes the objects that put the workload on its URL. It runs +// alongside the workload write, before the rollout: backends then register as +// instances come up, and the URL is ready within a second or two of the last +// city reaching Done. Declaring them after the rollout would add a visible +// stall to every deploy. +// +// It prints nothing. Nothing has happened yet that a user needs to read, and +// the rollout table comes next; publishing reports itself once the rollout is +// over and there is progress to show. +func declareURL(ctx context.Context, c client.Client, w *computev1alpha.Workload, portName string, port int32) error { + if port <= 0 { + return nil + } + + hostnames, err := existingHostnames(ctx, c, w.Name) + if err != nil { + return err + } + return url.Declare(ctx, c, w, portName, port, hostnames) +} + +// notReachable states the dead end this whole feature exists to close: a +// workload with no HTTP port is not on the internet, and no developer should +// have to work that out for themselves. +func notReachable(out io.Writer, workloadName string) { + fmt.Fprintf(out, "\n No HTTP port declared — this workload is not reachable from the internet.\n") + fmt.Fprintf(out, " To publish it: datumctl compute deploy %s --http-port 8080\n", workloadName) +} + +// publish waits for the URL declared before the rollout and prints it as the +// last line of the deploy, or explains why there is no URL to print. +// +// declareErr is whatever declareURL reported. It is carried this far rather +// than failing the deploy on the spot so that a user still gets the rollout +// table for a workload that is, after all, being deployed. +// +// It runs after the rollout, so the workload is already up: a failure here is +// a failure to publish, never a failure to deploy, and it says so before the +// error is returned. A user whose workload is running must not read a bare +// "Error:" as "the deploy failed". +func publish(ctx context.Context, out io.Writer, c client.Client, w *computev1alpha.Workload, port int32, opts *options, declareErr error) error { + if port <= 0 { + // --no-http was just told, line by line, that the URL is gone. Telling + // the same user to publish is answering a question nobody asked. + if !opts.noHTTP { + notReachable(out, w.Name) + } + return nil + } + + _, _ = fmt.Fprintln(out, "\nPublishing...") + + // The objects went in before the rollout, so there is nothing left to do + // here but watch — including for a user who detached, whose URL is already + // declared and coming up without them. + var info *url.Info + err := declareErr + if err == nil { + info, err = url.Wait(ctx, out, c, w.Name) + } + if err != nil { + fmt.Fprintf(out, "\n The rollout succeeded — the workload is deployed and running.\n") + fmt.Fprintf(out, " Only publishing its URL failed. Retry with:\n") + fmt.Fprintf(out, " datumctl compute deploy %s --image %s --http-port %d\n", w.Name, opts.image, port) + return fmt.Errorf("publishing URL for workload %q: %w", w.Name, err) + } + + // A nil Info is a detach, not a failure: url.Wait has already said how to + // pick the URL up again. + if info == nil || info.URL == "" { + return nil + } + + fmt.Fprintf(out, "\n %s\n", info.URL) + return nil +} + +// existingHostnames returns the custom hostnames already attached to the +// workload's URL, so republishing carries them forward. +// +// Publishing rewrites the proxy spec wholesale. Custom hostnames are not set by +// this plugin — they are configured out of band, by the ALB tooling that owns +// advanced proxy configuration — so without this every redeploy would silently +// detach them and the custom domain would stop answering. That matters more, +// not less, for hostnames this plugin cannot see itself having added. +// +// It fails closed. A workload that has never been published has no hostnames +// and that is a nil with no error, but a control plane that cannot be read is +// an error the caller must stop on: the two calls use different verbs on the +// same object — a List here, a Get in the apply — so a control plane that +// refuses one and answers the other would otherwise rewrite spec.Hostnames to +// nothing and report success. +func existingHostnames(ctx context.Context, c client.Client, workloadName string) ([]string, error) { + info, err := url.ForWorkload(ctx, c, workloadName) + if err != nil { + return nil, fmt.Errorf("reading the domains attached to %q: %w", workloadName, err) + } + if info == nil { + return nil, nil + } + return info.CustomHostnames, nil +} + +// planLine renders one line of the plan summary printed before the Apply +// prompt, with every value starting in the same column. +func planLine(label, value string) string { + return fmt.Sprintf(" %-*s %s", planLabelWidth, label+":", value) +} + +// planNote renders a continuation of the plan line above it, aligned under +// that line's value rather than carrying a label of its own. +func planNote(text string) string { + return fmt.Sprintf(" %-*s %s", planLabelWidth, "", text) +} + +// declaredHTTPPort returns the HTTP port a workload already declares, or 0. +// The port named "http" wins; failing that, the first declared port is the one +// the URL was built on, since that is what a flag-driven deploy writes. +func declaredHTTPPort(w *computev1alpha.Workload) int32 { + sandbox := w.Spec.Template.Spec.Runtime.Sandbox + if sandbox == nil { + return 0 + } + first := int32(0) + for _, container := range sandbox.Containers { + for _, p := range container.Ports { + if p.Name == httpPortName { + return p.Port + } + if first == 0 { + first = p.Port + } + } + } + return first } // deployFromFile implements Path C: deploy from a manifest file. @@ -387,7 +713,31 @@ func deployFromFile(cmd *cobra.Command, opts *options) error { watchCtx, cancel := signal.NotifyContext(cmd.Context(), os.Interrupt) defer cancel() - return watch.Rollout(watchCtx, c, out, project, workload.UID) + + if err := watch.Rollout(watchCtx, c, out, project, workload.UID); err != nil { + return err + } + + reportManifestReachability(out, &workload) + return nil +} + +// reportManifestReachability closes the dead end for the manifest path. +// +// TODO: the manifest path publishes nothing. A workload manifest has no way to +// declare "this is an HTTP service" — the flag path's --http-port has no +// equivalent field — and inferring one from a container port would publish +// workloads whose authors never asked for a URL. Resolving it means a field on +// the workload spec, which is an API decision, not a CLI one. +// +// The note, though, is not publishing. A workload nothing can reach is the +// same dead end however it was deployed, and a developer who reads it after a +// flag deploy but not after a -f deploy is a developer who concludes the URL +// is somewhere they have not looked. +func reportManifestReachability(out io.Writer, w *computev1alpha.Workload) { + if declaredHTTPPort(w) == 0 { + notReachable(out, w.Name) + } } // saveWorkloadYAML marshals the workload and writes it to workload.yaml in the diff --git a/internal/cmd/compute/deploy/deploy_test.go b/internal/cmd/compute/deploy/deploy_test.go new file mode 100644 index 00000000..d0f20a4b --- /dev/null +++ b/internal/cmd/compute/deploy/deploy_test.go @@ -0,0 +1,475 @@ +// SPDX-License-Identifier: AGPL-3.0-only + +package deploy + +import ( + "bytes" + "context" + "errors" + "reflect" + "strings" + "testing" + + corev1 "k8s.io/api/core/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/runtime" + "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" + gatewayv1 "sigs.k8s.io/gateway-api/apis/v1" + + computev1alpha "go.datum.net/compute/api/v1alpha" + "go.datum.net/compute/internal/cmd/compute/url" + "go.datum.net/compute/internal/cmd/compute/util" + networkingv1alpha "go.datum.net/network-services-operator/api/v1alpha" +) + +const ( + testWorkload = "api" + testCanonical = "a1b2c3d4.datumproxy.net" + testImage = "ghcr.io/acme/api:1" + + imageFlag = "--image=" + testImage + httpPortFlag = "--http-port=8080" + noHTTPFlag = "--no-http" + + wantPortRange = "between 1 and 65535" +) + +// TestValidateFlags is the migration matrix: which flag combinations the +// command refuses, and — the point of the whole break — that --port is one of +// them rather than a silent alias for --http-port. +func TestValidateFlags(t *testing.T) { + tests := []struct { + name string + args []string + wantErr string + }{ + { + name: "no http flags is the old behaviour", + args: []string{testWorkload, imageFlag}, + }, + { + name: "http-port publishes", + args: []string{testWorkload, imageFlag, httpPortFlag}, + }, + { + name: "no-http alone", + args: []string{testWorkload, imageFlag, noHTTPFlag}, + }, + { + name: "port is removed, not aliased", + args: []string{testWorkload, imageFlag, "--port=8080"}, + wantErr: "--port has been replaced by --http-port", + }, + { + name: "port zero still errors", + args: []string{testWorkload, imageFlag, "--port=0"}, + wantErr: "--port has been replaced by --http-port", + }, + { + name: "http-port and no-http conflict", + args: []string{testWorkload, imageFlag, httpPortFlag, noHTTPFlag}, + wantErr: "cannot be combined", + }, + { + name: "http-port with a manifest", + args: []string{"-f", "workload.yaml", httpPortFlag}, + wantErr: "--http-port cannot be combined with -f", + }, + { + name: "no-http with a manifest", + args: []string{"-f", "workload.yaml", noHTTPFlag}, + wantErr: "--no-http cannot be combined with -f", + }, + { + name: "http-port below range", + args: []string{testWorkload, imageFlag, "--http-port=0"}, + wantErr: wantPortRange, + }, + { + name: "http-port above range", + args: []string{testWorkload, imageFlag, "--http-port=70000"}, + wantErr: wantPortRange, + }, + { + name: "http-port negative", + args: []string{testWorkload, imageFlag, "--http-port=-1"}, + wantErr: wantPortRange, + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + cmd, opts := command() + if err := cmd.Flags().Parse(tc.args); err != nil { + t.Fatalf("parsing flags: %v", err) + } + + err := validateFlags(cmd, opts) + switch { + case tc.wantErr == "" && err != nil: + t.Fatalf("want no error, got %v", err) + case tc.wantErr != "" && err == nil: + t.Fatalf("want error containing %q, got nil", tc.wantErr) + case tc.wantErr != "" && !strings.Contains(err.Error(), tc.wantErr): + t.Fatalf("want error containing %q, got %v", tc.wantErr, err) + } + }) + } +} + +// TestPortErrorReachesTheUser runs the command the way a user does. Validation +// that only exists in a helper is validation an upgrade path can skip. +func TestPortErrorReachesTheUser(t *testing.T) { + cmd := Command() + cmd.SetOut(&bytes.Buffer{}) + cmd.SetErr(&bytes.Buffer{}) + cmd.SetArgs([]string{testWorkload, imageFlag, "--port=8080"}) + + err := cmd.Execute() + if err == nil { + t.Fatal("deploying with --port must fail, not publish the workload") + } + if !errors.Is(err, errPortRenamed) { + t.Fatalf("want the migration error, got %v", err) + } + for _, want := range []string{"--http-port 8080", noHTTPFlag} { + if !strings.Contains(err.Error(), want) { + t.Errorf("migration error does not mention %q: %v", want, err.Error()) + } + } +} + +// TestPortFlagStaysRegistered guards the difference between "errors" and +// "unknown flag": the migration message only lands if the flag still parses. +func TestPortFlagStaysRegistered(t *testing.T) { + cmd := Command() + + port := cmd.Flags().Lookup("port") + if port == nil { + t.Fatal("--port must stay registered for one release so it can error") + } + if !port.Hidden { + t.Error("--port must not be advertised in help") + } + if cmd.Flags().Lookup("http-port") == nil { + t.Error("--http-port must be registered") + } + if cmd.Flags().Lookup("no-http") == nil { + t.Error("--no-http must be registered") + } + if !strings.Contains(cmd.Example, httpPortFlag) { + t.Error("the example block must show --http-port") + } + if strings.Contains(cmd.Example, "--port=8080") { + t.Error("the example block must not show --port") + } +} + +// TestDeclaredHTTPPort covers the carry-forward rule: a deploy that does not +// mention a port must not silently take a live URL down. +func TestDeclaredHTTPPort(t *testing.T) { + tcp := corev1.ProtocolTCP + tests := []struct { + name string + workload computev1alpha.Workload + want int32 + }{ + { + name: "no runtime", + workload: computev1alpha.Workload{}, + }, + { + name: "no ports", + workload: workloadWithPorts(), + }, + { + name: "the http port", + workload: workloadWithPorts(computev1alpha.NamedPort{Name: httpPortName, Port: 8080, Protocol: &tcp}), + want: 8080, + }, + { + name: "http wins over an earlier port", + workload: workloadWithPorts( + computev1alpha.NamedPort{Name: "metrics", Port: 9090}, + computev1alpha.NamedPort{Name: "http", Port: 8080}, + ), + want: 8080, + }, + { + name: "falls back to the first port", + workload: workloadWithPorts(computev1alpha.NamedPort{Name: "web", Port: 3000}), + want: 3000, + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + if got := declaredHTTPPort(&tc.workload); got != tc.want { + t.Fatalf("declaredHTTPPort = %d, want %d", got, tc.want) + } + }) + } +} + +// TestPlanLine pins the plan summary's alignment: the HTTP service line has to +// line up under the placement line the developer reads it with. +func TestPlanLine(t *testing.T) { + placement := planLine(`Placement "default"`, "cities=[DFW, IAD], min=2") + http := planLine("HTTP service", "port 8080 → Datum-managed URL") + + if want := ` Placement "default": cities=[DFW, IAD], min=2`; placement != want { + t.Errorf("placement line = %q, want %q", placement, want) + } + if want := " HTTP service: port 8080 → Datum-managed URL"; http != want { + t.Errorf("http line = %q, want %q", http, want) + } + if strings.Index(placement, "cities") != strings.Index(http, "port 8080") { + t.Errorf("plan values do not start in the same column:\n%s\n%s", placement, http) + } +} + +// TestPublishWithoutHTTPPortSaysSo covers the dead end this feature exists to +// close: a workload with no HTTP port must never leave the developer guessing +// why there is nothing to open. +func TestPublishWithoutHTTPPortSaysSo(t *testing.T) { + var out bytes.Buffer + + // A nil client is deliberate: with no port there is nothing to publish, so + // nothing may be read or written. + if err := publish(context.Background(), &out, nil, workload(), 0, &options{}, nil); err != nil { + t.Fatalf("publish without a port must not fail: %v", err) + } + + got := out.String() + for _, want := range []string{ + "No HTTP port declared — this workload is not reachable from the internet.", + "To publish it: datumctl compute deploy api --http-port 8080", + } { + if !strings.Contains(got, want) { + t.Errorf("output missing %q:\n%s", want, got) + } + } + if strings.Contains(got, "Publishing") { + t.Errorf("nothing was published, so nothing should say so:\n%s", got) + } +} + +// TestPublishPrintsTheURLLast covers the product promise: the URL is the +// deliverable, on its own line, at the end. +func TestPublishPrintsTheURLLast(t *testing.T) { + w := workload() + c := newFakeClient(t, liveProxy(w), url.BuildNetworkService(w, "http", 8080)) + + var out bytes.Buffer + if err := publish(context.Background(), &out, c, w, 8080, &options{}, nil); err != nil { + t.Fatalf("publish: %v", err) + } + + got := out.String() + if !strings.Contains(got, "Publishing...") { + t.Errorf("output missing the publishing heading:\n%s", got) + } + + lines := strings.Split(strings.TrimRight(got, "\n"), "\n") + last := lines[len(lines)-1] + if want := " https://" + testCanonical; last != want { + t.Errorf("last line = %q, want %q\nfull output:\n%s", last, want, got) + } +} + +// TestPublishFailureStillReportsTheRollout is the rule that a healthy workload +// never reads as a failed deploy: the URL is declared before the rollout, so a +// failure to declare it is carried past the rollout table and reported as what +// it is — a failure to publish something that did deploy. +func TestPublishFailureStillReportsTheRollout(t *testing.T) { + boom := errors.New("connection refused") + c := interceptor.NewClient(newFakeClient(t), interceptor.Funcs{ + Create: func(context.Context, client.WithWatch, client.Object, ...client.CreateOption) error { + return boom + }, + }) + + declareErr := declareURL(context.Background(), c, workload(), "http", 8080) + if declareErr == nil { + t.Fatal("declaring a URL against a control plane that refuses writes must fail") + } + + var out bytes.Buffer + err := publish(context.Background(), &out, c, workload(), 8080, + &options{image: testImage}, declareErr) + + if err == nil { + t.Fatal("a failure to publish must be reported as an error") + } + if !errors.Is(err, boom) { + t.Errorf("the underlying failure must not be swallowed: %v", err) + } + + got := out.String() + for _, want := range []string{ + "The rollout succeeded — the workload is deployed and running.", + "Only publishing its URL failed.", + "datumctl compute deploy api --image " + testImage + " --http-port 8080", + } { + if !strings.Contains(got, want) { + t.Errorf("output missing %q:\n%s", want, got) + } + } +} + +// TestDeclareURLWritesTheObjectsBeforeTheRollout: declaring an HTTP port +// declares a URL, and the objects go in while the workload does — silently, +// because the rollout table is the next thing the user reads. +func TestDeclareURLWritesTheObjectsBeforeTheRollout(t *testing.T) { + c := newFakeClient(t) + + if err := declareURL(context.Background(), c, workload(), "http", 8080); err != nil { + t.Fatalf("declareURL: %v", err) + } + + key := types.NamespacedName{Namespace: util.ResourceNamespace, Name: url.ResourceName(testWorkload)} + if err := c.Get(context.Background(), key, &networkingv1alpha.NetworkService{}); err != nil { + t.Errorf("backends were not declared: %v", err) + } + if err := c.Get(context.Background(), key, &networkingv1alpha.HTTPProxy{}); err != nil { + t.Errorf("the URL was not declared: %v", err) + } +} + +// A workload with no HTTP port declares no URL, so nothing may be written for +// it — least of all a proxy publishing a workload the user kept internal. +func TestDeclareURLWritesNothingWithoutAPort(t *testing.T) { + // A nil client is the assertion: any write at all would panic. + if err := declareURL(context.Background(), nil, workload(), "", 0); err != nil { + t.Fatalf("declaring nothing must not fail: %v", err) + } +} + +// TestPublishAfterTheRolloutOnlyWaits is the ordering the spec is explicit +// about: the URL objects are created alongside the workload so that backends +// register as instances come up, and the URL answers within a second or two of +// the last city reaching Done. Publishing after the rollout therefore has +// nothing left to write — a write here is proof the objects were not declared +// earlier, and proof of the stall the spec says never to add. +func TestPublishAfterTheRolloutOnlyWaits(t *testing.T) { + w := workload() + + // The declared backends are deliberately out of date, so an apply running + // at this point would have to update them and be caught doing it. + declared := newFakeClient(t, liveProxy(w), url.BuildNetworkService(w, "http", 9090)) + c := interceptor.NewClient(declared, interceptor.Funcs{ + Create: func(_ context.Context, _ client.WithWatch, obj client.Object, _ ...client.CreateOption) error { + t.Errorf("publishing wrote %T after the rollout — the URL objects belong alongside the workload", obj) + return nil + }, + Update: func(_ context.Context, _ client.WithWatch, obj client.Object, _ ...client.UpdateOption) error { + t.Errorf("publishing wrote %T after the rollout — the URL objects belong alongside the workload", obj) + return nil + }, + }) + + var out bytes.Buffer + if err := publish(context.Background(), &out, c, w, 8080, &options{}, nil); err != nil { + t.Fatalf("publish: %v", err) + } + if !strings.Contains(out.String(), "https://"+testCanonical) { + t.Errorf("publishing must still wait for and print the URL:\n%s", out.String()) + } +} + +// Ctrl-C during the rollout detaches. The URL is already declared, so +// publishing has nothing to write and only stops watching — and a detach is +// never an error. +func TestPublishDetachedIsNotAnError(t *testing.T) { + ctx, cancel := context.WithCancel(context.Background()) + cancel() + + var out bytes.Buffer + if err := publish(ctx, &out, newFakeClient(t), workload(), 8080, &options{}, nil); err != nil { + t.Fatalf("detaching is never an error: %v", err) + } + if strings.Contains(out.String(), "https://") { + t.Errorf("no URL is known yet, so none may be printed:\n%s", out.String()) + } + if !strings.Contains(out.String(), "Detached.") { + t.Errorf("a detach must say publishing carries on:\n%s", out.String()) + } +} + +// TestPublishKeepsCustomHostnames is the cross-command contract between +// `domains add` and `deploy`: publishing rewrites the proxy spec, so a +// redeploy must carry forward the hostnames the user attached. Dropping them +// would unpublish a live custom domain on the next image bump, which only +// `domains remove` is allowed to do. +func TestPublishKeepsCustomHostnames(t *testing.T) { + w := workload() + existing := liveProxy(w) + existing.Spec.Hostnames = []gatewayv1.Hostname{"api.example.com"} + c := newFakeClient(t, existing, url.BuildNetworkService(w, "http", 8080)) + + if err := declareURL(context.Background(), c, w, "http", 8080); err != nil { + t.Fatalf("declareURL: %v", err) + } + + var got networkingv1alpha.HTTPProxy + key := types.NamespacedName{Namespace: util.ResourceNamespace, Name: url.ResourceName(testWorkload)} + if err := c.Get(context.Background(), key, &got); err != nil { + t.Fatalf("reading the proxy back: %v", err) + } + + want := []gatewayv1.Hostname{"api.example.com"} + if !reflect.DeepEqual(got.Spec.Hostnames, want) { + t.Errorf("hostnames after redeploy = %v, want %v — a redeploy must not detach a custom domain", got.Spec.Hostnames, want) + } +} + +// --- fixtures --- + +func workload() *computev1alpha.Workload { + return &computev1alpha.Workload{ + ObjectMeta: metav1.ObjectMeta{ + Name: testWorkload, + Namespace: util.ResourceNamespace, + UID: types.UID("11111111-2222-3333-4444-555555555555"), + }, + } +} + +func workloadWithPorts(ports ...computev1alpha.NamedPort) computev1alpha.Workload { + w := workload() + w.Spec.Template.Spec.Runtime.Sandbox = &computev1alpha.SandboxRuntime{ + Containers: []computev1alpha.SandboxContainer{{Name: "app", Image: "ghcr.io/acme/api:1", Ports: ports}}, + } + return *w +} + +// liveProxy is the proxy as the platform reports it once the URL answers. +func liveProxy(w *computev1alpha.Workload) *networkingv1alpha.HTTPProxy { + p := url.BuildHTTPProxy(w, "http", nil) + p.Status.CanonicalHostname = testCanonical + p.Status.Conditions = []metav1.Condition{ + {Type: networkingv1alpha.HTTPProxyConditionAccepted, Status: metav1.ConditionTrue, Reason: "Accepted"}, + {Type: networkingv1alpha.HTTPProxyConditionProgrammed, Status: metav1.ConditionTrue, Reason: "Programmed"}, + {Type: networkingv1alpha.HTTPProxyConditionCertificatesReady, Status: metav1.ConditionTrue, Reason: "AllCertificatesReady"}, + } + return p +} + +func newFakeClient(t *testing.T, objs ...client.Object) client.WithWatch { + t.Helper() + s := runtime.NewScheme() + if err := computev1alpha.AddToScheme(s); err != nil { + t.Fatalf("registering compute scheme: %v", err) + } + if err := networkingv1alpha.AddToScheme(s); err != nil { + t.Fatalf("registering networking scheme: %v", err) + } + return fake.NewClientBuilder(). + WithScheme(s). + WithStatusSubresource(&networkingv1alpha.HTTPProxy{}, &networkingv1alpha.NetworkService{}). + WithObjects(objs...). + Build() +} diff --git a/internal/cmd/compute/deploy/location_selector_test.go b/internal/cmd/compute/deploy/location_selector_test.go new file mode 100644 index 00000000..21cb652c --- /dev/null +++ b/internal/cmd/compute/deploy/location_selector_test.go @@ -0,0 +1,93 @@ +// SPDX-License-Identifier: AGPL-3.0-only + +package deploy + +import ( + "strings" + "testing" +) + +const ( + testCity = "DFW" + testLocation = "us-east-1" + errMutuallyExclusive = "mutually exclusive" +) + +// TestResolveLocationSelector pins the three mutually exclusive ways a deploy +// says where to run. This validation was lifted out of deployFromFlags to keep +// it under the complexity limit, so it needs its own coverage: nothing else +// exercises it without a live control plane behind the activation gate. +func TestResolveLocationSelector(t *testing.T) { + for _, tc := range []struct { + name string + opts options + wantErr string + wantNil bool + wantMatches map[string]string + }{{ + name: "no placement flag at all", + opts: options{}, + wantErr: "--location is required", + }, { + name: "location and city together", + opts: options{locations: []string{testLocation}, cities: []string{testCity}}, + wantErr: errMutuallyExclusive, + }, { + name: "location and selector together", + opts: options{locations: []string{testLocation}, locationSelector: "a=b"}, + wantErr: errMutuallyExclusive, + }, { + name: "all three together", + opts: options{locations: []string{testLocation}, cities: []string{testCity}, locationSelector: "a=b"}, + wantErr: errMutuallyExclusive, + }, { + name: "named locations need no selector", + opts: options{locations: []string{testLocation, "eu-west-1"}}, + wantNil: true, + }, { + name: "cities become a city-code selector", + opts: options{cities: []string{testCity}}, + wantMatches: map[string]string{"topology.datum.net/city-code": testCity}, + }, { + name: "an explicit selector is parsed", + opts: options{locationSelector: "topology.datum.net/region=us-east-1"}, + wantMatches: map[string]string{"topology.datum.net/region": testLocation}, + }, { + name: "an unparseable selector is reported, not ignored", + opts: options{locationSelector: "=="}, + wantErr: "invalid --location-selector", + }} { + t.Run(tc.name, func(t *testing.T) { + got, err := resolveLocationSelector(&tc.opts) + + if tc.wantErr != "" { + if err == nil { + t.Fatalf("want an error containing %q, got selector %v", tc.wantErr, got) + } + if !strings.Contains(err.Error(), tc.wantErr) { + t.Fatalf("error = %q, want it to contain %q", err, tc.wantErr) + } + return + } + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + + if tc.wantNil { + if got != nil { + t.Errorf("selector = %v, want nil — named locations select nothing", got) + } + return + } + + if got == nil { + t.Fatal("selector is nil, want one") + } + for k, v := range tc.wantMatches { + if got.MatchLabels[k] != v { + t.Errorf("matchLabels[%q] = %q, want %q (got %v)", k, got.MatchLabels[k], v, got.MatchLabels) + } + } + }) + } +} diff --git a/internal/cmd/compute/deploy/nohttp_test.go b/internal/cmd/compute/deploy/nohttp_test.go new file mode 100644 index 00000000..04cc271e --- /dev/null +++ b/internal/cmd/compute/deploy/nohttp_test.go @@ -0,0 +1,434 @@ +// SPDX-License-Identifier: AGPL-3.0-only + +package deploy + +import ( + "bytes" + "context" + "errors" + "strings" + "testing" + + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/client/interceptor" + gatewayv1 "sigs.k8s.io/gateway-api/apis/v1" + + computev1alpha "go.datum.net/compute/api/v1alpha" + "go.datum.net/compute/internal/cmd/compute/url" + "go.datum.net/compute/internal/cmd/compute/util" + networkingv1alpha "go.datum.net/network-services-operator/api/v1alpha" +) + +const testCustom = "api.example.com" + +func publishedClient(t *testing.T, hostnames ...string) client.WithWatch { + t.Helper() + w := workload() + proxy := liveProxy(w) + for _, h := range hostnames { + proxy.Spec.Hostnames = append(proxy.Spec.Hostnames, gatewayv1.Hostname(h)) + // A custom hostname only serves on the strength of its own entry here. + // Without one the platform has not checked it yet, and a hostname + // nobody has verified is not the URL to name in a plan. + proxy.Status.HostnameStatuses = append(proxy.Status.HostnameStatuses, + networkingv1alpha.HostnameStatus{ + Hostname: h, + Conditions: []metav1.Condition{ + {Type: networkingv1alpha.HostnameConditionVerified, Status: metav1.ConditionTrue, Reason: "Verified"}, + {Type: networkingv1alpha.HostnameConditionDNSRecordProgrammed, Status: metav1.ConditionTrue, Reason: "RecordCreated"}, + {Type: networkingv1alpha.HostnameConditionCertificateReady, Status: metav1.ConditionTrue, Reason: "CertificateIssued"}, + }, + }) + } + return newFakeClient(t, proxy, url.BuildNetworkService(w, "http", 8080)) +} + +func published(t *testing.T, c client.Client, obj client.Object) bool { + t.Helper() + return c.Get(context.Background(), client.ObjectKey{ + Namespace: util.ResourceNamespace, Name: url.ResourceName(testWorkload), + }, obj) == nil +} + +// TestPlanHTTPServiceStatesTheConsequence: --no-http takes a URL down, and the +// plan summary printed before the Apply prompt has to say which URL, by name. +// A user cannot consent to something the prompt does not mention. +func TestPlanHTTPServiceStatesTheConsequence(t *testing.T) { + tests := []struct { + name string + client func(t *testing.T) client.WithWatch + httpPort int32 + opts *options + creating bool + wantLine string + wantMissing string + wantRemoving string + }{ + { + name: "publishing names the port and says a URL is coming", + client: func(t *testing.T) client.WithWatch { return newFakeClient(t) }, + httpPort: 8080, + opts: &options{httpPort: 8080}, + wantLine: "HTTP service: port 8080 → Datum-managed URL", + }, + { + name: "removing names the URL that stops answering", + client: func(t *testing.T) client.WithWatch { return publishedClient(t) }, + opts: &options{noHTTP: true}, + wantLine: "HTTP service: removed — https://" + testCanonical + " will stop responding", + wantRemoving: "https://" + testCanonical, + }, + { + name: "the custom hostname is the one named, since it is the one people use", + client: func(t *testing.T) client.WithWatch { return publishedClient(t, testCustom) }, + opts: &options{noHTTP: true}, + wantLine: "https://" + testCustom + " will stop responding", + wantRemoving: "https://" + testCustom, + }, + { + name: "removing something that was never published says so plainly", + client: func(t *testing.T) client.WithWatch { return newFakeClient(t) }, + opts: &options{noHTTP: true}, + wantLine: "HTTP service: removed", + }, + { + name: "a workload being created has no URL to lose", + client: func(t *testing.T) client.WithWatch { return newFakeClient(t) }, + opts: &options{noHTTP: true}, + creating: true, + wantMissing: "HTTP service", + }, + { + name: "no HTTP flags at all is not an HTTP plan", + client: func(t *testing.T) client.WithWatch { return publishedClient(t) }, + opts: &options{}, + wantMissing: "HTTP service", + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + var out bytes.Buffer + got := planHTTPService(context.Background(), &out, tc.client(t), testWorkload, tc.httpPort, tc.opts, tc.creating) + + if tc.wantLine != "" && !strings.Contains(out.String(), tc.wantLine) { + t.Errorf("plan missing %q:\n%s", tc.wantLine, out.String()) + } + if tc.wantMissing != "" && strings.Contains(out.String(), tc.wantMissing) { + t.Errorf("plan should not mention %q:\n%s", tc.wantMissing, out.String()) + } + if got != tc.wantRemoving { + t.Errorf("removed URL = %q, want %q", got, tc.wantRemoving) + } + }) + } +} + +// A control plane that cannot be read must not stop a deploy at the plan +// stage: the line loses the hostname it would have named and nothing else. +func TestPlanHTTPServiceSurvivesAnUnreadableControlPlane(t *testing.T) { + c := interceptor.NewClient(publishedClient(t), interceptor.Funcs{ + List: func(context.Context, client.WithWatch, client.ObjectList, ...client.ListOption) error { + return errors.New("connection refused") + }, + }) + + var out bytes.Buffer + got := planHTTPService(context.Background(), &out, c, testWorkload, 0, &options{noHTTP: true}, false) + + if got != "" { + t.Errorf("removed URL = %q, want none: nothing was read", got) + } + if !strings.Contains(out.String(), "HTTP service: removed") { + t.Errorf("the plan must still state the removal:\n%s", out.String()) + } + if strings.Contains(out.String(), "will stop responding") { + t.Errorf("no URL is known, so none may be named:\n%s", out.String()) + } +} + +// TestRemoveHTTPServiceTakesBothObjectsDown is the whole point of --no-http: +// the workload stops declaring the port, and what answered for it goes with +// it. Leaving either object behind leaves a URL routing to a workload that no +// longer serves it. +func TestRemoveHTTPServiceTakesBothObjectsDown(t *testing.T) { + c := publishedClient(t, testCustom) + + var out bytes.Buffer + removedURL := "https://" + testCustom + if err := removeHTTPService(context.Background(), &out, c, testWorkload, removedURL, false); err != nil { + t.Fatalf("removeHTTPService: %v", err) + } + + if published(t, c, &networkingv1alpha.HTTPProxy{}) { + t.Error("the URL survived --no-http") + } + if published(t, c, &networkingv1alpha.NetworkService{}) { + t.Error("the URL backends survived --no-http") + } + if want := "HTTP service removed — " + removedURL + " no longer responds"; !strings.Contains(out.String(), want) { + t.Errorf("output missing %q:\n%s", want, out.String()) + } +} + +// Removing an HTTP service that was never there is a no-op with nothing to +// report — but only on a create. On an existing workload the user asked for +// something, so they are told it happened. +func TestRemoveHTTPServiceSaysNothingOnACreate(t *testing.T) { + c := newFakeClient(t) + + var creating bytes.Buffer + if err := removeHTTPService(context.Background(), &creating, c, testWorkload, "", true); err != nil { + t.Fatalf("removeHTTPService: %v", err) + } + if creating.Len() != 0 { + t.Errorf("a new workload never had an HTTP service, so nothing may be reported:\n%s", creating.String()) + } + + var existing bytes.Buffer + if err := removeHTTPService(context.Background(), &existing, c, testWorkload, "", false); err != nil { + t.Fatalf("removeHTTPService: %v", err) + } + if !strings.Contains(existing.String(), "HTTP service removed") { + t.Errorf("an existing workload's removal must be reported:\n%s", existing.String()) + } +} + +// A URL that could not be taken down is a failed deploy, not a warning: the +// workload has already been updated to stop serving, so a URL still routing to +// it is a live inconsistency the user has to know about. +func TestRemoveHTTPServiceReportsAFailure(t *testing.T) { + boom := errors.New("forbidden") + c := interceptor.NewClient(publishedClient(t), interceptor.Funcs{ + DeleteAllOf: func(context.Context, client.WithWatch, client.Object, ...client.DeleteAllOfOption) error { + return boom + }, + }) + + var out bytes.Buffer + err := removeHTTPService(context.Background(), &out, c, testWorkload, "https://"+testCanonical, false) + if err == nil { + t.Fatal("a URL that could not be removed must fail the deploy") + } + if !errors.Is(err, boom) { + t.Errorf("error = %v, want the server's failure", err) + } + if strings.Contains(out.String(), "no longer responds") { + t.Errorf("nothing was removed, so nothing may claim it was:\n%s", out.String()) + } +} + +// TestPublishKeepsCustomHostnamesWhenTheSpecChanges is the carry-forward test +// with the short circuit removed: changing the port forces the apply through +// the update path that rewrites the proxy spec, which is where a dropped +// hostname would actually be lost. Republishing an unchanged workload writes +// nothing at all, so it cannot prove this on its own. +func TestPublishKeepsCustomHostnamesWhenTheSpecChanges(t *testing.T) { + c := publishedClient(t, testCustom, "www.example.com") + + if err := declareURL(context.Background(), c, workload(), "http", 9090); err != nil { + t.Fatalf("declareURL: %v", err) + } + + var proxy networkingv1alpha.HTTPProxy + key := client.ObjectKey{Namespace: util.ResourceNamespace, Name: url.ResourceName(testWorkload)} + if err := c.Get(context.Background(), key, &proxy); err != nil { + t.Fatalf("reading the proxy back: %v", err) + } + + got := make([]string, 0, len(proxy.Spec.Hostnames)) + for _, h := range proxy.Spec.Hostnames { + got = append(got, string(h)) + } + if want := testCustom + "," + "www.example.com"; strings.Join(got, ",") != want { + t.Errorf("hostnames after a port change = %v, want %q — a redeploy must not detach a custom domain", got, want) + } + + // And the port change did land, so the test is exercising the update path. + var svc networkingv1alpha.NetworkService + if err := c.Get(context.Background(), key, &svc); err != nil { + t.Fatalf("reading the backends back: %v", err) + } + if svc.Spec.Ports[0].Port != 9090 { + t.Fatalf("port = %d, want the new port — the update path was not exercised", svc.Spec.Ports[0].Port) + } +} + +// TestExistingHostnames pins what the carry-forward reads, including the two +// cases where it deliberately reports none. +func TestExistingHostnames(t *testing.T) { + t.Run("attached hostnames are carried forward in declared order", func(t *testing.T) { + c := publishedClient(t, testCustom, "www.example.com") + got, err := existingHostnames(context.Background(), c, testWorkload) + if err != nil { + t.Fatalf("existingHostnames: %v", err) + } + if want := testCustom + ",www.example.com"; strings.Join(got, ",") != want { + t.Errorf("existingHostnames = %v, want %q", got, want) + } + }) + + t.Run("a workload that was never published has none", func(t *testing.T) { + got, err := existingHostnames(context.Background(), newFakeClient(t), testWorkload) + if err != nil { + t.Fatalf("a workload with no URL is not an error: %v", err) + } + if got != nil { + t.Errorf("existingHostnames = %v, want nil", got) + } + }) + + t.Run("an unreadable control plane fails closed", func(t *testing.T) { + boom := errors.New("connection refused") + c := interceptor.NewClient(publishedClient(t, testCustom), interceptor.Funcs{ + List: func(context.Context, client.WithWatch, client.ObjectList, ...client.ListOption) error { + return boom + }, + }) + got, err := existingHostnames(context.Background(), c, testWorkload) + if err == nil { + t.Fatal("a control plane that cannot be read must not report \"no custom domains\"") + } + if !errors.Is(err, boom) { + t.Errorf("error = %v, want the server's failure", err) + } + if got != nil { + t.Errorf("existingHostnames = %v, want nil alongside the error", got) + } + }) +} + +// TestDeclareURLDoesNotDetachHostnamesWhenTheLookupFails is the hole left in +// the carry-forward: existingHostnames reported none when it could not read the +// URL, and declaring then rewrote the proxy spec with none. The two calls use +// different verbs on the same object — a List for the carry-forward, a Get for +// the apply — so a control plane that answers one and refuses the other would +// detach every custom domain on the next deploy, while the deploy reported +// success. +// +// A missing list permission on httpproxies is the everyday shape of that. +func TestDeclareURLDoesNotDetachHostnamesWhenTheLookupFails(t *testing.T) { + boom := errors.New("httpproxies.networking.datumapis.com is forbidden") + c := interceptor.NewClient(publishedClient(t, testCustom), interceptor.Funcs{ + List: func(context.Context, client.WithWatch, client.ObjectList, ...client.ListOption) error { + return boom + }, + }) + + err := declareURL(context.Background(), c, workload(), "http", 8080) + if err == nil { + t.Fatal("a deploy that could not read the attached domains must fail before it rewrites them") + } + if !errors.Is(err, boom) { + t.Errorf("error = %v, want the server's failure", err) + } + + var proxy networkingv1alpha.HTTPProxy + key := client.ObjectKey{Namespace: util.ResourceNamespace, Name: url.ResourceName(testWorkload)} + if err := c.Get(context.Background(), key, &proxy); err != nil { + t.Fatalf("reading the proxy back: %v", err) + } + if len(proxy.Spec.Hostnames) != 1 || string(proxy.Spec.Hostnames[0]) != testCustom { + t.Errorf("hostnames = %v, want %q kept — a deploy that could not read the URL must not detach a domain", + proxy.Spec.Hostnames, testCustom) + } +} + +// And the failure reaches the user as a failure to publish, after the rollout +// table, rather than as a silent success with a detached domain. +func TestPublishReportsACarryForwardFailure(t *testing.T) { + boom := errors.New("httpproxies.networking.datumapis.com is forbidden") + c := interceptor.NewClient(publishedClient(t, testCustom), interceptor.Funcs{ + List: func(context.Context, client.WithWatch, client.ObjectList, ...client.ListOption) error { + return boom + }, + }) + + declareErr := declareURL(context.Background(), c, workload(), "http", 8080) + + var out bytes.Buffer + err := publish(context.Background(), &out, c, workload(), 8080, &options{image: testImage}, declareErr) + if err == nil { + t.Fatal("a URL that was never declared must not be reported as published") + } + if !errors.Is(err, boom) { + t.Errorf("error = %v, want the server's failure", err) + } + if !strings.Contains(out.String(), "The rollout succeeded") { + t.Errorf("a running workload must not read as a failed deploy:\n%s", out.String()) + } +} + +// TestPublishSaysNothingMoreAfterNoHTTP: --no-http has just told the user, by +// name, that their URL is gone. Following that with "this workload is not +// reachable from the internet — to publish it..." answers a question nobody +// asked, and reads as though the removal were a mistake. +func TestPublishSaysNothingMoreAfterNoHTTP(t *testing.T) { + var out bytes.Buffer + if err := publish(context.Background(), &out, nil, workload(), 0, &options{noHTTP: true}, nil); err != nil { + t.Fatalf("publish: %v", err) + } + if out.Len() != 0 { + t.Errorf("--no-http was already reported; nothing more may be said:\n%s", out.String()) + } +} + +// TestManifestDeployReportsTheDeadEnd: the spec guarantees the not-reachable +// note for any workload with no HTTP port, and a manifest deploy is no +// exception. Printing it only on the flag path leaves a -f user to conclude +// the URL is somewhere they have not looked. +func TestManifestDeployReportsTheDeadEnd(t *testing.T) { + t.Run("no port declared", func(t *testing.T) { + w := workloadWithPorts() + + var out bytes.Buffer + reportManifestReachability(&out, &w) + + for _, want := range []string{ + "No HTTP port declared — this workload is not reachable from the internet.", + "To publish it: datumctl compute deploy api --http-port 8080", + } { + if !strings.Contains(out.String(), want) { + t.Errorf("output missing %q:\n%s", want, out.String()) + } + } + }) + + t.Run("a declared http port is not a dead end", func(t *testing.T) { + w := workloadWithPorts(computev1alpha.NamedPort{Name: httpPortName, Port: 8080}) + + var out bytes.Buffer + reportManifestReachability(&out, &w) + + if out.Len() != 0 { + t.Errorf("a workload that declares an HTTP port is not unreachable:\n%s", out.String()) + } + }) +} + +// TestPlanWarnsThatTheEdgeSpeaksPlaintext: the edge reaches instances over +// plaintext inside the network, so a container terminating TLS itself answers +// nothing. The only symptom is a URL that does not work, which is why this is +// said before the Apply prompt rather than left to be discovered. +func TestPlanWarnsThatTheEdgeSpeaksPlaintext(t *testing.T) { + var out bytes.Buffer + planHTTPService(context.Background(), &out, newFakeClient(t), testWorkload, 8080, &options{httpPort: 8080}, true) + + got := out.String() + if !strings.Contains(got, "Datum terminates TLS; serve plain HTTP on this port.") { + t.Errorf("the plan must say the edge reaches the container over plaintext:\n%s", got) + } + for _, machinery := range []string{"NetworkService", "HTTPProxy"} { + if strings.Contains(got, machinery) { + t.Errorf("the plan names the machinery %q:\n%s", machinery, got) + } + } + + // And it is not said to a workload that publishes nothing. + var internal bytes.Buffer + planHTTPService(context.Background(), &internal, newFakeClient(t), testWorkload, 0, &options{}, false) + if strings.Contains(internal.String(), "TLS") { + t.Errorf("nothing is being published, so TLS is not the user's problem:\n%s", internal.String()) + } +} diff --git a/internal/cmd/compute/destroy/destroy.go b/internal/cmd/compute/destroy/destroy.go index bc5b1b00..d7d8d55d 100644 --- a/internal/cmd/compute/destroy/destroy.go +++ b/internal/cmd/compute/destroy/destroy.go @@ -4,25 +4,52 @@ import ( "bufio" "context" "fmt" + "io" "os" "strings" "github.com/spf13/cobra" "golang.org/x/term" k8serrors "k8s.io/apimachinery/pkg/api/errors" + "k8s.io/apimachinery/pkg/api/meta" + "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/types" + "sigs.k8s.io/controller-runtime/pkg/client" computev1alpha "go.datum.net/compute/api/v1alpha" + "go.datum.net/compute/internal/cmd/compute/url" "go.datum.net/compute/internal/cmd/compute/util" + networkingv1alpha "go.datum.net/network-services-operator/api/v1alpha" ) +// destroyPrompt states every consequence of the command, including the one a +// user is most likely to have forgotten: the workload's URL stops answering. +const destroyPrompt = "This will delete the workload, all its instances, and its URLs. Continue? (y/N): " + +// leftoverPrompt is for the second run of a destroy whose first run deleted +// the workload but could not delete its URL. +const leftoverPrompt = "This will delete the URLs left behind by %s. Continue? (y/N): " + +// summaryLabel keeps the summary block's values in one column. +const summaryLabel = 14 + +// leftoverBackendsDescription names what is left when a partial delete removed +// the URL and not the backends behind it. There is no hostname left to show, +// and the machinery is never named, so this is the plainest true thing to say. +const leftoverBackendsDescription = "URL backends from an unfinished destroy" + func Command() *cobra.Command { var yes bool cmd := &cobra.Command{ Use: "destroy ", - Short: "Delete a workload and all its instances", - Args: cobra.ExactArgs(1), + Short: "Delete a workload, all its instances, and its URLs", + Long: `Delete a workload and everything that serves it: its instances and the URLs it +answers on. + +Custom domains are not deleted. A verified domain is a project asset that +outlives any one workload.`, + Args: cobra.ExactArgs(1), RunE: func(cmd *cobra.Command, args []string) error { return runDestroy(cmd, args, yes) }, @@ -42,18 +69,131 @@ func runDestroy(cmd *cobra.Command, args []string, yes bool) error { return err } - ctx := context.Background() - workloadName := args[0] + return destroyWorkload(context.Background(), cmd.OutOrStdout(), cmd.ErrOrStderr(), c, project, args[0], yes) +} +// destroyWorkload deletes the workload and then the objects that put it on a +// URL. The client is a parameter so the whole flow is testable against a fake +// one. +func destroyWorkload(ctx context.Context, out, errOut io.Writer, c client.Client, project, workloadName string, yes bool) error { var workload computev1alpha.Workload - if err := c.Get(ctx, types.NamespacedName{Namespace: util.ResourceNamespace, Name: workloadName}, &workload); err != nil { - if k8serrors.IsNotFound(err) { - return fmt.Errorf("workload %q not found in project %s", workloadName, project) - } + err := c.Get(ctx, types.NamespacedName{Namespace: util.ResourceNamespace, Name: workloadName}, &workload) + switch { + case err == nil: + case k8serrors.IsNotFound(err): + // The workload is gone. Its URL may not be — a previous destroy can + // have deleted the workload and failed on the URL, and this is the + // command that run told the user to repeat. + return destroyLeftoverURLs(ctx, out, errOut, c, project, workloadName, yes) + default: return fmt.Errorf("getting workload: %w", err) } - // Summarize placements. + // The URL is read before anything is deleted: the summary has to be able + // to name what will stop answering. + info, urlErr := url.ForWorkload(ctx, c, workloadName) + if urlErr != nil { + fmt.Fprintf(errOut, "Warning: could not read URLs for %q: %v\n", workloadName, urlErr) + } + + printSummary(out, &workload, info) + + confirmed, err := confirm(out, yes, destroyPrompt) + if err != nil { + return err + } + if !confirmed { + fmt.Fprintln(out, "Aborted.") + return nil + } + + if err := c.Delete(ctx, &workload); err != nil { + return fmt.Errorf("deleting workload: %w", err) + } + fmt.Fprintf(out, "workload/%s deleted.\n", workloadName) + + // The URL objects are deleted explicitly rather than left to + // owner-reference garbage collection, which a project control plane does + // not guarantee. A failure here fails the command: the destroy was asked to + // stop the URLs answering and it did not, and a script reading exit 0 would + // carry on believing otherwise. The message still says the workload is + // gone, because it is. + if err := url.Unpublish(ctx, c, workloadName); err != nil { + reportLeftoverURLs(errOut, workloadName, info) + return err + } + + return nil +} + +// destroyLeftoverURLs handles a workload that is already gone. When it left no +// URL behind there is nothing to do and the workload really is missing; when +// it did, this cleans it up rather than making the user reach for the API. +func destroyLeftoverURLs(ctx context.Context, out, errOut io.Writer, c client.Client, project, workloadName string, yes bool) error { + info, err := url.ForWorkload(ctx, c, workloadName) + if err != nil { + fmt.Fprintf(errOut, "Warning: could not read URLs for %q: %v\n", workloadName, err) + } + + // A partial delete can remove the URL and leave its backends: the lookup + // keys on the URL, so it reports nothing while there is still something + // there. Asking for the backends directly is what makes the leftovers of + // every partial delete reachable — without it the user is left holding + // objects no command can remove. + backends, backendsErr := leftoverBackends(ctx, c, workloadName) + + // Fail closed. Telling a user there is nothing to clean up is a claim, and a + // read that failed is not evidence for it — the leftovers this command + // exists to remove would be exactly what went unseen. Same choice as + // deploy's existingHostnames, which fails closed rather than detaching + // domains it could not read. + if backendsErr != nil { + return fmt.Errorf("checking for leftover URL resources of %q: %w", workloadName, backendsErr) + } + if err != nil && !backends { + return fmt.Errorf("checking for leftover URLs of %q: %w", workloadName, err) + } + + if info == nil && !backends { + return fmt.Errorf("workload %q not found in project %s", workloadName, project) + } + + // With the URL itself already gone there is no hostname left to name, so + // the summary and the closing line both fall back to what does remain. + urls := hostnameURLs(info) + + fmt.Fprintf(out, "%-*s %s (already deleted)\n", summaryLabel, "Workload:", workloadName) + if len(urls) > 0 { + printURLs(out, info) + } else { + fmt.Fprintf(out, "%-*s %s\n", summaryLabel, "Leftovers:", leftoverBackendsDescription) + } + fmt.Fprintln(out) + + confirmed, err := confirm(out, yes, fmt.Sprintf(leftoverPrompt, workloadName)) + if err != nil { + return err + } + if !confirmed { + fmt.Fprintln(out, "Aborted.") + return nil + } + + // Nothing else is being deleted here, so a failure is the command failing. + if err := url.Unpublish(ctx, c, workloadName); err != nil { + return err + } + + if len(urls) > 0 { + fmt.Fprintf(out, "URLs for %s deleted.\n", workloadName) + } else { + fmt.Fprintf(out, "Leftover URL backends for %s deleted.\n", workloadName) + } + return nil +} + +// printSummary states what is about to be deleted, in the user's terms. +func printSummary(out io.Writer, workload *computev1alpha.Workload, info *url.Info) { var allLocations []string var totalMin int32 for _, p := range workload.Spec.Placements { @@ -63,32 +203,99 @@ func runDestroy(cmd *cobra.Command, args []string, yes bool) error { totalMin += p.ScaleSettings.MinReplicas } - out := cmd.OutOrStdout() - fmt.Fprintf(out, "Workload: %s\nPlacements: %d Locations: %s\nMin replicas: %d\n\n", - workloadName, - len(workload.Spec.Placements), - strings.Join(allLocations, ", "), - totalMin, - ) - - // Prompt unless --yes or non-interactive. - if !yes && term.IsTerminal(int(os.Stdin.Fd())) { - _, _ = fmt.Fprint(out, "This will delete workload and all its instances. Continue? (y/N): ") - line, err := bufio.NewReader(os.Stdin).ReadString('\n') - if err != nil { - return fmt.Errorf("reading confirmation: %w", err) - } - line = strings.TrimSpace(line) - if line != "y" && line != "Y" { - _, _ = fmt.Fprintln(out, "Aborted.") - return nil - } + fmt.Fprintf(out, "%-*s %s\n", summaryLabel, "Workload:", workload.Name) + fmt.Fprintf(out, "%-*s %d Locations: %s\n", summaryLabel, "Placements:", + len(workload.Spec.Placements), strings.Join(allLocations, ", ")) + fmt.Fprintf(out, "%-*s %d\n", summaryLabel, "Min replicas:", totalMin) + printURLs(out, info) + fmt.Fprintln(out) +} + +// leftoverBackends reports whether the URL backends published for a workload +// are still in the project. It is asked only about a workload that is already +// gone, where anything still labelled with its name is debris from a destroy +// that did not finish. +// +// A control plane that does not serve the kind at all has nothing left over: +// that is an empty project, not a failure to report. +func leftoverBackends(ctx context.Context, c client.Client, workloadName string) (bool, error) { + var services networkingv1alpha.NetworkServiceList + err := c.List(ctx, &services, + client.InNamespace(util.ResourceNamespace), + client.MatchingLabels{computev1alpha.WorkloadNameLabel: workloadName}) + switch { + case err == nil: + return len(services.Items) > 0, nil + case notServed(err): + return false, nil + default: + return false, err } +} - if err := c.Delete(ctx, &workload); err != nil { - return fmt.Errorf("deleting workload: %w", err) +// notServed reports whether a list error means the control plane does not +// serve this kind, rather than that the read failed. +func notServed(err error) bool { + return k8serrors.IsNotFound(err) || + meta.IsNoMatchError(err) || + runtime.IsNotRegisteredError(err) +} + +// printURLs adds the URLs line, when there is one. A workload with no HTTP +// port has no line at all rather than a line saying so: the summary lists what +// is being deleted. +func printURLs(out io.Writer, info *url.Info) { + urls := hostnameURLs(info) + if len(urls) == 0 { + return } + fmt.Fprintf(out, "%-*s %s\n", summaryLabel, "URLs:", strings.Join(urls, ", ")) +} - fmt.Fprintf(out, "workload/%s deleted.\n", workloadName) - return nil +// hostnameURLs lists every URL the workload answers on, custom hostnames +// first, the platform-managed one last, as the url package orders them. +func hostnameURLs(info *url.Info) []string { + if info == nil { + return nil + } + urls := make([]string, 0, len(info.Hostnames)) + for _, h := range info.Hostnames { + urls = append(urls, h.URL) + } + if len(urls) == 0 && info.URL != "" { + urls = append(urls, info.URL) + } + return urls +} + +// reportLeftoverURLs explains a URL that outlived its workload. It names +// exactly what is still answering and the command that finishes the job, +// because a URL still serving traffic for a workload the user believes they +// deleted is the worst possible way to be quiet. +// +// The cause is not repeated here: the caller returns it, and it is printed as +// the command's error immediately after this block. +func reportLeftoverURLs(errOut io.Writer, workloadName string, info *url.Info) { + fmt.Fprintf(errOut, "\nThe workload was deleted, but its URLs were not.\n") + for _, u := range hostnameURLs(info) { + fmt.Fprintf(errOut, " %s may keep answering.\n", u) + } + fmt.Fprintf(errOut, " Run 'datumctl compute destroy %s' again to remove them.\n", workloadName) +} + +// confirm asks the question and reports whether the user said yes. --yes skips +// it; so does a non-interactive run, which is the behaviour this command has +// always had. +func confirm(out io.Writer, yes bool, question string) (bool, error) { + if yes || !term.IsTerminal(int(os.Stdin.Fd())) { + return true, nil + } + + fmt.Fprint(out, question) + line, err := bufio.NewReader(os.Stdin).ReadString('\n') + if err != nil { + return false, fmt.Errorf("reading confirmation: %w", err) + } + line = strings.TrimSpace(line) + return line == "y" || line == "Y", nil } diff --git a/internal/cmd/compute/destroy/destroy_test.go b/internal/cmd/compute/destroy/destroy_test.go new file mode 100644 index 00000000..8c40cb84 --- /dev/null +++ b/internal/cmd/compute/destroy/destroy_test.go @@ -0,0 +1,324 @@ +// SPDX-License-Identifier: AGPL-3.0-only + +package destroy + +import ( + "bytes" + "context" + "errors" + "strings" + "testing" + + k8serrors "k8s.io/apimachinery/pkg/api/errors" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/runtime" + "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" + gatewayv1 "sigs.k8s.io/gateway-api/apis/v1" + + locationsv1alpha1 "go.miloapis.com/locations/api/v1alpha1" + + computev1alpha "go.datum.net/compute/api/v1alpha" + "go.datum.net/compute/internal/cmd/compute/url" + "go.datum.net/compute/internal/cmd/compute/util" + networkingv1alpha "go.datum.net/network-services-operator/api/v1alpha" +) + +const ( + testProject = "acme-prod" + testWorkload = "api" + testCanonical = "a1b2c3d4.datumproxy.net" + testCustom = "api.example.com" +) + +func testScheme(t *testing.T) *runtime.Scheme { + t.Helper() + s := runtime.NewScheme() + if err := computev1alpha.AddToScheme(s); err != nil { + t.Fatalf("registering compute scheme: %v", err) + } + if err := networkingv1alpha.AddToScheme(s); err != nil { + t.Fatalf("registering networking scheme: %v", err) + } + return s +} + +func testWorkloadObject() *computev1alpha.Workload { + minReplicas := int32(2) + return &computev1alpha.Workload{ + ObjectMeta: metav1.ObjectMeta{ + Name: testWorkload, + Namespace: util.ResourceNamespace, + UID: types.UID("uid-api"), + }, + Spec: computev1alpha.WorkloadSpec{ + Placements: []computev1alpha.WorkloadPlacement{{ + Name: "default", + Locations: []locationsv1alpha1.LocationReference{{Name: "us-east-1"}, {Name: "eu-west-1"}}, + ScaleSettings: computev1alpha.HorizontalScaleSettings{MinReplicas: minReplicas}, + }}, + }, + } +} + +// publishedProxy is the proxy the platform reports for a live URL. +func publishedProxy(customHostnames ...string) *networkingv1alpha.HTTPProxy { + hostnames := make([]gatewayv1.Hostname, 0, len(customHostnames)) + for _, h := range customHostnames { + hostnames = append(hostnames, gatewayv1.Hostname(h)) + } + return &networkingv1alpha.HTTPProxy{ + ObjectMeta: metav1.ObjectMeta{ + Name: testWorkload, + Namespace: util.ResourceNamespace, + Labels: map[string]string{computev1alpha.WorkloadNameLabel: testWorkload}, + }, + Spec: networkingv1alpha.HTTPProxySpec{Hostnames: hostnames}, + Status: networkingv1alpha.HTTPProxyStatus{ + CanonicalHostname: testCanonical, + Conditions: []metav1.Condition{ + {Type: networkingv1alpha.HTTPProxyConditionProgrammed, Status: metav1.ConditionTrue, Reason: "Programmed"}, + {Type: networkingv1alpha.HTTPProxyConditionCertificatesReady, Status: metav1.ConditionTrue, Reason: "Issued"}, + }, + }, + } +} + +func publishedService() *networkingv1alpha.NetworkService { + return &networkingv1alpha.NetworkService{ + ObjectMeta: metav1.ObjectMeta{ + Name: testWorkload, + Namespace: util.ResourceNamespace, + Labels: map[string]string{computev1alpha.WorkloadNameLabel: testWorkload}, + }, + Spec: networkingv1alpha.NetworkServiceSpec{ + Ports: []networkingv1alpha.NetworkServicePort{{Name: "http", Port: 8080}}, + }, + } +} + +func newFakeClient(t *testing.T, objs ...client.Object) client.WithWatch { + t.Helper() + return fake.NewClientBuilder().WithScheme(testScheme(t)).WithObjects(objs...).Build() +} + +func exists(t *testing.T, c client.Client, obj client.Object) bool { + t.Helper() + err := c.Get(context.Background(), types.NamespacedName{Namespace: util.ResourceNamespace, Name: testWorkload}, obj) + if err == nil { + return true + } + if k8serrors.IsNotFound(err) { + return false + } + t.Fatalf("get: %v", err) + return false +} + +// TestDestroyPrompt pins the sentence the user is asked to agree to. It has to +// name the URLs: a workload's URL is the part of a destroy a user is most +// likely not to have thought about. +func TestDestroyPrompt(t *testing.T) { + const want = "This will delete the workload, all its instances, and its URLs. Continue? (y/N): " + if destroyPrompt != want { + t.Errorf("destroyPrompt = %q, want %q", destroyPrompt, want) + } +} + +func TestDestroySummary(t *testing.T) { + tests := []struct { + name string + objs []client.Object + wantLines []string + wantMissing []string + }{ + { + name: "published workload lists every URL it answers on", + objs: []client.Object{testWorkloadObject(), publishedProxy(testCustom), publishedService()}, + wantLines: []string{ + "Workload: " + testWorkload, + "Placements: 1 Locations: us-east-1, eu-west-1", + "Min replicas: 2", + "URLs: https://" + testCustom + ", https://" + testCanonical, + "workload/api deleted.", + }, + }, + { + name: "workload with no URL has no URLs line", + objs: []client.Object{testWorkloadObject()}, + wantLines: []string{"Workload: " + testWorkload, "workload/api deleted."}, + wantMissing: []string{"URLs:"}, + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + var out, errOut bytes.Buffer + c := newFakeClient(t, tc.objs...) + + if err := destroyWorkload(context.Background(), &out, &errOut, c, testProject, testWorkload, true); err != nil { + t.Fatalf("destroyWorkload: %v", err) + } + + got := out.String() + for _, want := range tc.wantLines { + if !strings.Contains(got, want) { + t.Errorf("output missing %q:\n%s", want, got) + } + } + for _, missing := range tc.wantMissing { + if strings.Contains(got, missing) { + t.Errorf("output should not contain %q:\n%s", missing, got) + } + } + if errOut.Len() != 0 { + t.Errorf("unexpected stderr: %s", errOut.String()) + } + }) + } +} + +// TestDestroyUnpublishes: the URL objects are deleted explicitly, not left to +// owner-reference garbage collection. +func TestDestroyUnpublishes(t *testing.T) { + c := newFakeClient(t, testWorkloadObject(), publishedProxy(testCustom), publishedService()) + + var out, errOut bytes.Buffer + if err := destroyWorkload(context.Background(), &out, &errOut, c, testProject, testWorkload, true); err != nil { + t.Fatalf("destroyWorkload: %v", err) + } + + if exists(t, c, &computev1alpha.Workload{}) { + t.Error("workload survived destroy") + } + if exists(t, c, &networkingv1alpha.HTTPProxy{}) { + t.Error("URL survived destroy") + } + if exists(t, c, &networkingv1alpha.NetworkService{}) { + t.Error("URL backends survived destroy") + } +} + +// TestDestroyLeftoverURLFails: a URL that could not be deleted names exactly +// what is still answering and how to finish the job — and fails the command. +// The workload really is gone, but a script that reads exit 0 here would carry +// on believing the URLs stopped answering when they may not have. +func TestDestroyLeftoverURLFails(t *testing.T) { + boom := errors.New("forbidden") + c := fake.NewClientBuilder(). + WithScheme(testScheme(t)). + WithObjects(testWorkloadObject(), publishedProxy(testCustom), publishedService()). + WithInterceptorFuncs(interceptor.Funcs{ + DeleteAllOf: func(ctx context.Context, cl client.WithWatch, obj client.Object, opts ...client.DeleteAllOfOption) error { + if _, ok := obj.(*networkingv1alpha.HTTPProxy); ok { + return boom + } + return cl.DeleteAllOf(ctx, obj, opts...) + }, + }). + Build() + + var out, errOut bytes.Buffer + err := destroyWorkload(context.Background(), &out, &errOut, c, testProject, testWorkload, true) + if err == nil { + t.Fatal("URLs that outlived the workload must fail the command") + } + if !errors.Is(err, boom) { + t.Errorf("error = %v, want the server's failure", err) + } + + if !strings.Contains(out.String(), "workload/api deleted.") { + t.Errorf("stdout should still report the workload deleted:\n%s", out.String()) + } + + warning := errOut.String() + for _, want := range []string{ + "workload was deleted, but its URLs were not", + "https://" + testCustom, + "https://" + testCanonical, + "datumctl compute destroy " + testWorkload, + } { + if !strings.Contains(warning, want) { + t.Errorf("message missing %q:\n%s", want, warning) + } + } +} + +// TestDestroyLeftoverURLCleanup: the retry the warning advertises works — +// the workload is already gone, and destroy removes what it left behind. +func TestDestroyLeftoverURLCleanup(t *testing.T) { + c := newFakeClient(t, publishedProxy(testCustom), publishedService()) + + var out, errOut bytes.Buffer + if err := destroyWorkload(context.Background(), &out, &errOut, c, testProject, testWorkload, true); err != nil { + t.Fatalf("destroyWorkload: %v", err) + } + + if !strings.Contains(out.String(), "already deleted") { + t.Errorf("output should say the workload was already gone:\n%s", out.String()) + } + if !strings.Contains(out.String(), "URLs for api deleted.") { + t.Errorf("output should report the URLs deleted:\n%s", out.String()) + } + if exists(t, c, &networkingv1alpha.HTTPProxy{}) { + t.Error("leftover URL survived") + } + if exists(t, c, &networkingv1alpha.NetworkService{}) { + t.Error("leftover URL backends survived") + } +} + +// TestDestroyMissingWorkload: nothing to destroy and nothing left behind is +// the plain not-found error it always was. +func TestDestroyMissingWorkload(t *testing.T) { + c := newFakeClient(t) + + var out, errOut bytes.Buffer + err := destroyWorkload(context.Background(), &out, &errOut, c, testProject, testWorkload, true) + if err == nil { + t.Fatal("expected an error for a missing workload") + } + if !strings.Contains(err.Error(), `workload "api" not found in project acme-prod`) { + t.Errorf("unexpected error: %v", err) + } +} + +func TestHostnameURLs(t *testing.T) { + tests := []struct { + name string + objs []client.Object + want []string + }{ + { + name: "custom hostname first, managed last", + objs: []client.Object{publishedProxy(testCustom), publishedService()}, + want: []string{"https://" + testCustom, "https://" + testCanonical}, + }, + { + name: "managed hostname alone", + objs: []client.Object{publishedProxy(), publishedService()}, + want: []string{"https://" + testCanonical}, + }, + { + name: "unpublished workload has none", + objs: nil, + want: nil, + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + c := newFakeClient(t, tc.objs...) + info, err := url.ForWorkload(context.Background(), c, testWorkload) + if err != nil { + t.Fatalf("lookup: %v", err) + } + got := hostnameURLs(info) + if strings.Join(got, ",") != strings.Join(tc.want, ",") { + t.Errorf("hostnameURLs = %v, want %v", got, tc.want) + } + }) + } +} diff --git a/internal/cmd/compute/destroy/leftover_test.go b/internal/cmd/compute/destroy/leftover_test.go new file mode 100644 index 00000000..cb730af9 --- /dev/null +++ b/internal/cmd/compute/destroy/leftover_test.go @@ -0,0 +1,201 @@ +// SPDX-License-Identifier: AGPL-3.0-only + +package destroy + +import ( + "bytes" + "context" + "errors" + "strings" + "testing" + + "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/client/fake" + "sigs.k8s.io/controller-runtime/pkg/client/interceptor" + + computev1alpha "go.datum.net/compute/api/v1alpha" + networkingv1alpha "go.datum.net/network-services-operator/api/v1alpha" +) + +// clientFailingToDeleteBackends is the half of a partial delete that the +// existing tests do not cover: the URL comes down, the backends behind it do +// not. url.Unpublish deletes the proxy first, so this is the ordering a +// permission problem on one kind actually produces. +func clientFailingToDeleteBackends(t *testing.T, objs ...client.Object) client.WithWatch { + t.Helper() + return fake.NewClientBuilder(). + WithScheme(testScheme(t)). + WithObjects(objs...). + WithInterceptorFuncs(interceptor.Funcs{ + DeleteAllOf: func(ctx context.Context, cl client.WithWatch, obj client.Object, opts ...client.DeleteAllOfOption) error { + if _, ok := obj.(*networkingv1alpha.NetworkService); ok { + return errors.New("forbidden") + } + return cl.DeleteAllOf(ctx, obj, opts...) + }, + }). + Build() +} + +// TestDestroyFailsWhenOnlyTheBackendsAreLeft: the workload and its URL are +// gone, the backends are not. The destroy reports the workload deleted, names +// the retry — and still fails, because something it was asked to remove is +// still there. +func TestDestroyFailsWhenOnlyTheBackendsAreLeft(t *testing.T) { + c := clientFailingToDeleteBackends(t, testWorkloadObject(), publishedProxy(testCustom), publishedService()) + + var out, errOut bytes.Buffer + err := destroyWorkload(context.Background(), &out, &errOut, c, testProject, testWorkload, true) + if err == nil { + t.Fatal("leftover backends must fail the destroy") + } + + if !strings.Contains(out.String(), "workload/api deleted.") { + t.Errorf("stdout should still report the workload deleted:\n%s", out.String()) + } + if exists(t, c, &networkingv1alpha.HTTPProxy{}) { + t.Error("the URL should have been deleted before the backends were attempted") + } + if !exists(t, c, &networkingv1alpha.NetworkService{}) { + t.Fatal("test is not exercising the leftover-backends case") + } + if !strings.Contains(errOut.String(), "datumctl compute destroy "+testWorkload) { + t.Errorf("the message must name the command that finishes the job:\n%s", errOut.String()) + } +} + +// TestDestroyRetryRemovesLeftoverBackends is the promise the message above +// makes, taken at its word: run destroy again and what was left behind is +// removed. The workload is gone and so is the proxy the URL lookup keys on, so +// the only trace left is the backends — and destroy still has to find and +// remove them, because no other command can. +func TestDestroyRetryRemovesLeftoverBackends(t *testing.T) { + // The state the first run left: no workload, no proxy, backends still + // there. + c := newFakeClient(t, publishedService()) + + var out, errOut bytes.Buffer + if err := destroyWorkload(context.Background(), &out, &errOut, c, testProject, testWorkload, true); err != nil { + t.Fatalf("the retry the message advertises must work, got: %v", err) + } + if exists(t, c, &networkingv1alpha.NetworkService{}) { + t.Error("the leftover backends survived the retry, with no other command able to remove them") + } + if !strings.Contains(out.String(), "already deleted") { + t.Errorf("output should say the workload was already gone:\n%s", out.String()) + } + if !strings.Contains(out.String(), "Leftover URL backends for api deleted.") { + t.Errorf("output should report exactly what it removed:\n%s", out.String()) + } +} + +// TestDestroyRetryReportsWhatIsLeftBehind: the confirmation for a cleanup-only +// run has to describe what it is about to remove. When the proxy is gone there +// is no hostname left to name, so the summary says what remains in the user's +// vocabulary rather than printing nothing at all. +func TestDestroyRetryReportsWhatIsLeftBehind(t *testing.T) { + c := newFakeClient(t, publishedService()) + + var out, errOut bytes.Buffer + if err := destroyWorkload(context.Background(), &out, &errOut, c, testProject, testWorkload, true); err != nil { + t.Fatalf("destroyWorkload: %v", err) + } + if !strings.Contains(out.String(), "Leftovers:") { + t.Errorf("the summary must name what is still there:\n%s", out.String()) + } + for _, machinery := range []string{"NetworkService", "HTTPProxy"} { + if strings.Contains(out.String(), machinery) { + t.Errorf("output names the machinery %q:\n%s", machinery, out.String()) + } + } +} + +// The retry does work when it is the proxy that was left behind, because that +// is what the lookup keys on. Pinned so a fix for the case above is not read +// as a regression here. +func TestDestroyRetryRemovesALeftoverProxy(t *testing.T) { + c := newFakeClient(t, publishedProxy(testCustom)) + + var out, errOut bytes.Buffer + if err := destroyWorkload(context.Background(), &out, &errOut, c, testProject, testWorkload, true); err != nil { + t.Fatalf("destroyWorkload: %v", err) + } + if exists(t, c, &networkingv1alpha.HTTPProxy{}) { + t.Error("the leftover URL survived the retry") + } + if !strings.Contains(out.String(), "URLs for api deleted.") { + t.Errorf("output should report the URLs deleted:\n%s", out.String()) + } +} + +// TestDestroyReportsAFailedWorkloadDelete: the workload itself failing to +// delete is a failed destroy, and nothing may be unpublished after it — a URL +// removed from under a workload that still exists takes a live service down +// for no reason. +func TestDestroyReportsAFailedWorkloadDelete(t *testing.T) { + boom := errors.New("forbidden") + c := fake.NewClientBuilder(). + WithScheme(testScheme(t)). + WithObjects(testWorkloadObject(), publishedProxy(testCustom), publishedService()). + WithInterceptorFuncs(interceptor.Funcs{ + Delete: func(ctx context.Context, cl client.WithWatch, obj client.Object, opts ...client.DeleteOption) error { + if _, ok := obj.(*computev1alpha.Workload); ok { + return boom + } + return cl.Delete(ctx, obj, opts...) + }, + }). + Build() + + var out, errOut bytes.Buffer + err := destroyWorkload(context.Background(), &out, &errOut, c, testProject, testWorkload, true) + if err == nil { + t.Fatal("a workload that could not be deleted must fail the command") + } + if !errors.Is(err, boom) { + t.Errorf("error = %v, want the server's failure", err) + } + if !exists(t, c, &networkingv1alpha.HTTPProxy{}) { + t.Error("the URL was taken down for a workload that is still running") + } + if !exists(t, c, &networkingv1alpha.NetworkService{}) { + t.Error("the backends were taken down for a workload that is still running") + } +} + +// TestDestroySummaryWarnsWhenURLsCannotBeRead: a destroy whose URL lookup +// fails still deletes the workload, but the user has to be told the summary is +// incomplete rather than reading a missing URLs line as "there were none". +func TestDestroySummaryWarnsWhenURLsCannotBeRead(t *testing.T) { + c := fake.NewClientBuilder(). + WithScheme(testScheme(t)). + WithObjects(testWorkloadObject(), publishedProxy(testCustom), publishedService()). + WithInterceptorFuncs(interceptor.Funcs{ + List: func(ctx context.Context, cl client.WithWatch, list client.ObjectList, opts ...client.ListOption) error { + if _, ok := list.(*networkingv1alpha.HTTPProxyList); ok { + return errors.New("forbidden") + } + return cl.List(ctx, list, opts...) + }, + }). + Build() + + var out, errOut bytes.Buffer + if err := destroyWorkload(context.Background(), &out, &errOut, c, testProject, testWorkload, true); err != nil { + t.Fatalf("an unreadable URL must not fail the destroy: %v", err) + } + + if !strings.Contains(errOut.String(), "could not read URLs") { + t.Errorf("stderr must say the summary is incomplete:\n%s", errOut.String()) + } + if strings.Contains(out.String(), "URLs:") { + t.Errorf("no URL is known, so none may be claimed:\n%s", out.String()) + } + if exists(t, c, &computev1alpha.Workload{}) { + t.Error("workload survived destroy") + } + // The delete itself does not depend on the lookup: the URL still goes. + if exists(t, c, &networkingv1alpha.HTTPProxy{}) { + t.Error("URL survived destroy") + } +} diff --git a/internal/cmd/compute/url/lookup.go b/internal/cmd/compute/url/lookup.go new file mode 100644 index 00000000..4577b0cb --- /dev/null +++ b/internal/cmd/compute/url/lookup.go @@ -0,0 +1,475 @@ +// SPDX-License-Identifier: AGPL-3.0-only + +package url + +import ( + "context" + "fmt" + + k8serrors "k8s.io/apimachinery/pkg/api/errors" + "k8s.io/apimachinery/pkg/api/meta" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/labels" + "k8s.io/apimachinery/pkg/runtime" + "k8s.io/apimachinery/pkg/selection" + "sigs.k8s.io/controller-runtime/pkg/client" + + computev1alpha "go.datum.net/compute/api/v1alpha" + "go.datum.net/compute/internal/cmd/compute/util" + networkingv1alpha "go.datum.net/network-services-operator/api/v1alpha" +) + +// Status strings shown for a hostname. A hostname is active or it is not; when +// it is not, the server's own message is shown rather than a CLI translation +// of it — and never the server's condition reason, which is camelCase internal +// state the developer is not meant to read. +const ( + statusActive = "active" + statusPending = "pending" + + certificateValid = "valid" +) + +// Backends totals the instances serving a URL, across every city. +type Backends struct { + // Cities is how many cities the URL has backends in. + Cities int32 `json:"cities"` + // Total is how many backends are registered, healthy or not. + Total int32 `json:"total"` + // Healthy is how many of them are taking traffic. + Healthy int32 `json:"healthy"` +} + +// Location reports the backends a URL has in one city, and whether that city +// is taking traffic. +type Location struct { + // Location is the location as the platform reports it. + Location string `json:"location"` + // Backends is how many instances in this location back the URL. + Backends int32 `json:"backends"` + // Healthy is how many of them are taking traffic. + Healthy int32 `json:"healthy"` + // Serving reports whether this location is in rotation. + Serving bool `json:"serving"` +} + +// Hostname is the state of one hostname on a URL. +type Hostname struct { + // Hostname is the fully qualified name. + Hostname string `json:"hostname"` + // URL is the hostname as an https:// URL. Datum never serves plaintext. + URL string `json:"url"` + // Managed is true for the platform-assigned hostname, which the user + // neither creates nor removes. + Managed bool `json:"managed"` + // Active is true when this hostname is serving traffic. + Active bool `json:"active"` + // Status is "active", or the server's blocking message verbatim, or + // "pending" when the server has not said anything a human can read. + Status string `json:"status"` + // Certificate is "valid", the server's certificate message verbatim, + // "pending" when it is not ready and the server said nothing readable, or + // empty when the platform has not reported on a certificate at all. + Certificate string `json:"certificate,omitempty"` + // Detail is the server's message for the first blocking condition, shown + // verbatim. Empty when nothing is blocking. + Detail string `json:"detail,omitempty"` + // Conditions are the raw per-hostname conditions, for -o yaml and for + // callers that want to render more than Status. + Conditions []metav1.Condition `json:"-"` +} + +// Info is everything the CLI knows about one workload's URL. A workload that +// has not been published has no Info at all — callers get nil, not a zero +// value. +type Info struct { + // WorkloadName is the workload this URL belongs to. + WorkloadName string `json:"workloadName"` + // URL is the one URL to show the user: the first active custom hostname if + // there is one, otherwise the platform-managed hostname. Always https://. + URL string `json:"url"` + // CanonicalHostname is the platform-managed hostname. Empty until the + // server assigns one. + CanonicalHostname string `json:"canonicalHostname,omitempty"` + // CustomHostnames are the hostnames the user attached, in declared order. + CustomHostnames []string `json:"customHostnames,omitempty"` + // Hostnames carries per-hostname state for every hostname on the URL, + // custom ones first, the managed one last. + Hostnames []Hostname `json:"hostnames,omitempty"` + + // PortName and Port are the port the backends answer on. Protocol is the + // transport, always TCP today. + PortName string `json:"portName,omitempty"` + Port int32 `json:"port,omitempty"` + Protocol string `json:"protocol,omitempty"` + + // Backends totals the serving instances; Locations breaks that down by + // city, which is what makes a multi-city deployment legible. + Backends Backends `json:"backends"` + Locations []Location `json:"locations,omitempty"` + + // EdgeProgrammed is true once the edge is carrying the configuration. + EdgeProgrammed bool `json:"edgeProgrammed"` + // CertificateIssued is true once the primary hostname has a certificate. + CertificateIssued bool `json:"certificateIssued"` + // CertificateKnown is false when the platform has reported nothing about a + // certificate, which is different from reporting that there isn't one. + CertificateKnown bool `json:"-"` + + // ProxyConditions and ServiceConditions are the raw conditions behind the + // fields above. Render whatever the server emits; never branch on a reason. + ProxyConditions []metav1.Condition `json:"-"` + ServiceConditions []metav1.Condition `json:"-"` + + // Proxy and Service are the objects themselves, for `-o yaml`. They are + // the machinery: never name them in normal output. Reach them through + // Objects(), which is the shape structured output renders. + Proxy *networkingv1alpha.HTTPProxy `json:"-"` + Service *networkingv1alpha.NetworkService `json:"-"` +} + +// Objects is the raw platform state behind a URL, in the shape structured +// output renders it. It is the escape hatch the product promises: plain +// language in normal output, the real objects behind `-o yaml`. +// +// Both fields may be nil — a proxy exists for a moment before its backends do, +// and a control plane that does not serve the backend kind reports none. +type Objects struct { + HTTPProxy *networkingv1alpha.HTTPProxy `json:"httpProxy,omitempty"` + NetworkService *networkingv1alpha.NetworkService `json:"networkService,omitempty"` +} + +// Objects returns the real objects behind the URL, for a caller rendering +// `-o yaml` or `-o json`. It returns nil when there is nothing to show, so a +// caller can fall back to the human view without a second check. +// +// This is the only sanctioned way out of this package to the machinery: the +// default human output never names it. +func (i *Info) Objects() *Objects { + if i == nil || (i.Proxy == nil && i.Service == nil) { + return nil + } + return &Objects{HTTPProxy: i.Proxy, NetworkService: i.Service} +} + +// Live reports whether the URL is ready to be handed to the user: it has a +// hostname, the edge is programmed, and a certificate has been issued (or the +// platform reports no certificate state at all, in which case there is nothing +// to wait for). +func (i *Info) Live() bool { + if i == nil { + return false + } + return i.URL != "" && i.EdgeProgrammed && (i.CertificateIssued || !i.CertificateKnown) +} + +// ForWorkload returns the URL info for one workload, or nil when the workload +// has not been published. A workload without a URL is an ordinary state, not +// an error; so is a control plane that does not serve these kinds at all. +// Transport and permission errors propagate. +func ForWorkload(ctx context.Context, c client.Client, workloadName string) (*Info, error) { + proxies, services, err := list(ctx, c, labels.Set{computev1alpha.WorkloadNameLabel: workloadName}) + if err != nil { + return nil, err + } + if len(proxies) == 0 { + return nil, nil + } + return newInfo(workloadName, &proxies[0], serviceFor(services, workloadName)), nil +} + +// ForAll returns URL info for every published workload in the project, keyed +// by workload name. Workloads without a URL are absent from the map. +// +// It costs exactly two List calls no matter how many workloads there are: the +// list view renders a whole project through this. +func ForAll(ctx context.Context, c client.Client) (map[string]*Info, error) { + proxies, services, err := list(ctx, c, nil) + if err != nil { + return nil, err + } + + byWorkload := make(map[string]*networkingv1alpha.NetworkService, len(services)) + for i := range services { + byWorkload[workloadOf(services[i].Labels, services[i].Name)] = &services[i] + } + + infos := make(map[string]*Info, len(proxies)) + for i := range proxies { + name := workloadOf(proxies[i].Labels, proxies[i].Name) + infos[name] = newInfo(name, &proxies[i], byWorkload[name]) + } + return infos, nil +} + +// list fetches both published kinds with one call each. An empty match lists +// every URL the CLI published in the namespace — and only those: an HTTPProxy +// a user wrote by hand is theirs, and is never reported as a workload's URL. +func list(ctx context.Context, c client.Client, match labels.Set) ([]networkingv1alpha.HTTPProxy, []networkingv1alpha.NetworkService, error) { + selector := labels.SelectorFromSet(match) + if len(match) == 0 { + req, err := labels.NewRequirement(computev1alpha.WorkloadNameLabel, selection.Exists, nil) + if err != nil { + return nil, nil, fmt.Errorf("building URL selector: %w", err) + } + selector = labels.NewSelector().Add(*req) + } + + opts := []client.ListOption{ + client.InNamespace(util.ResourceNamespace), + client.MatchingLabelsSelector{Selector: selector}, + } + + var proxyList networkingv1alpha.HTTPProxyList + if err := c.List(ctx, &proxyList, opts...); err != nil { + if notPublished(err) { + return nil, nil, nil + } + return nil, nil, fmt.Errorf("listing URLs: %w", err) + } + + var serviceList networkingv1alpha.NetworkServiceList + if err := c.List(ctx, &serviceList, opts...); err != nil { + if notPublished(err) { + return proxyList.Items, nil, nil + } + return nil, nil, fmt.Errorf("listing URL backends: %w", err) + } + + return proxyList.Items, serviceList.Items, nil +} + +// notPublished reports whether an error means "there is nothing published +// here" rather than a real failure. A control plane that has never had these +// CRDs installed answers with a no-match error, and a client whose scheme +// lacks the kinds answers with a not-registered error; neither is something to +// report to a user who only asked for a URL. +func notPublished(err error) bool { + return err == nil || + k8serrors.IsNotFound(err) || + meta.IsNoMatchError(err) || + runtime.IsNotRegisteredError(err) +} + +// serviceFor picks the NetworkService belonging to a workload out of a list. +func serviceFor(services []networkingv1alpha.NetworkService, workloadName string) *networkingv1alpha.NetworkService { + for i := range services { + if workloadOf(services[i].Labels, services[i].Name) == workloadName { + return &services[i] + } + } + return nil +} + +// workloadOf reads the workload a published object belongs to from its labels, +// falling back to the object's own name, which ResourceName keeps in step. +func workloadOf(objLabels map[string]string, objName string) string { + if name := objLabels[computev1alpha.WorkloadNameLabel]; name != "" { + return name + } + return objName +} + +// newInfo assembles the user-facing view from the two objects. service may be +// nil: a proxy can exist for a moment before its backends do. +func newInfo(workloadName string, proxy *networkingv1alpha.HTTPProxy, service *networkingv1alpha.NetworkService) *Info { + info := &Info{ + WorkloadName: workloadName, + CanonicalHostname: proxy.Status.CanonicalHostname, + ProxyConditions: proxy.Status.Conditions, + Proxy: proxy, + Service: service, + } + + if c := util.FindCondition(proxy.Status.Conditions, networkingv1alpha.HTTPProxyConditionProgrammed); c != nil { + info.EdgeProgrammed = c.Status == metav1.ConditionTrue + } + + for _, h := range proxy.Spec.Hostnames { + info.CustomHostnames = append(info.CustomHostnames, string(h)) + } + + for _, h := range info.CustomHostnames { + info.Hostnames = append(info.Hostnames, hostnameInfo(proxy, h, false)) + } + if info.CanonicalHostname != "" { + info.Hostnames = append(info.Hostnames, hostnameInfo(proxy, info.CanonicalHostname, true)) + } + + info.URL, info.CertificateIssued, info.CertificateKnown = primary(info) + + if service != nil { + info.ServiceConditions = service.Status.Conditions + if len(service.Spec.Ports) > 0 { + p := service.Spec.Ports[0] + info.PortName, info.Port = p.Name, p.Port + info.Protocol = string(p.Protocol) + if info.Protocol == "" { + info.Protocol = string(networkingv1alpha.NetworkServiceProtocolTCP) + } + } + info.Backends = Backends{ + Cities: service.Status.Summary.Locations, + Total: service.Status.Summary.Members, + Healthy: service.Status.Summary.Healthy, + } + for _, l := range service.Status.Locations { + info.Locations = append(info.Locations, Location{ + Location: l.Name, + Backends: l.Members, + Healthy: l.Healthy, + Serving: l.Serving, + }) + } + } + + return info +} + +// primary chooses the URL to show and reports the certificate state behind it. +// An active custom hostname is what the user wants to see; until one is +// active, the platform-managed hostname is the one that actually answers. +func primary(info *Info) (url string, certIssued, certKnown bool) { + var fallback *Hostname + var managed *Hostname + + for i := range info.Hostnames { + h := &info.Hostnames[i] + switch { + case h.Managed: + managed = h + case h.Active: + return h.URL, h.Certificate == certificateValid, h.Certificate != "" + case fallback == nil: + fallback = h + } + } + + if managed != nil { + return managed.URL, managed.Certificate == certificateValid, managed.Certificate != "" + } + if fallback != nil { + return fallback.URL, fallback.Certificate == certificateValid, fallback.Certificate != "" + } + return "", false, false +} + +// hostnameInfo derives one hostname's state from that hostname's own entry in +// the proxy's per-hostname statuses. Nothing else can say whether a hostname is +// serving: the proxy-level conditions are a roll-up over every hostname on the +// proxy, so one hostname waiting on a certificate turns the proxy's certificate +// condition False while every other hostname carries on serving normally. +// +// Two rules follow from that, and both matter to a user: +// +// - A custom hostname the platform has published no status for is pending, +// never active. It has just been attached and nothing about it has been +// checked yet, so `domains add` must show the DNS records and wait. +// - The platform-managed hostname may fall back to the proxy-level +// conditions, because it is the hostname the proxy is for. On a control +// plane that publishes per-hostname statuses for other hostnames, only the +// conditions that are True may inform it: a True roll-up covers every +// hostname, while a False one names none of them. +func hostnameInfo(proxy *networkingv1alpha.HTTPProxy, hostname string, managed bool) Hostname { + h := Hostname{ + Hostname: hostname, + URL: "https://" + hostname, + Managed: managed, + Status: statusPending, + } + + conditions := perHostnameConditions(proxy, hostname) + h.Conditions = conditions + + if len(conditions) == 0 { + if !managed { + return h + } + if len(proxy.Status.HostnameStatuses) == 0 { + conditions = proxy.Status.Conditions + } else { + conditions = satisfied(proxy.Status.Conditions) + } + } + + // Certificate state, in the server's own words. + if c := certificateCondition(conditions); c != nil { + if c.Status == metav1.ConditionTrue { + h.Certificate = certificateValid + } else { + h.Certificate = humanReason(c) + } + } + + // A hostname is active when nothing about it is blocking and the edge is + // carrying the configuration. + blocked := firstBlocking(conditions) + programmed := util.FindCondition(proxy.Status.Conditions, networkingv1alpha.HTTPProxyConditionProgrammed) + switch { + case blocked != nil: + h.Status = humanReason(blocked) + h.Detail = blocked.Message + case programmed != nil && programmed.Status == metav1.ConditionTrue: + h.Status = statusActive + h.Active = true + } + + return h +} + +// humanReason renders why a condition is not satisfied, for a user to read. +// +// It is the server's message, which is written for a human, and never the +// condition's reason, which is camelCase internal state. A condition with no +// message says only that the platform has not finished — which is "pending", +// the same plain word an unreported hostname gets. +func humanReason(c *metav1.Condition) string { + if c.Message != "" { + return c.Message + } + return statusPending +} + +// satisfied returns the conditions that are True. Used to let a proxy-level +// roll-up vouch for the managed hostname without letting it blame it. +func satisfied(conditions []metav1.Condition) []metav1.Condition { + var ok []metav1.Condition + for _, c := range conditions { + if c.Status == metav1.ConditionTrue { + ok = append(ok, c) + } + } + return ok +} + +// perHostnameConditions returns the conditions the server published for one +// hostname, or nil when it published none. +func perHostnameConditions(proxy *networkingv1alpha.HTTPProxy, hostname string) []metav1.Condition { + for _, s := range proxy.Status.HostnameStatuses { + if s.Hostname == hostname { + return s.Conditions + } + } + return nil +} + +// certificateCondition finds whichever certificate condition the given set +// carries: per-hostname status uses one type, proxy-level status another. +func certificateCondition(conditions []metav1.Condition) *metav1.Condition { + if c := util.FindCondition(conditions, networkingv1alpha.HostnameConditionCertificateReady); c != nil { + return c + } + return util.FindCondition(conditions, networkingv1alpha.HTTPProxyConditionCertificatesReady) +} + +// firstBlocking returns the first condition that is not True, so its reason and +// message can be shown verbatim. Unknown counts as blocking: the platform has +// not yet said the hostname works. +func firstBlocking(conditions []metav1.Condition) *metav1.Condition { + for i := range conditions { + if conditions[i].Status != metav1.ConditionTrue { + return &conditions[i] + } + } + return nil +} diff --git a/internal/cmd/compute/url/lookup_test.go b/internal/cmd/compute/url/lookup_test.go new file mode 100644 index 00000000..e07b4caa --- /dev/null +++ b/internal/cmd/compute/url/lookup_test.go @@ -0,0 +1,444 @@ +// SPDX-License-Identifier: AGPL-3.0-only + +package url + +import ( + "bytes" + "context" + "errors" + "strings" + "testing" + + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/runtime" + "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" + + computev1alpha "go.datum.net/compute/api/v1alpha" + "go.datum.net/compute/internal/cmd/compute/util" + networkingv1alpha "go.datum.net/network-services-operator/api/v1alpha" +) + +// Fixtures shared by the tests in this package. +const ( + testWorkloadName = "api" + testPortName = "http" + testCanonical = "a1b2c3d4.datumproxy.net" + testCanonicalURL = "https://" + testCanonical + testCustomHostname = "api.example.com" + testCustomURL = "https://" + testCustomHostname + + kindProxy = "HTTPProxy" + kindService = "NetworkService" +) + +func testScheme(t *testing.T) *runtime.Scheme { + t.Helper() + s := runtime.NewScheme() + if err := computev1alpha.AddToScheme(s); err != nil { + t.Fatalf("registering compute scheme: %v", err) + } + if err := networkingv1alpha.AddToScheme(s); err != nil { + t.Fatalf("registering networking scheme: %v", err) + } + return s +} + +func newFakeClient(t *testing.T, objs ...client.Object) client.WithWatch { + t.Helper() + return fake.NewClientBuilder(). + WithScheme(testScheme(t)). + WithStatusSubresource(&networkingv1alpha.HTTPProxy{}, &networkingv1alpha.NetworkService{}). + WithObjects(objs...). + Build() +} + +func cond(condType string, status metav1.ConditionStatus, reason, message string) metav1.Condition { + return metav1.Condition{Type: condType, Status: status, Reason: reason, Message: message} +} + +// publishedProxy returns a proxy in the shape the platform reports once it is +// serving on the managed hostname alone. +func publishedProxy(workload, canonical string) *networkingv1alpha.HTTPProxy { + p := BuildHTTPProxy(workloadNamed(workload), testPortName, nil) + p.Status.CanonicalHostname = canonical + p.Status.Conditions = []metav1.Condition{ + cond(networkingv1alpha.HTTPProxyConditionAccepted, metav1.ConditionTrue, "Accepted", ""), + cond(networkingv1alpha.HTTPProxyConditionProgrammed, metav1.ConditionTrue, "Programmed", ""), + cond(networkingv1alpha.HTTPProxyConditionCertificatesReady, metav1.ConditionTrue, "AllCertificatesReady", ""), + } + return p +} + +func publishedService(workload string, port int32, locations ...networkingv1alpha.NetworkServiceLocationStatus) *networkingv1alpha.NetworkService { + s := BuildNetworkService(workloadNamed(workload), testPortName, port) + var members, healthy int32 + for _, l := range locations { + members += l.Members + healthy += l.Healthy + } + s.Status.Summary = networkingv1alpha.NetworkServiceSummary{ + Locations: int32(len(locations)), + Members: members, + Healthy: healthy, + } + s.Status.Locations = locations + s.Status.Conditions = []metav1.Condition{ + cond(networkingv1alpha.NetworkServiceMembersResolved, metav1.ConditionTrue, "MembersResolved", ""), + cond(networkingv1alpha.NetworkServiceReady, metav1.ConditionTrue, "Ready", ""), + } + return s +} + +func workloadNamed(name string) *computev1alpha.Workload { + w := testWorkload() + w.Name = name + w.UID = types.UID("uid-" + name) + return w +} + +func location(city string, members, healthy int32, serving bool) networkingv1alpha.NetworkServiceLocationStatus { + return networkingv1alpha.NetworkServiceLocationStatus{Name: city, Members: members, Healthy: healthy, Serving: serving} +} + +func TestForWorkloadPublished(t *testing.T) { + c := newFakeClient(t, + publishedProxy(testWorkloadName, testCanonical), + publishedService(testWorkloadName, 8080, location("DFW", 2, 2, true), location("IAD", 2, 2, true)), + ) + + info, err := ForWorkload(context.Background(), c, testWorkloadName) + if err != nil { + t.Fatalf("ForWorkload returned error: %v", err) + } + if info == nil { + t.Fatal("ForWorkload returned nil for a published workload") + } + + if info.URL != testCanonicalURL { + t.Errorf("URL = %q, want the managed hostname as https", info.URL) + } + if info.CanonicalHostname != testCanonical { + t.Errorf("CanonicalHostname = %q", info.CanonicalHostname) + } + if len(info.CustomHostnames) != 0 { + t.Errorf("CustomHostnames = %v, want none", info.CustomHostnames) + } + if !info.EdgeProgrammed || !info.CertificateIssued || !info.Live() { + t.Errorf("edge=%v cert=%v live=%v, want all true", info.EdgeProgrammed, info.CertificateIssued, info.Live()) + } + if info.Port != 8080 || info.PortName != testPortName || info.Protocol != "TCP" { + t.Errorf("backend port = %d/%s (%s), want 8080/http (TCP)", info.Port, info.PortName, info.Protocol) + } + want := Backends{Cities: 2, Total: 4, Healthy: 4} + if info.Backends != want { + t.Errorf("Backends = %+v, want %+v", info.Backends, want) + } + if len(info.Locations) != 2 || info.Locations[0] != (Location{Location: "DFW", Backends: 2, Healthy: 2, Serving: true}) { + t.Errorf("Locations = %+v", info.Locations) + } + if info.Proxy == nil || info.Service == nil { + t.Error("raw objects should be carried for -o yaml") + } + if len(info.ServiceConditions) == 0 || len(info.ProxyConditions) == 0 { + t.Error("conditions should be carried through for rendering") + } +} + +func TestForWorkloadDegraded(t *testing.T) { + c := newFakeClient(t, + publishedProxy(testWorkloadName, testCanonical), + publishedService(testWorkloadName, 8080, location("DFW", 2, 2, true), location("IAD", 2, 0, false)), + ) + + info, err := ForWorkload(context.Background(), c, testWorkloadName) + if err != nil { + t.Fatalf("ForWorkload returned error: %v", err) + } + if info.Backends.Healthy != 2 || info.Backends.Total != 4 { + t.Errorf("Backends = %+v, want 2 of 4 healthy", info.Backends) + } + // A degraded backend set does not stop the URL from answering. + if !info.Live() { + t.Error("URL should still be live while one city is out of rotation") + } +} + +func TestForWorkloadCustomHostnamePreferredWhenActive(t *testing.T) { + proxy := publishedProxy(testWorkloadName, testCanonical) + proxy.Spec.Hostnames = append(proxy.Spec.Hostnames, testCustomHostname) + proxy.Status.HostnameStatuses = []networkingv1alpha.HostnameStatus{{ + Hostname: testCustomHostname, + Conditions: []metav1.Condition{ + cond(networkingv1alpha.HostnameConditionVerified, metav1.ConditionTrue, "Verified", ""), + cond(networkingv1alpha.HostnameConditionDNSRecordProgrammed, metav1.ConditionTrue, "RecordCreated", ""), + cond(networkingv1alpha.HostnameConditionCertificateReady, metav1.ConditionTrue, "CertificateIssued", ""), + }, + }} + + c := newFakeClient(t, proxy, publishedService(testWorkloadName, 8080, location("DFW", 1, 1, true))) + + info, err := ForWorkload(context.Background(), c, testWorkloadName) + if err != nil { + t.Fatalf("ForWorkload returned error: %v", err) + } + if info.URL != testCustomURL { + t.Errorf("URL = %q, want the custom hostname", info.URL) + } + if len(info.Hostnames) != 2 { + t.Fatalf("Hostnames = %+v, want the custom one and the managed one", info.Hostnames) + } + if info.Hostnames[0].Managed || !info.Hostnames[1].Managed { + t.Errorf("hostname order = %+v, want custom first and managed last", info.Hostnames) + } + if info.Hostnames[0].Status != statusActive || info.Hostnames[0].Certificate != certificateValid { + t.Errorf("custom hostname = %+v, want active with a valid certificate", info.Hostnames[0]) + } +} + +func TestForWorkloadPendingCustomHostnameFallsBackToManaged(t *testing.T) { + proxy := publishedProxy(testWorkloadName, testCanonical) + proxy.Spec.Hostnames = append(proxy.Spec.Hostnames, testCustomHostname) + proxy.Status.HostnameStatuses = []networkingv1alpha.HostnameStatus{{ + Hostname: testCustomHostname, + Conditions: []metav1.Condition{ + cond(networkingv1alpha.HostnameConditionVerified, metav1.ConditionFalse, "DomainNotVerified", "waiting for TXT record"), + }, + }} + + c := newFakeClient(t, proxy, publishedService(testWorkloadName, 8080, location("DFW", 1, 1, true))) + + info, err := ForWorkload(context.Background(), c, testWorkloadName) + if err != nil { + t.Fatalf("ForWorkload returned error: %v", err) + } + if info.URL != testCanonicalURL { + t.Errorf("URL = %q, want the managed hostname while the custom one is pending", info.URL) + } + // The server's own words, unedited — its message, never its reason. + if info.Hostnames[0].Status != "waiting for TXT record" || info.Hostnames[0].Detail != "waiting for TXT record" { + t.Errorf("custom hostname = %+v, want the server's message verbatim", info.Hostnames[0]) + } + if info.Hostnames[0].Status == "DomainNotVerified" { + t.Error("a raw camelCase condition reason must never be shown as a status") + } + if info.Hostnames[0].Active { + t.Error("a hostname with a blocking condition is not active") + } +} + +func TestForWorkloadNotPublished(t *testing.T) { + c := newFakeClient(t, publishedProxy("other", "zzz.datumproxy.net")) + + info, err := ForWorkload(context.Background(), c, testWorkloadName) + if err != nil { + t.Fatalf("an unpublished workload is not an error, got: %v", err) + } + if info != nil { + t.Errorf("info = %+v, want nil for an unpublished workload", info) + } +} + +func TestForWorkloadWithoutTheCRDsInstalled(t *testing.T) { + // A control plane that never had the networking kinds answers with a + // no-match error. That means "no URLs here", not "the command failed". + s := runtime.NewScheme() + if err := computev1alpha.AddToScheme(s); err != nil { + t.Fatalf("registering compute scheme: %v", err) + } + c := fake.NewClientBuilder().WithScheme(s).Build() + + info, err := ForWorkload(context.Background(), c, testWorkloadName) + if err != nil { + t.Fatalf("a control plane without the kinds is not an error, got: %v", err) + } + if info != nil { + t.Errorf("info = %+v, want nil", info) + } + + all, err := ForAll(context.Background(), c) + if err != nil { + t.Fatalf("ForAll returned error: %v", err) + } + if len(all) != 0 { + t.Errorf("ForAll = %v, want empty", all) + } +} + +func TestForWorkloadPropagatesTransportErrors(t *testing.T) { + boom := errors.New("connection refused") + c := interceptor.NewClient(newFakeClient(t), interceptor.Funcs{ + List: func(context.Context, client.WithWatch, client.ObjectList, ...client.ListOption) error { + return boom + }, + }) + + if _, err := ForWorkload(context.Background(), c, testWorkloadName); !errors.Is(err, boom) { + t.Fatalf("error = %v, want the transport error to propagate", err) + } + if _, err := ForAll(context.Background(), c); !errors.Is(err, boom) { + t.Fatalf("error = %v, want the transport error to propagate", err) + } +} + +func TestForAllRendersAWholeProjectInTwoListCalls(t *testing.T) { + objs := []client.Object{ + publishedProxy(testWorkloadName, testCanonical), + publishedService(testWorkloadName, 8080, location("DFW", 2, 2, true), location("IAD", 2, 2, true)), + publishedProxy("web", "e5f6a7b8.datumproxy.net"), + publishedService("web", 3000, location("DFW", 1, 1, true)), + publishedProxy("docs", "c9d0e1f2.datumproxy.net"), + publishedService("docs", 80, location("IAD", 1, 0, false)), + } + // A workload with no URL at all: it must simply be absent from the map. + worker := workloadNamed("worker") + + lists := 0 + c := interceptor.NewClient(newFakeClient(t, objs...), interceptor.Funcs{ + List: func(ctx context.Context, cl client.WithWatch, list client.ObjectList, opts ...client.ListOption) error { + lists++ + return cl.List(ctx, list, opts...) + }, + }) + + infos, err := ForAll(context.Background(), c) + if err != nil { + t.Fatalf("ForAll returned error: %v", err) + } + if lists != 2 { + t.Errorf("List calls = %d, want exactly 2 no matter how many workloads", lists) + } + if len(infos) != 3 { + t.Fatalf("infos = %d entries, want 3", len(infos)) + } + if infos[worker.Name] != nil { + t.Errorf("unpublished workload %q should be absent from the map", worker.Name) + } + + if got := infos[testWorkloadName]; got == nil || got.URL != testCanonicalURL || got.Backends.Healthy != 4 { + t.Errorf("api = %+v", got) + } + if got := infos["web"]; got == nil || got.Port != 3000 || got.Backends.Total != 1 { + t.Errorf("web = %+v", got) + } + if got := infos["docs"]; got == nil || got.Backends.Healthy != 0 || len(got.Locations) != 1 { + t.Errorf("docs = %+v", got) + } + for name, info := range infos { + if info.WorkloadName != name { + t.Errorf("info keyed %q carries workload name %q", name, info.WorkloadName) + } + } +} + +func TestForWorkloadWithoutBackendsYet(t *testing.T) { + // The proxy can exist for a moment before the service does. That is a URL + // with no backends, not a lookup failure. + c := newFakeClient(t, publishedProxy(testWorkloadName, testCanonical)) + + info, err := ForWorkload(context.Background(), c, testWorkloadName) + if err != nil { + t.Fatalf("ForWorkload returned error: %v", err) + } + if info == nil { + t.Fatal("info = nil, want a URL with no backends") + } + if info.Backends != (Backends{}) || info.Service != nil { + t.Errorf("Backends = %+v, Service = %v, want empty", info.Backends, info.Service) + } + if info.Port != 0 { + t.Errorf("Port = %d, want 0 when no backends are known", info.Port) + } +} + +func TestNamespaceIsAlwaysTheProjectNamespace(t *testing.T) { + if util.ResourceNamespace != BuildHTTPProxy(testWorkload(), testPortName, nil).Namespace { + t.Error("published objects must live in the project namespace") + } +} + +func TestLookupsIgnoreHandWrittenProxies(t *testing.T) { + // A proxy the user wrote themselves is theirs. Reporting it as a + // workload's URL would make `destroy` offer to delete it. + handWritten := BuildHTTPProxy(workloadNamed(testWorkloadName), testPortName, nil) + handWritten.Name = "hand-written" + handWritten.Labels = nil + handWritten.OwnerReferences = nil + handWritten.Status.CanonicalHostname = testCanonical + + c := newFakeClient(t, handWritten) + + infos, err := ForAll(context.Background(), c) + if err != nil { + t.Fatalf("ForAll returned error: %v", err) + } + if len(infos) != 0 { + t.Errorf("ForAll = %v, want nothing — that proxy is not a workload URL", infos) + } + + info, err := ForWorkload(context.Background(), c, testWorkloadName) + if err != nil { + t.Fatalf("ForWorkload returned error: %v", err) + } + if info != nil { + t.Errorf("ForWorkload = %+v, want nil", info) + } +} + +// TestObjectsIsTheEscapeHatchToTheRealState pins the promise the spec makes +// twice over: plain language in normal output, and `-o yaml` showing the real +// objects. Without a way out of this package to them, `domains +// -o yaml` can only re-print the same summary the table already showed. +func TestObjectsIsTheEscapeHatchToTheRealState(t *testing.T) { + c := newFakeClient(t, + publishedProxy(testWorkloadName, testCanonical), + publishedService(testWorkloadName, 8080, location("DFW", 1, 1, true)), + ) + + info, err := ForWorkload(context.Background(), c, testWorkloadName) + if err != nil { + t.Fatalf("ForWorkload returned error: %v", err) + } + + objs := info.Objects() + if objs == nil { + t.Fatal("Objects() = nil, want the real objects behind the URL") + } + if objs.HTTPProxy != info.Proxy || objs.NetworkService != info.Service { + t.Error("Objects() must hand back the objects themselves, not a summary of them") + } + + // What `-o yaml` would render: the real spec and status, not the CLI's view. + var out bytes.Buffer + if err := util.PrintYAML(&out, objs); err != nil { + t.Fatalf("rendering the objects: %v", err) + } + got := out.String() + for _, want := range []string{"httpProxy", "networkService", testCanonical, "canonicalHostname"} { + if !strings.Contains(got, want) { + t.Errorf("-o yaml output missing %q:\n%s", want, got) + } + } + + // And the default structured view still carries none of the machinery. + var human bytes.Buffer + if err := util.PrintJSON(&human, info); err != nil { + t.Fatalf("rendering the URL: %v", err) + } + for _, unwanted := range []string{"httpProxy", "networkService"} { + if strings.Contains(human.String(), unwanted) { + t.Errorf("%q leaked into the default view:\n%s", unwanted, human.String()) + } + } + + // Nothing to show reads as nothing, so a caller needs no second check. + var missing *Info + if missing.Objects() != nil { + t.Error("Objects() on a workload with no URL must be nil") + } + if (&Info{}).Objects() != nil { + t.Error("Objects() with neither object must be nil") + } +} diff --git a/internal/cmd/compute/url/publish.go b/internal/cmd/compute/url/publish.go new file mode 100644 index 00000000..4ebf148f --- /dev/null +++ b/internal/cmd/compute/url/publish.go @@ -0,0 +1,405 @@ +// SPDX-License-Identifier: AGPL-3.0-only + +package url + +import ( + "context" + "fmt" + "io" + "reflect" + "strings" + "time" + + k8serrors "k8s.io/apimachinery/pkg/api/errors" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "sigs.k8s.io/controller-runtime/pkg/client" + + computev1alpha "go.datum.net/compute/api/v1alpha" + "go.datum.net/compute/internal/cmd/compute/util" + networkingv1alpha "go.datum.net/network-services-operator/api/v1alpha" +) + +const ( + // pollInterval matches the rollout watcher, so a deploy that publishes and + // a deploy that only rolls out feel the same. + pollInterval = 2 * time.Second + + // blockingGrace is how long to wait before repeating what the server says + // is holding the URL up. Every object starts out reporting "waiting for + // controller", and echoing that immediately is noise, not diagnosis. + blockingGrace = 20 * time.Second + + // labelWidth aligns the progress labels ("Backends", "Edge", + // "Certificate") in a fixed column. + labelWidth = 12 + + // maxReadFailures is how many consecutive failed reads the wait rides out + // before giving up and reporting the last one. + // + // A read that fails once is a blip and the next tick is a better answer + // than failing a deploy that is going fine. A read that fails every time is + // something else — a missing list permission is the everyday one — and + // polling it forever turns `deploy --http-port` into a silent hang. Three + // ticks is a few seconds: long enough for a blip, short enough that a user + // is told what is wrong rather than left watching a cursor. + maxReadFailures = 3 + + // maxWait bounds the whole wait, so no caller can hang forever even while + // the control plane answers happily and simply never finishes. It is far + // longer than publishing takes; reaching it means something is stuck. + maxWait = 15 * time.Minute +) + +// Publish creates or updates the two objects that put a workload on a URL and +// waits until that URL answers, printing progress as the platform reports it: +// +// Backends 4 healthy across DFW, IAD +// Edge programmed +// Certificate issued +// +// It prints progress lines only. The caller prints whatever heading precedes +// them and the final URL — the URL is the deliverable and belongs to the +// command that was asked for it. +// +// The NetworkService is written before the HTTPProxy: a proxy naming a service +// that does not exist yet reports a missing backend, which the user would see +// as a spurious failure. +// +// hostnames are custom hostnames; pass nil for the managed URL alone. +// +// Cancelling ctx (Ctrl-C) detaches: publishing continues on the platform, a +// note says how to pick it up again, and Publish returns (nil, nil). A nil +// Info with a nil error means "still going, we stopped watching" — never an +// error, and never a reason for the caller to fail. +func Publish( + ctx context.Context, + out io.Writer, + c client.Client, + w *computev1alpha.Workload, + portName string, + port int32, + hostnames []string, +) (*Info, error) { + // A write interrupted by Ctrl-C is a detach like any other. Reporting the + // cancelled context as a failure would exit 1 on a keystroke the user was + // told is safe. + if err := Declare(ctx, c, w, portName, port, hostnames); err != nil { + if ctx.Err() != nil { + detached(out, w.Name) + return nil, nil + } + return nil, err + } + + return Wait(ctx, out, c, w.Name) +} + +// Declare writes the two objects that put a workload on a URL and returns +// without waiting for that URL to answer. +// +// It is the half of Publish a caller wants when the URL has to be declared +// early — a deploy declares it alongside the workload so that backends +// register as instances come up, then waits only once the rollout is done. +// It prints nothing: at the point it runs there is nothing to report yet. +// +// The NetworkService is written before the HTTPProxy: a proxy naming a service +// that does not exist yet reports a missing backend, which the user would see +// as a spurious failure. +// +// hostnames are custom hostnames; pass nil for the managed URL alone. +func Declare( + ctx context.Context, + c client.Client, + w *computev1alpha.Workload, + portName string, + port int32, + hostnames []string, +) error { + if err := applyService(ctx, c, BuildNetworkService(w, portName, port)); err != nil { + return err + } + return applyProxy(ctx, c, BuildHTTPProxy(w, portName, hostnames)) +} + +// detached prints the note that says publishing carries on without us, and how +// to pick it back up. +func detached(out io.Writer, workloadName string) { + fmt.Fprintf(out, "\nDetached. Publishing continues in the background.\n") + fmt.Fprintf(out, " Check it with: datumctl compute workloads describe %s\n", workloadName) +} + +// Wait polls until the workload's URL is live, printing each stage as it +// lands. It is the half of Publish that a caller which already called Declare +// still needs. +// +// It always terminates: Ctrl-C detaches, a control plane that cannot be read +// gives up after maxReadFailures and returns what it was told, and the whole +// wait ends at maxWait however healthy the polling looks. +// +// Cancelling ctx (Ctrl-C) detaches: a note says how to pick the URL up again +// and Wait returns (nil, nil), which is never an error. +func Wait(ctx context.Context, out io.Writer, c client.Client, workloadName string) (*Info, error) { + p := &progress{out: out, started: time.Now(), seen: map[string]bool{}} + + ticker := time.NewTicker(pollInterval) + defer ticker.Stop() + + deadline := time.NewTimer(maxWait) + defer deadline.Stop() + + var failures int + for { + info, done, err := p.check(ctx, c, workloadName) + switch { + case done: + // A URL that came up on the same tick the user interrupted is + // still a URL, and worth more to them than a detach note. + return info, nil + + case ctx.Err() != nil: + // Otherwise detaching wins over whatever the last read said: a + // read that was cancelled failed because the user asked it to. + detached(out, workloadName) + return nil, nil + + case err != nil: + failures++ + if failures >= maxReadFailures { + return nil, fmt.Errorf("checking the URL for %q: %w", workloadName, err) + } + + default: + failures = 0 + } + + select { + case <-ctx.Done(): + detached(out, workloadName) + return nil, nil + + case <-deadline.C: + return nil, fmt.Errorf( + "the URL for %q was still not answering after %s — it may yet come up; check it with: datumctl compute workloads describe %s", + workloadName, maxWait, workloadName) + + case <-ticker.C: + } + } +} + +// progress prints each stage of publishing exactly once, and repeats nothing. +type progress struct { + out io.Writer + started time.Time + seen map[string]bool +} + +// check reads current state and reports whether the URL is live, along with +// whatever went wrong reading it. The caller decides how much failure to ride +// out; a single failure means nothing, since the objects were written a moment +// ago and the next tick is a better answer than failing a deploy that is going +// fine. +// +// A workload that is simply not published yet is not a failure: there is +// nothing to report and nothing to give up over. +func (p *progress) check(ctx context.Context, c client.Client, workloadName string) (*Info, bool, error) { + info, err := ForWorkload(ctx, c, workloadName) + if err != nil { + return nil, false, err + } + if info == nil { + return nil, false, nil + } + + if info.Backends.Healthy > 0 { + p.line("Backends", fmt.Sprintf("%d healthy across %s", info.Backends.Healthy, strings.Join(servingCities(info), ", "))) + } + if info.EdgeProgrammed { + p.line("Edge", "programmed") + } + if info.CertificateIssued { + p.line("Certificate", "issued") + } + + if info.Live() { + return info, true, nil + } + + if time.Since(p.started) > blockingGrace { + p.blocking(info) + } + return nil, false, nil +} + +// line prints one progress row, skipping rows already printed with the same +// value. A count that changes as instances register is worth reprinting; a +// repeat of the same fact is not. +func (p *progress) line(label, value string) { + key := label + "\x00" + value + if p.seen[key] { + return + } + p.seen[key] = true + fmt.Fprintf(p.out, " %-*s %s\n", labelWidth, label, value) +} + +// blocking echoes, verbatim and once each, whatever the server says is holding +// the URL up. The CLI never interprets a reason: a condition the CLI has never +// heard of shows up here without a release. +func (p *progress) blocking(info *Info) { + p.blockingFrom(info.ServiceConditions, networkingv1alpha.NetworkServiceReady) + p.blockingFrom(info.ProxyConditions, networkingv1alpha.HTTPProxyConditionProgrammed) + for _, h := range info.Hostnames { + if !h.Active && h.Detail != "" { + p.note(fmt.Sprintf("%s: %s", h.Hostname, h.Detail)) + } + } +} + +func (p *progress) blockingFrom(conditions []metav1.Condition, condType string) { + reason, message, blocked := util.ReadinessBlock(conditions, condType) + if !blocked || reason == "" { + return + } + p.note(HumanBlock(reason, message)) +} + +func (p *progress) note(text string) { + if p.seen[text] { + return + } + p.seen[text] = true + fmt.Fprintf(p.out, " %-*s %s\n", labelWidth, "", text) +} + +// servingCities names the cities taking traffic, for the backends line. It +// falls back to every city with members so the line is never empty while the +// platform is still deciding what is in rotation. +func servingCities(info *Info) []string { + serving := make([]string, 0, len(info.Locations)) + all := make([]string, 0, len(info.Locations)) + for _, l := range info.Locations { + all = append(all, l.Location) + if l.Serving { + serving = append(serving, l.Location) + } + } + if len(serving) > 0 { + return serving + } + return all +} + +// applyService creates the NetworkService, or brings an existing one in line +// with what the workload now declares. +func applyService(ctx context.Context, c client.Client, desired *networkingv1alpha.NetworkService) error { + var existing networkingv1alpha.NetworkService + err := c.Get(ctx, client.ObjectKeyFromObject(desired), &existing) + if k8serrors.IsNotFound(err) { + if err := c.Create(ctx, desired); err != nil { + return fmt.Errorf("publishing backends for %q: %w", desired.Name, err) + } + return nil + } + if err != nil { + return fmt.Errorf("reading published backends for %q: %w", desired.Name, err) + } + + if reflect.DeepEqual(existing.Spec, desired.Spec) && metaCurrent(&existing, desired) { + return nil + } + existing.Spec = desired.Spec + adoptMeta(&existing, desired) + if err := c.Update(ctx, &existing); err != nil { + return fmt.Errorf("updating published backends for %q: %w", desired.Name, err) + } + return nil +} + +// applyProxy creates the HTTPProxy, or brings an existing one in line with +// what the workload now declares. +func applyProxy(ctx context.Context, c client.Client, desired *networkingv1alpha.HTTPProxy) error { + var existing networkingv1alpha.HTTPProxy + err := c.Get(ctx, client.ObjectKeyFromObject(desired), &existing) + if k8serrors.IsNotFound(err) { + if err := c.Create(ctx, desired); err != nil { + return fmt.Errorf("publishing URL for %q: %w", desired.Name, err) + } + return nil + } + if err != nil { + return fmt.Errorf("reading published URL for %q: %w", desired.Name, err) + } + + if reflect.DeepEqual(existing.Spec, desired.Spec) && metaCurrent(&existing, desired) { + return nil + } + existing.Spec = desired.Spec + adoptMeta(&existing, desired) + if err := c.Update(ctx, &existing); err != nil { + return fmt.Errorf("updating published URL for %q: %w", desired.Name, err) + } + return nil +} + +// metaCurrent reports whether an existing object already carries the labels +// and owner reference the desired object declares. +func metaCurrent(existing, desired client.Object) bool { + for k, v := range desired.GetLabels() { + if existing.GetLabels()[k] != v { + return false + } + } + return hasOwner(existing, desired) +} + +// hasOwner reports whether existing already references the desired owner. +func hasOwner(existing, desired client.Object) bool { + owners := desired.GetOwnerReferences() + if len(owners) == 0 { + return true + } + for _, o := range existing.GetOwnerReferences() { + if o.UID == owners[0].UID && o.Kind == owners[0].Kind { + return true + } + } + return false +} + +// adoptMeta merges the labels and owner reference onto an object that already +// exists, without dropping anything a user put there. +func adoptMeta(existing, desired client.Object) { + labels := existing.GetLabels() + if labels == nil { + labels = map[string]string{} + } + for k, v := range desired.GetLabels() { + labels[k] = v + } + existing.SetLabels(labels) + + if !hasOwner(existing, desired) { + existing.SetOwnerReferences(append(existing.GetOwnerReferences(), desired.GetOwnerReferences()...)) + } +} + +// Unpublish removes a workload's URL: the HTTPProxy first, then the +// NetworkService behind it. Deleting the service first would leave the proxy +// reporting a missing backend for as long as the delete takes. +// +// Objects that are not there are not an error — unpublishing something that +// was never published is a no-op, which is what `destroy` needs. +func Unpublish(ctx context.Context, c client.Client, workloadName string) error { + sel := client.MatchingLabels{computev1alpha.WorkloadNameLabel: workloadName} + ns := client.InNamespace(util.ResourceNamespace) + + if err := c.DeleteAllOf(ctx, &networkingv1alpha.HTTPProxy{}, ns, sel); err != nil && !notPublished(err) { + return fmt.Errorf("removing URL for %q: %w", workloadName, err) + } + if err := c.DeleteAllOf(ctx, &networkingv1alpha.NetworkService{}, ns, sel); err != nil && !notPublished(err) { + return fmt.Errorf("removing URL backends for %q: %w", workloadName, err) + } + + return nil +} diff --git a/internal/cmd/compute/url/publish_failure_test.go b/internal/cmd/compute/url/publish_failure_test.go new file mode 100644 index 00000000..8cb6e3e4 --- /dev/null +++ b/internal/cmd/compute/url/publish_failure_test.go @@ -0,0 +1,367 @@ +// SPDX-License-Identifier: AGPL-3.0-only + +package url + +import ( + "bytes" + "context" + "errors" + "strings" + "testing" + "time" + + "k8s.io/apimachinery/pkg/types" + "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/client/interceptor" + + "go.datum.net/compute/internal/cmd/compute/util" + networkingv1alpha "go.datum.net/network-services-operator/api/v1alpha" +) + +// failDeleteOf returns interceptor funcs that fail DeleteAllOf for one kind +// and pass everything else through, which is what a partial permission looks +// like from the CLI's side. +func failDeleteOf(kind string, boom error) interceptor.Funcs { + return interceptor.Funcs{ + DeleteAllOf: func(ctx context.Context, c client.WithWatch, obj client.Object, opts ...client.DeleteAllOfOption) error { + if kindOf(obj) == kind { + return boom + } + return c.DeleteAllOf(ctx, obj, opts...) + }, + } +} + +func objectExists(t *testing.T, c client.Client, obj client.Object) bool { + t.Helper() + err := c.Get(context.Background(), types.NamespacedName{Namespace: util.ResourceNamespace, Name: testWorkloadName}, obj) + if err == nil { + return true + } + if !notPublished(err) { + t.Fatalf("reading %s back: %v", kindOf(obj), err) + } + return false +} + +// TestUnpublishStopsWhenTheURLCannotBeRemoved: a partial delete has to be +// reported, and it has to stop. Removing the backends out from under a proxy +// that is still routing to them is the one ordering this package exists to +// prevent, so a failure on the proxy must not be followed by deleting the +// service anyway. +func TestUnpublishStopsWhenTheURLCannotBeRemoved(t *testing.T) { + boom := errors.New("forbidden") + c := interceptor.NewClient(newFakeClient(t, + publishedProxy(testWorkloadName, testCanonical), + publishedService(testWorkloadName, 8080, location("DFW", 1, 1, true)), + ), failDeleteOf(kindProxy, boom)) + + err := Unpublish(context.Background(), c, testWorkloadName) + if err == nil { + t.Fatal("a URL that could not be removed must be reported") + } + if !errors.Is(err, boom) { + t.Errorf("error = %v, want the server's failure to survive wrapping", err) + } + if !strings.Contains(err.Error(), testWorkloadName) { + t.Errorf("error = %q, want it to name the workload", err) + } + + if !objectExists(t, c, &networkingv1alpha.NetworkService{}) { + t.Error("the backends were deleted behind a proxy that is still routing to them") + } + // And the URL is still findable, so a retry has something to act on. + info, err := ForWorkload(context.Background(), c, testWorkloadName) + if err != nil || info == nil { + t.Fatalf("the URL must remain visible for a retry, got info=%v err=%v", info, err) + } +} + +// The other half of a partial delete: the URL is gone and its backends are +// not. This is the one that leaves an object behind with no proxy pointing at +// it, so the error has to say which of the two failed. +func TestUnpublishReportsAFailureToRemoveTheBackends(t *testing.T) { + boom := errors.New("forbidden") + c := interceptor.NewClient(newFakeClient(t, + publishedProxy(testWorkloadName, testCanonical), + publishedService(testWorkloadName, 8080, location("DFW", 1, 1, true)), + ), failDeleteOf(kindService, boom)) + + err := Unpublish(context.Background(), c, testWorkloadName) + if err == nil { + t.Fatal("backends that could not be removed must be reported") + } + if !errors.Is(err, boom) { + t.Errorf("error = %v, want the server's failure to survive wrapping", err) + } + if !strings.Contains(err.Error(), "backends") { + t.Errorf("error = %q, want it to distinguish the backends from the URL itself", err) + } + + if objectExists(t, c, &networkingv1alpha.HTTPProxy{}) { + t.Error("the proxy should already be gone: it is deleted first") + } + if !objectExists(t, c, &networkingv1alpha.NetworkService{}) { + t.Fatal("test is not exercising the leftover-backends case") + } + + // The state this leaves behind: nothing that looks up a workload's URL can + // see the leftover backends, because lookups key on the proxy. + info, err := ForWorkload(context.Background(), c, testWorkloadName) + if err != nil { + t.Fatalf("ForWorkload returned error: %v", err) + } + if info != nil { + t.Errorf("info = %+v, want nil — the proxy is gone", info) + } +} + +// A second Unpublish after a partial failure has to finish the job. This is +// the retry every caller advertises, and it is idempotent over the half that +// already succeeded. +func TestUnpublishRetryFinishesAPartialDelete(t *testing.T) { + fail := true + c := interceptor.NewClient(newFakeClient(t, + publishedProxy(testWorkloadName, testCanonical), + publishedService(testWorkloadName, 8080, location("DFW", 1, 1, true)), + ), interceptor.Funcs{ + DeleteAllOf: func(ctx context.Context, cl client.WithWatch, obj client.Object, opts ...client.DeleteAllOfOption) error { + if fail && kindOf(obj) == kindService { + return errors.New("forbidden") + } + return cl.DeleteAllOf(ctx, obj, opts...) + }, + }) + + if err := Unpublish(context.Background(), c, testWorkloadName); err == nil { + t.Fatal("expected the first attempt to fail on the backends") + } + + fail = false + if err := Unpublish(context.Background(), c, testWorkloadName); err != nil { + t.Fatalf("the retry must finish the job, got: %v", err) + } + if objectExists(t, c, &networkingv1alpha.NetworkService{}) { + t.Error("the leftover backends survived the retry") + } +} + +// TestPublishFailsOnTheProxyAfterWritingTheBackends pins what a failed publish +// leaves behind, and that the error names the URL rather than the backends +// that did get written — a user reading it has to know which step failed. +func TestPublishFailsOnTheProxyAfterWritingTheBackends(t *testing.T) { + boom := errors.New("admission webhook denied the request") + c := interceptor.NewClient(newFakeClient(t), interceptor.Funcs{ + Create: func(ctx context.Context, cl client.WithWatch, obj client.Object, opts ...client.CreateOption) error { + if kindOf(obj) == kindProxy { + return boom + } + return cl.Create(ctx, obj, opts...) + }, + }) + + ctx, cancel := context.WithTimeout(context.Background(), time.Second) + defer cancel() + + var out bytes.Buffer + info, err := Publish(ctx, &out, c, workloadNamed(testWorkloadName), testPortName, 8080, nil) + if err == nil { + t.Fatal("a proxy that could not be created must fail the publish") + } + if !errors.Is(err, boom) { + t.Errorf("error = %v, want the server's message to reach the user", err) + } + if !strings.Contains(err.Error(), "URL") { + t.Errorf("error = %q, want it to say the URL is what failed", err) + } + if info != nil { + t.Errorf("info = %+v, want nil", info) + } + if out.Len() != 0 { + t.Errorf("nothing was published, so no progress may be printed:\n%s", out.String()) + } + + // A retry has to be able to succeed, so the half that was written stays. + if !objectExists(t, c, &networkingv1alpha.NetworkService{}) { + t.Error("the backends should remain, so a retry is an update and not a rebuild") + } +} + +// A failure to write the backends must stop before the proxy is created: a +// proxy naming a service that does not exist reports a broken backend, which +// is the spurious failure the write ordering exists to avoid. +func TestPublishDoesNotCreateAProxyWithoutBackends(t *testing.T) { + boom := errors.New("quota exceeded") + rec := &recorder{} + c := interceptor.NewClient(newFakeClient(t), interceptor.Funcs{ + Create: func(ctx context.Context, cl client.WithWatch, obj client.Object, opts ...client.CreateOption) error { + rec.creates = append(rec.creates, kindOf(obj)) + if kindOf(obj) == kindService { + return boom + } + return cl.Create(ctx, obj, opts...) + }, + }) + + if _, err := Publish(context.Background(), &bytes.Buffer{}, c, workloadNamed(testWorkloadName), testPortName, 8080, nil); !errors.Is(err, boom) { + t.Fatalf("error = %v, want the create failure", err) + } + if len(rec.creates) != 1 || rec.creates[0] != kindService { + t.Errorf("creates = %v, want the backends attempted and nothing after", rec.creates) + } + if objectExists(t, c, &networkingv1alpha.HTTPProxy{}) { + t.Error("a proxy was created with no backends to point at") + } +} + +// TestPublishReportsAReadFailureRatherThanOverwriting: an existing object that +// cannot be read is not an object that can safely be replaced. Publishing has +// to stop, not fall through to a blind create or an update built on nothing. +func TestPublishReportsAReadFailureRatherThanOverwriting(t *testing.T) { + boom := errors.New("connection reset") + rec := &recorder{} + c := interceptor.NewClient(newFakeClient(t), interceptor.Funcs{ + Get: func(context.Context, client.WithWatch, client.ObjectKey, client.Object, ...client.GetOption) error { + return boom + }, + Create: func(ctx context.Context, cl client.WithWatch, obj client.Object, opts ...client.CreateOption) error { + rec.creates = append(rec.creates, kindOf(obj)) + return cl.Create(ctx, obj, opts...) + }, + Update: func(ctx context.Context, cl client.WithWatch, obj client.Object, opts ...client.UpdateOption) error { + rec.updates = append(rec.updates, kindOf(obj)) + return cl.Update(ctx, obj, opts...) + }, + }) + + if _, err := Publish(context.Background(), &bytes.Buffer{}, c, workloadNamed(testWorkloadName), testPortName, 8080, nil); !errors.Is(err, boom) { + t.Fatalf("error = %v, want the read failure", err) + } + if len(rec.creates) != 0 || len(rec.updates) != 0 { + t.Errorf("creates = %v, updates = %v, want nothing written on an unreadable control plane", rec.creates, rec.updates) + } +} + +// TestPublishStopsWhenTheURLCanNeverBeRead: publishing treats a read failure +// as transient and polls again, which is right for the blip it was written +// for. Nothing escalates, though, so a failure that is not transient — a +// missing list permission is the everyday one — never becomes anything. +// +// The context here is unbounded on purpose: that is the one a deploy passes, +// and the only reason the rest of this package's tests do not hang on this is +// that they all pass a deadline. +func TestPublishStopsWhenTheURLCanNeverBeRead(t *testing.T) { + c := interceptor.NewClient(newFakeClient(t, + publishedProxy(testWorkloadName, testCanonical), + publishedService(testWorkloadName, 8080, location("DFW", 1, 1, true)), + ), interceptor.Funcs{ + List: func(context.Context, client.WithWatch, client.ObjectList, ...client.ListOption) error { + return errors.New("httpproxies.networking.datumapis.com is forbidden") + }, + }) + + type result struct { + info *Info + err error + out string + } + done := make(chan result, 1) + go func() { + var out bytes.Buffer + info, err := Publish(context.Background(), &out, c, workloadNamed(testWorkloadName), testPortName, 8080, nil) + done <- result{info, err, out.String()} + }() + + select { + case got := <-done: + if got.err == nil && !strings.Contains(got.out, "forbidden") { + t.Errorf("publishing gave up silently; output:\n%s", got.out) + } + if got.err != nil && !strings.Contains(got.err.Error(), "forbidden") { + t.Errorf("error = %v, want the server's own words for why it gave up", got.err) + } + if got.info != nil { + t.Errorf("info = %+v, want nil — the URL was never read", got.info) + } + case <-time.After(10 * time.Second): + t.Fatal("Publish never returned: a deploy against a control plane it cannot read hangs indefinitely") + } +} + +// TestPublishRidesOutATransientReadFailure is the other half of giving up: a +// read that fails once must still be a blip. The objects were written a moment +// ago, and failing a deploy on the first hiccup would be worse than the hang +// this bound exists to stop. +func TestPublishRidesOutATransientReadFailure(t *testing.T) { + var lists int + c := interceptor.NewClient(newFakeClient(t, + publishedProxy(testWorkloadName, testCanonical), + publishedService(testWorkloadName, 8080, location("DFW", 1, 1, true)), + ), interceptor.Funcs{ + List: func(ctx context.Context, cl client.WithWatch, list client.ObjectList, opts ...client.ListOption) error { + lists++ + if lists == 1 { + return errors.New("etcdserver: request timed out") + } + return cl.List(ctx, list, opts...) + }, + }) + + ctx, cancel := context.WithTimeout(context.Background(), 30*time.Second) + defer cancel() + + var out bytes.Buffer + info, err := Publish(ctx, &out, c, workloadNamed(testWorkloadName), testPortName, 8080, nil) + if err != nil { + t.Fatalf("one failed read must not fail a publish, got: %v", err) + } + if info == nil || info.URL != testCanonicalURL { + t.Fatalf("info = %+v, want the live URL on the next tick", info) + } + if strings.Contains(out.String(), "timed out") { + t.Errorf("a blip was reported to the user:\n%s", out.String()) + } +} + +// TestPublishDetachesWhenInterruptedMidWrite: Ctrl-C is a detach wherever it +// lands, including during the writes that precede the wait. Returning the +// cancelled context as an error exits 1 on a keystroke the command's own help +// says is safe. +func TestPublishDetachesWhenInterruptedMidWrite(t *testing.T) { + for _, tc := range []struct{ name, kind string }{ + {"during the backends", kindService}, + {"during the URL", kindProxy}, + } { + t.Run(tc.name, func(t *testing.T) { + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + + // The interrupt arrives while this write is in flight, so the write + // fails with the cancelled context — exactly as a real client does. + c := interceptor.NewClient(newFakeClient(t), interceptor.Funcs{ + Create: func(ctx context.Context, cl client.WithWatch, obj client.Object, opts ...client.CreateOption) error { + if kindOf(obj) == tc.kind { + cancel() + return context.Canceled + } + return cl.Create(ctx, obj, opts...) + }, + }) + + var out bytes.Buffer + info, err := Publish(ctx, &out, c, workloadNamed(testWorkloadName), testPortName, 8080, nil) + if err != nil { + t.Fatalf("detaching is not an error, got: %v", err) + } + if info != nil { + t.Errorf("info = %+v, want nil when we stopped watching", info) + } + if !strings.Contains(out.String(), "Detached") { + t.Errorf("output = %q, want the same detach note the wait prints", out.String()) + } + if !strings.Contains(out.String(), "datumctl compute workloads describe "+testWorkloadName) { + t.Errorf("output = %q, want a pointer to how to pick it up again", out.String()) + } + }) + } +} diff --git a/internal/cmd/compute/url/publish_test.go b/internal/cmd/compute/url/publish_test.go new file mode 100644 index 00000000..987113d9 --- /dev/null +++ b/internal/cmd/compute/url/publish_test.go @@ -0,0 +1,233 @@ +// SPDX-License-Identifier: AGPL-3.0-only + +package url + +import ( + "bytes" + "context" + "strings" + "testing" + "time" + + "k8s.io/apimachinery/pkg/types" + "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/client/interceptor" + + "go.datum.net/compute/internal/cmd/compute/util" + networkingv1alpha "go.datum.net/network-services-operator/api/v1alpha" +) + +// recorder records the order of writes, which is the part of publishing that +// has to be right: the service exists before anything references it. +type recorder struct { + creates []string + updates []string + deletes []string +} + +func (r *recorder) funcs() interceptor.Funcs { + return interceptor.Funcs{ + Create: func(ctx context.Context, c client.WithWatch, obj client.Object, opts ...client.CreateOption) error { + r.creates = append(r.creates, kindOf(obj)) + return c.Create(ctx, obj, opts...) + }, + Update: func(ctx context.Context, c client.WithWatch, obj client.Object, opts ...client.UpdateOption) error { + r.updates = append(r.updates, kindOf(obj)) + return c.Update(ctx, obj, opts...) + }, + DeleteAllOf: func(ctx context.Context, c client.WithWatch, obj client.Object, opts ...client.DeleteAllOfOption) error { + r.deletes = append(r.deletes, kindOf(obj)) + return c.DeleteAllOf(ctx, obj, opts...) + }, + } +} + +func kindOf(obj client.Object) string { + switch obj.(type) { + case *networkingv1alpha.NetworkService: + return kindService + case *networkingv1alpha.HTTPProxy: + return kindProxy + default: + return "other" + } +} + +func TestPublishCreatesBackendsBeforeTheProxy(t *testing.T) { + rec := &recorder{} + c := interceptor.NewClient(newFakeClient(t), rec.funcs()) + + // Nothing ever reports the URL live here, so the wait runs until the + // context is cancelled — the Ctrl-C path. + ctx, cancel := context.WithTimeout(context.Background(), 150*time.Millisecond) + defer cancel() + + var out bytes.Buffer + info, err := Publish(ctx, &out, c, workloadNamed(testWorkloadName), testPortName, 8080, nil) + if err != nil { + t.Fatalf("detaching is not an error, got: %v", err) + } + if info != nil { + t.Errorf("info = %+v, want nil when we stopped watching", info) + } + + want := []string{kindService, kindProxy} + if len(rec.creates) != 2 || rec.creates[0] != want[0] || rec.creates[1] != want[1] { + t.Fatalf("creates = %v, want %v — a proxy naming a missing service reports a broken backend", rec.creates, want) + } + + if !strings.Contains(out.String(), "Detached") { + t.Errorf("output = %q, want a detach note", out.String()) + } + if !strings.Contains(out.String(), "datumctl compute workloads describe api") { + t.Errorf("output = %q, want a pointer to `datumctl compute workloads describe`", out.String()) + } + + var svc networkingv1alpha.NetworkService + if err := c.Get(context.Background(), types.NamespacedName{Namespace: util.ResourceNamespace, Name: testWorkloadName}, &svc); err != nil { + t.Fatalf("network service was not created: %v", err) + } + if svc.Spec.Ports[0].Port != 8080 { + t.Errorf("port = %d, want 8080", svc.Spec.Ports[0].Port) + } + + var proxy networkingv1alpha.HTTPProxy + if err := c.Get(context.Background(), types.NamespacedName{Namespace: util.ResourceNamespace, Name: testWorkloadName}, &proxy); err != nil { + t.Fatalf("proxy was not created: %v", err) + } + if proxy.Spec.Rules[0].Backends[0].NetworkService.Name != testWorkloadName { + t.Errorf("backend = %+v, want a reference to the service", proxy.Spec.Rules[0].Backends[0]) + } +} + +func TestPublishReturnsWhenTheURLIsLive(t *testing.T) { + rec := &recorder{} + c := interceptor.NewClient(newFakeClient(t, + publishedProxy(testWorkloadName, testCanonical), + publishedService(testWorkloadName, 8080, location("DFW", 2, 2, true), location("IAD", 2, 2, true)), + ), rec.funcs()) + + ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second) + defer cancel() + + var out bytes.Buffer + info, err := Publish(ctx, &out, c, workloadNamed(testWorkloadName), testPortName, 8080, nil) + if err != nil { + t.Fatalf("Publish returned error: %v", err) + } + if info == nil || info.URL != testCanonicalURL { + t.Fatalf("info = %+v, want the live URL", info) + } + + // Already published and unchanged: no writes at all. + if len(rec.creates) != 0 || len(rec.updates) != 0 { + t.Errorf("creates = %v, updates = %v, want none for an unchanged workload", rec.creates, rec.updates) + } + + got := out.String() + for _, want := range []string{ + "Backends 4 healthy across DFW, IAD", + "Edge programmed", + "Certificate issued", + } { + if !strings.Contains(got, want) { + t.Errorf("output missing %q:\n%s", want, got) + } + } + // The URL is the caller's line to print, not this package's. + if strings.Contains(got, "https://") { + t.Errorf("Publish should not print the URL itself:\n%s", got) + } +} + +func TestPublishUpdatesAChangedPort(t *testing.T) { + rec := &recorder{} + c := interceptor.NewClient(newFakeClient(t, + publishedProxy(testWorkloadName, testCanonical), + publishedService(testWorkloadName, 8080, location("DFW", 1, 1, true)), + ), rec.funcs()) + + ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second) + defer cancel() + + if _, err := Publish(ctx, &bytes.Buffer{}, c, workloadNamed(testWorkloadName), testPortName, 9090, nil); err != nil { + t.Fatalf("Publish returned error: %v", err) + } + + var svc networkingv1alpha.NetworkService + if err := c.Get(context.Background(), types.NamespacedName{Namespace: util.ResourceNamespace, Name: testWorkloadName}, &svc); err != nil { + t.Fatalf("getting service: %v", err) + } + if svc.Spec.Ports[0].Port != 9090 { + t.Errorf("port = %d, want the new port 9090", svc.Spec.Ports[0].Port) + } + if len(rec.updates) != 1 || rec.updates[0] != kindService { + t.Errorf("updates = %v, want the service alone", rec.updates) + } +} + +func TestPublishAdoptsAnExistingUnlabelledObject(t *testing.T) { + // An object written before the labels existed must be brought in line, or + // lookups would never find it again. + svc := BuildNetworkService(workloadNamed(testWorkloadName), testPortName, 8080) + svc.Labels = nil + svc.OwnerReferences = nil + proxy := BuildHTTPProxy(workloadNamed(testWorkloadName), testPortName, nil) + proxy.Labels = nil + proxy.OwnerReferences = nil + + c := newFakeClient(t, svc, proxy) + + ctx, cancel := context.WithTimeout(context.Background(), 150*time.Millisecond) + defer cancel() + if _, err := Publish(ctx, &bytes.Buffer{}, c, workloadNamed(testWorkloadName), testPortName, 8080, nil); err != nil { + t.Fatalf("Publish returned error: %v", err) + } + + var got networkingv1alpha.NetworkService + if err := c.Get(context.Background(), types.NamespacedName{Namespace: util.ResourceNamespace, Name: testWorkloadName}, &got); err != nil { + t.Fatalf("getting service: %v", err) + } + assertPublishedLabels(t, got.Labels, workloadNamed(testWorkloadName)) + assertOwnerRef(t, got.OwnerReferences, workloadNamed(testWorkloadName)) +} + +func TestUnpublishRemovesTheProxyFirst(t *testing.T) { + rec := &recorder{} + c := interceptor.NewClient(newFakeClient(t, + publishedProxy(testWorkloadName, testCanonical), + publishedService(testWorkloadName, 8080, location("DFW", 1, 1, true)), + publishedProxy("web", "e5f6a7b8.datumproxy.net"), + publishedService("web", 3000, location("DFW", 1, 1, true)), + ), rec.funcs()) + + if err := Unpublish(context.Background(), c, testWorkloadName); err != nil { + t.Fatalf("Unpublish returned error: %v", err) + } + + want := []string{kindProxy, kindService} + if len(rec.deletes) != 2 || rec.deletes[0] != want[0] || rec.deletes[1] != want[1] { + t.Fatalf("deletes = %v, want %v — removing the backends first leaves the proxy reporting a missing backend", rec.deletes, want) + } + + info, err := ForWorkload(context.Background(), c, testWorkloadName) + if err != nil { + t.Fatalf("ForWorkload returned error: %v", err) + } + if info != nil { + t.Errorf("api still published: %+v", info) + } + + // Another workload's URL is untouched. + other, err := ForWorkload(context.Background(), c, "web") + if err != nil || other == nil { + t.Fatalf("web should be untouched, got info=%+v err=%v", other, err) + } +} + +func TestUnpublishIsANoOpWhenNothingIsPublished(t *testing.T) { + c := newFakeClient(t) + if err := Unpublish(context.Background(), c, testWorkloadName); err != nil { + t.Fatalf("unpublishing something that was never published is not an error, got: %v", err) + } +} diff --git a/internal/cmd/compute/url/reason.go b/internal/cmd/compute/url/reason.go new file mode 100644 index 00000000..2be4040c --- /dev/null +++ b/internal/cmd/compute/url/reason.go @@ -0,0 +1,73 @@ +package url + +import ( + "strings" + "unicode" +) + +// HumanBlock renders what the server says is holding something up, in words a +// developer can act on. +// +// The platform reports a blocked state as a camelCase reason plus a human +// message. Product principle 4 forbids showing the reason: "NoMatchingInterfaces" +// is internal state, and a developer who has never read the API types cannot do +// anything with it. The message is what was written for them, so it wins +// whenever there is one. +// +// When a condition carries no message there is still something worth saying, so +// the reason is spaced out and lowercased rather than dropped: "no matching +// interfaces" tells a developer more than silence and still never shows them an +// identifier. This is formatting, not interpretation — nothing here branches on +// which reason it was given, per the house rule in util/conditions.go. +func HumanBlock(reason, message string) string { + if message != "" { + return message + } + return humanizeReason(reason) +} + +// humanizeReason turns a camelCase condition reason into a lowercase phrase. +// Runs of capitals are kept together so "CertificateCARequired" reads as +// "certificate CA required" rather than "certificate c a required". +func humanizeReason(reason string) string { + if reason == "" { + return statusPending + } + + runes := []rune(reason) + var b strings.Builder + for i, r := range runes { + if i > 0 && unicode.IsUpper(r) { + prev := runes[i-1] + // A capital after a lowercase always starts a word; a capital that + // ends a run of capitals starts one only if a lowercase follows it. + startsWord := !unicode.IsUpper(prev) || + (i+1 < len(runes) && unicode.IsLower(runes[i+1])) + if startsWord { + b.WriteRune(' ') + } + } + b.WriteRune(r) + } + + words := strings.Fields(b.String()) + for i, w := range words { + // Leave acronyms as the server wrote them; lowercase ordinary words. + if !isAcronym(w) { + words[i] = strings.ToLower(w) + } + } + return strings.Join(words, " ") +} + +func isAcronym(w string) bool { + if len(w) < 2 { + return false + } + for _, r := range w { + if !unicode.IsUpper(r) && !unicode.IsDigit(r) { + return false + } + } + return true +} diff --git a/internal/cmd/compute/url/reason_test.go b/internal/cmd/compute/url/reason_test.go new file mode 100644 index 00000000..69cde865 --- /dev/null +++ b/internal/cmd/compute/url/reason_test.go @@ -0,0 +1,46 @@ +// SPDX-License-Identifier: AGPL-3.0-only + +package url + +import "testing" + +// TestHumanBlockNeverShowsAReason pins product principle 4: a developer sees +// what the server wrote for them, never the identifier it filed it under. +func TestHumanBlockNeverShowsAReason(t *testing.T) { + for _, tc := range []struct { + name string + reason string + message string + want string + }{{ + name: "the message wins whenever there is one", + reason: "NoMatchingInterfaces", + message: "selector matched no network interface", + want: "selector matched no network interface", + }, { + name: "a message-less reason is spaced out, not dropped", + reason: "NoMatchingInterfaces", + want: "no matching interfaces", + }, { + name: "a reason the CLI has never heard of still reads as words", + reason: "SomeReasonTheCLIHasNeverHeardOf", + want: "some reason the CLI has never heard of", + }, { + name: "acronyms survive", + reason: "CertificateCARequired", + want: "certificate CA required", + }, { + name: "a single word", + reason: "Pending", + want: "pending", + }, { + name: "nothing at all still says something", + want: statusPending, + }} { + t.Run(tc.name, func(t *testing.T) { + if got := HumanBlock(tc.reason, tc.message); got != tc.want { + t.Errorf("HumanBlock(%q, %q) = %q, want %q", tc.reason, tc.message, got, tc.want) + } + }) + } +} diff --git a/internal/cmd/compute/url/redeploy_test.go b/internal/cmd/compute/url/redeploy_test.go new file mode 100644 index 00000000..364a9655 --- /dev/null +++ b/internal/cmd/compute/url/redeploy_test.go @@ -0,0 +1,147 @@ +// SPDX-License-Identifier: AGPL-3.0-only + +package url + +import ( + "context" + "testing" + + "k8s.io/apimachinery/pkg/types" + "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/client/interceptor" + + "go.datum.net/compute/internal/cmd/compute/util" + networkingv1alpha "go.datum.net/network-services-operator/api/v1alpha" +) + +// writeCounts records what a redeploy actually sends to the API server. +type writeCounts struct { + creates int + updates int +} + +func countingClient(t *testing.T, counts *writeCounts, objs ...client.Object) client.WithWatch { + t.Helper() + return interceptor.NewClient(newFakeClient(t, objs...), interceptor.Funcs{ + Create: func(ctx context.Context, cl client.WithWatch, obj client.Object, opts ...client.CreateOption) error { + counts.creates++ + return cl.Create(ctx, obj, opts...) + }, + Update: func(ctx context.Context, cl client.WithWatch, obj client.Object, opts ...client.UpdateOption) error { + counts.updates++ + return cl.Update(ctx, obj, opts...) + }, + }) +} + +// TestDeclareIsIdempotent: redeploying an unchanged workload must not try to +// create a URL that already exists, and must not churn the objects either. The +// first Declare creates both; a second identical one writes nothing at all. +// +// This is the shape of every redeploy — `deploy` calls Declare on each run, +// including the runs that only change the image. +func TestDeclareIsIdempotent(t *testing.T) { + var counts writeCounts + w := workloadNamed(testWorkloadName) + c := countingClient(t, &counts) + + if err := Declare(context.Background(), c, w, "http", 8080, nil); err != nil { + t.Fatalf("first declare: %v", err) + } + if counts.creates != 2 { + t.Fatalf("creates = %d, want 2 (the backends and the URL)", counts.creates) + } + if counts.updates != 0 { + t.Errorf("updates = %d on a first declare, want 0", counts.updates) + } + + counts = writeCounts{} + if err := Declare(context.Background(), c, w, "http", 8080, nil); err != nil { + t.Fatalf("second declare: %v", err) + } + if counts.creates != 0 { + t.Errorf("creates = %d on redeploy, want 0 — a URL that exists must not be created again", counts.creates) + } + if counts.updates != 0 { + t.Errorf("updates = %d on an unchanged redeploy, want 0 — nothing changed, so nothing should be written", counts.updates) + } +} + +// TestDeclareUpdatesAChangedPort: the counterpart. Idempotence must not mean +// inertness — a workload that moves to another port has to take its URL with +// it, or the URL keeps routing to a port nothing answers on. +func TestDeclareUpdatesAChangedPort(t *testing.T) { + var counts writeCounts + w := workloadNamed(testWorkloadName) + c := countingClient(t, &counts) + + if err := Declare(context.Background(), c, w, "http", 8080, nil); err != nil { + t.Fatalf("first declare: %v", err) + } + + counts = writeCounts{} + if err := Declare(context.Background(), c, w, "http", 9090, nil); err != nil { + t.Fatalf("redeclare on a new port: %v", err) + } + if counts.creates != 0 { + t.Errorf("creates = %d, want 0 — the objects already exist", counts.creates) + } + if counts.updates == 0 { + t.Error("a changed port must be written through to the backends") + } + + info, err := ForWorkload(context.Background(), c, testWorkloadName) + if err != nil { + t.Fatalf("reading back: %v", err) + } + if info == nil { + t.Fatal("the workload lost its URL on redeploy") + } + if info.Port != 9090 { + t.Errorf("port = %d, want 9090", info.Port) + } +} + +// TestDeclareKeepsTheCanonicalHostnameAcrossRedeploys: the managed URL is the +// one a developer has already shared and scripted against. A redeploy that +// replaced the HTTPProxy rather than updating it would issue a new +// .datumproxy.net and silently break every existing reference. +func TestDeclareKeepsTheCanonicalHostnameAcrossRedeploys(t *testing.T) { + w := workloadNamed(testWorkloadName) + c := newFakeClient(t) + + if err := Declare(context.Background(), c, w, "http", 8080, nil); err != nil { + t.Fatalf("first declare: %v", err) + } + + // Stand in for the platform assigning the canonical hostname. + var proxy networkingv1alpha.HTTPProxy + key := types.NamespacedName{Namespace: util.ResourceNamespace, Name: ResourceName(testWorkloadName)} + if err := c.Get(context.Background(), key, &proxy); err != nil { + t.Fatalf("reading the URL back: %v", err) + } + proxy.Status.CanonicalHostname = testCanonical + if err := c.Status().Update(context.Background(), &proxy); err != nil { + t.Fatalf("seeding the canonical hostname: %v", err) + } + + if err := Declare(context.Background(), c, w, "http", 9090, nil); err != nil { + t.Fatalf("redeclare: %v", err) + } + + info, err := ForWorkload(context.Background(), c, testWorkloadName) + if err != nil { + t.Fatalf("reading back: %v", err) + } + if info == nil || info.CanonicalHostname != testCanonical { + t.Fatalf("canonical hostname = %q, want %q — a redeploy must not reissue the URL", + infoCanonical(info), testCanonical) + } +} + +func infoCanonical(i *Info) string { + if i == nil { + return "" + } + return i.CanonicalHostname +} diff --git a/internal/cmd/compute/url/render.go b/internal/cmd/compute/url/render.go new file mode 100644 index 00000000..3e156c73 --- /dev/null +++ b/internal/cmd/compute/url/render.go @@ -0,0 +1,178 @@ +// SPDX-License-Identifier: AGPL-3.0-only + +package url + +import ( + "fmt" + "io" + "strings" + + "go.datum.net/compute/internal/cmd/compute/util" + networkingv1alpha "go.datum.net/network-services-operator/api/v1alpha" +) + +// RenderDetail writes the per-URL detail view: where the URL is, what backs +// it, and — the part a multi-location platform owes its users — which one is +// actually serving. +// +// Nothing here names the machinery. When something is wrong, the server's own +// reason and message are printed verbatim, so a condition this CLI has never +// heard of still reaches the user. +// +// A nil Info means the workload has no URL; the caller says so in its own +// words, because only it knows how the user asked. +func RenderDetail(out io.Writer, info *Info) { + if info == nil { + return + } + + renderURLs(out, info) + renderBackendLine(out, info) + fmt.Fprintln(out) + // "Serving", not "Health": this block is embedded under a workload's own + // Health line, and two identically labelled rows at the same indent read as + // a contradiction rather than two different facts. Backend health is about + // whether the URL is taking traffic. + fmt.Fprintf(out, "%-*s %s\n", labelWidth, "Serving", health(info)) + + if len(info.Locations) > 0 { + fmt.Fprintln(out) + renderLocations(out, info) + } + + renderDiagnosis(out, info) +} + +// renderURLs lists every hostname the URL answers on, the working one first. +// A hostname that is not serving carries the server's reason beside it. +func renderURLs(out io.Writer, info *Info) { + label := "URL" + if len(info.Hostnames) == 0 { + fmt.Fprintf(out, "%-*s %s\n", labelWidth, label, "—") + return + } + + for _, h := range info.Hostnames { + line := h.URL + if !h.Active { + line += " (" + h.Status + ")" + } + fmt.Fprintf(out, "%-*s %s\n", labelWidth, label, line) + label = "" + } +} + +// renderBackendLine states what the edge forwards to. +func renderBackendLine(out io.Writer, info *Info) { + if info.Port == 0 { + return + } + protocol := strings.ToLower(info.Protocol) + if protocol == "" { + protocol = strings.ToLower(string(networkingv1alpha.NetworkServiceProtocolTCP)) + } + fmt.Fprintf(out, "%-*s port %d/%s\n", labelWidth, "Backend", info.Port, protocol) +} + +// renderLocations prints the per-location breakdown, indented under the label +// column so it reads as part of the health block. +func renderLocations(out io.Writer, info *Info) { + indent := strings.Repeat(" ", labelWidth+1) + tw := util.NewTabWriter(out) + fmt.Fprintf(tw, "%sLOCATION\tBACKENDS\tHEALTHY\tSERVING\n", indent) + for _, l := range info.Locations { + fmt.Fprintf(tw, "%s%s\t%d\t%d\t%s\n", indent, l.Location, l.Backends, l.Healthy, yesNo(l.Serving)) + } + _ = tw.Flush() +} + +// health summarises the URL in one line, from counts rather than from any +// reason string. +func health(info *Info) string { + b := info.Backends + switch { + case b.Total == 0: + return "Unavailable — no backends registered" + case b.Healthy == 0: + return fmt.Sprintf("Unavailable — 0 of %d backends healthy", b.Total) + case b.Healthy < b.Total: + return fmt.Sprintf("Degraded — %d of %d backends healthy", b.Healthy, b.Total) + default: + return fmt.Sprintf("Healthy — %d of %d backends healthy", b.Healthy, b.Total) + } +} + +// renderDiagnosis explains a URL that is not fully healthy and says what to +// run next. A healthy URL gets nothing: there is nothing to do. +func renderDiagnosis(out io.Writer, info *Info) { + var unhealthy []string + for _, l := range info.Locations { + if l.Backends > 0 && l.Healthy == 0 { + unhealthy = append(unhealthy, l.Location) + } + } + + blocking := blockingLines(info) + if len(unhealthy) == 0 && len(blocking) == 0 { + return + } + + fmt.Fprintln(out) + indent := strings.Repeat(" ", 7) + + for _, location := range unhealthy { + fmt.Fprintf(out, " %s: no healthy backends — instances are running but not passing health checks.\n", location) + serving := servingCities(info) + switch { + case len(serving) == 0: + fmt.Fprintf(out, "%sNo location is taking traffic, so the URL is not answering.\n", indent) + case len(serving) == 1: + fmt.Fprintf(out, "%sTraffic is being served from %s only.\n", indent, serving[0]) + default: + fmt.Fprintf(out, "%sTraffic is being served from %s.\n", indent, strings.Join(serving, ", ")) + } + } + + for _, line := range blocking { + fmt.Fprintf(out, " %s\n", line) + } + + fmt.Fprintln(out) + fmt.Fprintln(out, " Next steps:") + if len(unhealthy) == 0 { + fmt.Fprintf(out, " Check instances: datumctl compute instances --workload=%s\n", info.WorkloadName) + return + } + for _, location := range unhealthy { + fmt.Fprintf(out, " Check instances: datumctl compute instances --workload=%s --location=%s\n", info.WorkloadName, location) + } +} + +// blockingLines collects what the server says is wrong, in the server's own +// words. The condition reason is never shown — see HumanBlock. +func blockingLines(info *Info) []string { + var lines []string + add := func(reason, message string) { + lines = append(lines, HumanBlock(reason, message)) + } + + if reason, message, blocked := util.ReadinessBlock(info.ServiceConditions, networkingv1alpha.NetworkServiceReady); blocked && reason != "" { + add(reason, message) + } + if reason, message, blocked := util.ReadinessBlock(info.ProxyConditions, networkingv1alpha.HTTPProxyConditionProgrammed); blocked && reason != "" { + add(reason, message) + } + for _, h := range info.Hostnames { + if !h.Active && h.Detail != "" { + lines = append(lines, fmt.Sprintf("%s: %s", h.Hostname, h.Detail)) + } + } + return lines +} + +func yesNo(v bool) string { + if v { + return "yes" + } + return "no" +} diff --git a/internal/cmd/compute/url/render_test.go b/internal/cmd/compute/url/render_test.go new file mode 100644 index 00000000..40af88fa --- /dev/null +++ b/internal/cmd/compute/url/render_test.go @@ -0,0 +1,191 @@ +// SPDX-License-Identifier: AGPL-3.0-only + +package url + +import ( + "bytes" + "strings" + "testing" + + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + + networkingv1alpha "go.datum.net/network-services-operator/api/v1alpha" +) + +// row returns the whitespace-separated fields of the first line containing the +// given first field, so assertions do not depend on column widths. +func row(t *testing.T, output, first string) []string { + t.Helper() + for _, line := range strings.Split(output, "\n") { + fields := strings.Fields(line) + if len(fields) > 0 && fields[0] == first { + return fields + } + } + t.Fatalf("no line starting with %q in:\n%s", first, output) + return nil +} + +func TestRenderDetailDegraded(t *testing.T) { + info := newInfo(testWorkloadName, + publishedProxy(testWorkloadName, testCanonical), + publishedService(testWorkloadName, 8080, location("DFW", 2, 2, true), location("IAD", 2, 0, false)), + ) + + var out bytes.Buffer + RenderDetail(&out, info) + got := out.String() + + if f := row(t, got, "URL"); f[1] != testCanonicalURL { + t.Errorf("URL row = %v", f) + } + if f := row(t, got, "Backend"); f[1] != "port" || f[2] != "8080/tcp" { + t.Errorf("Backend row = %v, want port 8080/tcp", f) + } + // "Serving", not "Health": this block renders under a workload's own Health + // line in `workloads describe`, so the labels have to stay distinguishable. + if !strings.Contains(got, "Serving Degraded — 2 of 4 backends healthy") { + t.Errorf("missing the health summary:\n%s", got) + } + + if f := row(t, got, "LOCATION"); strings.Join(f, " ") != "LOCATION BACKENDS HEALTHY SERVING" { + t.Errorf("table header = %v", f) + } + if f := row(t, got, "DFW"); strings.Join(f[1:], " ") != "2 2 yes" { + t.Errorf("DFW row = %v, want 2 2 yes", f) + } + if f := row(t, got, "IAD:"); len(f) == 0 { + t.Error("expected a narrative line for the unhealthy location") + } + if f := row(t, got, "IAD"); strings.Join(f[1:], " ") != "2 0 no" { + t.Errorf("IAD row = %v, want 2 0 no", f) + } + + for _, want := range []string{ + "IAD: no healthy backends — instances are running but not passing health checks.", + "Traffic is being served from DFW only.", + "Next steps:", + "datumctl compute instances --workload=api --location=IAD", + } { + if !strings.Contains(got, want) { + t.Errorf("output missing %q:\n%s", want, got) + } + } + + // The user never sees the machinery. + for _, forbidden := range []string{kindService, kindProxy, "http://"} { + if strings.Contains(got, forbidden) { + t.Errorf("output names the machinery %q:\n%s", forbidden, got) + } + } +} + +func TestRenderDetailHealthySaysNothingToDo(t *testing.T) { + info := newInfo(testWorkloadName, + publishedProxy(testWorkloadName, testCanonical), + publishedService(testWorkloadName, 8080, location("DFW", 2, 2, true), location("IAD", 2, 2, true)), + ) + + var out bytes.Buffer + RenderDetail(&out, info) + got := out.String() + + if !strings.Contains(got, "Healthy — 4 of 4 backends healthy") { + t.Errorf("missing the health summary:\n%s", got) + } + if strings.Contains(got, "Next steps") { + t.Errorf("a healthy URL needs no next steps:\n%s", got) + } + if f := row(t, got, "IAD"); strings.Join(f[1:], " ") != "2 2 yes" { + t.Errorf("IAD row = %v", f) + } +} + +func TestRenderDetailBothHostnames(t *testing.T) { + proxy := publishedProxy(testWorkloadName, testCanonical) + proxy.Spec.Hostnames = append(proxy.Spec.Hostnames, testCustomHostname) + proxy.Status.HostnameStatuses = []networkingv1alpha.HostnameStatus{{ + Hostname: testCustomHostname, + Conditions: []metav1.Condition{ + cond(networkingv1alpha.HostnameConditionVerified, metav1.ConditionTrue, "Verified", ""), + cond(networkingv1alpha.HostnameConditionCertificateReady, metav1.ConditionTrue, "CertificateIssued", ""), + }, + }} + + var out bytes.Buffer + RenderDetail(&out, newInfo(testWorkloadName, proxy, publishedService(testWorkloadName, 8080, location("DFW", 1, 1, true)))) + got := out.String() + + lines := strings.Split(got, "\n") + if !strings.HasPrefix(lines[0], "URL") || !strings.Contains(lines[0], testCustomURL) { + t.Errorf("first line = %q, want the custom hostname", lines[0]) + } + if strings.Contains(lines[1], "URL") || !strings.Contains(lines[1], testCanonicalURL) { + t.Errorf("second line = %q, want the managed hostname under an empty label", lines[1]) + } +} + +func TestRenderDetailShowsTheServersOwnWords(t *testing.T) { + proxy := publishedProxy(testWorkloadName, testCanonical) + svc := publishedService(testWorkloadName, 8080) + svc.Status.Conditions = []metav1.Condition{ + cond(networkingv1alpha.NetworkServiceReady, metav1.ConditionFalse, + "SomeReasonTheCLIHasNeverHeardOf", "selector matched no network interface"), + } + + var out bytes.Buffer + RenderDetail(&out, newInfo(testWorkloadName, proxy, svc)) + got := out.String() + + // The server's message verbatim, and never its reason: a reason the CLI has + // never heard of is still an identifier, and product principle 4 keeps those + // away from the developer. TestRenderDetailPendingHostnameCarriesItsStatus + // asserts the same rule for hostnames. + if !strings.Contains(got, "selector matched no network interface") { + t.Errorf("the server's message must appear verbatim:\n%s", got) + } + if strings.Contains(got, "SomeReasonTheCLIHasNeverHeardOf") { + t.Errorf("a raw condition reason reached the user:\n%s", got) + } + if !strings.Contains(got, "Unavailable — no backends registered") { + t.Errorf("missing the health summary:\n%s", got) + } + if !strings.Contains(got, "datumctl compute instances --workload=api") { + t.Errorf("missing next steps:\n%s", got) + } +} + +func TestRenderDetailPendingHostnameCarriesItsStatus(t *testing.T) { + proxy := publishedProxy(testWorkloadName, testCanonical) + proxy.Spec.Hostnames = append(proxy.Spec.Hostnames, testCustomHostname) + proxy.Status.HostnameStatuses = []networkingv1alpha.HostnameStatus{{ + Hostname: testCustomHostname, + Conditions: []metav1.Condition{ + cond(networkingv1alpha.HostnameConditionVerified, metav1.ConditionFalse, "DomainNotVerified", "waiting for TXT record"), + }, + }} + + var out bytes.Buffer + RenderDetail(&out, newInfo(testWorkloadName, proxy, publishedService(testWorkloadName, 8080, location("DFW", 1, 1, true)))) + got := out.String() + + // The server's message, not its reason: "DomainNotVerified" is internal + // state and a developer must never be shown it. + if !strings.Contains(got, "https://api.example.com (waiting for TXT record)") { + t.Errorf("a pending hostname should carry the server's message:\n%s", got) + } + if strings.Contains(got, "DomainNotVerified") { + t.Errorf("a raw condition reason reached the user:\n%s", got) + } + if !strings.Contains(got, "api.example.com: waiting for TXT record") { + t.Errorf("the server's message should be shown verbatim:\n%s", got) + } +} + +func TestRenderDetailNilWritesNothing(t *testing.T) { + var out bytes.Buffer + RenderDetail(&out, nil) + if out.Len() != 0 { + t.Errorf("output = %q, want nothing — the caller words the no-URL case", out.String()) + } +} diff --git a/internal/cmd/compute/url/resources.go b/internal/cmd/compute/url/resources.go new file mode 100644 index 00000000..296937db --- /dev/null +++ b/internal/cmd/compute/url/resources.go @@ -0,0 +1,156 @@ +// SPDX-License-Identifier: AGPL-3.0-only + +// Package url owns the single mechanism by which a compute workload becomes a +// public HTTPS URL: a NetworkService that selects the workload's network +// interfaces by label, and an HTTPProxy whose only backend names that service. +// +// Every command that shows, creates, or removes a workload URL goes through +// this package so the two objects are always built, found, and deleted the +// same way. Nothing here prints machinery names — the vocabulary the user sees +// is "URL", "backends", "edge", and "certificate". +package url + +import ( + "fmt" + "strings" + + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + gatewayv1 "sigs.k8s.io/gateway-api/apis/v1" + + computev1alpha "go.datum.net/compute/api/v1alpha" + "go.datum.net/compute/internal/cmd/compute/util" + networkingv1alpha "go.datum.net/network-services-operator/api/v1alpha" +) + +// maxPortNameLength is the DNS-label limit NetworkServicePort.Name enforces. +const maxPortNameLength = 63 + +// ResourceName returns the name shared by the NetworkService and the HTTPProxy +// that publish a workload. Both objects are named after the workload so a +// human reading `datumctl get` output can tell what they belong to; lookups +// never depend on it, they select on labels. +func ResourceName(workloadName string) string { + return workloadName +} + +// PortName derives the NetworkServicePort name for a workload port. +// +// computev1alpha.NamedPort.Name has no pattern constraint, but +// NetworkServicePort.Name (and the backend reference naming it) must be a DNS +// label: lowercase alphanumerics and dashes, starting and ending with an +// alphanumeric, at most 63 characters. The name is sanitized to fit. A name +// with nothing usable in it is an error rather than an object the API server +// would reject with a message about a field the user never typed. +func PortName(p computev1alpha.NamedPort) (string, error) { + if strings.TrimSpace(p.Name) == "" { + return "", fmt.Errorf("port %d has no name — name the port to publish it on a URL", p.Port) + } + + var b strings.Builder + for _, r := range strings.ToLower(p.Name) { + switch { + case r >= 'a' && r <= 'z', r >= '0' && r <= '9': + b.WriteRune(r) + default: + b.WriteRune('-') + } + } + + name := strings.Trim(b.String(), "-") + if len(name) > maxPortNameLength { + name = strings.Trim(name[:maxPortNameLength], "-") + } + if name == "" { + return "", fmt.Errorf("port name %q cannot be used for a URL — use letters, digits and dashes", p.Name) + } + return name, nil +} + +// objectMeta returns the metadata both published objects share: the workload's +// namespace-scoped name, the labels every lookup selects on, and an owner +// reference back to the workload. +// +// The owner reference is belt and braces. Garbage collection in a project +// virtual control plane is unverified, so Unpublish deletes both objects +// explicitly and lookups match on labels; the reference exists so that a +// control plane which does collect owned objects does the right thing. +func objectMeta(w *computev1alpha.Workload) metav1.ObjectMeta { + return metav1.ObjectMeta{ + Name: ResourceName(w.Name), + Namespace: util.ResourceNamespace, + Labels: map[string]string{ + computev1alpha.WorkloadNameLabel: w.Name, + computev1alpha.WorkloadUIDLabel: string(w.UID), + }, + OwnerReferences: []metav1.OwnerReference{{ + APIVersion: computev1alpha.GroupVersion.String(), + Kind: "Workload", + Name: w.Name, + UID: w.UID, + Controller: ptr(false), + BlockOwnerDeletion: ptr(false), + }}, + } +} + +// BuildNetworkService returns the NetworkService that gathers the workload's +// instances into one set of backends. Membership is selected by label, so +// instances appearing, disappearing and moving between cities need no edit. +// +// TrafficDistribution is deliberately left unset: the default serves each +// request from the location nearest the edge that received it, which is what +// is wanted without saying so. +func BuildNetworkService(w *computev1alpha.Workload, portName string, port int32) *networkingv1alpha.NetworkService { + return &networkingv1alpha.NetworkService{ + ObjectMeta: objectMeta(w), + Spec: networkingv1alpha.NetworkServiceSpec{ + NetworkInterfaces: networkingv1alpha.NetworkServiceInterfaceSelector{ + Selector: metav1.LabelSelector{ + MatchLabels: map[string]string{ + computev1alpha.WorkloadNameLabel: w.Name, + }, + }, + }, + Ports: []networkingv1alpha.NetworkServicePort{{ + Name: portName, + Port: port, + Protocol: networkingv1alpha.NetworkServiceProtocolTCP, + }}, + }, + } +} + +// BuildHTTPProxy returns the HTTPProxy that puts the workload on the internet. +// One rule, one backend, no matches (the CRD defaults to a PathPrefix match on +// "/"), and never any backend TLS: the edge reaches instances over plaintext +// inside the network, and the API rejects backend TLS for this backend form. +// +// hostnames are custom hostnames only. The platform-managed hostname is +// assigned by the server and read back from status. +func BuildHTTPProxy(w *computev1alpha.Workload, portName string, hostnames []string) *networkingv1alpha.HTTPProxy { + proxy := &networkingv1alpha.HTTPProxy{ + ObjectMeta: objectMeta(w), + Spec: networkingv1alpha.HTTPProxySpec{ + Rules: []networkingv1alpha.HTTPProxyRule{{ + Backends: []networkingv1alpha.HTTPProxyRuleBackend{{ + NetworkService: &networkingv1alpha.NetworkServiceBackendRef{ + Name: ResourceName(w.Name), + Port: portName, + }, + }}, + }}, + }, + } + + for _, h := range hostnames { + h = strings.TrimSpace(h) + if h == "" { + continue + } + proxy.Spec.Hostnames = append(proxy.Spec.Hostnames, gatewayv1.Hostname(h)) + } + + return proxy +} + +func ptr[T any](v T) *T { return &v } diff --git a/internal/cmd/compute/url/resources_test.go b/internal/cmd/compute/url/resources_test.go new file mode 100644 index 00000000..793296fc --- /dev/null +++ b/internal/cmd/compute/url/resources_test.go @@ -0,0 +1,227 @@ +// SPDX-License-Identifier: AGPL-3.0-only + +package url + +import ( + "strings" + "testing" + + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/types" + + computev1alpha "go.datum.net/compute/api/v1alpha" + "go.datum.net/compute/internal/cmd/compute/util" + networkingv1alpha "go.datum.net/network-services-operator/api/v1alpha" +) + +func testWorkload() *computev1alpha.Workload { + w := &computev1alpha.Workload{} + w.Name = testWorkloadName + w.Namespace = util.ResourceNamespace + w.UID = types.UID("11111111-2222-3333-4444-555555555555") + return w +} + +func TestPortName(t *testing.T) { + tests := []struct { + name string + port computev1alpha.NamedPort + want string + wantErr bool + }{ + {name: "the common deploy path", port: computev1alpha.NamedPort{Name: "http", Port: 8080}, want: testPortName}, + {name: "uppercase is lowered", port: computev1alpha.NamedPort{Name: "HTTP", Port: 80}, want: testPortName}, + {name: "underscores become dashes", port: computev1alpha.NamedPort{Name: "web_port", Port: 80}, want: "web-port"}, + {name: "dots become dashes", port: computev1alpha.NamedPort{Name: "web.port", Port: 80}, want: "web-port"}, + {name: "leading and trailing junk is trimmed", port: computev1alpha.NamedPort{Name: "_http_", Port: 80}, want: testPortName}, + {name: "digits are kept", port: computev1alpha.NamedPort{Name: "h2c9", Port: 80}, want: "h2c9"}, + {name: "spaces become dashes", port: computev1alpha.NamedPort{Name: "my port", Port: 80}, want: "my-port"}, + { + name: "over-long names are truncated to a DNS label", + port: computev1alpha.NamedPort{Name: strings.Repeat("a", 70), Port: 80}, + want: strings.Repeat("a", 63), + }, + { + // Truncating must not leave a trailing dash, which the API rejects. + name: "truncation does not leave a trailing dash", + port: computev1alpha.NamedPort{Name: strings.Repeat("a", 62) + "-b" + strings.Repeat("c", 10), Port: 80}, + want: strings.Repeat("a", 62), + }, + {name: "empty name is an error", port: computev1alpha.NamedPort{Name: "", Port: 8080}, wantErr: true}, + {name: "whitespace-only name is an error", port: computev1alpha.NamedPort{Name: " ", Port: 8080}, wantErr: true}, + {name: "nothing usable is an error", port: computev1alpha.NamedPort{Name: "___", Port: 8080}, wantErr: true}, + {name: "non-ascii only is an error", port: computev1alpha.NamedPort{Name: "日本", Port: 8080}, wantErr: true}, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + got, err := PortName(tc.port) + if tc.wantErr { + if err == nil { + t.Fatalf("PortName(%q) = %q, want error", tc.port.Name, got) + } + return + } + if err != nil { + t.Fatalf("PortName(%q) returned error: %v", tc.port.Name, err) + } + if got != tc.want { + t.Errorf("PortName(%q) = %q, want %q", tc.port.Name, got, tc.want) + } + if !dnsLabel(got) { + t.Errorf("PortName(%q) = %q, which the API would reject", tc.port.Name, got) + } + }) + } +} + +// dnsLabel mirrors the pattern NetworkServicePort.Name is validated against. +func dnsLabel(s string) bool { + if s == "" || len(s) > 63 { + return false + } + for i, r := range s { + alnum := (r >= 'a' && r <= 'z') || (r >= '0' && r <= '9') + if !alnum && r != '-' { + return false + } + if (i == 0 || i == len(s)-1) && !alnum { + return false + } + } + return true +} + +func TestResourceNameIsSharedByBothObjects(t *testing.T) { + w := testWorkload() + svc := BuildNetworkService(w, testPortName, 8080) + proxy := BuildHTTPProxy(w, testPortName, nil) + + if svc.Name != ResourceName(w.Name) || proxy.Name != ResourceName(w.Name) { + t.Fatalf("names diverge: service %q, proxy %q, ResourceName %q", svc.Name, proxy.Name, ResourceName(w.Name)) + } + if proxy.Spec.Rules[0].Backends[0].NetworkService.Name != svc.Name { + t.Errorf("backend points at %q, but the service is named %q", + proxy.Spec.Rules[0].Backends[0].NetworkService.Name, svc.Name) + } +} + +func TestBuildNetworkService(t *testing.T) { + w := testWorkload() + svc := BuildNetworkService(w, testPortName, 8080) + + if svc.Namespace != util.ResourceNamespace { + t.Errorf("namespace = %q, want %q", svc.Namespace, util.ResourceNamespace) + } + assertPublishedLabels(t, svc.Labels, w) + assertOwnerRef(t, svc.OwnerReferences, w) + + want := map[string]string{computev1alpha.WorkloadNameLabel: w.Name} + got := svc.Spec.NetworkInterfaces.Selector.MatchLabels + if len(got) != len(want) { + t.Fatalf("selector matchLabels = %v, want %v", got, want) + } + for k, v := range want { + if got[k] != v { + t.Errorf("selector matchLabels[%s] = %q, want %q", k, got[k], v) + } + } + if len(svc.Spec.NetworkInterfaces.Selector.MatchExpressions) != 0 { + t.Errorf("selector should not use matchExpressions: %v", svc.Spec.NetworkInterfaces.Selector.MatchExpressions) + } + + if len(svc.Spec.Ports) != 1 { + t.Fatalf("ports = %d, want exactly 1", len(svc.Spec.Ports)) + } + p := svc.Spec.Ports[0] + if p.Name != testPortName || p.Port != 8080 || p.Protocol != networkingv1alpha.NetworkServiceProtocolTCP { + t.Errorf("port = %+v, want {http 8080 TCP}", p) + } + + // The type's own documentation says to leave this unset. + if (svc.Spec.TrafficDistribution != networkingv1alpha.NetworkServiceTrafficDistribution{}) { + t.Errorf("trafficDistribution = %+v, want unset", svc.Spec.TrafficDistribution) + } +} + +func TestBuildHTTPProxy(t *testing.T) { + w := testWorkload() + proxy := BuildHTTPProxy(w, testPortName, []string{testCustomHostname, " ", "www.example.com"}) + + if proxy.Namespace != util.ResourceNamespace { + t.Errorf("namespace = %q, want %q", proxy.Namespace, util.ResourceNamespace) + } + assertPublishedLabels(t, proxy.Labels, w) + assertOwnerRef(t, proxy.OwnerReferences, w) + + if len(proxy.Spec.Hostnames) != 2 { + t.Fatalf("hostnames = %v, want the two non-blank entries", proxy.Spec.Hostnames) + } + if string(proxy.Spec.Hostnames[0]) != testCustomHostname || string(proxy.Spec.Hostnames[1]) != "www.example.com" { + t.Errorf("hostnames = %v, want [api.example.com www.example.com]", proxy.Spec.Hostnames) + } + + if len(proxy.Spec.Rules) != 1 { + t.Fatalf("rules = %d, want exactly 1", len(proxy.Spec.Rules)) + } + rule := proxy.Spec.Rules[0] + if len(rule.Matches) != 0 { + t.Errorf("matches = %v, want none so the CRD default (PathPrefix /) applies", rule.Matches) + } + if len(rule.Backends) != 1 { + t.Fatalf("backends = %d, want exactly 1", len(rule.Backends)) + } + + b := rule.Backends[0] + if b.NetworkService == nil { + t.Fatal("backend does not reference a network service") + } + if b.NetworkService.Name != ResourceName(w.Name) || b.NetworkService.Port != testPortName { + t.Errorf("backend ref = %+v, want {api http}", *b.NetworkService) + } + // The API rejects backend TLS for this backend form, and the other backend + // forms are mutually exclusive with it. + if b.TLS != nil { + t.Error("backend TLS is set; the API rejects it for networkService backends") + } + if b.Endpoint != "" || b.Connector != nil || b.Instance != nil { + t.Errorf("backend sets a mutually exclusive field: %+v", b) + } +} + +func TestBuildHTTPProxyWithoutHostnames(t *testing.T) { + proxy := BuildHTTPProxy(testWorkload(), testPortName, nil) + if len(proxy.Spec.Hostnames) != 0 { + t.Errorf("hostnames = %v, want none — the managed hostname comes from status", proxy.Spec.Hostnames) + } +} + +func assertPublishedLabels(t *testing.T, got map[string]string, w *computev1alpha.Workload) { + t.Helper() + if got[computev1alpha.WorkloadNameLabel] != w.Name { + t.Errorf("label %s = %q, want %q", computev1alpha.WorkloadNameLabel, got[computev1alpha.WorkloadNameLabel], w.Name) + } + if got[computev1alpha.WorkloadUIDLabel] != string(w.UID) { + t.Errorf("label %s = %q, want %q", computev1alpha.WorkloadUIDLabel, got[computev1alpha.WorkloadUIDLabel], w.UID) + } +} + +func assertOwnerRef(t *testing.T, refs []metav1.OwnerReference, w *computev1alpha.Workload) { + t.Helper() + if len(refs) != 1 { + t.Fatalf("owner references = %d, want exactly 1", len(refs)) + } + ref := refs[0] + if ref.APIVersion != computev1alpha.GroupVersion.String() { + t.Errorf("owner apiVersion = %q, want %q", ref.APIVersion, computev1alpha.GroupVersion.String()) + } + if ref.Kind != "Workload" || ref.Name != w.Name || ref.UID != w.UID { + t.Errorf("owner ref = %+v, want Workload/%s/%s", ref, w.Name, w.UID) + } + if ref.Controller == nil || *ref.Controller { + t.Errorf("owner controller = %v, want false", ref.Controller) + } + if ref.BlockOwnerDeletion == nil || *ref.BlockOwnerDeletion { + t.Errorf("owner blockOwnerDeletion = %v, want false", ref.BlockOwnerDeletion) + } +} diff --git a/internal/cmd/compute/url/state_test.go b/internal/cmd/compute/url/state_test.go new file mode 100644 index 00000000..99451155 --- /dev/null +++ b/internal/cmd/compute/url/state_test.go @@ -0,0 +1,309 @@ +// SPDX-License-Identifier: AGPL-3.0-only + +package url + +import ( + "context" + "strings" + "testing" + + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/runtime" + "sigs.k8s.io/controller-runtime/pkg/client/fake" + + computev1alpha "go.datum.net/compute/api/v1alpha" + networkingv1alpha "go.datum.net/network-services-operator/api/v1alpha" +) + +// pendingCertProxy is the shape the platform reports in the window between +// `domains add` and the new hostname's certificate being issued: the +// per-hostname status says that one hostname is waiting, and the proxy-level +// certificate roll-up — whose True reason is "AllCertificatesReady" — is +// therefore False for the proxy as a whole. +func pendingCertProxy() *networkingv1alpha.HTTPProxy { + p := publishedProxy(testWorkloadName, testCanonical) + p.Spec.Hostnames = append(p.Spec.Hostnames, testCustomHostname) + p.Status.Conditions = []metav1.Condition{ + cond(networkingv1alpha.HTTPProxyConditionAccepted, metav1.ConditionTrue, "Accepted", ""), + cond(networkingv1alpha.HTTPProxyConditionProgrammed, metav1.ConditionTrue, "Programmed", ""), + cond(networkingv1alpha.HTTPProxyConditionCertificatesReady, metav1.ConditionFalse, + "CertificatePending", "issuing certificate for "+testCustomHostname), + } + p.Status.HostnameStatuses = []networkingv1alpha.HostnameStatus{{ + Hostname: testCustomHostname, + Conditions: []metav1.Condition{ + cond(networkingv1alpha.HostnameConditionVerified, metav1.ConditionTrue, "Verified", ""), + cond(networkingv1alpha.HostnameConditionCertificateReady, metav1.ConditionFalse, "Pending", "issuing"), + }, + }} + return p +} + +// TestManagedHostnameSurvivesAPendingCustomHostname is the state every user +// who runs `domains add` passes through, and the one no existing test covers: +// one hostname is waiting on a certificate while the platform-managed hostname +// carries on serving exactly as it did before. +// +// The managed hostname has no per-hostname status of its own here — control +// planes only publish HostnameStatuses for hostnames they are working on — and +// the proxy-level conditions are describing the *other* hostname. Nothing the +// platform says about a custom hostname may be attributed to this one. +func TestManagedHostnameSurvivesAPendingCustomHostname(t *testing.T) { + c := newFakeClient(t, pendingCertProxy(), + publishedService(testWorkloadName, 8080, location("DFW", 2, 2, true))) + + info, err := ForWorkload(context.Background(), c, testWorkloadName) + if err != nil { + t.Fatalf("ForWorkload returned error: %v", err) + } + + managed := info.Hostnames[len(info.Hostnames)-1] + if !managed.Managed { + t.Fatalf("last hostname = %+v, want the managed one", managed) + } + if !managed.Active || managed.Status != statusActive { + t.Errorf("managed hostname = %+v, want it still active — it was serving before the custom hostname was attached", managed) + } + if managed.Detail != "" { + t.Errorf("managed hostname detail = %q, want nothing: that message is about %s", managed.Detail, testCustomHostname) + } + + // The consequence that costs the most: url.Publish waits on Live(), so a + // redeploy of this workload blocks until an unrelated certificate issues. + if !info.Live() { + t.Errorf("URL is not live, so `deploy` will wait on a hostname the user did not ask about; info = %+v", info) + } +} + +// The custom hostname's own state is read from its own conditions and is +// correct even today — this pins the half that works, so a fix for the managed +// hostname cannot regress it. +func TestPendingCustomHostnameReportsItsOwnReason(t *testing.T) { + c := newFakeClient(t, pendingCertProxy(), + publishedService(testWorkloadName, 8080, location("DFW", 2, 2, true))) + + info, err := ForWorkload(context.Background(), c, testWorkloadName) + if err != nil { + t.Fatalf("ForWorkload returned error: %v", err) + } + + custom := info.Hostnames[0] + if custom.Managed || custom.Active { + t.Fatalf("first hostname = %+v, want the custom one, not serving yet", custom) + } + if custom.Status != "issuing" || custom.Certificate != "issuing" { + t.Errorf("custom hostname = %+v, want the server's own message", custom) + } + + // The URL shown is still the managed one: a hostname that is not serving + // must never be the address handed to the user. + if info.URL != testCanonicalURL { + t.Errorf("URL = %q, want the managed hostname while the custom one is pending", info.URL) + } +} + +// TestForWorkloadWhenTheBackendKindIsNotServed covers the half-installed +// control plane: URLs are served, their backends are not. The URL is still +// reported — with no backends — rather than the whole lookup failing. +func TestForWorkloadWhenTheBackendKindIsNotServed(t *testing.T) { + s := runtime.NewScheme() + if err := computev1alpha.AddToScheme(s); err != nil { + t.Fatalf("registering compute scheme: %v", err) + } + // Only the proxy kind, deliberately: listing NetworkServices answers with + // a not-registered error, which reads as "nothing published here". + s.AddKnownTypes(networkingv1alpha.GroupVersion, + &networkingv1alpha.HTTPProxy{}, &networkingv1alpha.HTTPProxyList{}) + metav1.AddToGroupVersion(s, networkingv1alpha.GroupVersion) + + c := fake.NewClientBuilder().WithScheme(s). + WithObjects(publishedProxy(testWorkloadName, testCanonical)).Build() + + info, err := ForWorkload(context.Background(), c, testWorkloadName) + if err != nil { + t.Fatalf("a missing backend kind is not a lookup failure, got: %v", err) + } + if info == nil { + t.Fatal("info = nil, want the URL to still be reported") + } + if info.URL != testCanonicalURL { + t.Errorf("URL = %q, want %q", info.URL, testCanonicalURL) + } + if info.Service != nil || info.Backends != (Backends{}) { + t.Errorf("Service = %v, Backends = %+v, want nothing known about backends", info.Service, info.Backends) + } + + all, err := ForAll(context.Background(), c) + if err != nil { + t.Fatalf("ForAll returned error: %v", err) + } + if len(all) != 1 || all[testWorkloadName] == nil { + t.Errorf("ForAll = %v, want the one URL", all) + } +} + +// TestPrimaryWithoutAManagedHostnameYet: between creating the proxy and the +// server assigning a hostname, a workload that was published with a custom +// hostname has exactly one hostname and it is not serving. Whatever is shown, +// it must be that hostname and not the empty string — an empty URL is what +// `open` turns into "the platform is still assigning one". +func TestPrimaryWithoutAManagedHostnameYet(t *testing.T) { + proxy := BuildHTTPProxy(workloadNamed(testWorkloadName), testPortName, []string{testCustomHostname}) + // No CanonicalHostname, and nothing programmed yet. + proxy.Status.Conditions = []metav1.Condition{ + cond(networkingv1alpha.HTTPProxyConditionAccepted, metav1.ConditionTrue, "Accepted", ""), + cond(networkingv1alpha.HTTPProxyConditionProgrammed, metav1.ConditionFalse, "Pending", "waiting for the edge"), + } + + c := newFakeClient(t, proxy) + + info, err := ForWorkload(context.Background(), c, testWorkloadName) + if err != nil { + t.Fatalf("ForWorkload returned error: %v", err) + } + if info.CanonicalHostname != "" { + t.Fatalf("CanonicalHostname = %q, want none assigned yet", info.CanonicalHostname) + } + if len(info.Hostnames) != 1 || info.Hostnames[0].Managed { + t.Fatalf("Hostnames = %+v, want the custom one alone", info.Hostnames) + } + if info.URL != testCustomURL { + t.Errorf("URL = %q, want the only hostname there is", info.URL) + } + // It is not live, so no command may present it as ready. + if info.Live() { + t.Error("a URL whose edge is not programmed must not report as live") + } +} + +// A URL nothing is known about at all — no hostname of either kind — must +// report an empty URL rather than "https://". +func TestPrimaryWithNoHostnamesAtAll(t *testing.T) { + c := newFakeClient(t, BuildHTTPProxy(workloadNamed(testWorkloadName), testPortName, nil)) + + info, err := ForWorkload(context.Background(), c, testWorkloadName) + if err != nil { + t.Fatalf("ForWorkload returned error: %v", err) + } + if info.URL != "" { + t.Errorf("URL = %q, want empty — there is no hostname to show", info.URL) + } + if info.Live() { + t.Error("a URL with no hostname is not live") + } +} + +// TestAFreshlyAttachedHostnameIsPending is the first seconds of `domains add`: +// the hostname is on the spec and the platform has not looked at it yet, so it +// has no entry in the per-hostname statuses. +// +// The proxy is already serving and every proxy-level condition is True, so a +// hostname that borrowed them would read as active and verified the instant it +// was attached — `domains add` would print a checkmark, skip the DNS records +// the user has to create, and exit. +func TestAFreshlyAttachedHostnameIsPending(t *testing.T) { + proxy := publishedProxy(testWorkloadName, testCanonical) + proxy.Spec.Hostnames = append(proxy.Spec.Hostnames, testCustomHostname) + // Deliberately no HostnameStatuses: nothing has been reported about it. + + c := newFakeClient(t, proxy, + publishedService(testWorkloadName, 8080, location("DFW", 1, 1, true))) + + info, err := ForWorkload(context.Background(), c, testWorkloadName) + if err != nil { + t.Fatalf("ForWorkload returned error: %v", err) + } + + custom := info.Hostnames[0] + if custom.Managed { + t.Fatalf("first hostname = %+v, want the custom one", custom) + } + if custom.Active || custom.Status != statusPending { + t.Errorf("custom hostname = %+v, want it pending: the platform has said nothing about it", custom) + } + if custom.Certificate != "" { + t.Errorf("certificate = %q, want nothing known — no certificate has been reported for this hostname", custom.Certificate) + } + if len(custom.Conditions) != 0 { + t.Errorf("conditions = %+v, want none: borrowing another hostname's checks is what puts a checkmark on an unverified domain", custom.Conditions) + } + + // The address handed to the user stays the one that answers. + if info.URL != testCanonicalURL { + t.Errorf("URL = %q, want the managed hostname", info.URL) + } + + // And the managed hostname still reads from the proxy-level conditions, + // which is all a control plane that reports nothing per-hostname publishes. + managed := info.Hostnames[1] + if !managed.Active || managed.Certificate != certificateValid { + t.Errorf("managed hostname = %+v, want it active with a valid certificate", managed) + } +} + +// TestABlockingConditionWithNoMessageNeverShowsItsReason: a condition reason is +// camelCase internal state, and the product promises a developer never sees +// one. When the server has no message to show, the CLI says the plain thing +// rather than leaking the reason into a table cell. +func TestABlockingConditionWithNoMessageNeverShowsItsReason(t *testing.T) { + const ( + verifyReason = "UnverifiedHostnamesPresent" + certReason = "CertificateRequestPending" + ) + + proxy := publishedProxy(testWorkloadName, testCanonical) + proxy.Spec.Hostnames = append(proxy.Spec.Hostnames, testCustomHostname) + proxy.Status.HostnameStatuses = []networkingv1alpha.HostnameStatus{{ + Hostname: testCustomHostname, + Conditions: []metav1.Condition{ + cond(networkingv1alpha.HostnameConditionVerified, metav1.ConditionFalse, verifyReason, ""), + cond(networkingv1alpha.HostnameConditionCertificateReady, metav1.ConditionFalse, certReason, ""), + }, + }} + + info := newInfo(testWorkloadName, proxy, nil) + h := info.Hostnames[0] + + if h.Status != statusPending { + t.Errorf("status = %q, want %q — the server gave no message to show", h.Status, statusPending) + } + if h.Certificate != statusPending { + t.Errorf("certificate = %q, want %q", h.Certificate, statusPending) + } + for _, field := range []string{h.Status, h.Certificate, h.Detail} { + for _, reason := range []string{verifyReason, certReason} { + if strings.Contains(field, reason) { + t.Errorf("%q reached the user: condition reasons are internal state", field) + } + } + } +} + +// The same rule with a message to show: the message is what a user reads, and +// the reason still does not appear anywhere. +func TestABlockingConditionShowsTheServersMessage(t *testing.T) { + proxy := publishedProxy(testWorkloadName, testCanonical) + proxy.Spec.Hostnames = append(proxy.Spec.Hostnames, testCustomHostname) + proxy.Status.HostnameStatuses = []networkingv1alpha.HostnameStatus{{ + Hostname: testCustomHostname, + Conditions: []metav1.Condition{ + cond(networkingv1alpha.HostnameConditionVerified, metav1.ConditionFalse, + "DomainNotVerified", "no TXT record found at _datum-challenge.api.example.com"), + cond(networkingv1alpha.HostnameConditionCertificateReady, metav1.ConditionFalse, + "CertificatePending", "waiting for the domain to verify"), + }, + }} + + h := newInfo(testWorkloadName, proxy, nil).Hostnames[0] + + if h.Status != "no TXT record found at _datum-challenge.api.example.com" { + t.Errorf("status = %q, want the server's message", h.Status) + } + if h.Certificate != "waiting for the domain to verify" { + t.Errorf("certificate = %q, want the server's message", h.Certificate) + } + if strings.Contains(h.Status+h.Certificate, "DomainNotVerified") || + strings.Contains(h.Status+h.Certificate, "CertificatePending") { + t.Errorf("a raw reason reached the user: status=%q certificate=%q", h.Status, h.Certificate) + } +} diff --git a/internal/cmd/compute/workloads/filter_test.go b/internal/cmd/compute/workloads/filter_test.go new file mode 100644 index 00000000..aab70be4 --- /dev/null +++ b/internal/cmd/compute/workloads/filter_test.go @@ -0,0 +1,176 @@ +// SPDX-License-Identifier: AGPL-3.0-only + +package workloads + +import ( + "bytes" + "context" + "encoding/json" + "errors" + "strings" + "testing" + + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/client/fake" + "sigs.k8s.io/controller-runtime/pkg/client/interceptor" + + computev1alpha "go.datum.net/compute/api/v1alpha" + "go.datum.net/compute/internal/cmd/compute/util" + networkingv1alpha "go.datum.net/network-services-operator/api/v1alpha" +) + +// The health values the filter selects on, and the workload names the cases +// name, spelled once each. +const ( + healthAvailable = "Available" + healthDegraded = "Degraded" + healthUnavailable = "Unavailable" + + wlAPI = "api" + wlBroken = "broken" + wlSlow = "slow" +) + +// unavailable returns a workload the platform reports as not available. A +// "Degraded" one is different again: available, but short of its desired +// replicas. The filter selects on the first word of either. +func unavailable(name, uid, image string, cities ...string) *computev1alpha.Workload { + w := workload(name, uid, image, cities...) + w.Status.Conditions = []metav1.Condition{{ + Type: computev1alpha.WorkloadAvailable, + Status: metav1.ConditionFalse, + Reason: "InsufficientCapacity", + }} + return w +} + +// TestListWorkloadsHealthFilter: the health filter is the one piece of the +// list view the URL work moved wholesale into a new function, and nothing +// exercised it afterwards. It has to still select on the first word of the +// health, case-insensitively, and the rows it keeps still carry their URLs. +func TestListWorkloadsHealthFilter(t *testing.T) { + objs := []client.Object{ + workload(wlAPI, "uid-api", "ghcr.io/acme/api:1.4.2", "DFW"), + unavailable(wlBroken, "uid-broken", "ghcr.io/acme/broken:1", "DFW"), + workload(wlSlow, "uid-slow", "ghcr.io/acme/slow:1", "DFW"), + deployment("api-dfw", "uid-api", "DFW", 2, 2), + deployment("broken-dfw", "uid-broken", "DFW", 0, 2), + deployment("slow-dfw", "uid-slow", "DFW", 1, 3), + publishedProxy(wlAPI, testCustom), + publishedService(wlAPI), + } + + tests := []struct { + name string + health string + wantNames []string + wantMissing []string + }{ + {name: "no filter lists them all", wantNames: []string{wlAPI, wlBroken, wlSlow}}, + {name: "available only", health: healthAvailable, wantNames: []string{wlAPI}, wantMissing: []string{wlBroken, wlSlow}}, + {name: "the filter is case-insensitive", health: "available", wantNames: []string{wlAPI}, wantMissing: []string{wlBroken}}, + {name: "unavailable only", health: healthUnavailable, wantNames: []string{wlBroken}, wantMissing: []string{wlAPI, wlSlow}}, + {name: "degraded is available but short of replicas", health: healthDegraded, wantNames: []string{wlSlow}, wantMissing: []string{wlAPI, wlBroken}}, + {name: "matching on the whole health string finds nothing", health: healthAvailable + " — all placements at desired replicas", wantMissing: []string{wlAPI, wlBroken, wlSlow}}, + {name: "a health nothing has", health: "Nonsense", wantMissing: []string{wlAPI, wlBroken, wlSlow}}, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + var out, errOut bytes.Buffer + c := newFakeClient(t, objs...) + + opts := listOptions{output: util.OutputJSON, health: tc.health} + if err := listWorkloads(context.Background(), &out, &errOut, c, testProject, opts); err != nil { + t.Fatalf("listWorkloads: %v", err) + } + + var views []workloadView + if err := json.Unmarshal(out.Bytes(), &views); err != nil { + t.Fatalf("output is not a JSON array: %v\n%s", err, out.String()) + } + got := map[string]workloadView{} + for _, v := range views { + got[v.Name] = v + } + + for _, name := range tc.wantNames { + if _, ok := got[name]; !ok { + t.Errorf("%q missing from the filtered list: %v", name, got) + } + } + for _, name := range tc.wantMissing { + if _, ok := got[name]; ok { + t.Errorf("%q should have been filtered out: %v", name, got) + } + } + if v, ok := got[wlAPI]; ok && v.URL != "https://"+testCustom { + t.Errorf("api url = %q, want the URL to survive filtering", v.URL) + } + }) + } +} + +// TestListWorkloadsFailsWhenWorkloadsCannotBeListed: an unreadable project is +// an error. Only the URL column degrades to "unknown" — the workloads +// themselves are the command. +func TestListWorkloadsFailsWhenWorkloadsCannotBeListed(t *testing.T) { + boom := errors.New("forbidden") + c := fake.NewClientBuilder(). + WithScheme(testScheme(t)). + WithObjects(projectObjects()...). + WithInterceptorFuncs(interceptor.Funcs{ + List: func(ctx context.Context, cl client.WithWatch, list client.ObjectList, opts ...client.ListOption) error { + if _, ok := list.(*computev1alpha.WorkloadList); ok { + return boom + } + return cl.List(ctx, list, opts...) + }, + }). + Build() + + var out, errOut bytes.Buffer + err := listWorkloads(context.Background(), &out, &errOut, c, testProject, listOptions{output: util.OutputTable}) + if !errors.Is(err, boom) { + t.Fatalf("error = %v, want the list failure", err) + } +} + +// TestListWorkloadsUnknownURLsInJSON: the table says "?" when the URLs could +// not be read. JSON has no such marker — the field is just empty — so a script +// reading `.url` cannot tell "no URL" from "could not tell". This pins the +// current shape so the gap is visible rather than assumed away. +func TestListWorkloadsUnknownURLsInJSON(t *testing.T) { + c := fake.NewClientBuilder(). + WithScheme(testScheme(t)). + WithObjects(projectObjects()...). + WithInterceptorFuncs(interceptor.Funcs{ + List: func(ctx context.Context, cl client.WithWatch, list client.ObjectList, opts ...client.ListOption) error { + if _, ok := list.(*networkingv1alpha.HTTPProxyList); ok { + return errors.New("forbidden") + } + return cl.List(ctx, list, opts...) + }, + }). + Build() + + var out, errOut bytes.Buffer + if err := listWorkloads(context.Background(), &out, &errOut, c, testProject, listOptions{output: util.OutputJSON}); err != nil { + t.Fatalf("listWorkloads: %v", err) + } + + var views []workloadView + if err := json.Unmarshal(out.Bytes(), &views); err != nil { + t.Fatalf("output is not a JSON array: %v\n%s", err, out.String()) + } + for _, v := range views { + if v.URL != "" { + t.Errorf("%s url = %q, want empty when the URLs could not be read", v.Name, v.URL) + } + } + // The warning is the only signal a script has, and it goes to stderr. + if !strings.Contains(errOut.String(), "could not read URLs") { + t.Errorf("stderr must carry the warning:\n%s", errOut.String()) + } +} diff --git a/internal/cmd/compute/workloads/workloads.go b/internal/cmd/compute/workloads/workloads.go index 4093ed3b..a813624f 100644 --- a/internal/cmd/compute/workloads/workloads.go +++ b/internal/cmd/compute/workloads/workloads.go @@ -3,6 +3,7 @@ package workloads import ( "context" "fmt" + "io" "strings" "github.com/spf13/cobra" @@ -14,6 +15,7 @@ import ( "sigs.k8s.io/controller-runtime/pkg/client" computev1alpha "go.datum.net/compute/api/v1alpha" + "go.datum.net/compute/internal/cmd/compute/url" "go.datum.net/compute/internal/cmd/compute/util" ) @@ -23,7 +25,16 @@ func Command() *cobra.Command { Use: "workloads", Short: "List or inspect workloads", Long: `List all workloads in the project, optionally filtered by health or location. -Use the describe subcommand for a unified config + health view of a single workload.`, +Use the describe subcommand for a unified config + health view of a single workload. + +The URL column shows where a workload answers on the public internet; a +workload that declares no HTTP port shows "—", and "?" means the URLs could not +be read at all. JSON and YAML output carry the same value as a "url" field, +alongside the whole workload resource under "workload"; there the two cases are +told apart by omitting "url" and, when the read failed, setting "urlError". + +The LOCATIONS column lists the locations a workload is placed in, shortened when +there are many; JSON and YAML always carry the full list under "locations".`, Example: ` # List all workloads datumctl compute workloads @@ -36,6 +47,9 @@ Use the describe subcommand for a unified config + health view of a single workl # Machine-readable output datumctl compute workloads -o json + # One workload's URL, for scripting + datumctl compute workloads -o json | jq -r '.[] | select(.name=="api") | .url' + # Describe a single workload datumctl compute workloads describe api`, RunE: func(cmd *cobra.Command, args []string) error { @@ -62,85 +76,176 @@ Use the describe subcommand for a unified config + health view of a single workl // workloads list // ----------------------------------------------------------------------- -//nolint:gocyclo // A list command branches over flag parsing, filtering, and several output formats; splitting it would scatter one linear flow. +const ( + // noURL is what the table shows for a workload that is not published. + noURL = "—" + // unknownURL is what the table shows when the URLs could not be read at + // all, which is a different thing from a workload having none. + unknownURL = "?" + // maxTableLocations is how many locations the LOCATIONS column names before + // it summarises the rest; a workload in a dozen locations must not push the + // columns to its right off the terminal. + maxTableLocations = 3 +) + +// listOptions are the parsed flags of the list view. +type listOptions struct { + output util.OutputFormat + health string + location string + noHeaders bool +} + +// workloadRow is one rendered line of the list view. +type workloadRow struct { + name string + health string + healthShort string // first word: the narrow table column, and the filter key + ready string + upToDate string + placements []string + locations []string + image string + age string + instType string + url string // "" when the workload has no URL + workload *computev1alpha.Workload +} + +// workloadView is the machine-readable form of a row. +// +// `-o json` / `-o yaml` used to emit the raw WorkloadList. They now emit a +// top-level array of these, because a workload's URL is not a field of a +// Workload — it lives on separate objects — and the contract is that +// `workloads -o json | jq -r '.[] | select(.name=="api") | .url'` works. +// +// No existing field changed shape or meaning: the raw resource is carried +// whole under `workload`, so `.items[].spec` becomes `.[].workload.spec`. +type workloadView struct { + Name string `json:"name"` + Health string `json:"health"` + Ready string `json:"ready"` + UpToDate string `json:"upToDate"` + Placements []string `json:"placements,omitempty"` + Locations []string `json:"locations,omitempty"` + Image string `json:"image,omitempty"` + InstanceType string `json:"instanceType,omitempty"` + Age string `json:"age,omitempty"` + + // URL is where the workload answers, and is omitted when it has none — + // the structured form of the table's "—". It is also omitted when the + // URLs could not be read, and then URLError carries the server's error, + // which is the structured form of the table's "?". Reporting both cases + // as `"url": ""` would tell a consumer a workload is unpublished when + // all that actually happened is that the lookup failed. + URL string `json:"url,omitempty"` + URLError string `json:"urlError,omitempty"` + + Workload *computev1alpha.Workload `json:"workload,omitempty"` +} + func runList(cmd *cobra.Command, _ []string) error { - ctx := context.Background() project := util.ProjectFromCmd(cmd) + c, err := util.NewClient(project) + if err != nil { + return err + } + outputFlag, _ := cmd.Flags().GetString("output") healthFilter, _ := cmd.Flags().GetString("health") locationFilter, _ := cmd.Flags().GetString("location") noHeaders, _ := cmd.Flags().GetBool("no-headers") - c, err := util.NewClient(project) + return listWorkloads(context.Background(), cmd.OutOrStdout(), cmd.ErrOrStderr(), c, project, listOptions{ + output: util.OutputFormat(outputFlag), + health: healthFilter, + location: locationFilter, + noHeaders: noHeaders, + }) +} + +// listWorkloads renders the list view. The client is a parameter so the whole +// view can be rendered against a fake one in tests. +func listWorkloads(ctx context.Context, out, errOut io.Writer, c client.Client, project string, opts listOptions) error { + result, err := collectRows(ctx, errOut, c, opts) if err != nil { return err } - var wlList computev1alpha.WorkloadList - if err := c.List(ctx, &wlList, client.InNamespace(util.ResourceNamespace)); err != nil { - return fmt.Errorf("listing workloads: %w", err) - } - - // JSON/YAML: emit raw API resource and return early. - switch util.OutputFormat(outputFlag) { + switch opts.output { case util.OutputJSON: - return util.PrintJSON(cmd.OutOrStdout(), &wlList) + return util.PrintJSON(out, viewsOf(result)) case util.OutputYAML: - return util.PrintYAML(cmd.OutOrStdout(), &wlList) + return util.PrintYAML(out, viewsOf(result)) + } + + if len(result.rows) == 0 { + printNoRows(out, project, opts) + return nil + } + + renderTable(out, result, opts) + return nil +} + +// listing is what reading the project produced: the rows the filters left +// standing, plus the URL lookup's own failure if it had one. A non-nil urlErr +// means every row's URL is unknown, which is not the same claim as a workload +// having none — both the table and the structured output draw that line. +type listing struct { + rows []workloadRow + urlErr error +} + +// urlsKnown reports whether the project's URLs could be read at all. +func (l listing) urlsKnown() bool { return l.urlErr == nil } + +// collectRows reads the project and assembles the rows the filters left +// standing. Its error is the command failing outright; a URL lookup that fails +// is carried on the listing instead, because the URL is a column, not the +// command. +func collectRows(ctx context.Context, errOut io.Writer, c client.Client, opts listOptions) (listing, error) { + var wlList computev1alpha.WorkloadList + if err := c.List(ctx, &wlList, client.InNamespace(util.ResourceNamespace)); err != nil { + return listing{}, fmt.Errorf("listing workloads: %w", err) } - // For table output we need deployment data to compute READY counts. var deployList computev1alpha.WorkloadDeploymentList if err := c.List(ctx, &deployList, client.InNamespace(util.ResourceNamespace)); err != nil { - return fmt.Errorf("listing deployments: %w", err) + return listing{}, fmt.Errorf("listing deployments: %w", err) } - // Build map: workloadUID → []WorkloadDeployment. + // URLs come in one pass for the whole project rather than a lookup per + // workload. A project whose URLs cannot be read still lists its workloads. + urls, urlErr := url.ForAll(ctx, c) + if urlErr != nil { + fmt.Fprintf(errOut, "Warning: could not read URLs: %v\n", urlErr) + } + + // workloadUID → its deployments, and the set of UIDs with a deployment in + // the requested city. deploysByWorkload := make(map[string][]computev1alpha.WorkloadDeployment) + locationFilteredUIDs := map[string]bool{} for _, d := range deployList.Items { wUID := d.Labels[computev1alpha.WorkloadUIDLabel] deploysByWorkload[wUID] = append(deploysByWorkload[wUID], d) - } - - // Location filter: collect workload UIDs with a deployment in the requested location. - locationFilteredUIDs := map[string]bool{} - if locationFilter != "" { - for _, d := range deployList.Items { - if d.Spec.LocationRef.Name == locationFilter { - wUID := d.Labels[computev1alpha.WorkloadUIDLabel] - locationFilteredUIDs[wUID] = true - } + if opts.location != "" && d.Spec.LocationRef.Name == opts.location { + locationFilteredUIDs[wUID] = true } } - type workloadRow struct { - name string - health string - healthShort string // first word, for filter comparison - readyStr string - upToDateStr string - placements string - image string - age string - instType string // wide only - } - - wide := util.OutputFormat(outputFlag) == util.OutputWide - var rows []workloadRow - for _, wl := range wlList.Items { - wUID := string(wl.UID) + for i := range wlList.Items { + wl := &wlList.Items[i] - // Location filter. - if locationFilter != "" && !locationFilteredUIDs[wUID] { + if opts.location != "" && !locationFilteredUIDs[string(wl.UID)] { continue } - deps := deploysByWorkload[wUID] var totalReady, totalUpdated, totalDesired int32 - for _, d := range deps { + for _, d := range deploysByWorkload[string(wl.UID)] { totalReady += d.Status.ReadyReplicas totalUpdated += d.Status.UpdatedReplicas totalDesired += d.Status.DesiredReplicas @@ -149,114 +254,215 @@ func runList(cmd *cobra.Command, _ []string) error { health := util.WorkloadHealth(wl.Status.Conditions, totalReady, totalDesired) healthShort := strings.SplitN(health, " ", 2)[0] // e.g. "Available", "Degraded" - // Health filter. - if healthFilter != "" && !strings.EqualFold(healthShort, healthFilter) { + if opts.health != "" && !strings.EqualFold(healthShort, opts.health) { continue } - // Placement names. - var placementNames []string - for _, p := range wl.Spec.Placements { - placementNames = append(placementNames, p.Name) - } - placements := strings.Join(placementNames, ", ") - if placements == "" { - placements = "(none)" - } - - // Image from first container. - image := "(vm)" + image := "" if wl.Spec.Template.Spec.Runtime.Sandbox != nil && len(wl.Spec.Template.Spec.Runtime.Sandbox.Containers) > 0 { - image = truncateImage(wl.Spec.Template.Spec.Runtime.Sandbox.Containers[0].Image) + image = wl.Spec.Template.Spec.Runtime.Sandbox.Containers[0].Image } - readyStr := fmt.Sprintf("%d/%d", totalReady, totalDesired) - upToDateStr := fmt.Sprintf("%d/%d", totalUpdated, totalDesired) - instType := wl.Spec.Template.Spec.Runtime.Resources.InstanceType - rows = append(rows, workloadRow{ name: wl.Name, health: health, healthShort: healthShort, - readyStr: readyStr, - upToDateStr: upToDateStr, - placements: placements, + ready: fmt.Sprintf("%d/%d", totalReady, totalDesired), + upToDate: fmt.Sprintf("%d/%d", totalUpdated, totalDesired), + placements: placementNames(wl), + locations: placementLocations(wl), image: image, age: util.RelativeAge(wl.CreationTimestamp), - instType: instType, + instType: wl.Spec.Template.Spec.Runtime.Resources.InstanceType, + url: urlOf(urls[wl.Name]), + workload: wl, }) } - // Tally health counts from the filtered rows (W9: count after filtering). - healthCounts := map[string]int{ - "Available": 0, - "Degraded": 0, - "Unavailable": 0, - "Unknown": 0, + return listing{rows: rows, urlErr: urlErr}, nil +} + +// urlOf is the one URL to show for a workload, or "" when it has none. The +// url package has already picked between a custom hostname and the +// platform-managed one; a nil Info is an unpublished workload. +func urlOf(info *url.Info) string { + if info == nil { + return "" } - for _, r := range rows { - switch r.healthShort { - case "Available": - healthCounts["Available"]++ - case "Degraded": - healthCounts["Degraded"]++ - case "Unavailable": - healthCounts["Unavailable"]++ - default: - healthCounts["Unknown"]++ + return info.URL +} + +// placementNames lists the workload's placements in declared order. +func placementNames(wl *computev1alpha.Workload) []string { + names := make([]string, 0, len(wl.Spec.Placements)) + for _, p := range wl.Spec.Placements { + names = append(names, p.Name) + } + return names +} + +// placementLocations lists every location the workload places into, +// deduplicated, in declared order. +// +// A placement can name its locations, resolve them through a topology +// selector, or still carry city codes stored before placement moved to +// locations. A selector cannot be expanded without asking the server, so it is +// reported as the selector itself — the same thing describe shows. +func placementLocations(wl *computev1alpha.Workload) []string { + var locations []string + seen := map[string]bool{} + add := func(name string) { + if name != "" && !seen[name] { + seen[name] = true + locations = append(locations, name) } } + for _, p := range wl.Spec.Placements { + for _, ref := range p.Locations { + add(ref.Name) + } + if len(p.Locations) == 0 { + if p.LocationSelector != nil { + add("selector: " + metav1.FormatLabelSelector(p.LocationSelector)) + continue + } + for _, city := range p.CityCodes { + add(city) + } + } + } + return locations +} - out := cmd.OutOrStdout() +// viewsOf converts rows to their machine-readable form. It always returns a +// non-nil slice so an empty project encodes as [] rather than null — `jq` over +// an empty project should iterate nothing, not fail. +func viewsOf(l listing) []workloadView { + // The lookup failed for the project, so it failed for every row: none of + // their URLs is known, and saying nothing at all would read as "none". + urlError := "" + if l.urlErr != nil { + urlError = l.urlErr.Error() + } - if len(rows) == 0 { - if healthFilter != "" { - fmt.Fprintf(out, "No workloads in project %s match health=%s.\n", project, healthFilter) - } else if locationFilter != "" { - fmt.Fprintf(out, "No workloads in project %s have a placement in location %s.\n", project, locationFilter) - } else { - fmt.Fprintf(out, "No workloads found in project %s.\n\n", project) - fmt.Fprintf(out, "Get started:\n") - fmt.Fprintf(out, " datumctl compute deploy api --image=ghcr.io/acme/api:v1.0.0 --location=us-east-1\n") - } - return nil + views := make([]workloadView, 0, len(l.rows)) + for _, r := range l.rows { + views = append(views, workloadView{ + Name: r.name, + Health: r.health, + Ready: r.ready, + UpToDate: r.upToDate, + Placements: r.placements, + Locations: r.locations, + Image: r.image, + InstanceType: r.instType, + Age: r.age, + URL: r.url, + URLError: urlError, + Workload: r.workload, + }) } + return views +} + +func renderTable(out io.Writer, l listing, opts listOptions) { + wide := opts.output == util.OutputWide + urlsKnown := l.urlsKnown() tw := util.NewTabWriter(out) - if !noHeaders { + if !opts.noHeaders { if wide { - fmt.Fprintf(tw, "NAME\tHEALTH\tREADY\tUP-TO-DATE\tPLACEMENTS\tIMAGE\tAGE\tINSTANCE TYPE\n") + fmt.Fprintf(tw, "NAME\tLOCATIONS\tHEALTH\tREADY\tUP-TO-DATE\tPLACEMENTS\tIMAGE\tAGE\tINSTANCE TYPE\tURL\n") } else { - fmt.Fprintf(tw, "NAME\tHEALTH\tREADY\tUP-TO-DATE\tPLACEMENTS\tIMAGE\tAGE\n") + fmt.Fprintf(tw, "NAME\tLOCATIONS\tHEALTH\tREADY\tUP-TO-DATE\tPLACEMENTS\tIMAGE\tAGE\tURL\n") } } - for _, r := range rows { + for _, r := range l.rows { if wide { - fmt.Fprintf(tw, "%s\t%s\t%s\t%s\t%s\t%s\t%s\t%s\n", - r.name, r.healthShort, r.readyStr, r.upToDateStr, r.placements, r.image, r.age, r.instType) + fmt.Fprintf(tw, "%s\t%s\t%s\t%s\t%s\t%s\t%s\t%s\t%s\t%s\n", + r.name, locationsColumn(r.locations), r.healthShort, r.ready, r.upToDate, + columnOrNone(r.placements), truncateImage(r.image), r.age, r.instType, + urlColumn(r.url, urlsKnown)) } else { - fmt.Fprintf(tw, "%s\t%s\t%s\t%s\t%s\t%s\t%s\n", - r.name, r.healthShort, r.readyStr, r.upToDateStr, r.placements, r.image, r.age) + fmt.Fprintf(tw, "%s\t%s\t%s\t%s\t%s\t%s\t%s\t%s\t%s\n", + r.name, locationsColumn(r.locations), r.healthShort, r.ready, r.upToDate, + columnOrNone(r.placements), truncateImage(r.image), r.age, + urlColumn(r.url, urlsKnown)) } } _ = tw.Flush() - // Footer summary. - fmt.Fprintf(out, "\n%d workloads — %d Available, %d Degraded, %d Unavailable, %d Unknown\n", - len(rows), - healthCounts["Available"], - healthCounts["Degraded"], - healthCounts["Unavailable"], - healthCounts["Unknown"], - ) + fmt.Fprintf(out, "\n%s\n", healthSummary(l.rows)) +} - return nil +// locationsColumn renders the LOCATIONS cell. A workload can be placed in more +// locations than a terminal column can hold, so past maxTableLocations the cell +// names the first few and counts the rest; `-o json` still carries all of +// them under "locations". +func locationsColumn(locations []string) string { + if len(locations) <= maxTableLocations { + return columnOrNone(locations) + } + return fmt.Sprintf("%s (+%d)", strings.Join(locations[:maxTableLocations], ", "), len(locations)-maxTableLocations) +} + +// urlColumn renders the URL cell: the URL, "—" when the workload has none, and +// "?" when the project's URLs could not be read at all. +func urlColumn(u string, urlsKnown bool) string { + switch { + case u != "": + return u + case !urlsKnown: + return unknownURL + default: + return noURL + } +} + +// columnOrNone joins a list for a table cell, or says so when it is empty. +func columnOrNone(values []string) string { + if len(values) == 0 { + return "(none)" + } + return strings.Join(values, ", ") +} + +// healthSummary tallies health across the rows that survived filtering. +func healthSummary(rows []workloadRow) string { + counts := map[string]int{} + for _, r := range rows { + switch r.healthShort { + case "Available", "Degraded", "Unavailable": + counts[r.healthShort]++ + default: + counts["Unknown"]++ + } + } + return fmt.Sprintf("%d workloads — %d Available, %d Degraded, %d Unavailable, %d Unknown", + len(rows), counts["Available"], counts["Degraded"], counts["Unavailable"], counts["Unknown"]) +} + +// printNoRows explains an empty table in terms of whatever the user asked for. +func printNoRows(out io.Writer, project string, opts listOptions) { + switch { + case opts.health != "": + fmt.Fprintf(out, "No workloads in project %s match health=%s.\n", project, opts.health) + case opts.location != "": + fmt.Fprintf(out, "No workloads in project %s have a placement in location %s.\n", project, opts.location) + default: + fmt.Fprintf(out, "No workloads found in project %s.\n\n", project) + fmt.Fprintf(out, "Get started:\n") + fmt.Fprintf(out, " datumctl compute deploy api --image=ghcr.io/acme/api:v1.0.0 --location=us-east-1\n") + } } // truncateImage strips the registry host from an image reference so the table // column stays compact. "ghcr.io/acme/api:v1" → "acme/api:v1". func truncateImage(image string) string { + if image == "" { + return "(vm)" + } parts := strings.SplitN(image, "/", 2) if len(parts) == 2 { // Only strip the first component if it looks like a registry host @@ -360,6 +566,16 @@ func runDescribe(cmd *cobra.Command, args []string) error { fmt.Fprintf(out, "%-12s %s\n", "Health", health) fmt.Fprintf(out, "\n") + // URL block. A workload that was never published simply has no URL, so a + // lookup failure is reported and skipped rather than failing the whole + // describe — the config and health above are still what the user asked for. + if info, err := url.ForWorkload(ctx, c, workloadName); err != nil { + fmt.Fprintf(out, "URL\n (could not be read: %v)\n\n", err) + } else if info != nil { + url.RenderDetail(out, info) + fmt.Fprintf(out, "\n") + } + // Placements block. fmt.Fprintf(out, "Placements\n") if len(wl.Spec.Placements) == 0 { @@ -441,6 +657,23 @@ func runDescribe(cmd *cobra.Command, args []string) error { return nil } +// placementLocationsSummary says where a placement runs: the locations it +// names, or the topology selector it resolves through. +func placementLocationsSummary(p computev1alpha.WorkloadPlacement) string { + if p.LocationSelector != nil { + return "selector: " + metav1.FormatLabelSelector(p.LocationSelector) + } + if len(p.CityCodes) > 0 { + // Stored before placement moved to locations and not yet rewritten. + return "cities: " + strings.Join(p.CityCodes, ", ") + } + names := make([]string, 0, len(p.Locations)) + for _, ref := range p.Locations { + names = append(names, ref.Name) + } + return "locations: " + strings.Join(names, ", ") +} + // degradedAnnotation returns a short annotation for a per-location line when the // deployment is not fully ready. It reads the blocking reason+message from the // deployment's own Available condition, which the server rolls up from the @@ -471,20 +704,3 @@ func formatEnvVar(e corev1.EnvVar) string { } return fmt.Sprintf("%-20s %s", e.Name, e.Value) } - -// placementLocationsSummary says where a placement runs: the locations it -// names, or the topology selector it resolves through. -func placementLocationsSummary(p computev1alpha.WorkloadPlacement) string { - if p.LocationSelector != nil { - return "selector: " + metav1.FormatLabelSelector(p.LocationSelector) - } - if len(p.CityCodes) > 0 { - // Stored before placement moved to locations and not yet rewritten. - return "cities: " + strings.Join(p.CityCodes, ", ") - } - names := make([]string, 0, len(p.Locations)) - for _, ref := range p.Locations { - names = append(names, ref.Name) - } - return "locations: " + strings.Join(names, ", ") -} diff --git a/internal/cmd/compute/workloads/workloads_test.go b/internal/cmd/compute/workloads/workloads_test.go new file mode 100644 index 00000000..7cd79b7e --- /dev/null +++ b/internal/cmd/compute/workloads/workloads_test.go @@ -0,0 +1,571 @@ +// SPDX-License-Identifier: AGPL-3.0-only + +package workloads + +import ( + "bytes" + "context" + "encoding/json" + "errors" + "strings" + "testing" + + gatewayv1 "sigs.k8s.io/gateway-api/apis/v1" + + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/runtime" + "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" + + locationsv1alpha1 "go.miloapis.com/locations/api/v1alpha1" + + computev1alpha "go.datum.net/compute/api/v1alpha" + "go.datum.net/compute/internal/cmd/compute/util" + networkingv1alpha "go.datum.net/network-services-operator/api/v1alpha" +) + +const ( + testProject = "acme-prod" + locEast = "us-east-1" + locWest = "eu-west-1" + locFra = "ap-south-1" + locLhr = "sa-east-1" + workerName = "worker" + testCanonical = "a1b2c3d4.datumproxy.net" + testCustom = "api.example.com" +) + +func testScheme(t *testing.T) *runtime.Scheme { + t.Helper() + s := runtime.NewScheme() + if err := computev1alpha.AddToScheme(s); err != nil { + t.Fatalf("registering compute scheme: %v", err) + } + if err := networkingv1alpha.AddToScheme(s); err != nil { + t.Fatalf("registering networking scheme: %v", err) + } + return s +} + +// workload returns a sandbox workload with one placement in the given locations. +func workload(name, uid, image string, locations ...string) *computev1alpha.Workload { + return &computev1alpha.Workload{ + ObjectMeta: metav1.ObjectMeta{ + Name: name, + Namespace: util.ResourceNamespace, + UID: types.UID(uid), + }, + Spec: computev1alpha.WorkloadSpec{ + Template: computev1alpha.InstanceTemplateSpec{ + Spec: computev1alpha.InstanceSpec{ + Runtime: computev1alpha.InstanceRuntimeSpec{ + Sandbox: &computev1alpha.SandboxRuntime{ + Containers: []computev1alpha.SandboxContainer{{Name: name, Image: image}}, + }, + }, + }, + }, + Placements: []computev1alpha.WorkloadPlacement{{ + Name: "default", + Locations: locationRefs(locations...), + }}, + }, + Status: computev1alpha.WorkloadStatus{ + Conditions: []metav1.Condition{{ + Type: computev1alpha.WorkloadAvailable, + Status: metav1.ConditionTrue, + Reason: "Available", + }}, + }, + } +} + +// locationRefs turns location names into the references a placement stores. +func locationRefs(names ...string) []locationsv1alpha1.LocationReference { + refs := make([]locationsv1alpha1.LocationReference, 0, len(names)) + for _, n := range names { + refs = append(refs, locationsv1alpha1.LocationReference{Name: n}) + } + return refs +} + +// deployment returns a deployment for a workload in one location. +func deployment(name, workloadUID, location string, ready, desired int32) *computev1alpha.WorkloadDeployment { + return &computev1alpha.WorkloadDeployment{ + ObjectMeta: metav1.ObjectMeta{ + Name: name, + Namespace: util.ResourceNamespace, + Labels: map[string]string{computev1alpha.WorkloadUIDLabel: workloadUID}, + }, + Spec: computev1alpha.WorkloadDeploymentSpec{ + LocationRef: locationsv1alpha1.LocationReference{Name: location}, + PlacementName: "default", + }, + Status: computev1alpha.WorkloadDeploymentStatus{ + ReadyReplicas: ready, + UpdatedReplicas: ready, + DesiredReplicas: desired, + }, + } +} + +// publishedProxy is the proxy the platform reports for a live URL. +func publishedProxy(workloadName string, customHostnames ...string) *networkingv1alpha.HTTPProxy { + hostnames := make([]gatewayv1.Hostname, 0, len(customHostnames)) + // A custom hostname serves on the strength of its own status entry and + // nothing else: the proxy-level conditions are a roll-up over every + // hostname and cannot vouch for any one of them. + statuses := make([]networkingv1alpha.HostnameStatus, 0, len(customHostnames)) + for _, h := range customHostnames { + hostnames = append(hostnames, gatewayv1.Hostname(h)) + statuses = append(statuses, networkingv1alpha.HostnameStatus{ + Hostname: h, + Conditions: []metav1.Condition{ + {Type: networkingv1alpha.HostnameConditionVerified, Status: metav1.ConditionTrue, Reason: "Verified"}, + {Type: networkingv1alpha.HostnameConditionDNSRecordProgrammed, Status: metav1.ConditionTrue, Reason: "RecordCreated"}, + {Type: networkingv1alpha.HostnameConditionCertificateReady, Status: metav1.ConditionTrue, Reason: "CertificateIssued"}, + }, + }) + } + return &networkingv1alpha.HTTPProxy{ + ObjectMeta: metav1.ObjectMeta{ + Name: workloadName, + Namespace: util.ResourceNamespace, + Labels: map[string]string{computev1alpha.WorkloadNameLabel: workloadName}, + }, + Spec: networkingv1alpha.HTTPProxySpec{Hostnames: hostnames}, + Status: networkingv1alpha.HTTPProxyStatus{ + CanonicalHostname: testCanonical, + HostnameStatuses: statuses, + Conditions: []metav1.Condition{ + {Type: networkingv1alpha.HTTPProxyConditionProgrammed, Status: metav1.ConditionTrue, Reason: "Programmed"}, + {Type: networkingv1alpha.HTTPProxyConditionCertificatesReady, Status: metav1.ConditionTrue, Reason: "Issued"}, + }, + }, + } +} + +func publishedService(workloadName string) *networkingv1alpha.NetworkService { + return &networkingv1alpha.NetworkService{ + ObjectMeta: metav1.ObjectMeta{ + Name: workloadName, + Namespace: util.ResourceNamespace, + Labels: map[string]string{computev1alpha.WorkloadNameLabel: workloadName}, + }, + Spec: networkingv1alpha.NetworkServiceSpec{ + Ports: []networkingv1alpha.NetworkServicePort{{Name: "http", Port: 8080}}, + }, + Status: networkingv1alpha.NetworkServiceStatus{ + Summary: networkingv1alpha.NetworkServiceSummary{Locations: 2, Members: 4, Healthy: 4}, + }, + } +} + +func newFakeClient(t *testing.T, objs ...client.Object) client.WithWatch { + t.Helper() + return fake.NewClientBuilder().WithScheme(testScheme(t)).WithObjects(objs...).Build() +} + +// project returns the objects for a project with a published wlAPI and an +// unpublished workerName. +func projectObjects() []client.Object { + return []client.Object{ + workload(wlAPI, "uid-api", "ghcr.io/acme/api:1.4.2", locEast, locWest), + workload(workerName, "uid-worker", "ghcr.io/acme/worker:2.0", locEast), + deployment("api-dfw", "uid-api", locEast, 2, 2), + deployment("api-iad", "uid-api", locWest, 2, 2), + deployment("worker-dfw", "uid-worker", locEast, 1, 1), + publishedProxy(wlAPI, testCustom), + publishedService(wlAPI), + } +} + +func TestListWorkloadsTable(t *testing.T) { + tests := []struct { + name string + objs []client.Object + opts listOptions + wantLines []string + wantMissing []string + }{ + { + name: "url column carries the custom hostname, unpublished shows a dash", + objs: projectObjects(), + opts: listOptions{output: util.OutputTable}, + wantLines: []string{ + "NAME", "URL", + wlAPI, "https://" + testCustom, + workerName, noURL, + }, + }, + { + name: "managed hostname when there is no custom one", + objs: []client.Object{ + workload(wlAPI, "uid-api", "ghcr.io/acme/api:1.4.2", locEast), + deployment("api-dfw", "uid-api", locEast, 2, 2), + publishedProxy(wlAPI), + publishedService(wlAPI), + }, + opts: listOptions{output: util.OutputTable}, + wantLines: []string{"https://" + testCanonical}, + }, + { + name: "no-headers drops the header row but keeps the url", + objs: projectObjects(), + opts: listOptions{output: util.OutputTable, noHeaders: true}, + wantLines: []string{"https://" + testCustom}, + wantMissing: []string{"UP-TO-DATE"}, + }, + { + name: "wide keeps the url last", + objs: projectObjects(), + opts: listOptions{output: util.OutputWide}, + wantLines: []string{"INSTANCE TYPE", "URL", "https://" + testCustom}, + }, + { + name: "location filter still resolves urls", + objs: projectObjects(), + opts: listOptions{output: util.OutputTable, location: locWest}, + wantLines: []string{wlAPI, "https://" + testCustom}, + wantMissing: []string{workerName}, + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + var out, errOut bytes.Buffer + c := newFakeClient(t, tc.objs...) + + if err := listWorkloads(context.Background(), &out, &errOut, c, testProject, tc.opts); err != nil { + t.Fatalf("listWorkloads: %v", err) + } + + got := out.String() + for _, want := range tc.wantLines { + if !strings.Contains(got, want) { + t.Errorf("output missing %q:\n%s", want, got) + } + } + for _, missing := range tc.wantMissing { + if strings.Contains(got, missing) { + t.Errorf("output should not contain %q:\n%s", missing, got) + } + } + if errOut.Len() != 0 { + t.Errorf("unexpected stderr: %s", errOut.String()) + } + }) + } +} + +// TestListWorkloadsURLField pins the scripting contract from the spec: +// `workloads -o json | jq -r '.[] | select(.name==wlAPI) | .url'`. +func TestListWorkloadsURLField(t *testing.T) { + var out, errOut bytes.Buffer + c := newFakeClient(t, projectObjects()...) + + if err := listWorkloads(context.Background(), &out, &errOut, c, testProject, listOptions{output: util.OutputJSON}); err != nil { + t.Fatalf("listWorkloads: %v", err) + } + + var views []map[string]any + if err := json.Unmarshal(out.Bytes(), &views); err != nil { + t.Fatalf("output is not a JSON array: %v\n%s", err, out.String()) + } + if len(views) != 2 { + t.Fatalf("got %d views, want 2:\n%s", len(views), out.String()) + } + + byName := map[string]map[string]any{} + for _, v := range views { + name, _ := v["name"].(string) + byName[name] = v + } + + if got := byName[wlAPI]["url"]; got != "https://"+testCustom { + t.Errorf("api url = %v, want https://%s", got, testCustom) + } + // An unpublished workload carries no url field at all — see + // TestListWorkloadsJSONNoURLVersusUnknown for why "" would be a lie. + if _, ok := byName[workerName]["url"]; ok { + t.Errorf("unpublished worker should carry no url field:\n%s", out.String()) + } + + // The raw resource is still there, whole, under "workload". + raw, ok := byName[wlAPI]["workload"].(map[string]any) + if !ok { + t.Fatalf("api view has no workload object:\n%s", out.String()) + } + if raw["spec"] == nil { + t.Errorf("workload object lost its spec:\n%s", out.String()) + } +} + +func TestListWorkloadsYAMLURLField(t *testing.T) { + var out, errOut bytes.Buffer + c := newFakeClient(t, projectObjects()...) + + if err := listWorkloads(context.Background(), &out, &errOut, c, testProject, listOptions{output: util.OutputYAML}); err != nil { + t.Fatalf("listWorkloads: %v", err) + } + + if !strings.Contains(out.String(), "url: https://"+testCustom) { + t.Errorf("yaml missing url field:\n%s", out.String()) + } +} + +func TestListWorkloadsEmptyJSONIsArray(t *testing.T) { + var out, errOut bytes.Buffer + c := newFakeClient(t) + + if err := listWorkloads(context.Background(), &out, &errOut, c, testProject, listOptions{output: util.OutputJSON}); err != nil { + t.Fatalf("listWorkloads: %v", err) + } + + if got := strings.TrimSpace(out.String()); got != "[]" { + t.Errorf("empty project encoded as %q, want []", got) + } +} + +// TestListWorkloadsURLsUnreadable: a project whose URLs cannot be read still +// lists its workloads. The column says "unknown", not "none". +func TestListWorkloadsURLsUnreadable(t *testing.T) { + c := fake.NewClientBuilder(). + WithScheme(testScheme(t)). + WithObjects(projectObjects()...). + WithInterceptorFuncs(interceptor.Funcs{ + List: func(ctx context.Context, cl client.WithWatch, list client.ObjectList, opts ...client.ListOption) error { + if _, ok := list.(*networkingv1alpha.HTTPProxyList); ok { + return errors.New("forbidden") + } + return cl.List(ctx, list, opts...) + }, + }). + Build() + + var out, errOut bytes.Buffer + if err := listWorkloads(context.Background(), &out, &errOut, c, testProject, listOptions{output: util.OutputTable}); err != nil { + t.Fatalf("listWorkloads: %v", err) + } + + if !strings.Contains(out.String(), wlAPI) { + t.Errorf("workloads should still list when their URLs cannot be read:\n%s", out.String()) + } + // Both rows say "unknown", and neither says "none" — an unreadable URL is + // not the same claim as a workload having no URL. + if got := strings.Count(out.String(), unknownURL); got != 2 { + t.Errorf("got %d unknown URL cells, want 2:\n%s", got, out.String()) + } + for _, line := range strings.Split(out.String(), "\n") { + if strings.HasSuffix(strings.TrimSpace(line), noURL) { + t.Errorf("unreadable URLs must not read as no URL:\n%s", out.String()) + } + } + if !strings.Contains(errOut.String(), "could not read URLs") { + t.Errorf("stderr should warn about the URL read:\n%s", errOut.String()) + } +} + +func TestURLColumn(t *testing.T) { + tests := []struct { + name string + url string + urlsKnown bool + want string + }{ + {"published", "https://api.example.com", true, "https://api.example.com"}, + {"unpublished", "", true, noURL}, + {"unknown", "", false, unknownURL}, + } + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + if got := urlColumn(tc.url, tc.urlsKnown); got != tc.want { + t.Errorf("urlColumn(%q, %v) = %q, want %q", tc.url, tc.urlsKnown, got, tc.want) + } + }) + } +} + +// manyLocations returns a workload placed in n locations, so the LOCATIONS column has +// to decide what to do with a list that does not fit. +func manyLocations(name, uid string, locations ...string) *computev1alpha.Workload { + return workload(name, uid, "ghcr.io/acme/"+name+":1.0", locations...) +} + +// TestListWorkloadsLocationsColumn pins the spec's list view: the locations a +// workload runs in are a table column, not a JSON-only field. +func TestListWorkloadsLocationsColumn(t *testing.T) { + for _, output := range []util.OutputFormat{util.OutputTable, util.OutputWide} { + t.Run(string(output), func(t *testing.T) { + var out, errOut bytes.Buffer + c := newFakeClient(t, projectObjects()...) + + if err := listWorkloads(context.Background(), &out, &errOut, c, testProject, listOptions{output: output}); err != nil { + t.Fatalf("listWorkloads: %v", err) + } + + got := out.String() + if !strings.Contains(got, "LOCATIONS") { + t.Errorf("table has no LOCATIONS header:\n%s", got) + } + if !strings.Contains(got, locEast+", "+locWest) { + t.Errorf("api row does not name its locations:\n%s", got) + } + // The mock's column order: NAME ... LOCATIONS ... READY ... IMAGE ... URL. + nameAt := strings.Index(got, "NAME") + locationsAt := strings.Index(got, "LOCATIONS") + readyAt := strings.Index(got, "READY") + urlAt := strings.Index(got, "URL") + if nameAt >= locationsAt || locationsAt >= readyAt || readyAt >= urlAt { + t.Errorf("columns out of spec order (name=%d locations=%d ready=%d url=%d):\n%s", + nameAt, locationsAt, readyAt, urlAt, got) + } + }) + } +} + +// TestLocationsColumnTruncates: a workload in many locations must not blow out the +// column, and the cell has to say that it is not the whole list. +func TestLocationsColumnTruncates(t *testing.T) { + tests := []struct { + name string + locations []string + want string + }{ + {"none", nil, "(none)"}, + {"one", []string{locEast}, locEast}, + {"at the limit", []string{locEast, locWest, locFra}, "us-east-1, eu-west-1, ap-south-1"}, + {"over the limit", []string{locEast, locWest, locFra, locLhr}, "us-east-1, eu-west-1, ap-south-1 (+1)"}, + {"well over", []string{locEast, locWest, locFra, locLhr, "af-south-1", "me-south-1"}, "us-east-1, eu-west-1, ap-south-1 (+3)"}, + } + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + if got := locationsColumn(tc.locations); got != tc.want { + t.Errorf("locationsColumn(%v) = %q, want %q", tc.locations, got, tc.want) + } + }) + } +} + +// TestListWorkloadsLocationsJSONKeepsFullList: truncation is a table concern. +// `-o json` still carries every location. +func TestListWorkloadsLocationsJSONKeepsFullList(t *testing.T) { + all := []string{locEast, locWest, locFra, locLhr, "af-south-1", "me-south-1"} + objs := []client.Object{manyLocations(wlAPI, "uid-api", all...)} + + var tableOut, jsonOut, errOut bytes.Buffer + c := newFakeClient(t, objs...) + if err := listWorkloads(context.Background(), &tableOut, &errOut, c, testProject, listOptions{output: util.OutputTable}); err != nil { + t.Fatalf("listWorkloads (table): %v", err) + } + if !strings.Contains(tableOut.String(), "us-east-1, eu-west-1, ap-south-1 (+3)") { + t.Errorf("table column should name the first locations and count the rest:\n%s", tableOut.String()) + } + if strings.Contains(tableOut.String(), "me-south-1") { + t.Errorf("table column was not truncated:\n%s", tableOut.String()) + } + + c = newFakeClient(t, objs...) + if err := listWorkloads(context.Background(), &jsonOut, &errOut, c, testProject, listOptions{output: util.OutputJSON}); err != nil { + t.Fatalf("listWorkloads (json): %v", err) + } + var views []struct { + Cities []string `json:"locations"` + } + if err := json.Unmarshal(jsonOut.Bytes(), &views); err != nil { + t.Fatalf("output is not a JSON array: %v\n%s", err, jsonOut.String()) + } + if len(views) != 1 { + t.Fatalf("got %d views, want 1:\n%s", len(views), jsonOut.String()) + } + if strings.Join(views[0].Cities, ",") != strings.Join(all, ",") { + t.Errorf("json locations = %v, want %v", views[0].Cities, all) + } +} + +// TestListWorkloadsJSONNoURLVersusUnknown: the table draws "—" for a workload +// with no URL and "?" for URLs that could not be read. The structured output +// has to draw the same distinction — a consumer cannot be told both as `""`. +func TestListWorkloadsJSONNoURLVersusUnknown(t *testing.T) { + t.Run("no url omits the field entirely", func(t *testing.T) { + var out, errOut bytes.Buffer + c := newFakeClient(t, projectObjects()...) + if err := listWorkloads(context.Background(), &out, &errOut, c, testProject, listOptions{output: util.OutputJSON}); err != nil { + t.Fatalf("listWorkloads: %v", err) + } + + byName := viewsByName(t, out.Bytes()) + if _, ok := byName[workerName]["url"]; ok { + t.Errorf("unpublished workload should not carry a url field:\n%s", out.String()) + } + if _, ok := byName[workerName]["urlError"]; ok { + t.Errorf("unpublished workload is not an error:\n%s", out.String()) + } + if got := byName[wlAPI]["url"]; got != "https://"+testCustom { + t.Errorf("api url = %v, want https://%s", got, testCustom) + } + }) + + t.Run("unreadable urls carry an explicit signal", func(t *testing.T) { + c := fake.NewClientBuilder(). + WithScheme(testScheme(t)). + WithObjects(projectObjects()...). + WithInterceptorFuncs(interceptor.Funcs{ + List: func(ctx context.Context, cl client.WithWatch, list client.ObjectList, opts ...client.ListOption) error { + if _, ok := list.(*networkingv1alpha.HTTPProxyList); ok { + return errors.New("forbidden") + } + return cl.List(ctx, list, opts...) + }, + }). + Build() + + var out, errOut bytes.Buffer + if err := listWorkloads(context.Background(), &out, &errOut, c, testProject, listOptions{output: util.OutputJSON}); err != nil { + t.Fatalf("listWorkloads: %v", err) + } + + byName := viewsByName(t, out.Bytes()) + for _, name := range []string{wlAPI, workerName} { + if _, ok := byName[name]["url"]; ok { + t.Errorf("%s: url must not be reported when it could not be read:\n%s", name, out.String()) + } + msg, ok := byName[name]["urlError"].(string) + if !ok || msg == "" { + t.Errorf("%s: expected a urlError signal:\n%s", name, out.String()) + } + if !strings.Contains(msg, "forbidden") { + t.Errorf("%s: urlError = %q, want the server's error in it", name, msg) + } + } + }) +} + +// TestListWorkloadsYAMLNoURLVersusUnknown: the YAML form draws the same +// distinction as the JSON one. +func TestListWorkloadsYAMLNoURLVersusUnknown(t *testing.T) { + var out, errOut bytes.Buffer + c := newFakeClient(t, projectObjects()...) + if err := listWorkloads(context.Background(), &out, &errOut, c, testProject, listOptions{output: util.OutputYAML}); err != nil { + t.Fatalf("listWorkloads: %v", err) + } + if strings.Contains(out.String(), `url: ""`) { + t.Errorf("an unpublished workload must not read as an empty url:\n%s", out.String()) + } +} + +func viewsByName(t *testing.T, raw []byte) map[string]map[string]any { + t.Helper() + var views []map[string]any + if err := json.Unmarshal(raw, &views); err != nil { + t.Fatalf("output is not a JSON array: %v\n%s", err, string(raw)) + } + byName := map[string]map[string]any{} + for _, v := range views { + name, _ := v["name"].(string) + byName[name] = v + } + return byName +}