diff --git a/docs/docs/examples/k3s/byo-host/README.md b/docs/docs/examples/k3s/byo-host/README.md index 2487f45ca..7c942495f 100644 --- a/docs/docs/examples/k3s/byo-host/README.md +++ b/docs/docs/examples/k3s/byo-host/README.md @@ -194,10 +194,17 @@ Stated plainly, because `scale:` will happily let you believe otherwise: - **One failure domain.** Replicas on one node survive a pod crash, not a machine reboot. `scale: {min: 1, max: 1}` in `client.yaml` is honesty, not a technical limit. -- **No multi-node yet.** `vmResource` resolves to exactly one host today; more than one - is refused rather than silently using the first. +- **Multi-node needs a private network.** More than one `hosts:` entry is accepted — see + the commented block in `server.yaml` — but every entry then needs `privateAddress:`, + and that is refused rather than defaulted. k3s otherwise advertises whichever address + the machine's default route carries, which on a single-NIC VPS is the public one, and + etcd plus flannel's unencrypted VXLAN overlay would cross the internet on a host where + SC has no firewall to write a rule in. If your machines share no private network, + run a [compose fleet](../../vm-compose/byo-host/README.md) instead — it needs no + node-to-node traffic at all. - **`local-storage` is the default storage class.** A PersistentVolume is a directory - on that one machine. It does not survive the machine. + on that one machine. It does not survive the machine, and nothing replicates it + across a pool. - **The control plane is pinned**, not tracking a release channel — k3s's installer resolves a channel at install time, which would otherwise make the deployed version depend on the day the deploy ran. Move it deliberately with `version:`. diff --git a/docs/docs/examples/k3s/byo-host/client.yaml b/docs/docs/examples/k3s/byo-host/client.yaml index 99cbb6474..af3332edf 100644 --- a/docs/docs/examples/k3s/byo-host/client.yaml +++ b/docs/docs/examples/k3s/byo-host/client.yaml @@ -34,11 +34,26 @@ stacks: # Reachability on a bare k3s stand. # - # `domain:` here behaves exactly as it does on GKE: SC creates an A record - # pointing at the cluster's ingress address, which for a single-node k3s cluster - # is the node's own address. It needs a registrar configured in the parent stack, - # and — because the node answers the ACME challenge itself — a record that is NOT - # proxied. Cloudflare's orange cloud breaks it. + # `domain:` here behaves as it does on GKE with one difference that matters on a + # pool: SC creates an A record per NODE, all sharing the name, because a k3s + # cluster has no load balancer in front of it. klipper-lb binds the Service's + # ports on every node, so every node is the edge; pointing the name at one of + # them would send every request to that node and lose the cluster the moment it + # reboots. On a single-node cluster that is one record, exactly as before. + # + # It needs a registrar configured in the parent stack, and — because the node + # answers the ACME challenge itself — a record that is NOT proxied. Cloudflare's + # orange cloud breaks it. + # + # What N records buy, stated plainly because DNS round-robin is routinely sold as + # high availability: load spreading, and nothing else. A resolver hands clients + # the addresses in some order and nothing checks whether a node is alive, so a + # dead node costs roughly 1/N of requests until it is replaced or removed from the + # record set. It is not failover. + # + # On a parent stack provisioned before the fleet export existed, this falls back + # to the single record on its own — re-run `sc provision` for the parent to get + # the whole pool into DNS. # # While smoking, leave this out and reach the Service directly instead. There is # no edge in the parent stack above (see its comments for why), so routing by host diff --git a/docs/docs/examples/k3s/byo-host/server.yaml b/docs/docs/examples/k3s/byo-host/server.yaml index 1d8351eeb..ce107e2a4 100644 --- a/docs/docs/examples/k3s/byo-host/server.yaml +++ b/docs/docs/examples/k3s/byo-host/server.yaml @@ -61,6 +61,46 @@ resources: port: 22 privateKey: ${secret:CLUSTER_VM_DEPLOY_KEY} hostKey: "203.0.113.10 ssh-ed25519 AAAAC3NzaC1lZDI1NTE5AAAAIEXAMPLEEXAMPLEEXAMPLEEXAMPLEEXAMPLE00" + # One machine, so no `role:` and no `privateAddress:` — both are inferred, + # and a single-node cluster has no node-to-node traffic to address. For + # more than one machine, both become required. The multi-node shape: + # + # hosts: + # - address: 203.0.113.10 # what YOU and SC reach it on + # privateAddress: 10.0.0.10 # what its SIBLINGS reach it on + # role: k3s-server + # user: ubuntu + # privateKey: ${secret:CLUSTER_VM_DEPLOY_KEY} + # - address: 203.0.113.11 + # privateAddress: 10.0.0.11 + # role: k3s-agent + # user: ubuntu + # privateKey: ${secret:CLUSTER_VM_DEPLOY_KEY} + # - address: 203.0.113.12 + # privateAddress: 10.0.0.12 + # role: k3s-agent + # user: ubuntu + # privateKey: ${secret:CLUSTER_VM_DEPLOY_KEY} + # + # `role:` is all-or-none across the pool — SC will not guess which of + # several machines runs etcd, and the guess decides whether a machine + # joins the cluster at all. Mixing `compose` with a `k3s-*` role in one vm + # resource is refused: one machine has one port 443. + # + # `privateAddress:` is REQUIRED on every host of a multi-node pool, and + # the refusal is deliberate rather than a convenience. SC passes it to + # k3s as `--node-ip`; without it k3s advertises whichever address the + # machine's default route carries, which on a single-NIC VPS is the PUBLIC + # one — and then etcd peer traffic (the cluster's whole state, every + # Secret) and flannel's unencrypted VXLAN overlay cross the public + # internet, on a `byo` host where SC has no firewall to write a rule in. + # That deploy SUCCEEDS: three nodes `Ready`, nothing in the output saying + # the overlay is exposed. Hence a refusal and not a warning. + # + # If your machines share no private network at all — a lot of cheap VPS + # hosting has none — there is nothing honest to put here yet. Run a + # compose fleet instead (../../vm-compose/byo-host/), which needs no + # node-to-node traffic; SC-managed WireGuard is the planned answer. # Deliberately absent here: `ingress`. # # On the vm-compose path, `ingress.enable: true` runs a Caddy container that @@ -88,6 +128,21 @@ resources: # after the vm resource it names, and reads the host facts from its output. vmResource: cluster-vm + # How many of the pool's machines run the control plane. Default 1. + # + # `servers: 1` + N agents uses **sqlite** and has no etcd at all, which is + # why it is the default: it is the simplest thing that works, and losing + # the server loses the control plane but not the running workloads. + # + # `servers: 3` opts into embedded etcd and real HA. It must be ODD and no + # larger than the host count — two members tolerate zero failures, so they + # are strictly worse than one, and four tolerate the same one as three + # while costing a machine. etcd is also latency-sensitive: across + # availability zones in one region (~1ms) is the intended topology; + # across regions or hosters (10-80ms) flaps raft heartbeats and triggers + # repeated leader elections. + # servers: 3 + # Pinned on purpose. k3s's installer resolves a RELEASE CHANNEL at install # time, so leaving this to the default would mean the control-plane version # depends on the day the deploy ran. SC pins a known version instead; set diff --git a/docs/docs/examples/k3s/cloudru-evolution/README.md b/docs/docs/examples/k3s/cloudru-evolution/README.md index b6e481466..196e998f7 100644 --- a/docs/docs/examples/k3s/cloudru-evolution/README.md +++ b/docs/docs/examples/k3s/cloudru-evolution/README.md @@ -275,7 +275,7 @@ Refused rather than accepted and ignored: | `hosts` | that is `provider: byo` | | `network` | the create call rejects `subnet_id`/`networks`/`nics` — the machine lands on the zone's default | | `extraSshKeys` | extra keys are authorised through user-data, which this cloud has none of; the create call takes exactly one key | -| more than one zone | this is a single machine | +| ~~more than one zone~~ | no longer refused at the config tier — but see the k3s caveat below before you use it | | `publicIP: false` | every delivery step is an outbound SSH connection, and SC has no bastion | To place a machine on your own network or security group, create it in the console and @@ -288,11 +288,18 @@ Stated plainly, because `scale:` will happily let you believe otherwise: - **One failure domain.** Replicas on one node survive a pod crash, not a machine reboot. `scale: {min: 1, max: 1}` is honesty, not a technical limit. -- **No multi-node yet.** `vmResource` resolves to exactly one host today; more than one - is refused rather than silently using the first. +- **Multi-node is not usable on this cloud yet, for two separate reasons.** The config + tier accepts it — `nodes:`, several `zones:`, `servers:` on the cluster resource — but + (1) no cloud driver creates more than one machine yet, and SC refuses the mismatch + rather than handing you a one-machine "fleet"; and (2) even once it does, an Evolution + VM create takes no `subnet_id`, so each machine lands on its own zone's default subnet + and the node-to-node firewall rules SC writes (flannel, kubelet, etcd) are scoped to + the subnet the machine landed on — which does not cover its siblings in other zones. + Keep a k3s pool in one zone here. A compose fleet is unaffected: those machines never + talk to each other. - **`local-storage` is the default storage class.** A PersistentVolume is a directory on that one machine, and it does not survive the machine. Cloud.ru's own block storage is - not wired to a CSI driver here. + not wired to a CSI driver here, and nothing replicates a volume across a pool. - **A destroy/provision cycle moves the address.** This driver allocates a new floating IP, so every saved kubeconfig and every DNS record points at the old one. Set `endpoint:` to a DNS name if that matters. diff --git a/docs/docs/examples/k3s/cloudru-evolution/server.yaml b/docs/docs/examples/k3s/cloudru-evolution/server.yaml index 0e843b356..3a35d61a0 100644 --- a/docs/docs/examples/k3s/cloudru-evolution/server.yaml +++ b/docs/docs/examples/k3s/cloudru-evolution/server.yaml @@ -79,6 +79,29 @@ resources: # Placement is the availability zone. zones: [ru.AZ-1] + # How many machines to create. Defaults to `len(zones)`, then 1 — so this + # file makes one. Listing three zones makes three machines, one per zone; + # `nodes: 3` with one zone makes three there; `nodes: 3` with two zones + # round-robins (0→a, 1→b, 2→a) and the assignment is index-stable, so a + # machine never changes zone on a later apply. + # nodes: 3 + # + # ⚠ ACCEPTED HERE, NOT YET HONOURED BY THIS DRIVER. Creating N machines + # is per-plugin work and `sc-plugin-cloudru` has not had it. SC refuses + # the mismatch rather than quietly giving you one machine: a resource + # that asks for three and gets one fails the provision, naming the + # driver and both counts. + # + # ⚠ On **Evolution specifically**, spreading k3s nodes across zones does + # not work yet. An Evolution VM create takes no `subnet_id`, so each + # machine lands on its own zone's default subnet — and the node-to-node + # firewall rules SC writes (flannel, kubelet, etcd) are scoped to "the + # subnet this machine landed on", which does not cover its siblings in + # other zones. Keep a k3s pool in ONE zone here until the SC-managed + # WireGuard mesh lands, which makes the rules one mesh CIDR regardless of + # zone. A compose fleet is unaffected: those machines never talk to each + # other. + # Required on every cloud driver. The kubeconfig SC exports points at this # address, and `sc deploy` connects to it from wherever it runs. publicIP: true diff --git a/docs/docs/examples/vm-compose/byo-host/server.yaml b/docs/docs/examples/vm-compose/byo-host/server.yaml index 6bb3376ba..7920ed85c 100644 --- a/docs/docs/examples/vm-compose/byo-host/server.yaml +++ b/docs/docs/examples/vm-compose/byo-host/server.yaml @@ -98,6 +98,41 @@ resources: # before you trust it — reading it off the network is what it protects # you from. hostKey: "203.0.113.10 ssh-ed25519 AAAAC3NzaC1lZDI1NTE5AAAAIEXAMPLEEXAMPLEEXAMPLEEXAMPLEEXAMPLE00" + # A FLEET is the same list with more entries. Add a second machine and every + # stack on this resource deploys a full container set onto each one, rolled + # one host at a time — the next host starts only after the previous host's + # new slot is live, so a bad image stops the roll instead of taking the + # fleet down. Each machine runs its own Caddy and its own blue-green pair; + # blue-green is WITHIN a host and is not, and will not be, across machines. + # + # hosts: + # - address: 203.0.113.10 + # user: ubuntu + # privateKey: ${secret:PORTAL_VM_DEPLOY_KEY} + # hostKey: "…" + # - address: 203.0.113.11 + # user: ubuntu + # privateKey: ${secret:PORTAL_VM_DEPLOY_KEY} + # hostKey: "…" + # + # No `role:` needed — a pool with no roles declared is a compose fleet, and + # no `privateAddress:` either, because these machines never talk to each + # other. That is the useful difference from a multi-node k3s pool (see + # ../../k3s/byo-host/server.yaml), and it is why a compose fleet works on + # cheap VPS hosting that offers no private network at all. + # + # Two things a fleet needs that a single machine does not: + # + # * `ingress.acmeStorage` — otherwise each machine runs its own ACME + # client against the same name and you get N certificates, N sets of + # rate-limit consumption, and HTTP-01 challenges answered by whichever + # node DNS happened to pick (which 404s a token another node minted). + # One shared store makes it one certificate. See `ingress` below. + # * More than one A record for the name, so requests reach more than one + # machine. Note what this does and does not give you: a dead node means + # roughly 1/N of requests fail and are retried by the client. It is + # round-robin, not failover — a cloud load balancer with health checks + # is strictly better where the cloud has one. # The front door: a Caddy container in its own compose project, terminating # TLS for every stack on this machine. It belongs to the resource and not to # a template because one machine has one port 443 — and because a stack's diff --git a/docs/docs/examples/vm-compose/cloudru-evolution/README.md b/docs/docs/examples/vm-compose/cloudru-evolution/README.md index 73e7c793c..84c44e1ac 100644 --- a/docs/docs/examples/vm-compose/cloudru-evolution/README.md +++ b/docs/docs/examples/vm-compose/cloudru-evolution/README.md @@ -181,9 +181,19 @@ stack plus the Caddy ingress without the build cache pushing it into swap. names are not the same across projects, and a guess here would be a guess about your quota. -`zones` takes at most one entry (`ru.AZ-1` … `ru.AZ-3`) — this is a single machine, and a -list of zones would describe a cluster. Left out, SC takes the first zone -`GET /availability-zones` reports. +`zones` (`ru.AZ-1` … `ru.AZ-3`) is also the fleet's size: SC creates one machine per +zone, and `nodes:` sets the count independently (`nodes: 3` with one zone puts three +there; with two it round-robins, index-stably, so a machine never changes zone on a +later apply). Left out, SC takes the first zone `GET /availability-zones` reports and +creates one machine. + +**But no cloud driver creates more than one machine yet** — that is per-plugin work and +`sc-plugin-cloudru` has not had it. SC refuses the mismatch rather than quietly giving +you a one-machine fleet: asking for three and receiving one fails the provision, naming +the driver and the count. So `nodes:`/multi-zone here is config you can write ahead of +the driver, not a fleet you can stand up today. On machines that already exist, +`provider: byo` with several `hosts:` entries works now — see +[that example](../byo-host/README.md). ## Which ports are open @@ -256,7 +266,7 @@ in which the operator is certain the setting applied. | `hosts` | SC creates the machine; a host list describes one you brought (`provider: byo`) | | `network` | the create call accepts no subnet or security group at all (measured), so the machine always joins the zone's defaults | | `extraSshKeys` | extra keys are authorised through user-data, which this cloud does not run; the create takes exactly one public key — the deploy key. Add one by hand, or keep console access for incidents | -| `zones` with more than one entry | one machine is in one zone | +| ~~`zones` with more than one entry~~ | no longer refused — it is the fleet's size, but see the driver caveat above | | `size.memory` that is not whole GiB | memory resolves a flavour by exact match; rounding would hand you a different machine | The `vm-compose` template's own refusals (`scale` other than 1, `domainProxied`, diff --git a/docs/docs/examples/vm-compose/cloudru-evolution/server.yaml b/docs/docs/examples/vm-compose/cloudru-evolution/server.yaml index b34c9f39b..52ea7530c 100644 --- a/docs/docs/examples/vm-compose/cloudru-evolution/server.yaml +++ b/docs/docs/examples/vm-compose/cloudru-evolution/server.yaml @@ -76,8 +76,28 @@ resources: # request. Placement is the availability zone. A `region:` SC silently # dropped would read as placement you controlled. # - # One entry at most: this is a single machine. Left out, SC takes the first - # zone `GET /availability-zones` reports. + # One entry here, because this file deploys one machine. Left out, SC takes + # the first zone `GET /availability-zones` reports. + # + # Several entries make several machines — a compose fleet, one zone each. + # `nodes:` sets the count independently: it defaults to `len(zones)`, then + # 1, so `nodes: 3` with one zone puts three machines there and `nodes: 3` + # with two zones round-robins (0→a, 1→b, 2→a). The assignment is + # index-stable, so a machine never moves zone on a later apply. + # + # A compose fleet spreads across zones safely: each machine runs a full + # container set and its own Caddy, and they never talk to each other. Give + # them `ingress.acmeStorage` so they share one certificate instead of + # racing for N, and point more than one A record at the name. (A multi-node + # k3s pool is NOT safe across zones on this cloud yet — see + # ../../k3s/cloudru-evolution/server.yaml for why.) + # + # ⚠ ACCEPTED HERE, NOT YET HONOURED BY THIS DRIVER. Creating N machines is + # per-plugin work and `sc-plugin-cloudru` has not had it. SC refuses the + # mismatch rather than quietly giving you one machine: a resource that asks + # for three and gets one fails the provision, naming the driver and both + # counts. Several machines work today on `provider: byo`, where they + # already exist. zones: [ru.AZ-1] # Required on every cloud driver, and refused if false: every delivery step diff --git a/docs/docs/examples/vm-compose/cloudru/README.md b/docs/docs/examples/vm-compose/cloudru/README.md index cf999eeb8..9a110e105 100644 --- a/docs/docs/examples/vm-compose/cloudru/README.md +++ b/docs/docs/examples/vm-compose/cloudru/README.md @@ -179,9 +179,16 @@ because rounding would hand you a different machine than the one you asked for. `disk` is **not** defaulted: left out, the image keeps its own root-disk size, which is a better answer than one SC invents. -`zones` takes at most one entry — this is a single machine, and a list of zones would -describe a cluster. Left out, SC takes the first zone the availability-zones datasource -reports (documented as alphanumeric, so stable). +`zones` is also the fleet's size: one machine per zone, with `nodes:` setting the count +independently (round-robin over the zones, index-stable). Left out, SC takes the first +zone the availability-zones datasource reports (documented as alphanumeric, so stable) +and creates one machine. + +**No cloud driver creates more than one machine yet.** SC refuses the mismatch rather +than silently handing you a one-machine fleet — asking for three and receiving one fails +the provision and names the driver. Several machines work today only on machines that +already exist: `provider: byo` with several `hosts:` entries +([that example](../byo-host/README.md)). ## Reachability from inside Russia @@ -213,7 +220,7 @@ in which the operator is certain the setting applied. |---|---| | `publicIP: false` | every delivery step is an outbound connection *from* wherever `sc deploy` runs, so a machine with no public address is created, billed and unreachable — and cloud-init has no egress either | | `hosts` | SC creates the machine; a host list describes one you brought (`provider: byo`) | -| `zones` with more than one entry | one machine is in one zone | +| ~~`zones` with more than one entry~~ | no longer refused — it is the fleet's size, but see the driver caveat above | | `network` with only `id` or only `subnets` | picking "some subnet in the VPC" places the machine in a zone, or behind a route table, you did not choose | | `size.memory` that is not whole GiB | the flavours datasource matches memory exactly; rounding would hand you a different machine | | `region` absent | stated on the resource, not inherited silently: it selects the IAM endpoint and the project as well as the placement, so a resource that did not say where it lives could be created somewhere the credential's default moved to | diff --git a/pkg/api/client.go b/pkg/api/client.go index e8d968755..8f10f50bc 100644 --- a/pkg/api/client.go +++ b/pkg/api/client.go @@ -131,8 +131,16 @@ type ContainerImageBuild struct { } type StackConfigSingleImage struct { - Image *ContainerImage `json:"image" yaml:"image"` - Domain string `json:"domain" yaml:"domain"` + Image *ContainerImage `json:"image" yaml:"image"` + Domain string `json:"domain" yaml:"domain"` + // DomainProxied mirrors StackConfigCompose.DomainProxied, which is where every + // target actually reads it from (vm-compose, gke-autopilot, kubernetes). Without + // it here the key parses and is then dropped on the floor by the single-image → + // compose conversion: no error, no warning, and an UNPROXIED record pointing at a + // fleet whose origin is deliberately plain HTTP. Live-caught 2026-11-08 on the + // 3-VPS compose smoke, where `domainProxied: true` produced three `proxied=false` + // A records and a name reachable only over cleartext. + DomainProxied *bool `json:"domainProxied" yaml:"domainProxied"` BaseDnsZone string `json:"baseDnsZone" yaml:"baseDnsZone"` // only necessary if differs from parent stack Env map[string]string `json:"env" yaml:"env"` Secrets map[string]string `json:"secrets" yaml:"secrets"` diff --git a/pkg/api/logger/logger.go b/pkg/api/logger/logger.go index 5996c09c3..114538499 100644 --- a/pkg/api/logger/logger.go +++ b/pkg/api/logger/logger.go @@ -83,3 +83,17 @@ func getLogLevel(ctx context.Context) int { } return defaultLogLevel } + +// LevelFromContext exposes the level SetLogLevel/Silent stored, for the one +// case the Logger interface cannot cover: handing SC's verbosity to a +// third-party logger that writes on its own. hashicorp/go-plugin takes an +// hclog.Logger and logs the whole spawn handshake through it, so without this +// a command that silenced SC still got go-plugin's output (see +// pkg/plugin/discover.go). +// +// Returns the same defaultLogLevel as the Logger methods when the context +// carries no level, so a caller cannot accidentally read "silent" from a bare +// context.Background(). +func LevelFromContext(ctx context.Context) int { + return getLogLevel(ctx) +} diff --git a/pkg/clouds/k3s/cluster.go b/pkg/clouds/k3s/cluster.go index 68a9d45bf..d3e57f434 100644 --- a/pkg/clouds/k3s/cluster.go +++ b/pkg/clouds/k3s/cluster.go @@ -138,6 +138,27 @@ type K3sClusterConfig struct { // why this field and the SAN list are the same field by default. Endpoint string `json:"endpoint,omitempty" yaml:"endpoint,omitempty"` + // Servers is how many of the vm resource's hosts run the control plane. The rest + // are agents. + // + // Default 1 (see EffectiveServers): one server and N-1 agents, which is the + // topology worth reaching for first — it schedules pods across the whole fleet + // and survives the loss of any agent, and it is the only shape a 2-machine pool + // can have. Its control plane is a single point of failure: while host 0 is down + // the API server is unreachable and nothing can be deployed, though pods already + // running on the agents keep serving. + // + // Set it to 3 (or 5) to run embedded etcd instead, which survives the loss of a + // control-plane node. It must be ODD: etcd needs a majority of its members to + // elect a leader, so 2 servers tolerate zero failures — strictly worse than 1, + // because either machine going away takes the API server down with it. Refused + // rather than rounded. + // + // Ignored on a byo pool that names its own `hosts[].role`, where the operator + // chose which machine runs what; a value disagreeing with that choice is refused + // rather than silently losing (pkg/clouds/pulumi/k3s/cluster.go). + Servers int `json:"servers,omitempty" yaml:"servers,omitempty"` + // TlsSans are additional names to put on the API server certificate, beyond // Endpoint and the node address. Spend them freely: adding a SAN later means // re-issuing the certificate and restarting the API server, while an unused SAN @@ -197,6 +218,16 @@ func (r *K3sClusterConfig) EffectiveVersion() string { return lo.If(r.Version == "", DefaultVersion).Else(r.Version) } +// EffectiveServers is the control-plane node count, defaulting to 1. +// +// The one place the default lives, so the provisioning tier never re-derives it: +// a resource that disagreed with itself about which hosts are servers would join +// a machine to a cluster as an agent on one apply and as an etcd member on the +// next, and the second of those is not reversible by changing the number back. +func (r *K3sClusterConfig) EffectiveServers() int { + return lo.If(r.Servers == 0, 1).Else(r.Servers) +} + func (r *K3sClusterConfig) EffectiveInstallScriptURL() string { return lo.If(r.InstallScriptURL == "", DefaultInstallScriptURL).Else(r.InstallScriptURL) } @@ -210,7 +241,8 @@ func (r *K3sClusterConfig) Validate() error { if r.VmResource == "" { return errors.Errorf( "`%s` resource requires `vmResource`: the name of the `type: vm` resource in this parent stack "+ - "whose hosts become the cluster's nodes", ResourceTypeK3sCluster) + "whose hosts become the cluster's nodes", ResourceTypeK3sCluster, + ) } if r.Version != "" && !versionPattern.MatchString(r.Version) { @@ -224,12 +256,27 @@ func (r *K3sClusterConfig) Validate() error { r.Version, DefaultVersion, hint) } + // The upper bound — servers must not exceed the pool — is NOT checked here: this + // config does not know how many hosts the vm resource it names has. The + // provisioning tier refuses that combination, where both numbers are in hand. + if r.Servers < 0 { + return errors.Errorf("`servers` must be positive, got %d", r.Servers) + } + if r.Servers > 1 && r.Servers%2 == 0 { + return errors.Errorf( + "`servers` is %d; an embedded-etcd control plane needs an odd number (1, 3, 5) because etcd elects "+ + "a leader by majority — %d members tolerate no failures at all, so they are strictly worse "+ + "than 1", r.Servers, r.Servers, + ) + } + if r.Endpoint != "" { if !endpointPattern.MatchString(r.Endpoint) { return errors.Errorf( "`endpoint` %q must be a bare host name or IP address — no scheme and no port. SC builds the "+ "kubeconfig URL (https://:%d) and the `--tls-san` entry from it, and both take a "+ - "host alone", r.Endpoint, APIPort) + "host alone", r.Endpoint, APIPort, + ) } } @@ -243,7 +290,8 @@ func (r *K3sClusterConfig) Validate() error { if c == "coredns" { return errors.Errorf( "`disable` must not contain %q: without cluster DNS no workload can resolve a Service, and "+ - "every deploy onto this cluster fails in a way that looks unrelated to this setting", c) + "every deploy onto this cluster fails in a way that looks unrelated to this setting", c, + ) } if !lo.Contains(DisableableComponents, c) { return errors.Errorf("`disable[%d]` %q is not a k3s packaged component; one of: %s", @@ -257,7 +305,8 @@ func (r *K3sClusterConfig) Validate() error { return errors.Errorf( "`extraServerArgs[%d]` sets %s, which SC sets itself: k3s takes the LAST occurrence of a "+ "repeated flag, so this would silently override SC's value. Use the dedicated field "+ - "instead (%s)", i, flag, fieldFor(flag)) + "instead (%s)", i, flag, fieldFor(flag), + ) } } diff --git a/pkg/clouds/k3s/cluster_test.go b/pkg/clouds/k3s/cluster_test.go index 0bf0667a2..2e9118361 100644 --- a/pkg/clouds/k3s/cluster_test.go +++ b/pkg/clouds/k3s/cluster_test.go @@ -106,6 +106,41 @@ func TestK3sClusterConfigValidate(t *testing.T) { return c }, }, + { + name: "one server is the default and is accepted explicitly", + cfg: func() *K3sClusterConfig { c := valid(); c.Servers = 1; return c }, + }, + { + name: "three servers are accepted", + cfg: func() *K3sClusterConfig { c := valid(); c.Servers = 3; return c }, + }, + { + // Refused rather than rounded: etcd elects a leader by majority, so two + // members tolerate zero failures — either machine going away takes the + // API server down with it, which is strictly worse than one server. + name: "two servers are refused with the quorum reason", + cfg: func() *K3sClusterConfig { c := valid(); c.Servers = 2; return c }, + wantErr: "tolerate no failures at all", + }, + { + name: "four servers are refused too", + cfg: func() *K3sClusterConfig { c := valid(); c.Servers = 4; return c }, + wantErr: "needs an odd number (1, 3, 5)", + }, + { + name: "a negative server count is refused", + cfg: func() *K3sClusterConfig { c := valid(); c.Servers = -1; return c }, + wantErr: "`servers` must be positive", + }, + { + // The UPPER bound is deliberately not checked here: this config does not + // know how many hosts the vm resource it names has. The provisioning + // tier refuses that combination, where both numbers are in hand + // (pkg/clouds/pulumi/k3s/cluster.go, rolesOf). + name: "a server count exceeding any plausible pool still validates here", + cfg: func() *K3sClusterConfig { c := valid(); c.Servers = 99; return c }, + wantErr: "", + }, } { t.Run(tc.name, func(t *testing.T) { RegisterTestingT(t) @@ -121,6 +156,18 @@ func TestK3sClusterConfigValidate(t *testing.T) { } } +// The default lives in exactly one place so the provisioning tier never re-derives +// it. A resource that disagreed with itself about which hosts are servers would join +// a machine as an agent on one apply and as an etcd member on the next, and the +// second of those is not reversible by changing the number back. +func TestEffectiveServers(t *testing.T) { + RegisterTestingT(t) + + Expect((&K3sClusterConfig{}).EffectiveServers()).To(Equal(1)) + Expect((&K3sClusterConfig{Servers: 1}).EffectiveServers()).To(Equal(1)) + Expect((&K3sClusterConfig{Servers: 3}).EffectiveServers()).To(Equal(3)) +} + // The dependency declaration is the entire mechanism by which a cluster sees its // machines: provision.go provisions dependency-free resources first and hands their // outputs to the ones that declare a dependency. A config that did not implement diff --git a/pkg/clouds/pulumi/api/fleet_dns.go b/pkg/clouds/pulumi/api/fleet_dns.go new file mode 100644 index 000000000..d793cdd50 --- /dev/null +++ b/pkg/clouds/pulumi/api/fleet_dns.go @@ -0,0 +1,166 @@ +// SPDX-License-Identifier: MIT +// Copyright (c) Simple Container + +package api + +import ( + "github.com/pkg/errors" + + sdk "github.com/pulumi/pulumi/sdk/v3/go/pulumi" + "github.com/pulumi/pulumi/sdk/v3/go/pulumi/internals" + + "github.com/simple-container-com/api/pkg/api" +) + +// FleetDnsRecords points a stack's domain at every machine of the fleet serving it: +// one A record per host, all sharing the name. +// +// Why here and not in the parent stack. A vm resource describes machines; it does not +// know which stacks will be deployed onto them or what domains those stacks claim. The +// record belongs to whoever named the domain, which is the stack — the same split every +// other template already uses (kubernetes/kube_run.go and +// gcp/gke_autopilot_stack.go:206 both read the cluster's ingress address out of the +// parent and create the A record from the child). +// +// Why in pulumi/api rather than beside its first caller. It began in pulumi/vm, for the +// compose fleet; the k3s path needs exactly the same thing (klipper-lb binds :80/:443 on +// every node, so a k3s cluster's front door is N addresses too) and pulumi/kubernetes +// cannot import pulumi/vm. Both already import this package, which owns the Registrar +// interface and NotConfiguredRegistrarError — the two things this function is written +// against. One implementation, because the collision guard below is the part nobody +// would think to write twice. +// +// Why N records rather than one with N values. api.DnsRecord is one name to one value +// (pkg/api/dns.go), and the NewDnsRecord RPC carries one value per call, so N machines +// are N calls. That is the right shape for Cloudflare, where N values at one name are N +// resources; it is the WRONG shape for a provider whose model is a recordset holding +// every value for a name+type, and that difference is the registrar's to absorb, not +// this function's. +// +// Not ordered behind the deploys, because it cannot be: Registrar.NewRecord takes a +// record and nothing else — no resource options, so no DependsOn. The record may +// therefore appear a moment before the containers answer on a machine. Harmless in +// practice (a first deploy has no clients, and a re-deploy keeps the previous slot +// serving until its replacement passes the health gate) but worth knowing before +// reading an early 502 as a DNS bug. +// +// What this does NOT give you, stated here because DNS round-robin is routinely sold as +// high availability: a resolver hands clients the addresses in some order and nothing +// checks whether a machine is alive. A dead node means roughly 1/N of requests fail and +// are retried client-side. It spreads load and survives nothing. A cloud load balancer +// with health checks is strictly better where the cloud has one. +func FleetDnsRecords( + ctx *sdk.Context, + params ProvisionParams, + stackName, domain string, + addresses []string, + proxied bool, +) error { + if domain == "" || len(addresses) == 0 { + return nil + } + if params.Registrar == nil { + // Not an error. A parent stack with no registrar is a supported arrangement — + // the operator creates the record by hand, which is what the docs for every + // cloud without a registrar entry already say — but it has to be said out + // loud, because "the certificate never appeared" is otherwise a silent + // failure with the cause two stacks away. + params.Log.Warn(ctx.Context(), + "stack %q claims domain %q but the parent stack declares no registrar, so SC creates no DNS record. "+ + "Point %q at %s yourself, %s", + stackName, domain, domain, addresses, ManualRecordAdvice(proxied)) + return nil + } + + urns := make(map[string]int, len(addresses)) + for i, address := range addresses { + out, err := params.Registrar.NewRecord(ctx, api.DnsRecord{ + Name: domain, + Type: "A", + Value: address, + Proxied: proxied, + }) + if err != nil { + if errors.Is(err, NotConfiguredRegistrarError) { + // Same situation as a nil Registrar, reached the other way: the + // descriptor exists but is not configured. + params.Log.Warn(ctx.Context(), + "stack %q claims domain %q but the parent stack's registrar is not configured, so SC creates "+ + "no DNS record. Point %q at %s yourself, %s", + stackName, domain, domain, addresses, ManualRecordAdvice(proxied)) + return nil + } + return errors.Wrapf(err, "failed to create the A record for %q at %s (host %d of the fleet)", + domain, address, i) + } + + // The collision guard, and the reason this loop collects anything at all. + // + // NewDnsRecordRequest carries no resource name or URN hint — just name, type, + // value, proxied, priority — so the plugin is the only side that can keep two + // records at one name apart, and each RPC sees its own call with no + // cross-record context. Three of the four registrar plugins derive the + // resource name from name+type ALONE (sc-plugin-cloudru, sc-plugin-gcp) or + // from the name alone (sc-plugin-yandex); only sc-plugin-cloudflare hashes the + // value in. On the other three, the second call silently REPLACES the first, + // and the fleet ends up with one A record pointing at one machine: a green + // deploy, a resolvable name, and two thirds of the fleet receiving no traffic. + // + // SC does not need to know which registrars are fixed to catch that. If two + // calls hand back the same resource, the registrar collapsed them. Refused + // rather than warned about for the usual reason — the deploy succeeds either + // way, so a warning is the one thing that would not be noticed. + ref, ok := out.Ref.(sdk.StringOutput) + if !ok { + // A registrar whose Ref is not a string Output. Nothing to compare, so + // nothing is claimed: the records were still created. + continue + } + res, err := internals.UnsafeAwaitOutput(ctx.Context(), ref) + if err != nil { + return errors.Wrapf(err, "failed to resolve the reference of the A record for %q at %s", domain, address) + } + id, _ := res.Value.(string) + if !res.Known || id == "" { + // Unknown during a preview, which is normal — the resource does not exist + // yet. The guard runs on apply. + continue + } + if first, dup := urns[id]; dup { + return errors.Errorf( + "the registrar gave the A records for hosts %d and %d of this fleet the same resource (%q), so the "+ + "second replaced the first and %q would resolve to one machine instead of %d. The registrar "+ + "plugin derives its resource name without the record's value, which is a bug in that plugin — "+ + "upgrade it, or point %q at the fleet by hand", + first, i, id, domain, len(addresses), domain, + ) + } + urns[id] = i + } + + params.Log.Info(ctx.Context(), + "stack %q: %q resolves to %d machine(s) of the fleet — %s. DNS round-robin spreads load; it does not fail "+ + "over, so a dead machine costs roughly 1/%d of requests until it is replaced", + stackName, domain, len(addresses), addresses, len(addresses)) + return nil +} + +// ManualRecordAdvice is what to tell an operator who has to create the records by hand +// because the parent stack has no usable registrar. +// +// Which half is right depends on the fleet's front door, and the wrong half is not +// merely unhelpful. Told to proxy a fleet that terminates its own TLS, they get a name +// whose HTTP-01 challenge is answered at Cloudflare's edge instead of on the machine, so +// no certificate ever issues and Caddy retries forever. Told NOT to proxy a fleet +// running `ingress.originTls: false`, they point clients straight at an origin that +// serves plain HTTP on :80 by design — the edge that was supposed to terminate TLS is +// bypassed, and the only thing that was ever protecting that hop is gone. +// +// Both halves used to be one hardcoded "unproxied", correct until #566 made +// `domainProxied: true` reachable. +func ManualRecordAdvice(proxied bool) string { + if proxied { + return "proxied — the fleet serves plain HTTP and the edge in front of it terminates TLS" + } + return "unproxied — the machines answer the ACME challenge themselves" +} diff --git a/pkg/clouds/pulumi/api/registrar.go b/pkg/clouds/pulumi/api/registrar.go index 981e0c373..c358d0cbc 100644 --- a/pkg/clouds/pulumi/api/registrar.go +++ b/pkg/clouds/pulumi/api/registrar.go @@ -4,11 +4,27 @@ package api import ( + "github.com/pkg/errors" + sdk "github.com/pulumi/pulumi/sdk/v3/go/pulumi" "github.com/simple-container-com/api/pkg/api" ) +// NotConfiguredRegistrarError is what every method of the no-op registrar returns. +// +// A parent stack that declares no registrar still gets a Registrar — the empty type is +// registered to NotConfiguredRegistrar (pulumi/init.go), which hands back a non-nil +// value whose methods all fail with this. So a `params.Registrar == nil` check does NOT +// tell a caller "nobody will create DNS for me"; this error is the only thing that +// does, and a caller for whom hand-made DNS is a supported arrangement has to match on +// it rather than on nil. +// +// It lives beside the Registrar interface, not beside that no-op implementation, because +// the subpackages that have to match on it (pulumi/vm) are imported BY the pulumi +// package and cannot import it back. +var NotConfiguredRegistrarError = errors.New("registrar is not configured") + type Registrar interface { MainDomain() string ProvisionRecords(ctx *sdk.Context, params ProvisionParams) (*api.ResourceOutput, error) diff --git a/pkg/clouds/pulumi/cancel.go b/pkg/clouds/pulumi/cancel.go index a834caf9b..11a7f5ee4 100644 --- a/pkg/clouds/pulumi/cancel.go +++ b/pkg/clouds/pulumi/cancel.go @@ -6,6 +6,8 @@ package pulumi import ( "context" + "github.com/pkg/errors" + "github.com/pulumi/pulumi/sdk/v3/go/auto" "github.com/simple-container-com/api/pkg/api" @@ -13,6 +15,18 @@ import ( ) func (p *pulumi) CancelStack(ctx context.Context, cfg *api.ConfigFile, stack api.Stack, params api.StackParams) error { + // Same pre-engine load as OutputsStack and for the same reason — cancel + // has to reach the backend to clear the lock, and a plugin-owned + // state storage only has a URL once its sidecar has run InitStateStore. + // Fixed alongside outputs rather than after its own incident: cancel is + // what an operator reaches for when a deploy is already wedged, so it + // failing with `diy URL has an illegal prefix` lands at the worst + // possible moment. + if err := p.loadPreEnginePlugins(ctx, stack); err != nil { + return errors.Wrap(err, "pre-engine plugin load") + } + defer p.closePreEnginePlugins() + if params.Environment != "" && params.StackName != "" { stack = toChildStack(stack, params) } diff --git a/pkg/clouds/pulumi/k3s/cloudrun.go b/pkg/clouds/pulumi/k3s/cloudrun.go index 993ec4193..16f3ec472 100644 --- a/pkg/clouds/pulumi/k3s/cloudrun.go +++ b/pkg/clouds/pulumi/k3s/cloudrun.go @@ -110,5 +110,16 @@ func K3sCloudrun(ctx *sdk.Context, stack api.Stack, input api.ResourceInput, par // default would miss the output and fail every workload that declares a // `domain` — and only those, which is why it would have shipped green. IngressIPExport: pulumiKubernetes.ToIngressIpExport(clusterName), + // And the reason a k3s cluster needs more than that one: there is no load + // balancer in front of it. klipper-lb binds the Service's ports on every node, + // so a name pointed at a single node sends every request to that node and + // loses the cluster the moment it reboots. One A record per node instead — + // which also means a `domain:` here behaves the way it already does on a + // compose fleet of the same machines. + // + // Harmless on a parent provisioned before that output existed: a missing output + // reads as empty and KubeRun falls back to the single record (see + // KubeRunOpts.IngressIPsExport). + IngressIPsExport: pulumiKubernetes.ToIngressIpsExport(clusterName), }) } diff --git a/pkg/clouds/pulumi/k3s/cloudrun_test.go b/pkg/clouds/pulumi/k3s/cloudrun_test.go index c1c6ba34c..c32b015a2 100644 --- a/pkg/clouds/pulumi/k3s/cloudrun_test.go +++ b/pkg/clouds/pulumi/k3s/cloudrun_test.go @@ -50,6 +50,10 @@ func TestClusterExportNamesAndCloudrunReadNamesAgree(t *testing.T) { Expect(readSide).To(Equal(exportSide)) Expect(pulumiKubernetes.ToKubeconfigExport(readSide)).To(Equal(pulumiKubernetes.ToKubeconfigExport(exportSide))) Expect(pulumiKubernetes.ToIngressIpExport(readSide)).To(Equal(pulumiKubernetes.ToIngressIpExport(exportSide))) + // The third output of the seam, and the one whose absence is silent in a + // new way: a reader that misses it does not fail, it falls back to a + // single A record and the other nodes of the cluster receive nothing. + Expect(pulumiKubernetes.ToIngressIpsExport(readSide)).To(Equal(pulumiKubernetes.ToIngressIpsExport(exportSide))) }) } } diff --git a/pkg/clouds/pulumi/k3s/cluster.go b/pkg/clouds/pulumi/k3s/cluster.go index a0522759c..02d9f2ffa 100644 --- a/pkg/clouds/pulumi/k3s/cluster.go +++ b/pkg/clouds/pulumi/k3s/cluster.go @@ -14,6 +14,9 @@ package k3s import ( + "fmt" + "strings" + "github.com/pkg/errors" "github.com/samber/lo" @@ -25,6 +28,7 @@ import ( pApi "github.com/simple-container-com/api/pkg/clouds/pulumi/api" pulumiKubernetes "github.com/simple-container-com/api/pkg/clouds/pulumi/kubernetes" pulumiVm "github.com/simple-container-com/api/pkg/clouds/pulumi/vm" + "github.com/simple-container-com/api/pkg/clouds/vm" ) // K3sOutput is what a provisioned k3s-cluster hands to anything in the same parent @@ -57,6 +61,14 @@ func K3sCluster(ctx *sdk.Context, stack api.Stack, input api.ResourceInput, para if err != nil { return nil, err } + roles, err := rolesOf(cfg, input, vmOut) + if err != nil { + return nil, err + } + // Host 0 is always a server, and the FIRST one: it is where SC installs before + // anything else, where it reads the join token, and where it reads the + // kubeconfig. rolesOf guarantees it (a byo pool that declares `k3s-agent` on its + // first host is refused there, with the reason). host := vmOut.Hosts[0] // The cluster name, and therefore the export name, is derived exactly as @@ -75,6 +87,9 @@ func K3sCluster(ctx *sdk.Context, stack api.Stack, input api.ResourceInput, para }).(sdk.StringOutput) disable := disableSet(cfg, vmOut) + if err := validateIngressPorts(cfg, input, vmOut, disable); err != nil { + return nil, err + } if vmOut.IngressEnabled && lo.Contains(disable, componentServiceLB) { // Said once, here, because it is a composition an operator can reasonably // want and will not otherwise understand the consequences of: the machine's @@ -89,26 +104,76 @@ func K3sCluster(ctx *sdk.Context, stack api.Stack, input api.ResourceInput, para version := cfg.EffectiveVersion() installURL := cfg.EffectiveInstallScriptURL() - params.Log.Info(ctx.Context(), "configure k3s %s cluster %q on host %q of vm %q", - version, clusterName, host.Name, cfg.VmResource) - - installScript := sdk.All(host.Address, endpoint).ApplyT(func(args []any) string { - addr := args[0].(string) - ep := args[1].(string) - // The node's own address is a SAN as well as the endpoint, and always: an - // operator who points `endpoint` at a DNS name still reaches the API server - // by IP from the console, and a SAN added later costs a certificate - // re-issue and an API server restart (k3s-all-clouds.md §4). - sans := lo.Uniq(append([]string{ep, addr}, cfg.TlsSans...)) - return InstallScript(version, installURL, ServerArgs(cfg, sans, disable)) + serverCount := lo.CountBy(roles, func(r vm.HostRole) bool { return r == vm.RoleK3sServer }) + params.Log.Info(ctx.Context(), + "configure k3s %s cluster %q on %d host(s) of vm %q: %d server(s), %d agent(s)", + version, clusterName, len(vmOut.Hosts), cfg.VmResource, serverCount, len(vmOut.Hosts)-serverCount) + + // The SAN list covers the endpoint and every SERVER's own address — and always: + // an operator who points `endpoint` at a DNS name still reaches the API server by + // IP from the console, and a SAN added later costs a certificate re-issue and an + // API server restart (k3s-all-clouds.md §4). + // + // Agents are deliberately NOT in it. They serve no API, so their addresses would + // buy nothing — and they would cost the one thing worth protecting here: the SAN + // list is part of every server's argument vector, so adding a fourth agent to a + // working cluster would change the args marker on all three control-plane nodes + // and reinstall them. Scaling the workers must not restart the control plane. + sanInputs := []any{endpoint} + for i, h := range vmOut.Hosts { + if roles[i] == vm.RoleK3sServer { + // Both addresses of every server. The public one is what an operator and + // the kubeconfig dial; the private one is what peers dial, because that is + // what `--advertise-address` makes the API server advertise. Omitting the + // private one is an x509 failure on every server-to-server call, and it + // presents as a cluster that forms and then cannot elect a leader. + sanInputs = append(sanInputs, h.Address, h.EffectivePrivateAddress()) + } + } + sans := sdk.All(sanInputs...).ApplyT(func(args []any) []string { + return lo.Uniq(append(lo.Map(args, func(a any, _ int) string { return a.(string) }), cfg.TlsSans...)) + }).(sdk.StringArrayOutput) + + // Where every other node registers. Host 0's own address and not `endpoint`: the + // endpoint may be a name that does not resolve yet (the registrar writes it in + // the same provision) or a load balancer whose health checks only pass once the + // nodes it balances are up — either way a join that waits for it deadlocks. + // + // Its PRIVATE address, because a join is node-to-node traffic. Equal to the + // public one whenever the distinction does not apply (see HostFacts). + joinURL := host.EffectivePrivateAddress().ApplyT(func(addr string) string { + return fmt.Sprintf("https://%s:%d", addr, k3s.APIPort) + }).(sdk.StringOutput) + + // Only on a multi-node cluster: a single node has no peers to advertise to, and + // pinning `--node-ip` there would change the args marker of every existing + // single-node cluster and reinstall its control plane for no behaviour change. + nodeIP := lo.If(len(vmOut.Hosts) > 1, host.EffectivePrivateAddress()).Else(sdk.String("").ToStringOutput()) + + installScript := sdk.All(sans, nodeIP).ApplyT(func(args []any) string { + // `--cluster-init` only when this cluster has more than one server: embedded + // etcd on a single node is a strictly worse sqlite (see JoinSpec). + join := JoinSpec{ClusterInit: serverCount > 1, NodeIP: args[1].(string)} + return InstallScript(version, installURL, ServerArgs(cfg, args[0].([]string), disable, join), ServerUnit, "") }).(sdk.StringOutput) conn := pulumiVm.ConnectionArgs(host) + // NOTE on the resource name: host 0's install keeps the un-suffixed name it has + // had since the single-node slice, and the joining nodes take `-install-`. + // That is what makes this change additive for an existing single-node stack — + // the URN of the one command it already has is byte-identical, so Pulumi updates + // it rather than replacing a control plane. install, err := remote.NewCommand(ctx, input.ToResName(input.Descriptor.Name+"-install"), &remote.CommandArgs{ Connection: conn, Create: installScript, Update: installScript, + // No Delete here — see uninstallOnDestroy, and TriggersForGuardedChange's + // third precondition: this command is replaced on every deploy by its own + // nonce, and pulumi-command performs a replacement as create-then-delete, so + // a Delete on this resource would uninstall the control plane seconds after + // installing it. + // // Without Triggers the command does not re-run once it has succeeded, so a // changed version or SAN list would be reported green having done nothing. // The script text covers every input that changes what gets installed, @@ -132,7 +197,29 @@ func K3sCluster(ctx *sdk.Context, stack api.Stack, input api.ResourceInput, para return nil, errors.Wrapf(err, "failed to install k3s on host %q for cluster %q", host.Name, clusterName) } - readyScript := NodeReadyScript() + if err := uninstallOnDestroy(ctx, input, host, ServerUnit); err != nil { + return nil, err + } + + joined, err := joinNodes(ctx, input, cfg, params, joinArgs{ + clusterName: clusterName, + vmOut: vmOut, + roles: roles, + disable: disable, + sans: sans, + joinURL: joinURL, + version: version, + installURL: installURL, + firstServer: install, + firstScript: installScript, + }) + if err != nil { + return nil, err + } + + // Counts the WHOLE fleet, so the gate is not satisfied by host 0 alone. See + // NodeReadyScript. + readyScript := NodeReadyScript(len(vmOut.Hosts)) ready, err := remote.NewCommand(ctx, input.ToResName(input.Descriptor.Name+"-node-ready"), &remote.CommandArgs{ Connection: conn, Create: sdk.String(readyScript), @@ -145,28 +232,29 @@ func K3sCluster(ctx *sdk.Context, stack api.Stack, input api.ResourceInput, para Triggers: pulumiVm.TriggersForRead(installScript, sdk.String(readyScript)), Logging: remote.LoggingNone.ToLoggingPtrOutput(), AddPreviousOutputInEnv: sdk.Bool(false), - }, sdk.DependsOn([]sdk.Resource{install})) + }, sdk.DependsOn(append([]sdk.Resource{install}, joined...))) if err != nil { return nil, errors.Wrapf(err, "failed to create node readiness check for cluster %q", clusterName) } kubeconfigScript := endpoint.ApplyT(KubeconfigScript).(sdk.StringOutput) - kubeconfig, err := remote.NewCommand(ctx, input.ToResName(input.Descriptor.Name+"-kubeconfig"), &remote.CommandArgs{ - Connection: conn, - Create: kubeconfigScript, - Update: kubeconfigScript, - // A read, and the one whose staleness is worst: this command's stdout IS the - // exported kubeconfig. With an invariant trigger set it ran once, so every - // later deploy exported the credential captured at cluster creation — and a - // k3s whose certificates were rotated (the installer does it at expiry) or - // which was re-installed hands every downstream `kubernetes-*` stack a - // kubeconfig that no longer authenticates, from a deploy that reported - // success. Re-reading costs one SSH round-trip; an unchanged file yields an - // unchanged output, so nothing downstream churns for it. - Triggers: pulumiVm.TriggersForRead(installScript, kubeconfigScript), - Logging: remote.LoggingNone.ToLoggingPtrOutput(), - AddPreviousOutputInEnv: sdk.Bool(false), - }, + kubeconfig, err := remote.NewCommand( + ctx, input.ToResName(input.Descriptor.Name+"-kubeconfig"), &remote.CommandArgs{ + Connection: conn, + Create: kubeconfigScript, + Update: kubeconfigScript, + // A read, and the one whose staleness is worst: this command's stdout IS the + // exported kubeconfig. With an invariant trigger set it ran once, so every + // later deploy exported the credential captured at cluster creation — and a + // k3s whose certificates were rotated (the installer does it at expiry) or + // which was re-installed hands every downstream `kubernetes-*` stack a + // kubeconfig that no longer authenticates, from a deploy that reported + // success. Re-reading costs one SSH round-trip; an unchanged file yields an + // unchanged output, so nothing downstream churns for it. + Triggers: pulumiVm.TriggersForRead(installScript, kubeconfigScript), + Logging: remote.LoggingNone.ToLoggingPtrOutput(), + AddPreviousOutputInEnv: sdk.Bool(false), + }, // Ordered after readiness, not after the install: the kubeconfig is written // the moment the server process starts, so reading it earlier would export a // credential for a cluster that cannot yet schedule anything. This way the @@ -210,6 +298,22 @@ func K3sCluster(ctx *sdk.Context, stack api.Stack, input api.ResourceInput, para // deploy, a repo away from this line. ctx.Export(pulumiKubernetes.ToIngressIpExport(clusterName), host.Address) + // And the whole fleet, which is what the front door of a k3s cluster actually is. + // klipper-lb runs a svclb DaemonSet, so a `type: LoadBalancer` Service binds its + // ports on EVERY node and any node serves any workload — the export above names one + // of them because that is all a single A record can hold, and a child stack that + // knows about this output points the name at all of them instead (kube_run.go). + // + // Every host, not only the servers: an agent serves ingress exactly as well as a + // server does, and on the default 1-server topology restricting this to servers + // would collapse it back to one address. + // + // Exported unconditionally, including for a single-node cluster, where it holds the + // one address and the reader produces the one record it always did. That keeps the + // two paths identical rather than conditional — a cluster that gains a second node + // does not also change which code path creates its DNS. + ctx.Export(pulumiKubernetes.ToIngressIpsExport(clusterName), ingressAddresses(vmOut.Hosts)) + return &api.ResourceOutput{Ref: &K3sOutput{ Kubeconfig: kubeconfigValue, Endpoint: endpoint, @@ -217,6 +321,274 @@ func K3sCluster(ctx *sdk.Context, stack api.Stack, input api.ResourceInput, para }}, nil } +// ingressAddresses renders every node's address as the one string the ingress-ips +// export carries, in host order. +// +// A string and not an array because a stack output crosses into the child stack through +// JSON: a []string comes back as []any, and pApi.GetValueFromStack[[]string] would fail +// the deploy on a value that is sitting right there. The vm resource's open-ports export +// is a formatted string for the same reason (vm.FormatPortSpec). +// +// Host order is preserved rather than sorted. Nothing depends on it — a resolver reorders +// the answers anyway — but a stable order keeps the export, and therefore the child +// stack's diff, empty when nothing has changed. +func ingressAddresses(hosts []pulumiVm.HostFacts) sdk.StringOutput { + addresses := lo.Map(hosts, func(h pulumiVm.HostFacts, _ int) any { return h.Address }) + return sdk.All(addresses...).ApplyT(func(args []any) string { + return strings.Join(lo.Map(args, func(a any, _ int) string { return a.(string) }), ",") + }).(sdk.StringOutput) +} + +// rolesOf decides which host runs what, and is the one place that decision is made. +// +// Two sources, and which one wins is not a preference: +// +// - The vm resource's own `hosts[].role`, when it declares any. A byo pool is +// heterogeneous by definition — the operator knows which box has the disk, the +// memory and the stable address — and SC has no way to learn that. A `servers:` +// that disagrees with it is refused rather than silently losing, because either +// outcome of a silent resolution is a machine in a role its owner did not pick. +// - Otherwise `servers:`, assigned BY INDEX: hosts 0..servers-1 are the control +// plane, the rest are agents. Deterministic and index-stable, so raising the +// agent count never converts an existing agent into an etcd member — a change +// that is not undone by putting the number back. +// +// The vm driver resolves an undeclared role to vm.RoleCompose (VmHost.EffectiveRole), +// so "no k3s role anywhere" is the signal that nothing was declared. That also means +// a pool explicitly marked `role: compose` and then handed to a k3s cluster is read +// as undeclared and takes the index assignment; the config tier refuses mixing the +// two role families, so the only thing lost is the chance to object to an all-compose +// pool being used as a cluster, which is a stack that works. +func rolesOf(cfg *k3s.K3sClusterConfig, input api.ResourceInput, vmOut *pulumiVm.VmOutput) ([]vm.HostRole, error) { + declared := lo.Map(vmOut.Hosts, func(h pulumiVm.HostFacts, _ int) vm.HostRole { return h.Role }) + if lo.SomeBy(declared, func(r vm.HostRole) bool { return r.IsK3s() }) { + servers := lo.CountBy(declared, func(r vm.HostRole) bool { return r == vm.RoleK3sServer }) + if cfg.Servers != 0 && cfg.Servers != servers { + return nil, errors.Errorf( + "%s %q sets `servers: %d`, but vm resource %q declares %d host(s) with `role: %s`. Remove one of "+ + "the two: on a pool whose hosts name their own roles, the roles are the operator's choice and "+ + "this field can only contradict it", + k3s.ResourceTypeK3sCluster, input.Descriptor.Name, cfg.Servers, cfg.VmResource, servers, + vm.RoleK3sServer, + ) + } + if declared[0] != vm.RoleK3sServer { + // Not an arbitrary convention: SC installs on host 0 first, reads the join + // token there and reads the kubeconfig there, so an agent in that slot + // would have to be joined to a cluster that does not exist yet. + return nil, errors.Errorf( + "%s %q runs on vm resource %q whose first host declares `role: %s`; the FIRST host must be a %q — "+ + "it is the node SC installs first, takes the join token from and exports the kubeconfig of. "+ + "Reorder `hosts` so a server comes first", + k3s.ResourceTypeK3sCluster, input.Descriptor.Name, cfg.VmResource, declared[0], vm.RoleK3sServer, + ) + } + return declared, nil + } + + servers := cfg.EffectiveServers() + if servers > len(vmOut.Hosts) { + return nil, errors.Errorf( + "%s %q sets `servers: %d`, but vm resource %q has %d host(s): every server needs a machine of its "+ + "own. Either add hosts to %q or lower `servers`", + k3s.ResourceTypeK3sCluster, input.Descriptor.Name, servers, cfg.VmResource, len(vmOut.Hosts), + cfg.VmResource, + ) + } + return lo.Map(vmOut.Hosts, func(_ pulumiVm.HostFacts, i int) vm.HostRole { + return lo.If(i < servers, vm.RoleK3sServer).Else(vm.RoleK3sAgent) + }), nil +} + +// joinArgs is what every node after the first needs in order to join. A struct +// because it is nine values that are all computed once for the cluster, and a +// nine-parameter function is a call site nobody can read. +type joinArgs struct { + clusterName string + vmOut *pulumiVm.VmOutput + roles []vm.HostRole + disable []string + sans sdk.StringArrayOutput + joinURL sdk.StringOutput + version string + installURL string + // firstServer and firstScript are host 0's install command and the script it + // ran: the dependency every join is ordered after, and the trigger input that + // makes a join re-run when the control plane itself was reinstalled. + firstServer *remote.Command + firstScript sdk.StringOutput +} + +// joinNodes installs k3s on every host but the first, and returns the commands so the +// readiness gate can wait on all of them. +// +// Nothing happens here for a single-host cluster — not even the token read. That keeps +// a single-node stack's resource set exactly what it was before this slice, which is +// what makes the change additive for one, and saves an SSH round-trip per deploy that +// could not be used for anything. +func joinNodes( + ctx *sdk.Context, input api.ResourceInput, cfg *k3s.K3sClusterConfig, params pApi.ProvisionParams, a joinArgs, +) ([]sdk.Resource, error) { + if len(a.vmOut.Hosts) < 2 { + return nil, nil + } + + token, err := readJoinToken(ctx, input, a) + if err != nil { + return nil, err + } + + created := make([]sdk.Resource, 0, len(a.vmOut.Hosts)-1) + // Servers are chained — server i waits for server i-1 — rather than installed in + // parallel. Each one adds itself as an etcd member, and etcd serialises member + // changes anyway: two joins racing means one of them gets a rejected proposal and + // fails its provision, on a cluster that is otherwise fine. + previous := sdk.Resource(a.firstServer) + for i, role := range a.roles { + if i == 0 || role != vm.RoleK3sServer { + continue + } + cmd, err := installNode(ctx, input, cfg, a, i, token, []sdk.Resource{a.vmOut.Ready[i], previous}) + if err != nil { + return nil, err + } + previous = cmd + created = append(created, cmd) + } + // Agents, in parallel with each other: an agent registers with the API server and + // touches no datastore state that another agent could conflict with. They wait on + // the LAST server, so an agent never registers against a control plane that is + // still gaining members. + lastServer := previous + for i, role := range a.roles { + if i == 0 || role != vm.RoleK3sAgent { + continue + } + cmd, err := installNode(ctx, input, cfg, a, i, token, []sdk.Resource{a.vmOut.Ready[i], lastServer}) + if err != nil { + return nil, err + } + created = append(created, cmd) + } + + params.Log.Info(ctx.Context(), "cluster %q joins %d host(s) to the control plane on host %q", + a.clusterName, len(created), a.vmOut.Hosts[0].Name) + return created, nil +} + +// readJoinToken reads the cluster's join secret off host 0, once the server is up. +func readJoinToken(ctx *sdk.Context, input api.ResourceInput, a joinArgs) (sdk.StringOutput, error) { + script := TokenReadScript() + cmd, err := remote.NewCommand( + ctx, input.ToResName(input.Descriptor.Name+"-join-token"), &remote.CommandArgs{ + Connection: pulumiVm.ConnectionArgs(a.vmOut.Hosts[0]), + Create: sdk.String(script), + Update: sdk.String(script), + // A read, so it takes the per-run nonce. Without it the token is answered out + // of the checkpoint forever, and a control plane that was reinstalled — which + // mints a new one — would have every join authenticate with the old value. + Triggers: pulumiVm.TriggersForRead(a.firstScript, sdk.String(script)), + Logging: remote.LoggingNone.ToLoggingPtrOutput(), + AddPreviousOutputInEnv: sdk.Bool(false), + }, + sdk.DependsOn([]sdk.Resource{a.firstServer}), + // This stdout admits a machine to the control plane, and pulumi-command does + // not mark stdout secret. Same two protections as the kubeconfig read below, + // for the same reason — see the comment there, and + // forge docs/roadmap/items/yc-provider-key-in-deploy-log.md. + sdk.AdditionalSecretOutputs([]string{"stdout"}), + ) + if err != nil { + return sdk.StringOutput{}, errors.Wrapf(err, + "failed to read the k3s join token from host %q for cluster %q", a.vmOut.Hosts[0].Name, a.clusterName) + } + // Marked again at the value level: AdditionalSecretOutputs covers the property in + // the checkpoint, this covers everything derived from it — including the install + // scripts of every other node, which embed it. + return sdk.ToSecret(cmd.Stdout).(sdk.StringOutput), nil +} + +// installNode installs k3s on host i, as the role rolesOf gave it. +func installNode( + ctx *sdk.Context, input api.ResourceInput, cfg *k3s.K3sClusterConfig, a joinArgs, + i int, token sdk.StringOutput, deps []sdk.Resource, +) (*remote.Command, error) { + host := a.vmOut.Hosts[i] + role := a.roles[i] + unit := UnitFor(role) + + // The argument vector is rendered WITHOUT the token (see ServerArgs) and is + // therefore safe to use as a trigger: the token is in the script, and a trigger + // set holding it would put a cluster credential in every preview diff. + // Every node reached here is part of a multi-node cluster by construction — a + // single-node cluster has no node to join — so `--node-ip` is always set, to + // THIS host's private address rather than host 0's. + nodeArgs := sdk.All(a.sans, a.joinURL, host.EffectivePrivateAddress()).ApplyT(func(args []any) string { + sans := args[0].([]string) + join := JoinSpec{ServerURL: args[1].(string), NodeIP: args[2].(string)} + if role == vm.RoleK3sAgent { + return AgentArgs(join) + } + return ServerArgs(cfg, sans, a.disable, join) + }).(sdk.StringOutput) + + script := sdk.All(nodeArgs, token).ApplyT(func(args []any) string { + return InstallScript(a.version, a.installURL, args[0].(string), unit, strings.TrimSpace(args[1].(string))) + }).(sdk.StringOutput) + + cmd, err := remote.NewCommand(ctx, input.ToResName(fmt.Sprintf("%s-install-%d", input.Descriptor.Name, i)), + &remote.CommandArgs{ + Connection: pulumiVm.ConnectionArgs(host), + Create: script, + Update: script, + // Guarded (the install's own args-marker check) and nonce-triggered for + // the same reason host 0's is: a node whose k3s was stopped out of band + // has drifted with no input here moving, and the readiness gate would + // then fail a provision that had no way to repair it. Deliberately NOT + // triggered on `script`, which holds the token — `nodeArgs` plus the + // control plane's own script covers every input that changes what gets + // installed, and a token rotation already reinstalls the control plane + // this depends on. + Triggers: pulumiVm.TriggersForGuardedChange(nodeArgs, a.firstScript, host.Address), + Logging: remote.LoggingNone.ToLoggingPtrOutput(), + AddPreviousOutputInEnv: sdk.Bool(false), + }, sdk.DependsOn(deps)) + if err != nil { + return nil, errors.Wrapf(err, "failed to join host %q as %s to cluster %q", host.Name, role, a.clusterName) + } + if err := uninstallOnDestroy(ctx, input, host, unit); err != nil { + return nil, err + } + return cmd, nil +} + +// uninstallOnDestroy registers the one thing that removes k3s from a machine when the +// cluster resource goes away. +// +// Its own resource with NO triggers, which is not a style choice: the install commands +// are replaced on every deploy by their per-run nonce, and pulumi-command performs a +// replacement as create-then-delete — so an uninstall living on an install would run +// moments after it, tearing down what it had just built. That exact fault was live on +// the vm ingress until 2026-10-18 (see TriggersForGuardedChange's third precondition +// and pkg/clouds/pulumi/vm/caddy.go's teardown). A resource with no triggers is never +// replaced, so this Delete runs on destroy and at no other time. +// +// Without it a destroyed cluster leaves k3s running on every machine: holding ports +// 80/443 against whatever is deployed there next, and — for the agents — retrying +// forever against a control plane that no longer answers. +func uninstallOnDestroy(ctx *sdk.Context, input api.ResourceInput, host pulumiVm.HostFacts, unit string) error { + if _, err := remote.NewCommand(ctx, input.ToResName(host.Name+"-k3s-uninstall"), &remote.CommandArgs{ + Connection: pulumiVm.ConnectionArgs(host), + Create: sdk.String("true"), + Delete: sdk.String(UninstallScript(unit)), + Logging: remote.LoggingNone.ToLoggingPtrOutput(), + AddPreviousOutputInEnv: sdk.Bool(false), + }); err != nil { + return errors.Wrapf(err, "failed to register the k3s uninstall of host %q", host.Name) + } + return nil +} + // componentServiceLB is k3s's bundled load-balancer controller (klipper-lb). It binds // the node's ports 80 and 443 for every `type: LoadBalancer` Service, which is what // makes a single-node cluster reachable without a cloud load balancer — and what @@ -245,6 +617,43 @@ func disableSet(cfg *k3s.K3sClusterConfig, vmOut *pulumiVm.VmOutput) []string { return disable } +// validateIngressPorts refuses a cluster whose k3s would fight the machine's own +// Caddy for ports 80 and 443. +// +// The default `disable` set already avoids this (disableSet drops traefik always and +// servicelb whenever the vm resource has an ingress), so the only way to reach this +// error is an explicit `disable:` that puts one of them back on an ingressed host — +// which an operator does on purpose, usually without knowing the machine already has +// something on :80. +// +// A refusal rather than a warning, and that is the point of roadmap item +// sc-vm-k3s-port80-race-not-validated: the two components bind the same port and +// race, so the symptom is whichever lost reporting a bind failure in a log nobody is +// reading, or — worse — traffic reaching the wrong backend from a provision that +// exited 0. On one host that is a confusing afternoon; on a fleet it is a confusing +// afternoon that only reproduces on some of the nodes. +func validateIngressPorts( + cfg *k3s.K3sClusterConfig, input api.ResourceInput, vmOut *pulumiVm.VmOutput, disable []string, +) error { + if !vmOut.IngressEnabled { + return nil + } + conflicting := lo.Filter([]string{componentTraefik, componentServiceLB}, func(c string, _ int) bool { + return !lo.Contains(disable, c) + }) + if len(conflicting) == 0 { + return nil + } + return errors.Errorf( + "%s %q runs on vm resource %q, whose Caddy ingress owns ports 80 and 443 on every host — but this "+ + "cluster leaves %s enabled, which binds the same ports. They do not share: whichever starts "+ + "second fails to bind, and SC cannot tell you which that was. Either add %s to the cluster's "+ + "`disable` list, or set `ingress.enable: false` on %q and let k3s own the edge", + k3s.ResourceTypeK3sCluster, input.Descriptor.Name, cfg.VmResource, strings.Join(conflicting, " and "), + strings.Join(conflicting, " and "), cfg.VmResource, + ) +} + // hostsOf resolves the vm resource this cluster runs on out of the outputs of the // resources provisioned before it. // @@ -261,29 +670,22 @@ func hostsOf(cfg *k3s.K3sClusterConfig, input api.ResourceInput, stack api.Stack "`type: vm` resource in the SAME parent stack and environment — a cluster cannot depend on a "+ "machine described somewhere else", k3s.ResourceTypeK3sCluster, input.Descriptor.Name, cfg.VmResource, stack.Name, - input.StackParams.Environment) + input.StackParams.Environment, + ) } vmOut, ok := out.Ref.(*pulumiVm.VmOutput) if !ok { return nil, errors.Errorf( "%s %q references `vmResource: %s`, which is not a `type: %s` resource (its output is %T)", - k3s.ResourceTypeK3sCluster, input.Descriptor.Name, cfg.VmResource, "vm", out.Ref) + k3s.ResourceTypeK3sCluster, input.Descriptor.Name, cfg.VmResource, "vm", out.Ref, + ) } if len(vmOut.Hosts) == 0 || len(vmOut.Ready) != len(vmOut.Hosts) { return nil, errors.Errorf("vm resource %q produced no usable hosts for %s %q", cfg.VmResource, k3s.ResourceTypeK3sCluster, input.Descriptor.Name) } - // Single-node only, and refused rather than silently partial: a multi-host pool - // needs the server/agent split — a join token read off the first node, delivered - // to the others over stdin, and `--server` on each agent. That is the next slice - // of roadmap item sc-k3s-cluster-resource, and installing a second control plane - // on every host in the meantime would be a worse outcome than this error. - if len(vmOut.Hosts) > 1 { - return nil, errors.Errorf( - "%s %q runs on vm resource %q, which has %d hosts: a single-node cluster is all this slice "+ - "supports. Multi-node (server + agents, join token) is the next slice of roadmap item "+ - "sc-k3s-cluster-resource", - k3s.ResourceTypeK3sCluster, input.Descriptor.Name, cfg.VmResource, len(vmOut.Hosts)) - } + // Any host count from here. Which machine runs what is rolesOf's decision, not + // this function's — it only establishes that the pool exists and that every host + // has a readiness resource to be ordered after. return vmOut, nil } diff --git a/pkg/clouds/pulumi/k3s/cluster_test.go b/pkg/clouds/pulumi/k3s/cluster_test.go index 2b2af7327..5402f47ff 100644 --- a/pkg/clouds/pulumi/k3s/cluster_test.go +++ b/pkg/clouds/pulumi/k3s/cluster_test.go @@ -4,12 +4,19 @@ package k3s import ( + "fmt" "testing" . "github.com/onsi/gomega" + "github.com/samber/lo" + sdk "github.com/pulumi/pulumi/sdk/v3/go/pulumi" + "github.com/pulumi/pulumi/sdk/v3/go/pulumi/internals" + + "github.com/simple-container-com/api/pkg/api" cfgK3s "github.com/simple-container-com/api/pkg/clouds/k3s" pulumiVm "github.com/simple-container-com/api/pkg/clouds/pulumi/vm" + "github.com/simple-container-com/api/pkg/clouds/vm" "github.com/simple-container-com/api/pkg/provisioner/placeholders" ) @@ -71,3 +78,209 @@ func TestDisableSetSurvivesPlaceholderWalk(t *testing.T) { Expect(cfg.Disable).To(BeNil(), "an unset `disable` must stay nil through the placeholder walk") Expect(disableSet(&cfg, &pulumiVm.VmOutput{})).To(Equal([]string{"traefik"})) } + +// rolesOf is the one place the server/agent split is decided, and the table below is +// the whole decision. Two sources, and which wins is not a preference: a byo pool +// that names its own roles has an operator who knows which box has the disk, the +// memory and the stable address, and SC cannot learn that. +func TestRolesOf(t *testing.T) { + pool := func(roles ...vm.HostRole) *pulumiVm.VmOutput { + return &pulumiVm.VmOutput{ + Hosts: lo.Map(roles, func(r vm.HostRole, i int) pulumiVm.HostFacts { + return pulumiVm.HostFacts{Name: fmt.Sprintf("host-%d", i), Role: r} + }), + } + } + input := api.ResourceInput{ + Descriptor: &api.ResourceDescriptor{Name: "cluster"}, + StackParams: &api.StackParams{Environment: "staging"}, + } + + for _, tc := range []struct { + name string + servers int + out *pulumiVm.VmOutput + want []vm.HostRole + wantErr string + }{ + { + // The default, and the topology worth reaching for first: it schedules + // pods across the whole fleet and survives the loss of any agent. + name: "undeclared pool defaults to one server and the rest agents", + out: pool(vm.RoleCompose, vm.RoleCompose, vm.RoleCompose), + want: []vm.HostRole{vm.RoleK3sServer, vm.RoleK3sAgent, vm.RoleK3sAgent}, + }, + { + name: "a single undeclared host is the lone server", + out: pool(vm.RoleCompose), + want: []vm.HostRole{vm.RoleK3sServer}, + }, + { + // By INDEX, and that is load-bearing: raising the agent count must never + // convert an existing agent into an etcd member, which is not undone by + // putting the number back. + name: "servers 3 takes the first three hosts", + servers: 3, + out: pool(vm.RoleCompose, vm.RoleCompose, vm.RoleCompose, vm.RoleCompose), + want: []vm.HostRole{ + vm.RoleK3sServer, vm.RoleK3sServer, vm.RoleK3sServer, vm.RoleK3sAgent, + }, + }, + { + name: "more servers than hosts is refused", + servers: 3, + out: pool(vm.RoleCompose, vm.RoleCompose), + wantErr: "every server needs a machine of its own", + }, + { + name: "a declared pool is taken as written", + out: pool(vm.RoleK3sServer, vm.RoleK3sAgent, vm.RoleK3sAgent), + want: []vm.HostRole{vm.RoleK3sServer, vm.RoleK3sAgent, vm.RoleK3sAgent}, + }, + { + name: "a declared three-server pool needs no servers field", + out: pool(vm.RoleK3sServer, vm.RoleK3sServer, vm.RoleK3sServer), + want: []vm.HostRole{vm.RoleK3sServer, vm.RoleK3sServer, vm.RoleK3sServer}, + }, + { + name: "a servers field agreeing with the declaration is allowed", + servers: 1, + out: pool(vm.RoleK3sServer, vm.RoleK3sAgent), + want: []vm.HostRole{vm.RoleK3sServer, vm.RoleK3sAgent}, + }, + { + // Refused rather than silently resolved, because either outcome of a + // silent resolution puts a machine in a role its owner did not pick. + name: "a servers field contradicting the declaration is refused", + servers: 3, + out: pool(vm.RoleK3sServer, vm.RoleK3sAgent, vm.RoleK3sAgent), + wantErr: "can only contradict it", + }, + { + // Not an arbitrary convention: SC installs on host 0 first, reads the + // join token there and exports the kubeconfig from there, so an agent in + // that slot would have to join a cluster that does not exist yet. + name: "an agent in the first slot is refused", + out: pool(vm.RoleK3sAgent, vm.RoleK3sServer), + wantErr: "the FIRST host must be a", + }, + } { + t.Run(tc.name, func(t *testing.T) { + RegisterTestingT(t) + + roles, err := rolesOf( + &cfgK3s.K3sClusterConfig{VmResource: "vm", Servers: tc.servers}, input, tc.out, + ) + if tc.wantErr != "" { + Expect(err).NotTo(BeNil()) + Expect(err.Error()).To(ContainSubstring(tc.wantErr)) + return + } + Expect(err).To(BeNil()) + Expect(roles).To(Equal(tc.want)) + }) + } +} + +// Roadmap item sc-vm-k3s-port80-race-not-validated: the vm's Caddy ingress and k3s's +// own ingress/LB controllers both want :80, and until this refusal SC provisioned the +// combination and let them race. The default disable set already avoids it, so the +// only way here is an explicit `disable:` that puts one of them back. +func TestValidateIngressPortsRefusesTheRace(t *testing.T) { + input := api.ResourceInput{ + Descriptor: &api.ResourceDescriptor{Name: "cluster"}, + StackParams: &api.StackParams{Environment: "staging"}, + } + + for _, tc := range []struct { + name string + ingress bool + disable []string + wantErr string + }{ + { + name: "no vm ingress: the cluster owns the edge and nothing conflicts", + disable: nil, + }, + { + name: "the default disable set on an ingressed host is fine", + ingress: true, + disable: []string{componentTraefik, componentServiceLB}, + }, + { + // `disable: []` is the documented escape hatch for an operator who wants + // traefik after all — and on an ingressed host it is the exact fault. + name: "an empty explicit disable on an ingressed host is refused", + ingress: true, + disable: []string{}, + wantErr: "traefik and servicelb", + }, + { + name: "re-enabling only servicelb is refused and names only it", + ingress: true, + disable: []string{componentTraefik}, + wantErr: "leaves servicelb enabled", + }, + { + name: "re-enabling only traefik is refused and names only it", + ingress: true, + disable: []string{componentServiceLB}, + wantErr: "leaves traefik enabled", + }, + } { + t.Run(tc.name, func(t *testing.T) { + RegisterTestingT(t) + + err := validateIngressPorts( + &cfgK3s.K3sClusterConfig{VmResource: "vm"}, input, + &pulumiVm.VmOutput{IngressEnabled: tc.ingress}, tc.disable, + ) + if tc.wantErr == "" { + Expect(err).To(BeNil()) + return + } + Expect(err).NotTo(BeNil()) + Expect(err.Error()).To(ContainSubstring(tc.wantErr)) + // The remedy has to be in the message: an operator who set `disable` + // explicitly did so for a reason, and "pick one of these two" is the + // only actionable thing to say. + Expect(err.Error()).To(ContainSubstring("`ingress.enable: false`")) + }) + } +} + +// The front door of a k3s cluster is every node, and this is the value that says so. +// +// Asserted on the rendered string rather than through a provision, because the single +// thing that can go wrong here is silent: a list the child stack cannot split back +// apart ends up in an A record's value, and a record whose value is not an address is +// still a successful deploy. +func TestIngressAddressesJoinsEveryNodeInOrder(t *testing.T) { + RegisterTestingT(t) + + for name, tc := range map[string]struct { + addresses []string + want string + }{ + // The default topology, and the one that must keep behaving exactly as it did + // before this export existed: one node, one address, one record. + "single node": {[]string{"203.0.113.10"}, "203.0.113.10"}, + "three nodes": {[]string{"203.0.113.10", "203.0.113.11", "203.0.113.12"}, "203.0.113.10,203.0.113.11,203.0.113.12"}, + } { + t.Run(name, func(t *testing.T) { + RegisterTestingT(t) + + hosts := lo.Map(tc.addresses, func(a string, _ int) pulumiVm.HostFacts { + return pulumiVm.HostFacts{Address: sdk.String(a).ToStringOutput()} + }) + Expect(sdk.RunErr(func(ctx *sdk.Context) error { + res, err := internals.UnsafeAwaitOutput(ctx.Context(), ingressAddresses(hosts)) + if err != nil { + return err + } + Expect(res.Value).To(Equal(tc.want)) + return nil + }, sdk.WithMocks("project", "stack", &clusterMocks{}))).To(BeNil()) + }) + } +} diff --git a/pkg/clouds/pulumi/k3s/scripts.go b/pkg/clouds/pulumi/k3s/scripts.go index eec838e92..c77ca38ad 100644 --- a/pkg/clouds/pulumi/k3s/scripts.go +++ b/pkg/clouds/pulumi/k3s/scripts.go @@ -7,22 +7,29 @@ import ( "fmt" "strings" + "github.com/samber/lo" + "github.com/simple-container-com/api/pkg/clouds/k3s" "github.com/simple-container-com/api/pkg/clouds/vm" ) const ( - // ArgsMarkerPath records the server arguments SC installed with, on the machine. + // ServerUnit and AgentUnit are the systemd units the installer writes. Which one + // it writes is decided by the FIRST positional argument it is given: `agent` + // makes it `k3s-agent`, anything else `k3s`. That is also why an agent must not + // be handed the server's flags — see AgentArgs. + ServerUnit = "k3s" + AgentUnit = "k3s-agent" + + // ServerTokenPath is the join secret k3s writes on a server once it is up. Every + // other node — agent or joining server — authenticates to the cluster with it. // - // It exists because the version alone is not enough to decide whether an install - // can be skipped. The remote half of this path has to be idempotent — a - // re-provision must not restart a working API server — but a guard that only - // compares versions would also skip an install whose `--tls-san` or `--disable` - // set had changed, and the operator would be looking at a cluster that does not - // match its own config with every layer reporting success. Comparing the full - // argument string makes "nothing to do" and "reconfigure" distinguishable - // remotely, which is the only place the question can be answered. - ArgsMarkerPath = vm.StateDir + "/k3s-server.args" + // Read off the first server rather than generated by SC and passed in. Both work + // for a cluster SC creates; only this one works for the cluster SC finds, which + // is the normal case on byo hosts and after any out-of-band repair: a machine + // that already has a k3s datastore keeps its own token, and a token SC invented + // would be rejected by it with an authentication error on every join. + ServerTokenPath = "/var/lib/rancher/k3s/server/node-token" // InstallerPath is where the install script is downloaded before it is run. // Downloaded and then executed, rather than piped into a shell: a truncated @@ -45,11 +52,89 @@ const ( nodeReadyInterval = 5 ) -// ServerArgs is the full argument vector SC installs the k3s server with, starting +// ArgsMarkerPath records the arguments SC installed a unit with, on the machine. +// +// It exists because the version alone is not enough to decide whether an install +// can be skipped. The remote half of this path has to be idempotent — a +// re-provision must not restart a working API server — but a guard that only +// compares versions would also skip an install whose `--tls-san` or `--disable` +// set had changed, and the operator would be looking at a cluster that does not +// match its own config with every layer reporting success. Comparing the full +// argument string makes "nothing to do" and "reconfigure" distinguishable +// remotely, which is the only place the question can be answered. +// +// Per unit, so a machine's marker says which thing it is: a host that was an agent +// and is now a server has a `k3s.args` to write and a stale `k3s-agent.args` that +// says nothing about it, rather than one file whose meaning changed underneath the +// comparison. +func ArgsMarkerPath(unit string) string { + return vm.StateDir + "/" + unit + ".args" +} + +// JoinSpec is how one node attaches to the cluster. +// +// The three shapes are the whole topology story, and they are mutually exclusive: +// the lone server of a single-server cluster (zero value), the first server of an +// embedded-etcd one (ClusterInit), and everything that joins an existing cluster +// (ServerURL — every agent, and every server after the first). +type JoinSpec struct { + // ServerURL is the API server a node registers with: `https://:6443`. + // Empty on the first server, which has nothing to register with. + ServerURL string + + // ClusterInit asks k3s for an embedded-etcd datastore instead of sqlite, and is + // set on the FIRST server of a multi-server cluster only. + // + // Not a function of "is this host 0": a single-server cluster deliberately does + // not get it, because etcd on one node is a strictly worse sqlite — same single + // point of failure, plus a quorum to lose. Raising `servers` from 1 later changes + // this flag and so the args marker, which re-runs the install; k3s does migrate + // sqlite to etcd on that restart, but it is a datastore conversion on a live + // control plane and not something to do on a whim. + ClusterInit bool + + // NodeIP is the address this node advertises to the rest of the cluster, and + // becomes `--node-ip`. Empty on a single-node cluster, where there is no rest of + // the cluster and k3s's own default is harmless. + // + // Set on every node of a multi-node cluster, because k3s's default is to use the + // address of the default-route interface. On a cloud VM reached through a + // floating/NAT IP that is already the internal address; on a single-NIC VPS it is + // the PUBLIC one, and then etcd peer traffic and flannel's unencrypted VXLAN + // overlay cross the public internet. Config-tier validation refuses a multi-node + // pool whose private addresses are unknown (vm.VmHost.PrivateAddress), so by the + // time this is empty on a multi-node cluster something upstream is wrong. + // + // It belongs in the args vector — and therefore in the args marker — on purpose: + // a node whose advertised address changed must be reinstalled, not left + // advertising a stale one. + NodeIP string +} + +// ServerArgs is the full argument vector SC installs a k3s server with, starting // with the `server` subcommand. Rendered as one string because that is both what the // installer takes and what ArgsMarkerPath compares. -func ServerArgs(cfg *k3s.K3sClusterConfig, sans []string, disable []string) string { +// +// The join token is NOT here, deliberately: it would land in the marker file on the +// machine and in the Pulumi checkpoint as part of this string, and it would make the +// marker compare unequal on a cluster whose token was rotated — reinstalling a +// healthy control plane for a value the comparison has no business knowing. It +// travels as K3S_TOKEN instead (InstallScript). +func ServerArgs(cfg *k3s.K3sClusterConfig, sans []string, disable []string, join JoinSpec) string { args := []string{"server"} + if join.ClusterInit { + args = append(args, "--cluster-init") + } + if join.ServerURL != "" { + args = append(args, "--server", join.ServerURL) + } + if join.NodeIP != "" { + // Both flags, and both to the same value. `--node-ip` is what the node + // advertises and what flannel and the kubelet bind to; without + // `--advertise-address` the API server would still advertise its + // default-route address to peers, which is the value this exists to avoid. + args = append(args, "--node-ip", join.NodeIP, "--advertise-address", join.NodeIP) + } for _, san := range sans { args = append(args, "--tls-san", san) } @@ -60,6 +145,29 @@ func ServerArgs(cfg *k3s.K3sClusterConfig, sans []string, disable []string) stri return strings.Join(args, " ") } +// AgentArgs is the argument vector for a worker node. +// +// Short by design, and short in a load-bearing way: `--tls-san`, `--disable` and +// `extraServerArgs` are server-only flags. k3s's agent does not merely ignore them — +// it exits with an unknown-flag error, and the installer that forwarded them reports +// a service that will not start. +// +// `--node-ip` is NOT in that category: the agent accepts it, and an agent is where +// it matters most, since the kubelet and flannel both bind to the advertised +// address. `--advertise-address` is server-only and so is absent here. +func AgentArgs(join JoinSpec) string { + args := []string{"agent", "--server", join.ServerURL} + if join.NodeIP != "" { + args = append(args, "--node-ip", join.NodeIP) + } + return strings.Join(args, " ") +} + +// UnitFor is the systemd unit the installer will write for a role. +func UnitFor(role vm.HostRole) string { + return lo.If(role == vm.RoleK3sAgent, AgentUnit).Else(ServerUnit) +} + // InstallScript is the remote half of standing up the control plane. // // Everything it does is conditional on the machine not already being in the wanted @@ -68,18 +176,28 @@ func ServerArgs(cfg *k3s.K3sClusterConfig, sans []string, disable []string) stri // are needed — without Triggers a changed script never runs at all, and without the // marker a re-run with an unchanged config would restart the API server of a live // cluster for nothing. -func InstallScript(version, installURL, serverArgs string) string { +func InstallScript(version, installURL, nodeArgs, unit, token string) string { + marker := ArgsMarkerPath(unit) + // The token rides the environment, never the argument vector (see ServerArgs). + // The installer writes it 0600 into the unit's env file; `sudo env` is used for + // the same reason INSTALL_K3S_VERSION is — it is the one form that survives + // sudo's env_reset. + tokenEnv := "" + if token != "" { + tokenEnv = fmt.Sprintf(` K3S_TOKEN=%q`, token) + } return strings.Join([]string{ `set -eu`, fmt.Sprintf(`WANT_VERSION=%q`, version), - fmt.Sprintf(`WANT_ARGS=%q`, serverArgs), + fmt.Sprintf(`WANT_ARGS=%q`, nodeArgs), + fmt.Sprintf(`UNIT=%q`, unit), // The skip condition, spelled out: the service is running, the binary is the // wanted release, and the arguments on disk are the ones this config asks // for. Anything else falls through to a (re)install. - fmt.Sprintf(`if sudo systemctl is-active --quiet k3s && `+ + fmt.Sprintf(`if sudo systemctl is-active --quiet "$UNIT" && `+ `sudo k3s --version 2>/dev/null | grep -qF "$WANT_VERSION" && `+ - `[ -f %[1]s ] && [ "$(sudo cat %[1]s)" = "$WANT_ARGS" ]; then`, ArgsMarkerPath), - ` echo "k3s $WANT_VERSION already running with the requested configuration"`, + `[ -f %[1]s ] && [ "$(sudo cat %[1]s)" = "$WANT_ARGS" ]; then`, marker), + ` echo "k3s $WANT_VERSION already running as $UNIT with the requested configuration"`, ` exit 0`, `fi`, fmt.Sprintf(`curl -fsSL %q -o %s`, installURL, InstallerPath), @@ -94,7 +212,7 @@ func InstallScript(version, installURL, serverArgs string) string { // is anything else); `extraServerArgs` is word-split on spaces, which is the // documented behaviour of a pass-through field and the reason each entry // should be one flag. - fmt.Sprintf(`sudo env INSTALL_K3S_VERSION="$WANT_VERSION" sh %s $WANT_ARGS`, InstallerPath), + fmt.Sprintf(`sudo env INSTALL_K3S_VERSION="$WANT_VERSION"%s sh %s $WANT_ARGS`, tokenEnv, InstallerPath), fmt.Sprintf(`rm -f %s`, InstallerPath), // The installer does NOT guarantee a running service, and this is the line // that cost a provision: get.k3s.io diffs the unit and env files it would @@ -119,11 +237,64 @@ func InstallScript(version, installURL, serverArgs string) string { // per-run trigger worked), printed the line above, wrote the marker, exited // 0 — and k3s stayed down, with nothing in its journal between the stop two // hours earlier and the failed gate. - `sudo systemctl enable --now k3s`, + `sudo systemctl enable --now "$UNIT"`, // Written only after a successful install, so a failed one does not leave a // marker claiming the machine is configured. - fmt.Sprintf(`printf '%%s' "$WANT_ARGS" | sudo tee %s >/dev/null`, ArgsMarkerPath), - `echo "installed k3s $WANT_VERSION"`, + fmt.Sprintf(`printf '%%s' "$WANT_ARGS" | sudo tee %s >/dev/null`, marker), + `echo "installed k3s $WANT_VERSION as $UNIT"`, + }, "\n") +} + +// UninstallScript removes k3s from a machine, and is what a `remote.Command`'s +// Delete runs when the resource goes away. +// +// Without it a destroyed cluster leaves k3s running on every machine. On one host +// that is a nuisance; on a fleet it is N nodes still serving, still holding ports 80 +// and 443 against a compose stack that may be deployed there next, and — for the +// agents — still trying to register with a control plane that no longer answers. +// +// The installer writes the uninstaller, so its presence is the only honest test of +// whether k3s was ever installed here: `|| exit 0` rather than an error, because a +// destroy must succeed against a machine that failed its install (the common case — +// the resource being destroyed is often the one that did not come up). +func UninstallScript(unit string) string { + path := lo.If(unit == AgentUnit, "/usr/local/bin/k3s-agent-uninstall.sh"). + Else("/usr/local/bin/k3s-uninstall.sh") + return strings.Join([]string{ + `set -eu`, + fmt.Sprintf(`[ -x %[1]s ] || { echo "%[1]s is not present: nothing to uninstall"; exit 0; }`, path), + fmt.Sprintf(`sudo %s`, path), + // The marker is in SC's own state dir, which the uninstaller knows nothing + // about. Left behind it would make the next install's skip condition compare + // against arguments belonging to a cluster that no longer exists. + fmt.Sprintf(`sudo rm -f %s`, ArgsMarkerPath(unit)), + fmt.Sprintf(`echo "uninstalled %s"`, unit), + }, "\n") +} + +// TokenReadScript prints the cluster's join secret, read off a server that is up. +// +// Its stdout is a credential that admits a machine to the control plane, so the +// command built from it carries the same two protections as the kubeconfig read +// (Logging: LoggingNone AND AdditionalSecretOutputs on stdout) — pulumi-command does +// not mark stdout secret, which is how a Yandex service-account key reached a +// world-readable Actions log on 2026-09-30. +// +// The emptiness check is not defensive padding: the file is written asynchronously +// after the server process starts, so a read that wins the race returns success and +// nothing, and the joins built from it then fail one by one as authentication errors +// against a cluster that is perfectly healthy. +func TokenReadScript() string { + return strings.Join([]string{ + `set -eu`, + fmt.Sprintf(`test -f %[1]s || { echo "%[1]s does not exist: this host is not a k3s server" >&2; exit 1; }`, + ServerTokenPath), + fmt.Sprintf(`TOKEN=$(sudo cat %s)`, ServerTokenPath), + `[ -n "$TOKEN" ] || { echo "the k3s join token file is empty" >&2; exit 1; }`, + // No trailing newline: this value is interpolated into another node's install + // command, and a stray newline there is a token mismatch with no message that + // says so. + `printf '%s' "$TOKEN"`, }, "\n") } @@ -155,7 +326,7 @@ func KubeconfigScript(endpoint string) string { }, "\n") } -// NodeReadyScript waits until the cluster's own API says every node is Ready. +// NodeReadyScript waits until the cluster's own API says `want` nodes are Ready. // // Why this exists next to the install command: `systemctl is-active k3s` is true the // moment the process starts, seconds before the API server serves and well before the @@ -163,23 +334,34 @@ func KubeconfigScript(endpoint string) string { // `kubectl apply` against a cluster with no node, which fails as an unschedulable pod // rather than as a readiness problem — the same class of fault as a deploy that waits // for SSH and then finds no docker (vm-matrix.md §3.4). -func NodeReadyScript() string { +// +// The count is what makes it a FLEET gate rather than a node gate. Without it the +// script passes the instant the first server registers — which on a multi-node +// cluster is before any agent has joined, so a deploy ordered after this gate +// schedules its pods onto a cluster of one machine and the fleet it asked for arrives +// minutes later, if at all. "No node is NotReady" does not catch it either: a node +// that has not registered yet is not NotReady, it is absent. +func NodeReadyScript(want int) string { return strings.Join([]string{ `set -eu`, + fmt.Sprintf(`WANT=%d`, want), fmt.Sprintf(`for i in $(seq 1 %d); do`, nodeReadyAttempts), ` STATUS=$(sudo k3s kubectl get nodes --no-headers 2>/dev/null | awk '{print $2}' || true)`, - // NotReady is excluded explicitly because it CONTAINS "Ready": a grep for - // Ready alone passes on a cluster whose only node is NotReady. - ` if [ -n "$STATUS" ] && ! echo "$STATUS" | grep -q NotReady && echo "$STATUS" | grep -q Ready; then`, - ` echo "all nodes ready"`, + // Anchored, so it counts `Ready` and `Ready,SchedulingDisabled` and cannot + // count `NotReady` — which the unanchored grep below still has to exclude + // explicitly for the same reason: NotReady CONTAINS Ready. + ` READY=$(echo "$STATUS" | grep -c '^Ready' || true)`, + ` if [ -n "$STATUS" ] && ! echo "$STATUS" | grep -q NotReady && [ "$READY" -ge "$WANT" ]; then`, + ` echo "$READY of $WANT nodes ready"`, ` exit 0`, ` fi`, fmt.Sprintf(` sleep %d`, nodeReadyInterval), `done`, - fmt.Sprintf(`echo "no k3s node became Ready within %ds" >&2`, nodeReadyAttempts*nodeReadyInterval), + fmt.Sprintf(`echo "fewer than $WANT k3s nodes became Ready within %ds" >&2`, + nodeReadyAttempts*nodeReadyInterval), `sudo k3s kubectl get nodes -o wide >&2 || true`, - `sudo systemctl status k3s --no-pager >&2 || true`, - `sudo journalctl -u k3s --no-pager -n 50 >&2 || true`, + fmt.Sprintf(`sudo systemctl status %s --no-pager >&2 || true`, ServerUnit), + fmt.Sprintf(`sudo journalctl -u %s --no-pager -n 50 >&2 || true`, ServerUnit), `exit 1`, }, "\n") } diff --git a/pkg/clouds/pulumi/k3s/scripts_test.go b/pkg/clouds/pulumi/k3s/scripts_test.go index b5832b11f..e2427eaee 100644 --- a/pkg/clouds/pulumi/k3s/scripts_test.go +++ b/pkg/clouds/pulumi/k3s/scripts_test.go @@ -11,6 +11,7 @@ import ( cfgK3s "github.com/simple-container-com/api/pkg/clouds/k3s" pulumiKubernetes "github.com/simple-container-com/api/pkg/clouds/pulumi/kubernetes" + "github.com/simple-container-com/api/pkg/clouds/vm" ) func TestServerArgs(t *testing.T) { @@ -20,6 +21,7 @@ func TestServerArgs(t *testing.T) { &cfgK3s.K3sClusterConfig{ExtraServerArgs: []string{"--kube-apiserver-arg=v=2"}}, []string{"k8s.example.ru", "203.0.113.10"}, []string{"traefik", "servicelb"}, + JoinSpec{}, ) Expect(args).To(Equal("server " + @@ -29,25 +31,159 @@ func TestServerArgs(t *testing.T) { // `server` first: the installer reads the subcommand off the front of the vector // it is given, and an install with the flags but no subcommand starts an agent. Expect(strings.HasPrefix(args, "server ")).To(BeTrue()) + // The zero JoinSpec is the lone server of a single-server cluster. Neither flag + // belongs there: `--cluster-init` would hand it an embedded etcd it has no use + // for (a quorum of one, and a datastore conversion to undo), and `--server` + // would point it at itself. + Expect(args).NotTo(ContainSubstring("--cluster-init")) + Expect(args).NotTo(ContainSubstring("--server")) +} + +// The three JoinSpec shapes are the whole topology story, and the flags are not +// interchangeable: `--cluster-init` on a joining server makes it found a SECOND +// cluster of one rather than join the first, which presents as a working provision +// and a split brain. +func TestServerArgsRendersEachTopology(t *testing.T) { + RegisterTestingT(t) + + cfg := &cfgK3s.K3sClusterConfig{} + first := ServerArgs(cfg, []string{"a"}, nil, JoinSpec{ClusterInit: true}) + Expect(first).To(HavePrefix("server --cluster-init ")) + Expect(first).NotTo(ContainSubstring("--server ")) + + joining := ServerArgs(cfg, []string{"a"}, nil, JoinSpec{ServerURL: "https://203.0.113.10:6443"}) + Expect(joining).To(Equal("server --server https://203.0.113.10:6443 --tls-san a")) + Expect(joining).NotTo(ContainSubstring("--cluster-init")) + + // An agent gets `--server` and nothing else. `--tls-san`, `--disable` and + // extraServerArgs are server-only flags and k3s's agent does not ignore them — + // it exits with an unknown-flag error, so the node never starts. + agent := AgentArgs(JoinSpec{ServerURL: "https://203.0.113.10:6443"}) + Expect(agent).To(Equal("agent --server https://203.0.113.10:6443")) + for _, serverOnly := range []string{"--tls-san", "--disable", "--cluster-init"} { + Expect(agent).NotTo(ContainSubstring(serverOnly)) + } + + // The unit follows the role, because the installer decides which unit to write + // from the FIRST positional argument it is handed. + Expect(UnitFor(vm.RoleK3sServer)).To(Equal(ServerUnit)) + Expect(UnitFor(vm.RoleK3sAgent)).To(Equal(AgentUnit)) + Expect(ArgsMarkerPath(ServerUnit)).NotTo(Equal(ArgsMarkerPath(AgentUnit))) +} + +// NodeIP is the one addressing decision SC makes for k3s, and the reason it exists +// is that k3s's own default is wrong on a single-NIC VPS: it advertises the +// default-route address, which there is the PUBLIC one, putting etcd peer traffic +// and flannel's unencrypted VXLAN overlay on the internet (vm.VmHost.PrivateAddress). +func TestNodeIPPinsBothAdvertisedAddressesOnAServerAndOnlyNodeIPOnAnAgent(t *testing.T) { + RegisterTestingT(t) + + cfg := &cfgK3s.K3sClusterConfig{} + + // `--node-ip` alone is not enough on a server: the API server would still + // advertise its default-route address to its peers, which is the value this + // exists to avoid. Both flags, both to the same address. + first := ServerArgs(cfg, []string{"a"}, nil, JoinSpec{ClusterInit: true, NodeIP: "10.0.0.10"}) + Expect(first).To(ContainSubstring("--node-ip 10.0.0.10")) + Expect(first).To(ContainSubstring("--advertise-address 10.0.0.10")) + + joining := ServerArgs(cfg, []string{"a"}, nil, + JoinSpec{ServerURL: "https://10.0.0.10:6443", NodeIP: "10.0.0.11"}) + Expect(joining).To(ContainSubstring("--node-ip 10.0.0.11")) + Expect(joining).To(ContainSubstring("--advertise-address 10.0.0.11")) + + // `--advertise-address` is server-only; an agent handed it exits with an + // unknown-flag error and the node never starts. + agent := AgentArgs(JoinSpec{ServerURL: "https://10.0.0.10:6443", NodeIP: "10.0.0.12"}) + Expect(agent).To(Equal("agent --server https://10.0.0.10:6443 --node-ip 10.0.0.12")) + Expect(agent).NotTo(ContainSubstring("--advertise-address")) + + // An empty NodeIP renders nothing at all, on both roles. That is what keeps a + // single-node cluster's args marker byte-identical to what it was before this + // field existed — otherwise every existing control plane reinstalls for no + // behaviour change. + Expect(ServerArgs(cfg, []string{"a"}, nil, JoinSpec{ClusterInit: true})). + NotTo(ContainSubstring("--node-ip")) + Expect(AgentArgs(JoinSpec{ServerURL: "https://10.0.0.10:6443"})). + NotTo(ContainSubstring("--node-ip")) +} + +// The join token must not reach the argument vector. It would land in the args +// marker on the machine and in the Pulumi checkpoint as part of that string, and a +// token rotation would then make the marker compare unequal — reinstalling a healthy +// control plane for a value the comparison has no business knowing. +func TestTheJoinTokenTravelsInTheEnvironmentAndNotTheArgs(t *testing.T) { + RegisterTestingT(t) + + const token = "K10deadbeef::server:cafe" + args := ServerArgs(&cfgK3s.K3sClusterConfig{}, []string{"a"}, nil, + JoinSpec{ServerURL: "https://203.0.113.10:6443"}) + Expect(args).NotTo(ContainSubstring(token)) + Expect(args).NotTo(ContainSubstring("--token")) + + script := InstallScript("v1.36.5+k3s1", cfgK3s.DefaultInstallScriptURL, args, ServerUnit, token) + // `sudo env`, for the same reason INSTALL_K3S_VERSION uses it: it is the one + // form that survives sudo's env_reset. + Expect(script).To(ContainSubstring(`sudo env INSTALL_K3S_VERSION="$WANT_VERSION" K3S_TOKEN="` + token + `"`)) + Expect(script).To(ContainSubstring(`WANT_ARGS="` + args + `"`)) + + // And no token at all when there is none to pass — the first server of a cluster + // has nothing to authenticate to. + Expect(InstallScript("v1.36.5+k3s1", cfgK3s.DefaultInstallScriptURL, args, ServerUnit, "")). + NotTo(ContainSubstring("K3S_TOKEN")) +} + +// A destroy has to succeed against a machine whose install never finished — which is +// the common case, since the resource being destroyed is often the one that did not +// come up. The installer writes the uninstaller, so its presence is the only honest +// test of whether k3s was ever here. +func TestUninstallScriptIsRoleAwareAndSucceedsOnAnUntouchedMachine(t *testing.T) { + RegisterTestingT(t) + + Expect(UninstallScript(ServerUnit)).To(ContainSubstring("/usr/local/bin/k3s-uninstall.sh")) + Expect(UninstallScript(AgentUnit)).To(ContainSubstring("/usr/local/bin/k3s-agent-uninstall.sh")) + Expect(UninstallScript(AgentUnit)).NotTo(ContainSubstring("/usr/local/bin/k3s-uninstall.sh")) + + Expect(UninstallScript(ServerUnit)).To(ContainSubstring("exit 0")) + // The marker lives in SC's own state dir, which the uninstaller knows nothing + // about. Left behind, it makes the next install's skip condition compare against + // the arguments of a cluster that no longer exists. + Expect(UninstallScript(ServerUnit)).To(ContainSubstring("rm -f " + ArgsMarkerPath(ServerUnit))) +} + +// The token file is written asynchronously after the server process starts, so a +// read that wins the race returns success and nothing — and every join built from it +// then fails as an authentication error against a cluster that is perfectly healthy. +func TestTokenReadScriptRefusesAnEmptyToken(t *testing.T) { + RegisterTestingT(t) + + script := TokenReadScript() + Expect(script).To(ContainSubstring(ServerTokenPath)) + Expect(script).To(ContainSubstring(`[ -n "$TOKEN" ]`)) + // No trailing newline: this value is interpolated into another node's install + // command, where a stray newline is a token mismatch with no message saying so. + Expect(script).To(ContainSubstring(`printf '%s' "$TOKEN"`)) + Expect(script).NotTo(ContainSubstring(`echo "$TOKEN"`)) } func TestInstallScriptIsIdempotentOnConfigAndNotJustOnVersion(t *testing.T) { RegisterTestingT(t) - script := InstallScript("v1.36.5+k3s1", cfgK3s.DefaultInstallScriptURL, "server --tls-san a") + script := InstallScript("v1.36.5+k3s1", cfgK3s.DefaultInstallScriptURL, "server --tls-san a", ServerUnit, "") // The three halves of the skip condition. Dropping any one of them turns a // re-provision into either a pointless control-plane restart (no condition) or a // silent no-op against a changed config (version only) — the second being the // one that reports green while doing nothing. - Expect(script).To(ContainSubstring(`sudo systemctl is-active --quiet k3s`)) + Expect(script).To(ContainSubstring(`sudo systemctl is-active --quiet "$UNIT"`)) + Expect(script).To(ContainSubstring(`UNIT="` + ServerUnit + `"`)) Expect(script).To(ContainSubstring(`sudo k3s --version 2>/dev/null | grep -qF "$WANT_VERSION"`)) - Expect(script).To(ContainSubstring(`[ "$(sudo cat ` + ArgsMarkerPath + `)" = "$WANT_ARGS" ]`)) + Expect(script).To(ContainSubstring(`[ "$(sudo cat ` + ArgsMarkerPath(ServerUnit) + `)" = "$WANT_ARGS" ]`)) // The marker is written after the install, never before: a failed install must // not leave behind a claim that the machine is configured. installAt := strings.Index(script, "sudo env INSTALL_K3S_VERSION") - markerAt := strings.Index(script, "| sudo tee "+ArgsMarkerPath) + markerAt := strings.Index(script, "| sudo tee "+ArgsMarkerPath(ServerUnit)) Expect(installAt).To(BeNumerically(">", 0)) Expect(markerAt).To(BeNumerically(">", installAt)) @@ -78,13 +214,13 @@ func TestInstallScriptIsIdempotentOnConfigAndNotJustOnVersion(t *testing.T) { func TestInstallScriptStartsTheServiceTheInstallerDeclinedToStart(t *testing.T) { RegisterTestingT(t) - script := InstallScript("v1.36.5+k3s1", cfgK3s.DefaultInstallScriptURL, "server --tls-san a") + script := InstallScript("v1.36.5+k3s1", cfgK3s.DefaultInstallScriptURL, "server --tls-san a", ServerUnit, "") - Expect(script).To(ContainSubstring(`sudo systemctl enable --now k3s`)) + Expect(script).To(ContainSubstring(`sudo systemctl enable --now "$UNIT"`)) installAt := strings.Index(script, "sudo env INSTALL_K3S_VERSION") - startAt := strings.Index(script, "sudo systemctl enable --now k3s") - markerAt := strings.Index(script, "| sudo tee "+ArgsMarkerPath) + startAt := strings.Index(script, `sudo systemctl enable --now "$UNIT"`) + markerAt := strings.Index(script, "| sudo tee "+ArgsMarkerPath(ServerUnit)) Expect(startAt).To(BeNumerically(">", installAt), "the service start has to follow the installer, which writes the unit file it starts") Expect(markerAt).To(BeNumerically(">", startAt), @@ -119,13 +255,13 @@ func TestKubeconfigScriptAssertsTheRewriteHappened(t *testing.T) { func TestNodeReadyScriptDoesNotAcceptNotReady(t *testing.T) { RegisterTestingT(t) - script := NodeReadyScript() + script := NodeReadyScript(1) // "NotReady" contains "Ready", so a grep for Ready alone passes on a cluster // whose only node is NotReady — i.e. the gate would open on exactly the state it - // exists to wait out. + // exists to wait out. The count below is anchored for the same reason. Expect(script).To(ContainSubstring(`! echo "$STATUS" | grep -q NotReady`)) - Expect(script).To(ContainSubstring(`echo "$STATUS" | grep -q Ready`)) + Expect(script).To(ContainSubstring(`grep -c '^Ready'`)) // An empty node list is not readiness either: `kubectl get nodes` against an API // server that is up but has no registered node prints nothing and exits 0. Expect(script).To(ContainSubstring(`[ -n "$STATUS" ]`)) @@ -134,6 +270,20 @@ func TestNodeReadyScriptDoesNotAcceptNotReady(t *testing.T) { Expect(script).To(ContainSubstring("journalctl -u k3s")) } +// The count is what makes it a FLEET gate rather than a node gate. Without it the +// script passes the instant the first server registers, which on a multi-node cluster +// is before any agent has joined — so a deploy ordered after this gate schedules its +// pods onto a cluster of one machine. "No node is NotReady" does not catch it: a node +// that has not registered yet is not NotReady, it is absent. +func TestNodeReadyScriptWaitsForTheWholeFleet(t *testing.T) { + RegisterTestingT(t) + + script := NodeReadyScript(3) + + Expect(script).To(ContainSubstring(`WANT=3`)) + Expect(script).To(ContainSubstring(`[ "$READY" -ge "$WANT" ]`)) +} + // The kubeconfig export name is the one string in this package that has to match // another package byte for byte: every consumer of a cluster // (`kubernetes-cloudrun`, the Caddy edge, the helm operators) reads the name diff --git a/pkg/clouds/pulumi/kubernetes/caddy.go b/pkg/clouds/pulumi/kubernetes/caddy.go index 51336b916..b9707e1fb 100644 --- a/pkg/clouds/pulumi/kubernetes/caddy.go +++ b/pkg/clouds/pulumi/kubernetes/caddy.go @@ -470,6 +470,28 @@ func ToIngressIpExport(clusterName string) string { return fmt.Sprintf("%s-ingress-ip", clusterName) } +// ToIngressIpsExport names the output of a cluster whose front door is MORE than one +// address, so a domain-bearing workload on it gets one A record per address instead of +// one record pointing at whichever node happens to be first. +// +// A managed cluster has a single load-balancer address and exports only +// ToIngressIpExport. A k3s pool has no load balancer: klipper-lb binds :80/:443 on +// every node, so every node is the edge, and pointing the name at one of them throws +// away the other N-1 (and makes a node that is merely rebooting look like an outage). +// +// Deliberately a SEPARATE output rather than a change to ToIngressIpExport's value. +// That output is read by a child stack that may be deployed from a checkout older than +// the parent, and it must stay exactly one address: KubeRun writes it straight into an +// A record's value, where a comma-joined list is not a resolvable address but IS a +// successful deploy. A reader that does not know about this output keeps working; one +// that does gets the whole fleet. Joined with commas and read back with strings.Split +// for the same reason ToOpenPortsExport is a formatted string — a Pulumi stack output +// crosses the boundary through JSON, where a []string comes back as []any and a typed +// read of it fails. +func ToIngressIpsExport(clusterName string) string { + return fmt.Sprintf("%s-ingress-ips", clusterName) +} + func ToClusterName(input api.ResourceInput, resName string) string { return input.ToResName(resName) } diff --git a/pkg/clouds/pulumi/kubernetes/kube_run.go b/pkg/clouds/pulumi/kubernetes/kube_run.go index 7240e6893..789691d0f 100644 --- a/pkg/clouds/pulumi/kubernetes/kube_run.go +++ b/pkg/clouds/pulumi/kubernetes/kube_run.go @@ -8,6 +8,7 @@ import ( "encoding/hex" "encoding/json" "fmt" + "strings" "github.com/pkg/errors" "github.com/samber/lo" @@ -47,6 +48,44 @@ type KubeRunOpts struct { // the bare parent stack name, and a parent may run several Caddies. Without // caddyResource it keeps asking for the bare parent stack name. IngressIPExport string + + // IngressIPsExport names an OPTIONAL parent-stack output holding EVERY address of + // the cluster's front door, comma-separated (see ToIngressIpsExport). Set it only + // for a flavour whose cluster exports one — k3s does, because klipper-lb makes + // every node an edge; a managed cluster has a single load-balancer address and + // leaves this empty. + // + // Three ways it degrades, all of them to today's behaviour, because every one of + // them is reachable in a real deploy: + // + // - empty here (every flavour but k3s) → the single record, unchanged; + // - set here but ABSENT from the parent stack (a k3s parent provisioned before + // this output existed, which is every k3s parent alive at the time of writing) + // → the single record, because GetValueFromStack answers a missing output with + // the zero value and no error; + // - `clusterIPAddress:` set on the client stack → that address wins outright and + // neither output is read at all. An operator naming one address means one + // record. + // + // So this is additive: the fleet records appear on the first deploy after the + // PARENT has been re-provisioned, and nothing in between fails. + IngressIPsExport string +} + +// fleetAddresses reads the cluster's ingress-ips output back into the addresses it +// carries, and is the single point at which "the fleet" and "fall back to one record" +// are decided: an empty result means the single-address path, so every way the output +// can be useless has to land on empty here. +// +// Hence Compact. A missing output arrives as "" and strings.Split("", ",") is [""] — +// one empty address, which is not a fleet of nothing, it is a DnsRecord with an empty +// value, and a registrar is entitled to accept that. A trailing separator, or an +// address the parent could not resolve, is the same failure in the middle of a real +// list; dropping it costs one record out of N rather than pointing the name at nothing. +func fleetAddresses(joined string) []string { + return lo.Compact(lo.Map(strings.Split(joined, ","), func(s string, _ int) string { + return strings.TrimSpace(s) + })) } func (o KubeRunOpts) ingressIPExport(parentStack, caddyCluster string) string { @@ -251,16 +290,29 @@ func KubeRunWithProvider( return nil, errors.Errorf("cannot provision domain %q for stack %q in %q: registrar is not configured", domain, stackName, input.StackParams.Environment) } var clusterIPAddress string + // The fleet, when the cluster has one. Empty everywhere else, and empty is the + // single-address path — see KubeRunOpts.IngressIPsExport for the three ways + // that happens. + var clusterIPAddresses []string + suffix := lo.If(params.ParentStack.DependsOnResource != nil, "--"+lo.FromPtr(params.ParentStack.DependsOnResource).Name).Else("") if lo.FromPtr(stackCfg.ClusterIPAddress) != "" { params.Log.Info(ctx.Context(), "Using specified cluster IP address for domain %q (%q)", domain, lo.FromPtr(stackCfg.ClusterIPAddress)) clusterIPAddress = lo.FromPtr(stackCfg.ClusterIPAddress) } else { params.Log.Info(ctx.Context(), "Using provisioned cluster IP address for domain %q", domain) - suffix := lo.If(params.ParentStack.DependsOnResource != nil, "--"+lo.FromPtr(params.ParentStack.DependsOnResource).Name).Else("") - clusterIPAddress, err = pApi.GetValueFromStack[string](ctx, fmt.Sprintf("%s-%s%s-ip", stackName, input.StackParams.Environment, suffix), fullParentReference, opts.ingressIPExport(parentStack, caddyCluster), false) - if err != nil { - return nil, errors.Wrapf(err, "failed to get cluster IP address from parent stack's resources") + if opts.IngressIPsExport != "" { + joined, err := pApi.GetValueFromStack[string](ctx, fmt.Sprintf("%s-%s%s-ips", stackName, input.StackParams.Environment, suffix), fullParentReference, opts.IngressIPsExport, false) + if err != nil { + return nil, errors.Wrapf(err, "failed to get the cluster's ingress addresses from parent stack's resources") + } + clusterIPAddresses = fleetAddresses(joined) + } + if len(clusterIPAddresses) == 0 { + clusterIPAddress, err = pApi.GetValueFromStack[string](ctx, fmt.Sprintf("%s-%s%s-ip", stackName, input.StackParams.Environment, suffix), fullParentReference, opts.ingressIPExport(parentStack, caddyCluster), false) + if err != nil { + return nil, errors.Wrapf(err, "failed to get cluster IP address from parent stack's resources") + } } } // Determine if domain should be proxied - defaults to true if not explicitly set to false @@ -269,14 +321,24 @@ func KubeRunWithProvider( domainProxied = *stackCfg.DomainProxied } - _, err = params.Registrar.NewRecord(ctx, api.DnsRecord{ - Name: domain, - Type: "A", - Value: clusterIPAddress, - Proxied: domainProxied, - }) - if err != nil { - return nil, errors.Wrapf(err, "failed to provision domain %q for stack %q in %q", domain, stackName, environment) + if len(clusterIPAddresses) > 0 { + // One A record per node, sharing the name, plus the guard that catches a + // registrar collapsing them into one — the same implementation the compose + // fleet uses, deliberately, because the failure it refuses (a green deploy + // whose name resolves to a single machine) is identical on both paths. + if err := pApi.FleetDnsRecords(ctx, params, stackName, domain, clusterIPAddresses, domainProxied); err != nil { + return nil, errors.Wrapf(err, "failed to provision domain %q for stack %q in %q", domain, stackName, environment) + } + } else { + _, err = params.Registrar.NewRecord(ctx, api.DnsRecord{ + Name: domain, + Type: "A", + Value: clusterIPAddress, + Proxied: domainProxied, + }) + if err != nil { + return nil, errors.Wrapf(err, "failed to provision domain %q for stack %q in %q", domain, stackName, environment) + } } } diff --git a/pkg/clouds/pulumi/kubernetes/kube_run_opts_test.go b/pkg/clouds/pulumi/kubernetes/kube_run_opts_test.go index d55a6f069..db31b1e75 100644 --- a/pkg/clouds/pulumi/kubernetes/kube_run_opts_test.go +++ b/pkg/clouds/pulumi/kubernetes/kube_run_opts_test.go @@ -43,6 +43,55 @@ func TestKubeRunOptsIngressIPExportOverrideIsUsedVerbatim(t *testing.T) { Expect(opts.ingressIPExport("my-parent", "caddy-std--likeclaw-us")).To(Equal("ru-cluster--staging-ingress-ip")) } +// ToIngressIpsExport must NOT collide with ToIngressIpExport, and the difference has to +// be more than a plural: both outputs live on the same parent stack at the same time, +// and a reader asking for the wrong one gets either a comma-joined list written into an +// A record's value or a single node of a fleet. +func TestIngressIpsExportIsADistinctOutput(t *testing.T) { + RegisterTestingT(t) + + Expect(ToIngressIpsExport("ru-cluster--staging")).To(Equal("ru-cluster--staging-ingress-ips")) + Expect(ToIngressIpsExport("ru-cluster--staging")).ToNot(Equal(ToIngressIpExport("ru-cluster--staging"))) +} + +// The switch between one A record and N. Empty means the single-address path, so every +// way the parent's output can be useless has to arrive here as empty — including the +// missing output, which is "" and which strings.Split turns into one EMPTY address +// rather than none. +func TestFleetAddresses(t *testing.T) { + RegisterTestingT(t) + + for name, tc := range map[string]struct { + joined string + want []string + }{ + "missing output": {"", []string{}}, + "single address": {"203.0.113.10", []string{"203.0.113.10"}}, + "three addresses": {"203.0.113.10,203.0.113.11,203.0.113.12", []string{"203.0.113.10", "203.0.113.11", "203.0.113.12"}}, + "trailing sep": {"203.0.113.10,203.0.113.11,", []string{"203.0.113.10", "203.0.113.11"}}, + "unresolved middle": {"203.0.113.10,,203.0.113.12", []string{"203.0.113.10", "203.0.113.12"}}, + "separators only": {",,", []string{}}, + "padded": {" 203.0.113.10 , 203.0.113.11 ", []string{"203.0.113.10", "203.0.113.11"}}, + } { + t.Run(name, func(t *testing.T) { + RegisterTestingT(t) + // Equal and not ConsistOf: order is the host order of the fleet, and + // len() == 0 is what the caller reads as "fall back to one record". + Expect(fleetAddresses(tc.joined)).To(Equal(tc.want)) + }) + } +} + +// And the option itself defaults off: every flavour but k3s leaves it empty, which is +// the single-record behaviour KubeRun has always had. +func TestKubeRunOptsIngressIPsExportIsOptional(t *testing.T) { + RegisterTestingT(t) + + Expect(KubeRunOpts{}.IngressIPsExport).To(BeEmpty()) + Expect(KubeRunOpts{IngressIPExport: "ru-cluster--staging-ingress-ip"}.IngressIPsExport).To(BeEmpty(), + "setting the single-address override must not opt a flavour into the fleet path") +} + // A cloud-compose client stack that omits `size:` leaves StackConfig.Size nil, and // reading the ephemeral size straight through it used to panic the deploy with a bare // SIGSEGV before any resource was touched. diff --git a/pkg/clouds/pulumi/plugin_preengine.go b/pkg/clouds/pulumi/plugin_preengine.go index 75de913de..53e17a012 100644 --- a/pkg/clouds/pulumi/plugin_preengine.go +++ b/pkg/clouds/pulumi/plugin_preengine.go @@ -87,8 +87,21 @@ type preEnginePluginBundle struct { logCancel context.CancelFunc } -func newPreEnginePluginBundle(log logger.Logger) *preEnginePluginBundle { - ctx, cancel := context.WithCancel(context.Background()) +// newPreEnginePluginBundle takes the CALLER's context, not Background, because +// SC's log level travels in the context (logger.Silent stores it as a value). +// logCtx is what every LogStream consumer logs against, so building it from +// Background silently reset the level to the Info default and a plugin's +// LOG_SEVERITY_INFO events printed even under `sc stack outputs`, which +// silences its logger precisely so its stdout is a clean JSON document — and +// SC's logger writes to stdout, so those lines landed in the middle of it and +// broke `| jq`. Live-caught 2026-11-08 on forge infra, where the aws plugin's +// `configuring s3-bucket backend` line showed up in the outputs payload. +// +// Cancellation is unchanged: Close still cancels logCtx, and the caller's +// context always outlives the bundle (every entry point defers +// closePreEnginePlugins), so deriving from it cannot end the consumers early. +func newPreEnginePluginBundle(callerCtx context.Context, log logger.Logger) *preEnginePluginBundle { + ctx, cancel := context.WithCancel(callerCtx) return &preEnginePluginBundle{ log: log, clients: map[string]*scPlugin.Client{}, diff --git a/pkg/clouds/pulumi/plugin_preengine_logctx_test.go b/pkg/clouds/pulumi/plugin_preengine_logctx_test.go new file mode 100644 index 000000000..3ffa9d9c7 --- /dev/null +++ b/pkg/clouds/pulumi/plugin_preengine_logctx_test.go @@ -0,0 +1,77 @@ +// SPDX-License-Identifier: MIT +// Copyright (c) Simple Container + +package pulumi + +import ( + "context" + "testing" + + . "github.com/onsi/gomega" + + "github.com/simple-container-com/api/pkg/api/logger" +) + +// The bundle's logCtx is what every LogStream consumer logs against, and SC's +// log level lives in the context as a value. Building logCtx from +// context.Background() therefore reset the level to the Info default, so a +// plugin's LOG_SEVERITY_INFO events printed even under `sc stack outputs` — +// which silences its logger precisely because SC's logger writes to STDOUT and +// the command's stdout is a JSON document. forge infra's aws plugin logged +// `configuring s3-bucket backend` into the middle of the payload and broke +// `| jq`. +func TestPreEngineBundleInheritsLogLevelFromCaller(t *testing.T) { + RegisterTestingT(t) + + log := logger.New() + + t.Run("silence reaches the LogStream consumers", func(t *testing.T) { + RegisterTestingT(t) + + b := newPreEnginePluginBundle(log.Silent(context.Background()), log) + defer b.Close() + + Expect(logger.LevelFromContext(b.logCtx)).To(Equal(logger.LogLevelSilent), + "logCtx dropped the caller's level: plugin log events will print into the outputs JSON") + }) + + t.Run("an ordinary caller is unaffected", func(t *testing.T) { + RegisterTestingT(t) + + b := newPreEnginePluginBundle(context.Background(), log) + defer b.Close() + + // provision/deploy must keep showing plugin progress. + Expect(logger.LevelFromContext(b.logCtx)).To(Equal(logger.LogLevelInfo)) + }) + + t.Run("Close still cancels the consumers", func(t *testing.T) { + RegisterTestingT(t) + + b := newPreEnginePluginBundle(context.Background(), log) + Expect(b.logCtx.Err()).ToNot(HaveOccurred()) + + b.Close() + + // Deriving logCtx from the caller must not break the teardown the + // consumers rely on to end their stream RPC. + Expect(b.logCtx.Err()).To(MatchError(context.Canceled)) + }) + + t.Run("a cancelled caller cancels the bundle", func(t *testing.T) { + RegisterTestingT(t) + + ctx, cancel := context.WithCancel(context.Background()) + b := newPreEnginePluginBundle(ctx, log) + defer b.Close() + + cancel() + + // Inherited cancellation is the price of inheriting values, and it is + // the behaviour we want: if the operation is abandoned, the log + // consumers should stop too. Safe because every entry point defers + // closePreEnginePlugins, so the caller's context always outlives the + // bundle in the normal path. + Expect(b.logCtx.Err()).To(MatchError(context.Canceled)) + }) +} diff --git a/pkg/clouds/pulumi/preengine_entrypoints_test.go b/pkg/clouds/pulumi/preengine_entrypoints_test.go new file mode 100644 index 000000000..a755b7e1e --- /dev/null +++ b/pkg/clouds/pulumi/preengine_entrypoints_test.go @@ -0,0 +1,190 @@ +// SPDX-License-Identifier: MIT +// Copyright (c) Simple Container + +package pulumi + +import ( + "context" + "path/filepath" + "reflect" + "testing" + + . "github.com/onsi/gomega" + + "github.com/simple-container-com/api/pkg/api" + "github.com/simple-container-com/api/pkg/api/logger" +) + +// Every api.Provisioner entry point that reaches the Pulumi backend has to run +// the pre-engine plugin load first, because when the state storage is +// plugin-owned the sidecar is the only thing that knows the backend URL. An +// `s3-bucket` state-storage declares `bucketName:` and nothing else; the plugin +// assembles `s3://` in InitStateStore and publishes it via +// PULUMI_BACKEND_URL, which login() reads only if the sidecar already ran. +// +// Skip the load and StorageUrl() returns the empty string that +// *api.PluginFieldConfig yields for a key the operator never wrote, so +// diy.Login fails with +// +// diy URL has an illegal prefix; expected one of: azblob, file, gs, postgres, s3 +// +// — an error naming no stack, no plugin and no field, with a telltale double +// space where the URL should be. OutputsStack and CancelStack were both missing +// the load; `sc stack outputs -s infra` against forge's s3-bucket backend is +// what surfaced it. +// +// The table is the guard: a new entry point added without the load fails here +// rather than at an operator's terminal. Each case drives the real method with +// a plugin spec that cannot resolve, so the only way to pass is to attempt the +// load — a stubbed-out bundle would make the test vacuous. +func TestBackendEntryPointsLoadPreEnginePlugins(t *testing.T) { + RegisterTestingT(t) + + // A path that cannot be spawned. The load fails at the plugin client, far + // earlier than any backend login, so no case here touches the network or a + // real checkpoint. + stackWithUnresolvablePlugin := func(t *testing.T) api.Stack { + return api.Stack{ + Name: "infra", + Server: api.ServerDescriptor{ + Plugins: map[string]api.PluginSpec{ + "nonexistent": {Path: filepath.Join(t.TempDir(), "sc-plugin-nonexistent")}, + }, + }, + } + } + + cases := []struct { + name string + call func(context.Context, *pulumi, *api.ConfigFile, api.Stack) error + }{ + { + // The two this test was written for. + name: "OutputsStack", + call: func(ctx context.Context, p *pulumi, cfg *api.ConfigFile, s api.Stack) error { + _, err := p.OutputsStack(ctx, cfg, s, api.StackParams{StackName: "infra"}) + return err + }, + }, + { + name: "CancelStack", + call: func(ctx context.Context, p *pulumi, cfg *api.ConfigFile, s api.Stack) error { + return p.CancelStack(ctx, cfg, s, api.StackParams{StackName: "infra"}) + }, + }, + // The six that already had it, pinned so a refactor cannot quietly drop + // one and reintroduce the same class of bug elsewhere. + { + name: "ProvisionStack", + call: func(ctx context.Context, p *pulumi, cfg *api.ConfigFile, s api.Stack) error { + return p.ProvisionStack(ctx, cfg, s, api.ProvisionParams{}) + }, + }, + { + name: "DeployStack", + call: func(ctx context.Context, p *pulumi, cfg *api.ConfigFile, s api.Stack) error { + return p.DeployStack(ctx, cfg, s, api.DeployParams{}) + }, + }, + { + name: "PreviewStack", + call: func(ctx context.Context, p *pulumi, cfg *api.ConfigFile, s api.Stack) error { + _, err := p.PreviewStack(ctx, cfg, s, api.ProvisionParams{}) + return err + }, + }, + { + name: "PreviewChildStack", + call: func(ctx context.Context, p *pulumi, cfg *api.ConfigFile, s api.Stack) error { + _, err := p.PreviewChildStack(ctx, cfg, s, api.DeployParams{}) + return err + }, + }, + { + name: "DestroyParentStack", + call: func(ctx context.Context, p *pulumi, cfg *api.ConfigFile, s api.Stack) error { + return p.DestroyParentStack(ctx, cfg, s, api.DestroyParams{}, true) + }, + }, + { + name: "DestroyChildStack", + call: func(ctx context.Context, p *pulumi, cfg *api.ConfigFile, s api.Stack) error { + return p.DestroyChildStack(ctx, cfg, s, api.DestroyParams{}, true) + }, + }, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + RegisterTestingT(t) + + p := &pulumi{logger: logger.New()} + err := tc.call(context.Background(), p, &api.ConfigFile{}, stackWithUnresolvablePlugin(t)) + + Expect(err).To(HaveOccurred(), + "%s accepted a stack whose declared plugin cannot be spawned", tc.name) + // The wrap is the evidence the load was attempted. Without it the + // call proceeds to the backend and fails with diy's + // `illegal prefix` instead — or, worse, succeeds against a stale + // PULUMI_BACKEND_URL left in the environment. + Expect(err.Error()).To(ContainSubstring("pre-engine plugin load"), + "%s failed for some other reason than the pre-engine plugin load: %v", tc.name, err) + }) + } + + // The table above is hand-written, so on its own it would let entry point + // #9 through the same door: a new api.Provisioner method just wouldn't be + // listed, and nothing would complain. Derive the expected set from the + // interface instead and fail closed. + // + // The rule is structural, not an allowlist — a method qualifies when it + // takes a context AND an api.Stack, which is exactly the shape of a call + // that resolves a stack against the backend. SetPublicKey and + // SetConfigReader are plain setters and match neither, so they drop out on + // their own rather than needing to be named here (and a future setter will + // too). + covered := map[string]bool{} + for _, tc := range cases { + covered[tc.name] = true + } + iface := reflect.TypeOf((*api.Provisioner)(nil)).Elem() + for i := range iface.NumMethod() { + m := iface.Method(i) + if !takesContextAndStack(m.Type) { + continue + } + Expect(covered).To(HaveKey(m.Name), + "api.Provisioner.%s resolves a stack against the backend but is not in this test's "+ + "table. Add it — and make sure it calls loadPreEnginePlugins, or a plugin-owned "+ + "state storage will fail it with `diy URL has an illegal prefix`.", m.Name) + } +} + +// takesContextAndStack reports whether an api.Provisioner method's signature +// is the backend-touching shape: a context plus the stack to resolve. Method +// values off an interface type have no receiver in their In() list. +func takesContextAndStack(fn reflect.Type) bool { + if fn.NumIn() == 0 || fn.In(0) != reflect.TypeOf((*context.Context)(nil)).Elem() { + return false + } + stackType := reflect.TypeOf(api.Stack{}) + for i := 1; i < fn.NumIn(); i++ { + if fn.In(i) == stackType { + return true + } + } + return false +} + +// A stack that declares no plugins must not be charged for the lookup: every +// in-tree state storage (fs, and the pulumi-cloud backend) still has to work +// with no `plugins:` block at all, and the entry points above call this +// unconditionally. +func TestLoadPreEnginePluginsNoOpsWithoutPlugins(t *testing.T) { + RegisterTestingT(t) + + p := &pulumi{logger: logger.New()} + Expect(p.loadPreEnginePlugins(context.Background(), api.Stack{Name: "infra"})).To(Succeed()) + Expect(p.preEnginePlugins).To(BeNil(), + "a bundle was created for a stack with no plugins; closePreEnginePlugins would then tear down nothing") +} diff --git a/pkg/clouds/pulumi/preview.go b/pkg/clouds/pulumi/preview.go index 02a0c75f2..95dca477e 100644 --- a/pkg/clouds/pulumi/preview.go +++ b/pkg/clouds/pulumi/preview.go @@ -69,6 +69,36 @@ func (p *pulumi) previewChildStack(ctx context.Context, cfg *api.ConfigFile, sta } func (p *pulumi) OutputsStack(ctx context.Context, cfg *api.ConfigFile, stack api.Stack, params api.StackParams) (*api.OutputsResult, error) { + // Reading outputs needs the backend, and when the state storage is + // plugin-owned only the sidecar knows its URL: an `s3-bucket` + // state-storage declares `bucketName:` and the plugin assembles + // `s3://` at InitStateStore time, publishing it through + // PULUMI_BACKEND_URL for login() to pick up (see the default branch of + // the switch there). With no pre-engine load the sidecar never runs, so + // StorageUrl() is the empty string *api.PluginFieldConfig returns and + // diy.Login rejects it with `diy URL has an illegal prefix` — which + // names no stack, no plugin and no field, so it reads like a malformed + // config rather than a missing plugin load. Live-caught on + // `sc stack outputs -s infra` against forge's s3-bucket backend. + // + // Loaded from the PARENT descriptor, before the toChildStack rewrite + // below: `plugins:` is declared on the parent server.yaml, same as + // DestroyChildStack does it. + // + // This is the full load, which spawns every declared plugin and asks each + // for its Schema — forge's infra declares six, so an `outputs` call that + // was ~200ms becomes a couple of seconds. Deliberate for now: correct and + // slow beats fast and broken, and the alternative needs a cache. A + // state-store-only variant can't help by itself, because which plugin owns + // the state store is itself advertised in a Schema that only exists after + // the spawn; the way out is to trust the on-disk schema cache the + // pre-engine already writes and let read-only paths like this one and + // CancelStack load cache-only, falling back to a spawn on a miss. + if err := p.loadPreEnginePlugins(ctx, stack); err != nil { + return nil, errors.Wrap(err, "pre-engine plugin load") + } + defer p.closePreEnginePlugins() + if params.Environment != "" && params.StackName != "" { stack = toChildStack(stack, params) } diff --git a/pkg/clouds/pulumi/provisioner.go b/pkg/clouds/pulumi/provisioner.go index 8fa4d4a45..68b1ac7ac 100644 --- a/pkg/clouds/pulumi/provisioner.go +++ b/pkg/clouds/pulumi/provisioner.go @@ -42,8 +42,10 @@ type pulumi struct { configFile *api.ConfigFile project *workspace.Project - // preEnginePlugins is populated at the start of a ProvisionStack call - // (or Destroy*/Preview*) when stack.Server.Plugins is non-empty. The + // preEnginePlugins is populated at the start of every api.Provisioner + // entry point that reaches the backend — Provision/Deploy/Preview*/ + // Destroy* and, since the `diy URL has an illegal prefix` fix, + // Outputs/Cancel too — when stack.Server.Plugins is non-empty. The // pre-engine phase registers InitStateStore sidecars from these plugins' // Schemas, so SC's login flow picks them up transparently. The in-engine // phase (pluginSession, created inside provisionProgram) RE-USES these @@ -120,7 +122,7 @@ func (p *pulumi) loadPreEnginePlugins(ctx context.Context, stack api.Stack) erro if len(stack.Server.Plugins) == 0 { return nil } - b := newPreEnginePluginBundle(p.logger) + b := newPreEnginePluginBundle(ctx, p.logger) if err := b.Load(ctx, stack.Server.Plugins); err != nil { b.Close() return err diff --git a/pkg/clouds/pulumi/registrar.go b/pkg/clouds/pulumi/registrar.go index d6d9e1e55..785f7c89c 100644 --- a/pkg/clouds/pulumi/registrar.go +++ b/pkg/clouds/pulumi/registrar.go @@ -4,15 +4,16 @@ package pulumi import ( - "github.com/pkg/errors" - sdk "github.com/pulumi/pulumi/sdk/v3/go/pulumi" "github.com/simple-container-com/api/pkg/api" pApi "github.com/simple-container-com/api/pkg/clouds/pulumi/api" ) -var NotConfiguredRegistrarError = errors.New("regisrar is not configured") +// NotConfiguredRegistrarError is an alias; the sentinel moved to pApi so that +// subpackages this package imports (pulumi/vm) can match on it without a cycle. Kept +// here under its original name because it is exported and matched on by name. +var NotConfiguredRegistrarError = pApi.NotConfiguredRegistrarError type notConfigured struct{} diff --git a/pkg/clouds/pulumi/vm/byo.go b/pkg/clouds/pulumi/vm/byo.go index 751b214da..eb3c5ff57 100644 --- a/pkg/clouds/pulumi/vm/byo.go +++ b/pkg/clouds/pulumi/vm/byo.go @@ -40,8 +40,8 @@ func (byoDriver) Hosts( ) ([]HostFacts, error) { hosts := make([]HostFacts, 0, len(cfg.Hosts)) for i, h := range cfg.Hosts { - params.Log.Info(ctx.Context(), "vm %q uses byo host %q (user %q)", - input.Descriptor.Name, h.Address, h.EffectiveUser(cfg.SshUser)) + params.Log.Info(ctx.Context(), "vm %q uses byo host %q (user %q, role %q)", + input.Descriptor.Name, h.Address, h.EffectiveUser(cfg.SshUser), h.EffectiveRole()) hosts = append(hosts, HostFacts{ Name: fmt.Sprintf("%s-host-%d", input.Descriptor.Name, i), @@ -49,8 +49,13 @@ func (byoDriver) Hosts( // to wait on — but still an Output, so a cloud driver that genuinely // has to resolve an address later is not a signature change here. Address: sdk.String(h.Address).ToStringOutput(), - User: h.EffectiveUser(cfg.SshUser), - Port: h.EffectivePort(), + // Falls back to Address, which is only reached on shapes that have no + // node-to-node traffic to misdirect — a compose fleet or a single host. + // A multi-host k3s pool naming no `privateAddress` never gets here; + // validateHostRoles refuses it. + PrivateAddress: sdk.String(h.EffectivePrivateAddress()).ToStringOutput(), + User: h.EffectiveUser(cfg.SshUser), + Port: h.EffectivePort(), // Both halves marked secret here, at the single point where they enter // Pulumi. See the HostFacts.PrivateKey comment for why that is not // optional. @@ -64,10 +69,13 @@ func (byoDriver) Hosts( // consequence of "did not" is theirs to accept — unlike a cloud driver, // where SC creates the machine and can therefore create its identity too. HostKeyPinned: h.HostKey != "", - // A byo host is always a compose host for now; k3s on a byo host is the - // next slice (roadmap item sc-k3s-cluster-resource, K.0) and will pass - // its own role in. - Role: vm.RoleCompose, + // The operator's own `hosts[].role`, or RoleCompose when they wrote none + // — which is what this driver hardcoded before the field existed, so a + // config written against P5 produces an identical HostFacts. SC cannot + // infer which of several machines should run etcd; on a cloud driver the + // machines are interchangeable and the role is derived by index, but a + // byo pool is heterogeneous by definition (see vm.VmHost.Role). + Role: h.EffectiveRole(), }) } return hosts, nil diff --git a/pkg/clouds/pulumi/vm/compose_fleet_test.go b/pkg/clouds/pulumi/vm/compose_fleet_test.go new file mode 100644 index 000000000..b8cbacaf1 --- /dev/null +++ b/pkg/clouds/pulumi/vm/compose_fleet_test.go @@ -0,0 +1,203 @@ +// SPDX-License-Identifier: MIT +// Copyright (c) Simple Container + +package vm + +import ( + "fmt" + "strings" + "testing" + + . "github.com/onsi/gomega" + "github.com/samber/lo" + + "github.com/pulumi/pulumi/sdk/v3/go/common/resource" + + "github.com/simple-container-com/api/pkg/clouds/vm" +) + +// A vm resource can hold more than one machine, and a vm-compose stack is deployed onto +// every one of them. What makes that safe is not the loop — it is four properties of +// what reaches Pulumi, none of which a passing deploy would show: +// +// - host 0's resource names are byte-identical to a single-host deploy's, so growing +// a fleet APPENDS machines instead of replacing the one already serving +// - the machines are deployed one at a time, so a release that fails its health gate +// stops with the rest of the fleet still on the version that worked +// - an image is built once for the fleet and shipped once per machine +// - every machine is actually a different machine (a loop that read index 0 N times +// would deploy the same host three times and look entirely green doing it) + +// fleetHostAddress is machine i's address in the fixture's parent stack. Distinct per +// host so that a deploy which read the wrong index cannot pass. +func fleetHostAddress(i int) string { + return fmt.Sprintf("10.20.30.%d", 40+i) +} + +// withHosts grows the parent stack's vm resource to n machines. +// +// Host 0 is left exactly as newComposeMocks wrote it, which is the point: the +// single-host fixture and the fleet fixture agree about machine 0 down to the address, +// so a name that differs between the two runs is a real difference and not an artefact +// of the fixture. +func (f *composeFixture) withHosts(n int) *composeFixture { + f.mocks.outputs[resource.PropertyKey(ToHostCountExport(testVmResource))] = resource.NewNumberProperty(float64(n)) + for i := 1; i < n; i++ { + f.mocks.outputs[resource.PropertyKey(ToHostAddressExport(testVmResource, i))] = resource.NewStringProperty(fleetHostAddress(i)) + f.mocks.outputs[resource.PropertyKey(ToHostUserExport(testVmResource, i))] = resource.NewStringProperty(vm.DefaultSshUser) + f.mocks.outputs[resource.PropertyKey(ToHostPortExport(testVmResource, i))] = resource.NewNumberProperty(22) + f.mocks.outputs[resource.PropertyKey(ToHostPrivateKeyExport(testVmResource, i))] = resource.MakeSecret(resource.NewStringProperty(testPrivateKey)) + f.mocks.outputs[resource.PropertyKey(ToHostPrivateKeyPasswordExport(testVmResource, i))] = resource.MakeSecret(resource.NewStringProperty("")) + f.mocks.outputs[resource.PropertyKey(ToHostKeyExport(testVmResource, i))] = resource.NewStringProperty(testHostKeyValue) + } + return f +} + +// resourceNames returns the logical names of every recorded resource of one type, with +// ToResName's environment suffix removed. Unlike commands()/localCommands() it keeps +// the whole name rather than the step, because what these tests assert about IS the +// name. +func (m *composeMocks) resourceNames(token string) []string { + m.mu.Lock() + defer m.mu.Unlock() + var out []string + for _, a := range m.created { + if a.TypeToken != token { + continue + } + out = append(out, strings.TrimSuffix(a.Name, "--"+testEnv)) + } + return out +} + +// connectionHost is the machine a remote command was pointed at. +func (m *composeMocks) connectionHost(step string) string { + m.mu.Lock() + defer m.mu.Unlock() + for _, a := range m.created { + if a.TypeToken != tokenRemoteCommand || !strings.Contains(a.Name, "-vmc-"+step+"--") { + continue + } + // The whole `connection` arrives secret-wrapped — it carries the deploy key, + // and Pulumi marks the containing object and not only the leaf — so it has to + // be peeled at both levels. A helper that only looked at the object would read + // every connection as absent and make these assertions vacuous. + conn, _ := unwrapSecret(a.Inputs["connection"]) + if !conn.IsObject() { + return "" + } + host, _ := unwrapSecret(conn.ObjectValue()["host"]) + if !host.IsString() { + return "" + } + return host.StringValue() + } + return "" +} + +// The naming contract, stated as code: host 0 is un-suffixed so that an existing +// single-host stack sees no rename, and every later host carries its index. A rename +// here is not cosmetic — a renamed remote.Command is a NEW resource whose Create runs +// and whose predecessor's Delete tears down the containers it just started. +func TestHostResSuffix_HostZeroIsUnsuffixed(t *testing.T) { + RegisterTestingT(t) + + Expect(hostResSuffix(0)).To(BeEmpty()) + Expect(hostResSuffix(1)).To(Equal("-1")) + Expect(hostResSuffix(2)).To(Equal("-2")) + Expect(hostResSuffix(11)).To(Equal("-11")) +} + +func TestVmCompose_FleetDeploysOntoEveryHostAndLeavesHostZerosNamesAlone(t *testing.T) { + RegisterTestingT(t) + + single := newComposeFixture() + _, err := single.run() + Expect(err).ToNot(HaveOccurred()) + singleSteps := lo.Keys(single.mocks.commands()) + Expect(singleSteps).ToNot(BeEmpty()) + + fleet := newComposeFixture().withHosts(3) + out, err := fleet.run() + Expect(err).ToNot(HaveOccurred()) + Expect(out.Hosts).To(HaveLen(3)) + + steps := fleet.mocks.commands() + // Every step the single-host deploy ran is present under the identical name. This + // is the assertion that protects a running stack from being torn down by the + // upgrade that adds its second machine. + for _, step := range singleSteps { + Expect(steps).To(HaveKey(step), "host 0's %q was renamed by growing the fleet", step) + } + // And the other two machines added their own, suffixed. + for _, step := range []string{"preflight-1", "prepare-1", "up-1", "finalize-1", "up-2", "finalize-2"} { + Expect(steps).To(HaveKey(step)) + } + Expect(steps).ToNot(HaveKey("up-3")) + + // Three machines and not the same machine three times. + Expect(fleet.mocks.connectionHost("up")).To(Equal(testHostAddress)) + Expect(fleet.mocks.connectionHost("up-1")).To(Equal(fleetHostAddress(1))) + Expect(fleet.mocks.connectionHost("up-2")).To(Equal(fleetHostAddress(2))) + + // Same thing read off the returned output, which is what a caller sees: Host/Slot/ + // URL stay host 0's, and Hosts carries the fleet. + Expect(out.Host.Name).To(Equal(testVmResource + "-host-0")) + Expect(lo.Map(out.Hosts, func(h VmComposeHostOutput, _ int) string { return h.Host.Name })).To(Equal( + []string{testVmResource + "-host-0", testVmResource + "-host-1", testVmResource + "-host-2"}, + )) + for _, h := range out.Hosts { + Expect(h.Host.Role).To(Equal(vm.RoleCompose)) + } +} + +// The roll. A fleet exists so that a bad release does not take every replica with it, +// and Pulumi would happily deploy all three machines at once — the dependency edge is +// the only thing that makes the deploy stop at the machine that failed. +// +// The edge is on the previous host's `finalize` rather than its `up`: between those two +// a machine is serving a slot its state file does not yet name, and moving on from +// there would leave the fleet in that state if the next machine failed. +func TestVmCompose_FleetRollsOneMachineAtATime(t *testing.T) { + RegisterTestingT(t) + + f := newComposeFixture().withHosts(3) + _, err := f.run() + Expect(err).ToNot(HaveOccurred()) + + // Host 1's first step — `preflight`, the head of each machine's chain — waits for + // host 0 to have finished, and host 2's for host 1. + Expect(f.mocks.dependsOn("preflight-1")).To(ContainElement( + ContainSubstring("-vmc-finalize--" + testEnv), + )) + Expect(f.mocks.dependsOn("preflight-2")).To(ContainElement( + ContainSubstring("-vmc-finalize-1--" + testEnv), + )) + // And host 0 waits for nothing: it is the start of the roll, not the end of one. + Expect(f.mocks.dependsOn("preflight")).ToNot(ContainElement(ContainSubstring("-vmc-finalize"))) +} + +// An image is a property of the code and a ship is a property of the machine, so a +// fleet shares the first and not the second. Registering the build per host would be N +// identical docker builds under N resource names — Pulumi would reject the duplicates, +// and if it did not, the deploy would pay for the same build three times. +func TestVmCompose_FleetBuildsOnceAndShipsToEveryMachine(t *testing.T) { + RegisterTestingT(t) + + f := newComposeFixture().withSshTarBuild().withHosts(3) + _, err := f.run() + Expect(err).ToNot(HaveOccurred()) + + Expect(f.mocks.resourceNames(tokenDockerImage)).To(HaveLen(1), + "one build for the fleet, not one per machine") + + ships := f.mocks.resourceNames(tokenLocalCommand) + Expect(ships).To(ContainElement(testComposeStack + "-app-ship")) + Expect(ships).To(ContainElement(testComposeStack + "-app-ship-1")) + Expect(ships).To(ContainElement(testComposeStack + "-app-ship-2")) + + // Each machine's containers wait for the image to have reached THAT machine. An + // `up` ordered behind another host's ship would start containers against an image + // the local docker does not have. + Expect(f.mocks.dependsOn("up-2")).To(ContainElement(ContainSubstring("-app-ship-2--" + testEnv))) +} diff --git a/pkg/clouds/pulumi/vm/compose_images.go b/pkg/clouds/pulumi/vm/compose_images.go index a312a84b8..8ae44ebf4 100644 --- a/pkg/clouds/pulumi/vm/compose_images.go +++ b/pkg/clouds/pulumi/vm/compose_images.go @@ -72,6 +72,21 @@ type imagePlan struct { // deploy can say which images it put there — under `externalImages: ship` the // machine never pulls them, so they appear in no pull output. ExternalRefs []string + // builds are the images SC built here under `ssh-tar` delivery, in declaration + // order. They are built ONCE per deploy and shipped once per HOST, which is the + // whole reason they are recorded rather than shipped where they are built: an + // image is a property of the code, a ship is a property of the machine, and a + // fleet shares the first and not the second. Empty under `registry` delivery, + // where the push IS the delivery and every machine pulls for itself. + builds []localBuild +} + +// localBuild is an ssh-tar image after it has been built on the runner and before it +// has been sent to any machine. +type localBuild struct { + Service string + LocalRef string + Image *docker.Image } // triggerInputs is ContentIDs as Pulumi inputs, for appending to a Triggers array. @@ -79,7 +94,7 @@ func (p *imagePlan) triggerInputs() []sdk.Input { return lo.Map(p.ContentIDs, func(id sdk.StringOutput, _ int) sdk.Input { return id }) } -// buildImages builds every service's image and makes it reachable from the machine. +// buildImages builds every service's image and makes it reachable from the machines. // // Two delivery modes, and the difference is one decision made once here rather than // threaded through the rest of the deploy: with `registry` the image is pushed and @@ -89,13 +104,17 @@ func (p *imagePlan) triggerInputs() []sdk.Input { // situation of every operator who has a VM and a docker install and nothing else, // and of anyone inside Russia for whom reaching a registry is itself the open // question (compose-on-vm.md §5, D14). +// +// It takes no host: the build is the same whichever machines the stack lands on, and +// registering it once per host would be N identical docker builds under N resource +// names. The ssh-tar ships are left to shipImagesTo, which the caller runs once per +// machine in the fleet. func buildImages( ctx *sdk.Context, stack api.Stack, input api.ResourceInput, params pApi.ProvisionParams, in *vm.VmComposeInput, - host HostFacts, ) (*imagePlan, error) { version := lo.If(input.StackParams.Version != "", input.StackParams.Version).Else("latest") if !dockerTagRe.MatchString(version) { @@ -226,13 +245,9 @@ func buildImages( return nil, errors.Wrapf(err, "failed to build image for service %q of stack %q", container.Name, stack.Name) } - ship, err := shipImage(ctx, input, params, stack, host, container.Name, localRef, version, built) - if err != nil { - return nil, err - } - // The ship, not the build, is what the compose up has to wait for: the image - // existing on the runner tells the machine nothing. - plan.Opts = append(plan.Opts, sdk.DependsOn([]sdk.Resource{ship})) + // Recorded, not shipped: which machines this image goes to is the caller's + // decision, taken once per host in shipImagesTo. + plan.builds = append(plan.builds, localBuild{Service: container.Name, LocalRef: localRef, Image: built}) plan.Refs[container.Name] = sdk.String(localRef).ToStringOutput() // RepoDigest and not ImageName: under SkipPush there is no registry and no // manifest digest, and pulumi-docker documents the field as carrying the @@ -242,22 +257,50 @@ func buildImages( repos[repo] = true } + plan.ExternalRefs = lo.Uniq(plan.ExternalRefs) + sort.Strings(plan.ExternalRefs) + plan.PruneRepos = lo.Keys(repos) + sort.Strings(plan.PruneRepos) + return plan, nil +} + +// shipImagesTo sends everything this deploy built to ONE machine of the fleet. +// +// Returns the resource options the commands that run containers on that machine must +// carry. The ship, and not the build, is what a `docker compose up` has to wait for: +// an image sitting on the runner tells the machine nothing. +// +// Nothing to do under `registry` delivery — plan.builds is empty there, the push +// already happened, and plan.Opts (the scan/sign gates) covers every host at once. +func shipImagesTo( + ctx *sdk.Context, + input api.ResourceInput, + params pApi.ProvisionParams, + stack api.Stack, + plan *imagePlan, + version string, + host HostFacts, + idx int, +) ([]sdk.ResourceOption, error) { + var opts []sdk.ResourceOption + for _, b := range plan.builds { + ship, err := shipImage(ctx, input, params, stack, host, idx, b.Service, b.LocalRef, version, b.Image) + if err != nil { + return nil, err + } + opts = append(opts, sdk.DependsOn([]sdk.Resource{ship})) + } // One ship for every third-party image rather than one per service: `docker save` // deduplicates the layers they share, and a compose project's mongo/redis/nginx // usually share a base with each other. if len(plan.ExternalRefs) > 0 { - plan.ExternalRefs = lo.Uniq(plan.ExternalRefs) - sort.Strings(plan.ExternalRefs) - ship, err := shipExternalImages(ctx, input, params, stack, host, plan.ExternalRefs) + ship, err := shipExternalImages(ctx, input, params, stack, host, idx, plan.ExternalRefs) if err != nil { return nil, err } - plan.Opts = append(plan.Opts, sdk.DependsOn([]sdk.Resource{ship})) + opts = append(opts, sdk.DependsOn([]sdk.Resource{ship})) } - - plan.PruneRepos = lo.Keys(repos) - sort.Strings(plan.PruneRepos) - return plan, nil + return opts, nil } // shipExternalImages streams the images SC did not build to the machine. @@ -276,6 +319,7 @@ func shipExternalImages( params pApi.ProvisionParams, stack api.Stack, host HostFacts, + idx int, refs []string, ) (*local.Command, error) { for _, ref := range refs { @@ -305,7 +349,8 @@ func shipExternalImages( }) }).(sdk.StringOutput) - cmd, err := local.NewCommand(ctx, input.ToResName(fmt.Sprintf("%s-external-ship", stack.Name)), &local.CommandArgs{ + resName := fmt.Sprintf("%s-external-ship%s", stack.Name, hostResSuffix(idx)) + cmd, err := local.NewCommand(ctx, input.ToResName(resName), &local.CommandArgs{ Create: script, Update: script, Triggers: sdk.Array{ @@ -341,6 +386,7 @@ func shipImage( params pApi.ProvisionParams, stack api.Stack, host HostFacts, + idx int, service, localRef, version string, built *docker.Image, ) (*local.Command, error) { @@ -367,7 +413,8 @@ func shipImage( }) }).(sdk.StringOutput) - cmd, err := local.NewCommand(ctx, input.ToResName(fmt.Sprintf("%s-%s-ship", stack.Name, service)), &local.CommandArgs{ + resName := fmt.Sprintf("%s-%s-ship%s", stack.Name, service, hostResSuffix(idx)) + cmd, err := local.NewCommand(ctx, input.ToResName(resName), &local.CommandArgs{ Create: script, // Both set to the same script: without Update, pulumi-command treats an // input change as a replacement and runs Create after Delete, and there is diff --git a/pkg/clouds/pulumi/vm/compose_stack.go b/pkg/clouds/pulumi/vm/compose_stack.go index 8ec997da5..91d53a548 100644 --- a/pkg/clouds/pulumi/vm/compose_stack.go +++ b/pkg/clouds/pulumi/vm/compose_stack.go @@ -25,15 +25,52 @@ import ( // VmComposeOutput is what a vm-compose deploy hands back. type VmComposeOutput struct { - Host HostFacts + // Hosts is every machine of the fleet the stack was deployed onto, in fleet + // order. Always at least one entry. + Hosts []VmComposeHostOutput Images *imagePlan - // Slot is the slot this deploy brought up. An Output because it is decided from - // state read off the machine. + + // Host, Slot, URL and Up are the FIRST host's, and are what the single-machine + // path returned before a vm resource could hold more than one. Kept because a + // fleet of one is still the ordinary case; anything that has to account for + // every machine must read Hosts. + Host HostFacts + Slot sdk.StringOutput + URL sdk.StringOutput + Up *remote.Command +} + +// VmComposeHostOutput is one machine's share of a vm-compose deploy. +type VmComposeHostOutput struct { + Host HostFacts + // Slot is the slot this deploy brought up ON THIS MACHINE. An Output because it + // is decided from state read off the machine — and per machine, because each + // machine's blue-green history is its own: two hosts of one fleet can legitimately + // be serving from different slots, and forcing them into step would mean reading + // one machine's state and acting on another's. Slot sdk.StringOutput - // URL is where the stack answers after this deploy. In P5.2 that is always - // http://: — see the `domain` warning in VmComposeStack. + // URL is where this machine answers. The fleet's domain when Caddy fronts it (so + // the same string on every host), otherwise this machine's own address and the + // published host port. URL sdk.StringOutput Up *remote.Command + // last is the final resource of this host's chain. The next host is ordered after + // it, which is what makes a fleet deploy roll rather than fan out. + last sdk.Resource +} + +// hostResSuffix is what makes a per-host resource name unique WITHOUT renaming the +// resources of a fleet of one. +// +// Host 0 gets no suffix, so every stack already deployed onto a single-host vm +// resource keeps byte-identical resource names — and so its URNs, its checkpoint +// history and its running containers. Adding a second machine APPENDS resources +// instead of replacing the first machine's: a fleet is grown, not rebuilt. +func hostResSuffix(i int) string { + if i == 0 { + return "" + } + return fmt.Sprintf("-%d", i) } // VmComposeStack deploys one client stack onto one VM as docker compose projects. @@ -83,12 +120,12 @@ func VmComposeStack(ctx *sdk.Context, stack api.Stack, input api.ResourceInput, version := lo.If(input.StackParams.Version != "", input.StackParams.Version).Else("latest") stackConfig := in.Deployment.StackConfig - host, parentVm, err := hostFromParent(ctx, input, params, in.VmResource) + hosts, parentVm, err := hostsFromParent(ctx, input, params, in.VmResource) if err != nil { return nil, err } - plan, err := buildImages(ctx, stack, input, params, in, host) + plan, err := buildImages(ctx, stack, input, params, in) if err != nil { return nil, err } @@ -105,6 +142,33 @@ func VmComposeStack(ctx *sdk.Context, stack api.Stack, input api.ResourceInput, // run at once, and what the stack's URL is. servesDomain := vm.ServesDomain(in, parentVm.IngressEnabled) + // The scheme the machine answers on. Caddy either terminates TLS or serves plain + // HTTP for an edge that does (`ingress.originTls: false` on the vm resource) — and + // every log line and export below has to say which, because "http" in a URL an + // operator pastes into a browser is the only visible difference between a working + // no-origin-TLS fleet and a broken HTTPS one. + originScheme := lo.If(parentVm.NoOriginTls, "http").Else("https") + + // `domainProxied: true` asks for a Cloudflare-proxied record, and the proxy + // terminates TLS at the edge. That breaks the HTTP-01 challenge Caddy runs for + // itself — so it is legal only on a machine that runs no ACME. + // + // Refused here and not in validateStackConfig (clouds/vm/convert.go, which still + // carries the note) because the answer lives in the PARENT stack's resource and the + // convert tier holds no parent reference. The cost is a later refusal; the benefit + // is that the arrangement is reachable at all. + if lo.FromPtr(stackConfig.DomainProxied) && !parentVm.NoOriginTls { + return nil, errors.Errorf( + "stack %q sets `domainProxied: true`, but vm %q terminates TLS itself: Caddy obtains the "+ + "certificate over HTTP-01, and a proxied domain answers that challenge at Cloudflare's edge "+ + "instead of on the machine, so no certificate ever issues. Either set `ingress.originTls: "+ + "false` on the `type: %s` resource in the parent stack and re-provision it — the machine then "+ + "serves plain HTTP for Cloudflare to terminate in front — or drop `domainProxied` and let the "+ + "machine serve HTTPS directly", + stackName, in.VmResource, vm.ResourceTypeVm, + ) + } + // The ingress host port is published only when Caddy is NOT in front. That is not // a tidiness choice: a published port is a singleton, and it is the one thing that // prevents both slots from running at once (see stopOtherFirst below). Setting it @@ -116,9 +180,9 @@ func VmComposeStack(ctx *sdk.Context, stack api.Stack, input api.ResourceInput, "stack %q declares no ingress service, so nothing is reachable from outside the machine", stackName) case servesDomain: params.Log.Info(ctx.Context(), - "vm %q serves %q of stack %q on https://%s — no host port is published, and the deploy flips slots "+ + "vm %q serves %q of stack %q on %s://%s — no host port is published, and the deploy flips slots "+ "with a Caddy reload", - in.VmResource, in.Deployment.IngressContainer.Name, stackName, stackConfig.Domain) + in.VmResource, in.Deployment.IngressContainer.Name, stackName, originScheme, stackConfig.Domain) default: hostPort = vm.IngressHostPort(stackName, env) params.Log.Info(ctx.Context(), "publishing ingress service %q of stack %q on host port %d", @@ -177,6 +241,191 @@ func VmComposeStack(ctx *sdk.Context, stack api.Stack, input api.ResourceInput, in.VmResource, stackName, hostPort) } + // The env file is rendered ONCE for the fleet: every machine runs the same stack + // with the same resolved values, and a per-host render would be a second place for + // them to come from. + // env file. `extra` carries what the stack cannot know about itself: the base + // variables the cloud provider contributes, and the values resolved from + // `uses`/`dependencies`. Filtered the same way yandex/serverless_container.go + // filters them, so an explicit `env:` or `secrets:` in client.yaml still wins + // over a resolved one of the same name. + // + // SIMPLE_CONTAINER_VERSION and SIMPLE_CONTAINER_CLOUD are deliberately absent: + // pkg/clouds/vm/render.go puts them in each service's compose `environment:`, + // which takes precedence over `env_file`, and two sources for one variable is how + // they come to disagree. + extra := map[string]string{ + api.ComputeEnv.StackName: stackName, + api.ComputeEnv.StackEnv: env, + } + for name, value := range params.BaseEnvVariables { + extra[name] = value + } + for _, v := range params.ComputeContext.EnvVariables() { + if stackConfig.Env[v.Name] == "" { + extra[v.Name] = v.Value + } + } + for _, v := range params.ComputeContext.SecretEnvVariables() { + if stackConfig.Secrets[v.Name] == "" { + extra[v.Name] = v.Value + } + } + envContent, err := vm.RenderEnvFile(in, extra) + if err != nil { + return nil, errors.Wrapf(err, "failed to render the env file of stack %q", stackName) + } + // Hashed, never triggered on directly: a Trigger is stored in the checkpoint in + // the clear, and this content is every secret the stack has. + envHash := hashOf(envContent) + + // `scale` on this path is the fleet's size, and the fleet is declared in the + // PARENT stack — so this is the first place both numbers are in hand. Said out + // loud either way: a stack that asks for three replicas and gets one machine is a + // capacity surprise, and silence about it is what the "support it or refuse it" + // rule exists to prevent (compose-on-vm.md §2.2). + if s := stackConfig.Scale; s != nil && s.Max > 1 { + if s.Max == len(hosts) { + params.Log.Info(ctx.Context(), + "stack %q asks for %d replicas and vm %q has %d host(s): one replica of each service per machine", + stackName, s.Max, in.VmResource, len(hosts)) + } else { + params.Log.Warn(ctx.Context(), + "stack %q sets `scale.max: %d`, but vm %q has %d host(s) and this path runs one replica of each "+ + "service per machine — so the stack gets %d, not %d. Add hosts to the `type: %s` resource in "+ + "the parent stack, or move the stack to a k3s/Kubernetes parent, which is what scaling WITHIN "+ + "a machine needs", + stackName, s.Max, in.VmResource, len(hosts), len(hosts), s.Max, vm.ResourceTypeVm) + } + } + + if len(hosts) > 1 { + params.Log.Info(ctx.Context(), + "vm %q has %d hosts: stack %q is deployed onto every one of them, one machine at a time. Blue-green "+ + "is per machine — two machines' slots are independent, and a release that fails its health gate "+ + "stops at the machine it failed on with the rest of the fleet still serving the previous one", + in.VmResource, len(hosts), stackName) + } + + shared := composeDeploy{ + ctx: ctx, stack: stack, input: input, params: params, in: in, + stackName: stackName, env: env, version: version, stackConfig: stackConfig, + plan: plan, hostPort: hostPort, servesDomain: servesDomain, + noOriginTls: parentVm.NoOriginTls, + recreate: in.EffectiveStrategy() == vm.StrategyRecreate, + envHash: envHash, envContent: envContent, + } + + // Rolling, not fanned out. The machines are independent and Pulumi would happily + // deploy them at once — but a fleet exists so that a bad release does not take + // every replica with it, and a parallel deploy hands it all of them before the + // first health gate has answered. Ordering each host after the previous one's + // last command means a failure stops the deploy with the untouched machines still + // serving the version that worked. + deploys := make([]VmComposeHostOutput, 0, len(hosts)) + var after []sdk.Resource + for i, h := range hosts { + out, err := deployComposeToHost(shared, h, i, after) + if err != nil { + return nil, err + } + after = []sdk.Resource{out.last} + deploys = append(deploys, *out) + } + + // The name, pointed at every machine that now serves it. + // + // Only when Caddy fronts the stack: `servesDomain` false means the stack answers + // on a host port and an A record at `domain` would resolve to a machine with + // nothing listening on 443. The warnings above have already said why in that case. + // + // `DomainProxied` is read rather than hardcoded because it is now reachable. The + // refusal moved into this function (above) and allows it on a fleet that declares + // `ingress.originTls: false`, where proxying is not merely permitted but the entire + // front door: Cloudflare terminates TLS and the machines answer plain HTTP, so an + // unproxied record would hand clients a cleartext origin. On a fleet that terminates + // its own TLS the refusal has already returned by here, so this still reads false + // there — a proxied domain answers the HTTP-01 challenge at the edge instead of on + // the machine and no certificate ever issues. + if servesDomain && stackConfig.Domain != "" { + if err := pApi.FleetDnsRecords(ctx, params, stackName, stackConfig.Domain, + parentVm.Addresses, lo.FromPtr(stackConfig.DomainProxied)); err != nil { + return nil, err + } + } + + // Host 0's exports keep the names they have always had (hostResSuffix is empty at + // index 0), so nothing reading a single-host stack's outputs breaks; the rest are + // indexed, and the count is exported so a consumer can enumerate them without + // guessing how far to count. + for i, d := range deploys { + ctx.Export(toComposeSlotExport(stackName)+hostResSuffix(i), d.Slot) + ctx.Export(toComposeURLExport(stackName)+hostResSuffix(i), d.URL) + } + ctx.Export(toComposeHostCountExport(stackName), sdk.Int(len(deploys))) + + return &api.ResourceOutput{Ref: &VmComposeOutput{ + Hosts: deploys, + Images: plan, + Host: deploys[0].Host, + Slot: deploys[0].Slot, + URL: deploys[0].URL, + Up: deploys[0].Up, + }}, nil +} + +// composeDeploy is everything a per-host deploy needs that is NOT about the host. +// +// A struct because the alternative is a seventeen-parameter function whose arguments +// are all strings and bools, and because the split is the point: what is in here is +// decided ONCE for the whole fleet (the images, the rendered env file, whether Caddy +// fronts the stack, which host port is published), and what is passed alongside it is +// the machine. A field that turned out to be per-host would have to move out of here, +// which is exactly the review question worth having. +type composeDeploy struct { + ctx *sdk.Context + stack api.Stack + input api.ResourceInput + params pApi.ProvisionParams + in *vm.VmComposeInput + stackName string + env string + version string + stackConfig *api.StackConfigCompose + plan *imagePlan + hostPort int + servesDomain bool + // noOriginTls is parentVmFacts.NoOriginTls, carried down because the site file and + // the exported URL are both written per host and both have to name the scheme the + // RESOURCE decided. + noOriginTls bool + recreate bool + envHash string + envContent string +} + +// deployComposeToHost runs the whole deploy against ONE machine of the fleet. +// +// `after` orders this host behind the previous one's last command; it is empty for +// the first host. The returned output carries that last command so the next host can +// be ordered behind this one. +func deployComposeToHost( + d composeDeploy, host HostFacts, idx int, after []sdk.Resource, +) (*VmComposeHostOutput, error) { + ctx, stack, input, params, in := d.ctx, d.stack, d.input, d.params, d.in + stackName, env, version, stackConfig := d.stackName, d.env, d.version, d.stackConfig + plan, hostPort, servesDomain, recreate := d.plan, d.hostPort, d.servesDomain, d.recreate + envHash, envContent := d.envHash, d.envContent + // The suffix every resource name on this host carries. Empty at index 0. + sfx := hostResSuffix(idx) + + // Where this deploy's images come from, for THIS machine: the build happened once + // for the fleet, the ship happens here. + shipOpts, err := shipImagesTo(ctx, input, params, stack, plan, version, host, idx) + if err != nil { + return nil, err + } + conn := ConnectionArgs(host) // Shared by every command: Pulumi's own default is to log a remote command's // output, and AddPreviousOutputInEnv would put the last run's stdout into this @@ -191,7 +440,10 @@ func VmComposeStack(ctx *sdk.Context, stack api.Stack, input api.ResourceInput, // Sequenced rather than fanned out. Three commands over one SSH connection // writing into one directory is not worth the concurrency, and a linear chain is // what makes the failure message name the step that failed. - var prev []sdk.Resource + // + // The chain starts at `after`, which is the previous machine's last command — so + // the same linearity that orders the steps within a host also orders the hosts. + prev := after step := func(name, script string, stdin sdk.StringPtrInput, quiet bool, extraTriggers ...sdk.Input) (*remote.Command, error) { triggers := sdk.Array{host.Address, sdk.String(hashOf(script))} for _, t := range extraTriggers { @@ -211,10 +463,10 @@ func VmComposeStack(ctx *sdk.Context, stack api.Stack, input api.ResourceInput, if quiet { args.Logging = remote.LoggingNone.ToLoggingPtrOutput() } - cmd, err := remote.NewCommand(ctx, input.ToResName(fmt.Sprintf("%s-vmc-%s", stackName, name)), args, + cmd, err := remote.NewCommand(ctx, input.ToResName(fmt.Sprintf("%s-vmc-%s%s", stackName, name, sfx)), args, sdk.DependsOn(prev)) if err != nil { - return nil, errors.Wrapf(err, "failed to create %q command for stack %q on vm %q", name, stackName, in.VmResource) + return nil, errors.Wrapf(err, "failed to create %q command for stack %q on host %q", name, stackName, host.Name) } prev = []sdk.Resource{cmd} return cmd, nil @@ -247,40 +499,6 @@ func VmComposeStack(ctx *sdk.Context, stack api.Stack, input api.ResourceInput, return nil, err } - // env file. `extra` carries what the stack cannot know about itself: the base - // variables the cloud provider contributes, and the values resolved from - // `uses`/`dependencies`. Filtered the same way yandex/serverless_container.go - // filters them, so an explicit `env:` or `secrets:` in client.yaml still wins - // over a resolved one of the same name. - // - // SIMPLE_CONTAINER_VERSION and SIMPLE_CONTAINER_CLOUD are deliberately absent: - // pkg/clouds/vm/render.go puts them in each service's compose `environment:`, - // which takes precedence over `env_file`, and two sources for one variable is how - // they come to disagree. - extra := map[string]string{ - api.ComputeEnv.StackName: stackName, - api.ComputeEnv.StackEnv: env, - } - for name, value := range params.BaseEnvVariables { - extra[name] = value - } - for _, v := range params.ComputeContext.EnvVariables() { - if stackConfig.Env[v.Name] == "" { - extra[v.Name] = v.Value - } - } - for _, v := range params.ComputeContext.SecretEnvVariables() { - if stackConfig.Secrets[v.Name] == "" { - extra[v.Name] = v.Value - } - } - envContent, err := vm.RenderEnvFile(in, extra) - if err != nil { - return nil, errors.Wrapf(err, "failed to render the env file of stack %q", stackName) - } - // Hashed, never triggered on directly: a Trigger is stored in the checkpoint in - // the clear, and this content is every secret the stack has. - envHash := hashOf(envContent) if _, err := step( "write-env", writeFileScript(vm.EnvFilePath(stackName, env), vm.EnvFileMode), @@ -347,7 +565,6 @@ func VmComposeStack(ctx *sdk.Context, stack api.Stack, input api.ResourceInput, return nil, err } - recreate := in.EffectiveStrategy() == vm.StrategyRecreate slotOut := readState.Stdout.ApplyT(func(out string) string { active := strings.TrimSpace(out) if recreate { @@ -409,7 +626,7 @@ func VmComposeStack(ctx *sdk.Context, stack api.Stack, input api.ResourceInput, }), nil }).(sdk.StringOutput) - up, err := remote.NewCommand(ctx, input.ToResName(stackName+"-vmc-up"), baseArgs(&remote.CommandArgs{ + up, err := remote.NewCommand(ctx, input.ToResName(stackName+"-vmc-up"+sfx), baseArgs(&remote.CommandArgs{ Create: upScriptOut, Update: upScriptOut, // The script itself is the trigger, which covers everything that can change @@ -425,10 +642,11 @@ func VmComposeStack(ctx *sdk.Context, stack api.Stack, input api.ResourceInput, // see the Triggers of the ship command in compose_images.go. Triggers: append( sdk.Array{host.Address, sdk.String(version), upScriptOut, sdk.String(envHash)}, - plan.triggerInputs()...), - }), append(plan.Opts, sdk.DependsOn(prev))...) + plan.triggerInputs()..., + ), + }), append(append(plan.Opts, shipOpts...), sdk.DependsOn(prev))...) if err != nil { - return nil, errors.Wrapf(err, "failed to bring stack %q up on vm %q", stackName, in.VmResource) + return nil, errors.Wrapf(err, "failed to bring stack %q up on host %q", stackName, host.Name) } // The flip. Between `up` (both slots running, old one serving) and `finalize` @@ -439,10 +657,11 @@ func VmComposeStack(ctx *sdk.Context, stack api.Stack, input api.ResourceInput, if servesDomain { routeOut := slotOut.ApplyT(func(slot string) (string, error) { site, err := vm.RenderCaddySite(in, vm.RenderOptions{ - Stack: stackName, - Env: env, - Slot: slot, - Version: version, + Stack: stackName, + Env: env, + Slot: slot, + Version: version, + NoOriginTls: d.noOriginTls, }) if err != nil { return "", errors.Wrapf(err, "failed to render the Caddy site of stack %q slot %q", stackName, slot) @@ -456,7 +675,7 @@ func VmComposeStack(ctx *sdk.Context, stack api.Stack, input api.ResourceInput, }), nil }).(sdk.StringOutput) - route, err := remote.NewCommand(ctx, input.ToResName(stackName+"-vmc-route"), baseArgs(&remote.CommandArgs{ + route, err := remote.NewCommand(ctx, input.ToResName(stackName+"-vmc-route"+sfx), baseArgs(&remote.CommandArgs{ Create: routeOut, Update: routeOut, // The script carries the rendered site file, so it changes whenever the @@ -482,8 +701,8 @@ func VmComposeStack(ctx *sdk.Context, stack api.Stack, input api.ResourceInput, Triggers: TriggersForGuardedChange(host.Address, routeOut), }), sdk.DependsOn([]sdk.Resource{up})) if err != nil { - return nil, errors.Wrapf(err, "failed to route %q to stack %q on vm %q", - stackConfig.Domain, stackName, in.VmResource) + return nil, errors.Wrapf(err, "failed to route %q to stack %q on host %q", + stackConfig.Domain, stackName, host.Name) } routeDeps = []sdk.Resource{route} } @@ -504,7 +723,7 @@ func VmComposeStack(ctx *sdk.Context, stack api.Stack, input api.ResourceInput, }) }).(sdk.StringOutput) - if _, err := remote.NewCommand(ctx, input.ToResName(stackName+"-vmc-finalize"), baseArgs(&remote.CommandArgs{ + finalize, err := remote.NewCommand(ctx, input.ToResName(stackName+"-vmc-finalize"+sfx), baseArgs(&remote.CommandArgs{ Create: finalizeOut, Update: finalizeOut, // The content ids for the same reason as on `up`, and it matters more here: @@ -513,9 +732,11 @@ func VmComposeStack(ctx *sdk.Context, stack api.Stack, input api.ResourceInput, // slot with the state file still naming the old one. Triggers: append( sdk.Array{host.Address, sdk.String(version), finalizeOut}, - plan.triggerInputs()...), - }), sdk.DependsOn(routeDeps)); err != nil { - return nil, errors.Wrapf(err, "failed to finalize deploy of stack %q on vm %q", stackName, in.VmResource) + plan.triggerInputs()..., + ), + }), sdk.DependsOn(routeDeps)) + if err != nil { + return nil, errors.Wrapf(err, "failed to finalize deploy of stack %q on host %q", stackName, host.Name) } // Cleanup lives on its own resource whose Create does nothing, and deliberately @@ -524,31 +745,40 @@ func VmComposeStack(ctx *sdk.Context, stack api.Stack, input api.ResourceInput, // input change that Pulumi chose to treat as a replace would tear the stack down // moments after bringing it up. A resource with no triggers is never replaced, // so its Delete runs exactly when the stack is destroyed. - if _, err := remote.NewCommand(ctx, input.ToResName(stackName+"-vmc-teardown"), baseArgs(&remote.CommandArgs{ + if _, err := remote.NewCommand(ctx, input.ToResName(stackName+"-vmc-teardown"+sfx), baseArgs(&remote.CommandArgs{ Create: sdk.String("true"), Delete: sdk.String(teardownScript(stackName, env)), })); err != nil { - return nil, errors.Wrapf(err, "failed to register teardown of stack %q on vm %q", stackName, in.VmResource) + return nil, errors.Wrapf(err, "failed to register teardown of stack %q on host %q", stackName, host.Name) } // The URL an operator (and `sc deploy`'s own summary) should use. Not derived from // whether a domain is configured but from whether it is actually SERVED — a stack // whose domain was ignored for one of the reasons warned about above must not // export an https:// URL that answers nothing. + // + // The scheme follows the resource, not the hope: on `ingress.originTls: false` the + // machine answers on :80 and only the edge in front speaks HTTPS, so exporting + // `https://` here would hand an operator a URL that fails against the origin and + // works only once Cloudflare is actually in place. url := sdk.Sprintf("http://%s:%d", host.Address, hostPort) if servesDomain { - url = sdk.String("https://" + stackConfig.Domain).ToStringOutput() + scheme := lo.If(d.noOriginTls, "http").Else("https") + url = sdk.String(scheme + "://" + stackConfig.Domain).ToStringOutput() } - ctx.Export(toComposeSlotExport(stackName), slotOut) - ctx.Export(toComposeURLExport(stackName), url) - - return &api.ResourceOutput{Ref: &VmComposeOutput{ - Host: host, - Images: plan, - Slot: slotOut, - URL: url, - Up: up, - }}, nil + // Exported by the caller, which is the only place that knows this host's index + // in the fleet. + return &VmComposeHostOutput{ + Host: host, + Slot: slotOut, + URL: url, + Up: up, + // finalize and not `up`: the next machine must not start until this one has + // recorded its active slot and retired the old one, or a failure between the + // two would leave a machine serving a slot its state file does not name while + // the roll moves on. + last: finalize, + }, nil } func toComposeSlotExport(stackName string) string { @@ -559,6 +789,15 @@ func toComposeURLExport(stackName string) string { return fmt.Sprintf("%s-vm-compose-url", stackName) } +// toComposeHostCountExport is how many machines this stack was deployed onto. +// +// Exported so that a consumer of the slot/URL exports can enumerate them instead of +// probing `-1`, `-2`, … until one is missing — the same reason the vm resource itself +// exports a host count (ToHostCountExport). +func toComposeHostCountExport(stackName string) string { + return fmt.Sprintf("%s-vm-compose-host-count", stackName) +} + // parentVmFacts is what a child stack learns about the MACHINE (as opposed to the // host it connects to) from the parent stack's outputs. // @@ -574,35 +813,98 @@ type parentVmFacts struct { // filtering SC does not manage, or a parent stack provisioned before the // export existed. OpenPorts []vm.PortRange + // NoOriginTls is `ingress.originTls: false` on the resource: Caddy serves plain + // HTTP and runs no ACME. False on a parent stack that has never heard of the + // setting, which is the old behaviour and the reason the export carries this sense + // rather than its opposite. + NoOriginTls bool + // Addresses is every machine's address, in host order — the same strings the + // HostFacts carry, kept as literals because a DNS record's value has to be one. + // HostFacts lifts the address into an Output for the driver side, where a cloud + // VM's address genuinely is not known until the resource exists; on this side it + // came out of the parent stack's exports already resolved, and the record + // producer needs it that way (see pApi.FleetDnsRecords). + Addresses []string } -// hostFromParent reads the facts of the machine out of the parent stack's outputs. +// hostsFromParent reads the facts of every machine of a vm resource out of the +// parent stack's outputs. // -// The machine is described in the parent stack and deployed onto from the child, so -// this is the one seam where the two meet. It returns a HostFacts identical in shape -// to the one the driver produced, which is what lets connectionArgs and shipScript be -// the same code on both sides, plus the two facts about the MACHINE that this stack's -// deploy has to agree with: whether it declared `ingress.enable`, and which ports SC -// opened in its firewall. -func hostFromParent( +// The machines are described in the parent stack and deployed onto from the child, so +// this is the one seam where the two meet. Each HostFacts comes back identical in +// shape to the one the driver produced, which is what lets ConnectionArgs and +// shipScript be the same code on both sides. Alongside them come the two facts about +// the RESOURCE that this stack's deploy has to agree with: whether it declared +// `ingress.enable`, and which ports SC opened in its firewall. Both are per-resource +// rather than per-host — SC runs the same ingress and opens the same ports on every +// machine of one vm resource — which is why they are read once and not in the loop. +func hostsFromParent( ctx *sdk.Context, input api.ResourceInput, params pApi.ProvisionParams, vmResource string, -) (HostFacts, parentVmFacts, error) { - parentRef := params.ParentStack.FullReference - suffix := lo.If(params.ParentStack.DependsOnResource != nil, - "--"+lo.FromPtr(params.ParentStack.DependsOnResource).Name).Else("") - // Every GetValueFromStack call creates its own sdk.StackReference, and two - // references with the same name in one stack is an error — so the field name is - // part of it. - refName := func(field string) string { - return fmt.Sprintf("%s%s-vmc-%s", vmResource, suffix, field) - } - - countRaw, err := pApi.GetValueFromStack[any](ctx, refName("host-count"), parentRef, ToHostCountExport(vmResource), false) +) ([]HostFacts, parentVmFacts, error) { + r := parentReader{ + ctx: ctx, + params: params, + vmResource: vmResource, + parentRef: params.ParentStack.FullReference, + suffix: lo.If(params.ParentStack.DependsOnResource != nil, + "--"+lo.FromPtr(params.ParentStack.DependsOnResource).Name).Else(""), + } + + count, err := r.hostCount() + if err != nil { + return nil, parentVmFacts{}, err + } + facts, err := r.vmFacts() + if err != nil { + return nil, parentVmFacts{}, err + } + + hosts := make([]HostFacts, 0, count) + for i := 0; i < count; i++ { + host, address, err := r.host(input, i) + if err != nil { + return nil, parentVmFacts{}, err + } + hosts = append(hosts, host) + facts.Addresses = append(facts.Addresses, address) + } + return hosts, facts, nil +} + +// parentReader reads one vm resource's exports out of the parent stack. +// +// A type rather than a few closures because the reference NAMES have to be derived in +// exactly one place: every GetValueFromStack call creates its own sdk.StackReference, +// two references with the same name in one stack is an error, and the names now vary +// by host index as well as by field. +type parentReader struct { + ctx *sdk.Context + params pApi.ProvisionParams + vmResource string + parentRef string + suffix string +} + +// refName is the stack-reference name for one field of one host. +// +// Host 0 keeps the un-indexed name it has always had. A renamed StackReference would +// be harmless in itself — it reads, it owns nothing — but a stable name is what keeps +// a single-host stack's preview empty, and an empty preview is the only cheap way an +// operator can see that a release changed nothing. +func (r parentReader) refName(i int, field string) string { + return fmt.Sprintf("%s%s-vmc%s-%s", r.vmResource, r.suffix, hostResSuffix(i), field) +} + +// hostCount is how many machines the vm resource has, and the check that it exists at +// all. +func (r parentReader) hostCount() (int, error) { + countRaw, err := pApi.GetValueFromStack[any](r.ctx, r.refName(0, "host-count"), r.parentRef, + ToHostCountExport(r.vmResource), false) if err != nil { - return HostFacts{}, parentVmFacts{}, errors.Wrapf(err, "failed to read host count of vm %q from parent stack %q", vmResource, parentRef) + return 0, errors.Wrapf(err, "failed to read host count of vm %q from parent stack %q", r.vmResource, r.parentRef) } count, ok := toInt(countRaw) if !ok || count < 1 { @@ -610,74 +912,110 @@ func hostFromParent( // errors.Wrapf(nil, ...) is nil — so a missing resource arrives here as a // successful read of nothing, and this check is the only thing that turns it // into a message naming what is actually wrong. - return HostFacts{}, parentVmFacts{}, errors.Errorf( + return 0, errors.Errorf( "parent stack %q has no `type: %s` resource named %q (its host-count output is %v). "+ "Check `vmResource` in the `%s` template against the resource name in the parent's server.yaml", - parentRef, vm.ResourceTypeVm, vmResource, countRaw, vm.TemplateTypeVmCompose, + r.parentRef, vm.ResourceTypeVm, r.vmResource, countRaw, vm.TemplateTypeVmCompose, ) } - if count > 1 { - params.Log.Warn(ctx.Context(), - "vm %q has %d hosts; this slice deploys onto the first one only. Multi-host compose is not blue-green "+ - "across machines and is not supported yet", vmResource, count) + return count, nil +} + +// vmFacts reads what the parent decided for the resource as a whole. +func (r parentReader) vmFacts() (parentVmFacts, error) { + ingressRaw, err := pApi.GetValueFromStack[any](r.ctx, r.refName(0, "ingress"), r.parentRef, + ToIngressEnabledExport(r.vmResource), false) + if err != nil { + return parentVmFacts{}, errors.Wrapf(err, "failed to read the ingress setting of vm %q", r.vmResource) + } + // Read through toBool for the same reason the ints are: an sdk.Bool export comes + // back off a JSON checkpoint, and GetValueFromStack[bool] would panic rather than + // fail if the decoder handed it anything else. A machine whose ingress setting is + // unreadable is treated as not having one — this slice publishes a host port + // either way, and the only thing the flag changes here is the wording of a + // warning. + ingressEnabled, _ := toBool(ingressRaw) + + // What the parent actually opened. Read as a string (see vm.FormatPortSpec for + // why it crosses the boundary as one) and empty on an older parent stack or on a + // machine whose firewall SC does not manage — in both cases the caller must stay + // quiet rather than guess, since "I cannot see the firewall" and "the port is + // shut" are different statements and only one of them is actionable. + openPortsSpec, err := pApi.GetValueFromStack[string](r.ctx, r.refName(0, "open-ports"), r.parentRef, + ToOpenPortsExport(r.vmResource), false) + if err != nil { + return parentVmFacts{}, errors.Wrapf(err, "failed to read the open ports of vm %q", r.vmResource) } - address, err := pApi.GetValueFromStack[string](ctx, refName("address"), parentRef, ToHostAddressExport(vmResource, 0), false) + // Same read-through-toBool treatment, same tolerance: an unreadable or absent + // value means "the machine terminates TLS", which is what every parent stack + // provisioned before this export did. + noOriginTlsRaw, err := pApi.GetValueFromStack[any](r.ctx, r.refName(0, "no-origin-tls"), r.parentRef, + ToIngressNoOriginTlsExport(r.vmResource), false) if err != nil { - return HostFacts{}, parentVmFacts{}, errors.Wrapf(err, "failed to read the address of vm %q from parent stack %q", vmResource, parentRef) + return parentVmFacts{}, errors.Wrapf(err, "failed to read the origin-TLS setting of vm %q", r.vmResource) + } + noOriginTls, _ := toBool(noOriginTlsRaw) + + return parentVmFacts{ + IngressEnabled: ingressEnabled, + NoOriginTls: noOriginTls, + OpenPorts: vm.ParsePortSpec(openPortsSpec), + }, nil +} + +// host reads one machine of the fleet, returning its facts and — separately — its +// address as a literal, because HostFacts can only carry it as an Output and a DNS +// record's value cannot be one. +func (r parentReader) host(input api.ResourceInput, i int) (HostFacts, string, error) { + address, err := pApi.GetValueFromStack[string](r.ctx, r.refName(i, "address"), r.parentRef, + ToHostAddressExport(r.vmResource, i), false) + if err != nil { + return HostFacts{}, "", errors.Wrapf(err, "failed to read the address of host %d of vm %q from parent stack %q", + i, r.vmResource, r.parentRef) } if address == "" { - return HostFacts{}, parentVmFacts{}, errors.Errorf("vm %q in parent stack %q has an empty host address", vmResource, parentRef) + // Reachable on a fleet in a way it was not on a single machine: the host count + // says N, so the parent provisioned N machines, and an empty address at index i + // means its exports are incomplete rather than absent. + return HostFacts{}, "", errors.Errorf( + "host %d of vm %q in parent stack %q has an empty address; re-run `sc provision` for the parent stack", + i, r.vmResource, r.parentRef, + ) } - privateKey, err := pApi.GetValueFromStack[string](ctx, refName("key"), parentRef, ToHostPrivateKeyExport(vmResource, 0), true) + privateKey, err := pApi.GetValueFromStack[string](r.ctx, r.refName(i, "key"), r.parentRef, + ToHostPrivateKeyExport(r.vmResource, i), true) if err != nil { - return HostFacts{}, parentVmFacts{}, errors.Wrapf(err, "failed to read the deploy key of vm %q from parent stack %q", vmResource, parentRef) + return HostFacts{}, "", errors.Wrapf(err, "failed to read the deploy key of host %d of vm %q from parent stack %q", + i, r.vmResource, r.parentRef) } if privateKey == "" { - return HostFacts{}, parentVmFacts{}, errors.Errorf( - "vm %q in parent stack %q exported no deploy key; re-run `sc provision` for the parent stack", vmResource, parentRef, + return HostFacts{}, "", errors.Errorf( + "host %d of vm %q in parent stack %q exported no deploy key; re-run `sc provision` for the parent stack", + i, r.vmResource, r.parentRef, ) } - keyPassword, err := pApi.GetValueFromStack[string](ctx, refName("key-password"), parentRef, - ToHostPrivateKeyPasswordExport(vmResource, 0), true) + keyPassword, err := pApi.GetValueFromStack[string](r.ctx, r.refName(i, "key-password"), r.parentRef, + ToHostPrivateKeyPasswordExport(r.vmResource, i), true) if err != nil { - return HostFacts{}, parentVmFacts{}, errors.Wrapf(err, "failed to read the deploy key passphrase of vm %q", vmResource) + return HostFacts{}, "", errors.Wrapf(err, "failed to read the deploy key passphrase of host %d of vm %q", i, r.vmResource) } // Not an error when empty, and not read back through a check: an unencrypted key // is the normal case and has no passphrase. - user, err := pApi.GetValueFromStack[string](ctx, refName("user"), parentRef, ToHostUserExport(vmResource, 0), false) + user, err := pApi.GetValueFromStack[string](r.ctx, r.refName(i, "user"), r.parentRef, + ToHostUserExport(r.vmResource, i), false) if err != nil { - return HostFacts{}, parentVmFacts{}, errors.Wrapf(err, "failed to read the ssh user of vm %q", vmResource) + return HostFacts{}, "", errors.Wrapf(err, "failed to read the ssh user of host %d of vm %q", i, r.vmResource) } - portRaw, err := pApi.GetValueFromStack[any](ctx, refName("port"), parentRef, ToHostPortExport(vmResource, 0), false) + portRaw, err := pApi.GetValueFromStack[any](r.ctx, r.refName(i, "port"), r.parentRef, + ToHostPortExport(r.vmResource, i), false) if err != nil { - return HostFacts{}, parentVmFacts{}, errors.Wrapf(err, "failed to read the ssh port of vm %q", vmResource) + return HostFacts{}, "", errors.Wrapf(err, "failed to read the ssh port of host %d of vm %q", i, r.vmResource) } - hostKey, err := pApi.GetValueFromStack[string](ctx, refName("host-key"), parentRef, ToHostKeyExport(vmResource, 0), false) - if err != nil { - return HostFacts{}, parentVmFacts{}, errors.Wrapf(err, "failed to read the host key of vm %q", vmResource) - } - ingressRaw, err := pApi.GetValueFromStack[any](ctx, refName("ingress"), parentRef, ToIngressEnabledExport(vmResource), false) - if err != nil { - return HostFacts{}, parentVmFacts{}, errors.Wrapf(err, "failed to read the ingress setting of vm %q", vmResource) - } - // Read through toBool for the same reason the ints are: an sdk.Bool export comes - // back off a JSON checkpoint, and GetValueFromStack[bool] would panic rather than - // fail if the decoder handed it anything else. A machine whose ingress setting is - // unreadable is treated as not having one — this slice publishes a host port - // either way, and the only thing the flag changes here is the wording of a - // warning. - ingressEnabled, _ := toBool(ingressRaw) - - // What the parent actually opened. Read as a string (see vm.FormatPortSpec for - // why it crosses the boundary as one) and empty on an older parent stack or on a - // machine whose firewall SC does not manage — in both cases the caller must stay - // quiet rather than guess, since "I cannot see the firewall" and "the port is - // shut" are different statements and only one of them is actionable. - openPortsSpec, err := pApi.GetValueFromStack[string](ctx, refName("open-ports"), parentRef, - ToOpenPortsExport(vmResource), false) + hostKey, err := pApi.GetValueFromStack[string](r.ctx, r.refName(i, "host-key"), r.parentRef, + ToHostKeyExport(r.vmResource, i), false) if err != nil { - return HostFacts{}, parentVmFacts{}, errors.Wrapf(err, "failed to read the open ports of vm %q", vmResource) + return HostFacts{}, "", errors.Wrapf(err, "failed to read the host key of host %d of vm %q", i, r.vmResource) } port, ok := toInt(portRaw) @@ -688,14 +1026,14 @@ func hostFromParent( // The parent stack warns too, but this is the side that carries the env // file: an unverified connection here hands every secret of this stack to // whoever answers on that address. - params.Log.Warn(ctx.Context(), - "vm %q has no `hostKey`, so this deploy cannot verify the machine it is sending stack %q's env file "+ - "and deploy key to. Pin it in the parent stack with `ssh-keyscan -t ed25519 %s`", - vmResource, input.StackParams.StackName, address) + r.params.Log.Warn(r.ctx.Context(), + "host %d of vm %q has no `hostKey`, so this deploy cannot verify the machine it is sending stack %q's "+ + "env file and deploy key to. Pin it in the parent stack with `ssh-keyscan -t ed25519 %s`", + i, r.vmResource, input.StackParams.StackName, address) } return HostFacts{ - Name: fmt.Sprintf("%s-host-0", vmResource), + Name: fmt.Sprintf("%s-host-%d", r.vmResource, i), Address: sdk.String(address).ToStringOutput(), User: lo.If(user == "", vm.DefaultSshUser).Else(user), Port: port, @@ -708,7 +1046,7 @@ func hostFromParent( HostKey: sdk.String(hostKey).ToStringOutput(), HostKeyPinned: hostKey != "", Role: vm.RoleCompose, - }, parentVmFacts{IngressEnabled: ingressEnabled, OpenPorts: vm.ParsePortSpec(openPortsSpec)}, nil + }, address, nil } // hashOf is the stand-in for content in a Trigger. diff --git a/pkg/clouds/pulumi/vm/compose_stack_test.go b/pkg/clouds/pulumi/vm/compose_stack_test.go index e5f5bd846..e9a8712db 100644 --- a/pkg/clouds/pulumi/vm/compose_stack_test.go +++ b/pkg/clouds/pulumi/vm/compose_stack_test.go @@ -5,6 +5,7 @@ package vm import ( "context" + "encoding/base64" "fmt" "strings" "sync" @@ -69,13 +70,15 @@ func newComposeMocks() *composeMocks { resource.PropertyKey(ToHostAddressExport(testVmResource, 0)): resource.NewStringProperty(testHostAddress), resource.PropertyKey(ToHostUserExport(testVmResource, 0)): resource.NewStringProperty(vm.DefaultSshUser), // A number, not an int — this is the shape a stack reference hands back after - // a round trip through the checkpoint's JSON, and the reason hostFromParent + // a round trip through the checkpoint's JSON, and the reason parentReader.host // reads it as `any` and coerces. GetValueFromStack[int] would panic here. resource.PropertyKey(ToHostPortExport(testVmResource, 0)): resource.NewNumberProperty(22), resource.PropertyKey(ToHostPrivateKeyExport(testVmResource, 0)): resource.MakeSecret( - resource.NewStringProperty(testPrivateKey)), + resource.NewStringProperty(testPrivateKey), + ), resource.PropertyKey(ToHostPrivateKeyPasswordExport(testVmResource, 0)): resource.MakeSecret( - resource.NewStringProperty("")), + resource.NewStringProperty(""), + ), resource.PropertyKey(ToHostKeyExport(testVmResource, 0)): resource.NewStringProperty(testHostKeyValue), resource.PropertyKey(ToIngressEnabledExport(testVmResource)): resource.NewBoolProperty(false), }} @@ -235,6 +238,9 @@ func (f *composeFixture) withIngress() *composeFixture { type composeFixture struct { in *vm.VmComposeInput mocks *composeMocks + // Nil unless a test sets it, which is the production default too: a parent stack + // that declares no registrar gets no DNS record from SC. + registrar pApi.Registrar // Resolved inside the Pulumi run, because an Output can only be awaited while // the context that produced it is alive. url string @@ -250,6 +256,7 @@ func (f *composeFixture) run() (*VmComposeOutput, error) { err := pulumi.RunErr(func(ctx *pulumi.Context) error { params := pApi.ProvisionParams{ Log: logger.New(), + Registrar: f.registrar, ComputeContext: pApi.NewComputeContextCollector(context.Background(), logger.New(), testComposeStack, testEnv), ParentStack: &pApi.ParentInfo{StackName: "infra", FullReference: "organization/forge/infra--" + testEnv}, BaseEnvVariables: map[string]string{ @@ -715,3 +722,99 @@ func TestVmCompose_TheShipKeepsTheDeployKeyOffTheCommandLine(t *testing.T) { Expect(logging.StringValue()).To(Equal("none"), "the ship streams a customer image over a connection authenticated by the key on stdin") } + +// withNoOriginTls models a machine the operator provisioned with +// `ingress.originTls: false`: Caddy fronts the stacks but terminates no TLS, because +// something in front of it does. +func (f *composeFixture) withNoOriginTls() *composeFixture { + f.mocks.outputs[resource.PropertyKey(ToIngressNoOriginTlsExport(testVmResource))] = resource.NewBoolProperty(true) + return f +} + +// `domainProxied: true` used to be refused in the convert tier for every vm resource. +// It is now decided here, against the parent — and both outcomes matter, because the +// refusal is the thing standing between an operator and a fleet whose certificates +// never issue, while the allow is the only front door available on a cloud with no +// bucket. +func TestVmCompose_DomainProxiedNeedsNoOriginTls(t *testing.T) { + RegisterTestingT(t) + + // The machine terminates TLS: a proxied domain sends the HTTP-01 challenge to + // Cloudflare's edge, so no certificate ever issues. Refused, with the remedy named. + refused := newComposeFixture().withIngress() + refused.in.Deployment.StackConfig.DomainProxied = lo.ToPtr(true) + _, err := refused.run() + Expect(err).To(HaveOccurred()) + Expect(err.Error()).To(ContainSubstring("terminates TLS itself")) + Expect(err.Error()).To(ContainSubstring("ingress.originTls")) + + // The same stack onto a machine that declares no origin TLS: allowed, and the URL + // it hands back says http, because that is what the origin answers on. + allowed := newComposeFixture().withIngress().withNoOriginTls() + allowed.in.Deployment.StackConfig.DomainProxied = lo.ToPtr(true) + _, err = allowed.run() + Expect(err).To(BeNil()) + Expect(allowed.url).To(Equal("http://" + testDomain)) + + // `domainProxied: false` is a no-op and must not be refused on either machine — + // it says "do not proxy", which is already what SC does. + unproxied := newComposeFixture().withIngress() + unproxied.in.Deployment.StackConfig.DomainProxied = lo.ToPtr(false) + _, err = unproxied.run() + Expect(err).To(BeNil()) +} + +// The scheme has to reach the site file too, not just the exported URL: the site +// address is what suppresses automatic HTTPS for that site and puts the listener on +// :80. A deploy that exported `http://` and wrote an HTTPS site block would answer +// nothing at all. +func TestVmCompose_NoOriginTlsRendersAnHttpSite(t *testing.T) { + RegisterTestingT(t) + + f := newComposeFixture().withIngress().withNoOriginTls() + _, err := f.run() + Expect(err).To(BeNil()) + Expect(f.url).To(Equal("http://" + testDomain)) + + route := f.mocks.commands()["route"] + Expect(route).ToNot(BeNil(), "a stack whose domain is served must write a site file") + create, _ := unwrapSecret(route["create"]) + // The site file travels inside the script base64-encoded (it is written with + // `printf | base64 -d`, which is what keeps a multi-line config out of the shell's + // quoting rules), so the assertion has to decode it rather than grep the script. + Expect(siteFromRouteScript(create.StringValue())).To(ContainSubstring("http://" + testDomain + " {")) +} + +// Backwards compatibility at the seam that actually carries the risk: a parent stack +// provisioned before `ingress.originTls` existed exports nothing for it, and +// GetValueFromStack returns the zero value for an output that is not there. The zero +// has to mean "the machine terminates TLS" — the behaviour that fleet already had. +// Exporting the positive sense instead would have flipped every un-re-provisioned +// fleet to plain HTTP on its next deploy. +func TestVmCompose_OldParentStackStillServesHttps(t *testing.T) { + RegisterTestingT(t) + + f := newComposeFixture().withIngress() + // withIngress sets the ingress export and nothing else, so the origin-TLS output + // is genuinely absent here — the same state an old parent stack presents. + Expect(f.mocks.outputs).ToNot(HaveKey( + resource.PropertyKey(ToIngressNoOriginTlsExport(testVmResource)), + )) + + _, err := f.run() + Expect(err).To(BeNil()) + Expect(f.url).To(Equal("https://" + testDomain)) +} + +// siteFromRouteScript pulls the rendered Caddy site back out of a route command. +func siteFromRouteScript(script string) string { + const marker = "printf %s '" + i := strings.Index(script, marker) + Expect(i).To(BeNumerically(">=", 0), "the route script must embed the site file") + rest := script[i+len(marker):] + j := strings.Index(rest, "'") + Expect(j).To(BeNumerically(">=", 0), "the embedded site file must be quoted") + decoded, err := base64.StdEncoding.DecodeString(rest[:j]) + Expect(err).ToNot(HaveOccurred()) + return string(decoded) +} diff --git a/pkg/clouds/pulumi/vm/dns_test.go b/pkg/clouds/pulumi/vm/dns_test.go new file mode 100644 index 000000000..93a2babb7 --- /dev/null +++ b/pkg/clouds/pulumi/vm/dns_test.go @@ -0,0 +1,338 @@ +// SPDX-License-Identifier: MIT +// Copyright (c) Simple Container + +package vm + +import ( + "fmt" + "sync" + "testing" + + . "github.com/onsi/gomega" + "github.com/pkg/errors" + "github.com/samber/lo" + + sdk "github.com/pulumi/pulumi/sdk/v3/go/pulumi" + + "github.com/simple-container-com/api/pkg/api" + "github.com/simple-container-com/api/pkg/api/logger" + pApi "github.com/simple-container-com/api/pkg/clouds/pulumi/api" +) + +// The function under test lives in pulumi/api (pApi.FleetDnsRecords) since the k3s path +// needed it too, but these tests stay here: half of them assert the WIRING through +// VmComposeStack, and they all run on this package's compose fixtures (newComposeMocks, +// newComposeFixture, fleetHostAddress). Moving them would mean moving the fixtures into a +// package that has no business owning a compose stack. +// +// Pointing one name at N machines is one A record per machine, and the thing that can +// go wrong is invisible: the registrar plugin — not SC — decides what resource a record +// becomes, and three of the four derive that from the record's name and type WITHOUT +// its value. On those, record 2 replaces record 1, the deploy is green, the name +// resolves, and two thirds of the fleet never receive a request. These tests are about +// catching that from SC's side, which is possible because NewRecord hands back the +// resource's reference. + +// errRegistrarRefused is a registrar failure that is NOT "not configured" — the kind +// that must fail the deploy. +var errRegistrarRefused = errors.New("zone not found") + +// fakeRegistrar records what it was asked to create and hands back whatever reference +// the test tells it to — the two things that matter here. +type fakeRegistrar struct { + mu sync.Mutex + records []api.DnsRecord + // ref decides what the i-th call returns. A registrar that collapses same-name + // records is modelled by returning one reference for every call. + ref func(i int) sdk.Output + // err, when set, is what every call fails with. + err error +} + +func (f *fakeRegistrar) MainDomain() string { return "example.com" } + +func (f *fakeRegistrar) NewRecord(_ *sdk.Context, rec api.DnsRecord) (*api.ResourceOutput, error) { + f.mu.Lock() + i := len(f.records) + f.records = append(f.records, rec) + f.mu.Unlock() + if f.err != nil { + return nil, f.err + } + return &api.ResourceOutput{Ref: f.ref(i)}, nil +} + +func (f *fakeRegistrar) ProvisionRecords(*sdk.Context, pApi.ProvisionParams) (*api.ResourceOutput, error) { + return nil, nil +} + +func (f *fakeRegistrar) NewOverrideHeaderRule(*sdk.Context, api.Stack, pApi.OverrideHeaderRule) (*api.ResourceOutput, error) { + return nil, nil +} + +func (f *fakeRegistrar) ProvisionDomainForEndpoint(*sdk.Context, api.Stack, pApi.DomainEndpoint) (*api.ResourceOutput, error) { + return nil, nil +} + +// distinctRefs is a correct registrar: every record is its own resource. +func distinctRefs(i int) sdk.Output { + return sdk.String(fmt.Sprintf("urn:pulumi:rec-%d", i)).ToStringOutput() +} + +// collapsedRefs is the bug, modelled: every record is the SAME resource, which is what +// a plugin deriving its resource name from name+type alone produces. +func collapsedRefs(int) sdk.Output { + return sdk.String("urn:pulumi:example-com-A").ToStringOutput() +} + +// runFleetDns calls pApi.FleetDnsRecords inside a Pulumi run, which it needs because the +// guard awaits the references it is handed. +func runFleetDns(reg pApi.Registrar, addresses []string) error { + return runFleetDnsWith(reg, addresses, false) +} + +// runFleetDnsWith is runFleetDns with the proxied flag under the test's control, which +// matters since #566: `domainProxied: true` is reachable on a fleet that terminates no +// origin TLS. +func runFleetDnsWith(reg pApi.Registrar, addresses []string, proxied bool) error { + return sdk.RunErr(func(ctx *sdk.Context) error { + params := pApi.ProvisionParams{Log: logger.New()} + if reg != nil { + params.Registrar = reg + } + return pApi.FleetDnsRecords(ctx, params, testComposeStack, testDomain, addresses, proxied) + }, sdk.WithMocks("project", testComposeStack, newComposeMocks())) +} + +func TestFleetDns_OneRecordPerMachine(t *testing.T) { + RegisterTestingT(t) + + reg := &fakeRegistrar{ref: distinctRefs} + Expect(runFleetDns(reg, []string{"10.0.0.1", "10.0.0.2", "10.0.0.3"})).To(Succeed()) + + Expect(reg.records).To(HaveLen(3), "one A record per machine of the fleet") + for i, rec := range reg.records { + Expect(rec.Name).To(Equal(testDomain), "record %d", i) + Expect(rec.Type).To(Equal("A"), "record %d", i) + } + // Every machine, and each exactly once: a loop that read one index N times would + // create three records that all passed the collision guard and all pointed at the + // same host. + Expect([]string{reg.records[0].Value, reg.records[1].Value, reg.records[2].Value}). + To(ConsistOf("10.0.0.1", "10.0.0.2", "10.0.0.3")) +} + +// THE assertion of this file. SC does not know which registrar plugins hash the value +// into their resource name, and does not need to: if two calls hand back the same +// resource, the registrar collapsed them. Refused rather than warned about, because the +// deploy succeeds either way and a warning is the one thing nobody would notice. +func TestFleetDns_CollapsedRegistrarIsRefused(t *testing.T) { + RegisterTestingT(t) + + reg := &fakeRegistrar{ref: collapsedRefs} + err := runFleetDns(reg, []string{"10.0.0.1", "10.0.0.2", "10.0.0.3"}) + + Expect(err).To(HaveOccurred()) + Expect(err.Error()).To(ContainSubstring("the same resource")) + // Both hosts, the name, and the fleet's real size — enough to tell the two + // failures apart without reading the code: a collapsed registrar versus a fleet + // that genuinely has one machine. + Expect(err.Error()).To(ContainSubstring("hosts 0 and 1")) + Expect(err.Error()).To(ContainSubstring(testDomain)) + Expect(err.Error()).To(ContainSubstring("one machine instead of 3")) + // Named as the plugin's bug, because the operator's first instinct on a DNS + // failure is to go looking at their own `domain:` setting. + Expect(err.Error()).To(ContainSubstring("bug in that plugin")) +} + +// A fleet of one is the overwhelmingly common case and must behave exactly as it did +// before any of this existed — including on a registrar that collapses, where there is +// nothing to collapse. +func TestFleetDns_SingleHostIsUnaffected(t *testing.T) { + RegisterTestingT(t) + + reg := &fakeRegistrar{ref: collapsedRefs} + Expect(runFleetDns(reg, []string{"10.0.0.1"})).To(Succeed()) + Expect(reg.records).To(HaveLen(1)) + Expect(reg.records[0].Value).To(Equal("10.0.0.1")) +} + +// A parent stack with no registrar is a supported arrangement — the operator points the +// name at the fleet by hand, which is what the docs for every cloud without a registrar +// already say. So this warns and continues; a refusal here would break every vm stack +// that has ever set `domain:`. +func TestFleetDns_NoRegistrarIsNotAFailure(t *testing.T) { + RegisterTestingT(t) + + // Nil, which is what a test fixture and some callers have. + Expect(runFleetDns(nil, []string{"10.0.0.1", "10.0.0.2"})).To(Succeed()) + + // And the way it actually arrives in production: the empty registrar type is + // registered to NotConfiguredRegistrar, so params.Registrar is NON-nil and every + // method fails with the sentinel. A `== nil` check alone would turn this into a + // hard deploy failure for every stack whose parent declares no registrar. + reg := &fakeRegistrar{ref: distinctRefs, err: pApi.NotConfiguredRegistrarError} + Expect(runFleetDns(reg, []string{"10.0.0.1", "10.0.0.2"})).To(Succeed()) +} + +// A registrar whose call genuinely fails must fail the deploy — the point of the +// not-configured branch above is that it is the ONE error that does not. +func TestFleetDns_RealRegistrarErrorFails(t *testing.T) { + RegisterTestingT(t) + + reg := &fakeRegistrar{ref: distinctRefs, err: errRegistrarRefused} + err := runFleetDns(reg, []string{"10.0.0.1", "10.0.0.2"}) + + Expect(err).To(HaveOccurred()) + Expect(err.Error()).To(ContainSubstring("host 0 of the fleet")) + Expect(err.Error()).To(ContainSubstring("zone not found")) +} + +// During a preview the records do not exist yet, so their references are unknown. The +// guard has nothing to compare and must not invent a verdict either way — comparing +// unknowns would make every multi-host preview fail with a collision that is not there. +func TestFleetDns_UnknownRefsAreSkipped(t *testing.T) { + RegisterTestingT(t) + + unknown := func(int) sdk.Output { + return sdk.UnsafeUnknownOutput(nil).ApplyT(func(any) string { return "" }) + } + reg := &fakeRegistrar{ref: unknown} + Expect(runFleetDns(reg, []string{"10.0.0.1", "10.0.0.2", "10.0.0.3"})).To(Succeed()) + Expect(reg.records).To(HaveLen(3), "the records are still created; only the guard stands down") +} + +// A registrar whose Ref is not a string Output at all. Nothing to compare, so nothing +// is claimed: the records were created and the deploy proceeds. +func TestFleetDns_NonStringRefsAreSkipped(t *testing.T) { + RegisterTestingT(t) + + reg := &fakeRegistrar{ref: func(int) sdk.Output { return sdk.Int(7).ToIntOutput() }} + Expect(runFleetDns(reg, []string{"10.0.0.1", "10.0.0.2"})).To(Succeed()) + Expect(reg.records).To(HaveLen(2)) +} + +// The wiring, asserted end to end through VmComposeStack rather than on +// pApi.FleetDnsRecords directly — because the bug this catches is not in the function, it is +// in what the function is handed. The addresses have to come from the parent stack's +// per-host exports, and `withHosts` gives each machine a distinct address precisely so +// that a caller passing host 0's address N times cannot pass. +func TestVmCompose_FleetGetsOneARecordPerMachine(t *testing.T) { + RegisterTestingT(t) + + reg := &fakeRegistrar{ref: distinctRefs} + f := newComposeFixture().withIngress().withHosts(3) + f.registrar = reg + + _, err := f.run() + Expect(err).ToNot(HaveOccurred()) + + Expect(reg.records).To(HaveLen(3)) + values := make([]string, 0, len(reg.records)) + for _, rec := range reg.records { + Expect(rec.Name).To(Equal(testDomain)) + values = append(values, rec.Value) + } + // Machine 0 keeps the single-host fixture's address; 1 and 2 are the fleet's. + Expect(values).To(ConsistOf(testHostAddress, fleetHostAddress(1), fleetHostAddress(2))) +} + +// And the other half of the wiring: no record when Caddy is not in front. An A record +// at `domain` would then resolve to a machine with nothing listening on 443 — the stack +// answers on a host port instead, which the existing warnings already say. +func TestVmCompose_NoIngressMeansNoDnsRecord(t *testing.T) { + RegisterTestingT(t) + + reg := &fakeRegistrar{ref: distinctRefs} + // withHosts but NOT withIngress: the stack sets no domain and the machine runs no + // Caddy. + f := newComposeFixture().withHosts(2) + f.registrar = reg + + _, err := f.run() + Expect(err).ToNot(HaveOccurred()) + Expect(reg.records).To(BeEmpty()) +} + +// Nothing to point anywhere. Both guards exist because the caller reads the addresses +// out of the parent stack, which answers with nothing at all when the parent predates +// the export. +func TestFleetDns_NothingToDo(t *testing.T) { + RegisterTestingT(t) + + reg := &fakeRegistrar{ref: distinctRefs} + Expect(sdk.RunErr(func(ctx *sdk.Context) error { + params := pApi.ProvisionParams{Log: logger.New(), Registrar: reg} + if err := pApi.FleetDnsRecords(ctx, params, testComposeStack, "", []string{"10.0.0.1"}, false); err != nil { + return err + } + return pApi.FleetDnsRecords(ctx, params, testComposeStack, testDomain, nil, false) + }, sdk.WithMocks("project", testComposeStack, newComposeMocks()))).To(Succeed()) + Expect(reg.records).To(BeEmpty()) +} + +// The front door #566 opened, asserted at the far end of the wiring it depends on. +// +// `ingress.originTls: false` means the machines serve plain HTTP and the edge in front +// terminates TLS — which only works if the records are actually PROXIED. Nothing before +// this test checked that `domainProxied` reaches `api.DnsRecord`, and the failure would +// have been silent in the worst way: a green deploy, a name that resolves, an edge that +// is never in the path, and every client talking cleartext to the origin over the public +// internet. Asserted through VmComposeStack rather than on pApi.FleetDnsRecords, because a +// flag that is read correctly and then dropped on the way down is exactly the bug. +func TestVmCompose_ProxiedReachesEveryRecordOfTheFleet(t *testing.T) { + RegisterTestingT(t) + + reg := &fakeRegistrar{ref: distinctRefs} + f := newComposeFixture().withIngress().withNoOriginTls().withHosts(3) + f.in.Deployment.StackConfig.DomainProxied = lo.ToPtr(true) + f.registrar = reg + + _, err := f.run() + Expect(err).ToNot(HaveOccurred()) + + Expect(reg.records).To(HaveLen(3)) + for i, rec := range reg.records { + Expect(rec.Proxied).To(BeTrue(), "record %d of a no-origin-TLS fleet must be proxied", i) + } + + // And the default is unchanged: a fleet that terminates its own TLS gets unproxied + // records, because Caddy answers the HTTP-01 challenge on the machine. + own := &fakeRegistrar{ref: distinctRefs} + g := newComposeFixture().withIngress().withHosts(2) + g.registrar = own + + _, err = g.run() + Expect(err).ToNot(HaveOccurred()) + Expect(own.records).To(HaveLen(2)) + for i, rec := range own.records { + Expect(rec.Proxied).To(BeFalse(), "record %d of a TLS-terminating fleet must not be proxied", i) + } +} + +// The flag reaches the record on the direct path too, where a caller with no fleet to +// deploy onto still has to get it right. +func TestFleetDns_ProxiedIsPassedThrough(t *testing.T) { + RegisterTestingT(t) + + reg := &fakeRegistrar{ref: distinctRefs} + Expect(runFleetDnsWith(reg, []string{"10.0.0.1", "10.0.0.2"}, true)).To(Succeed()) + Expect(reg.records).To(HaveLen(2)) + for i, rec := range reg.records { + Expect(rec.Proxied).To(BeTrue(), "record %d", i) + } +} + +// The advice SC prints when it cannot create the records itself. Both halves are +// asserted because both are reachable and the wrong one is harmful rather than merely +// unhelpful — see pApi.ManualRecordAdvice's own comment for which way each fails. +func TestManualRecordAdvice(t *testing.T) { + RegisterTestingT(t) + + Expect(pApi.ManualRecordAdvice(true)).To(HavePrefix("proxied")) + Expect(pApi.ManualRecordAdvice(true)).To(ContainSubstring("terminates TLS")) + Expect(pApi.ManualRecordAdvice(true)).ToNot(ContainSubstring("ACME")) + + Expect(pApi.ManualRecordAdvice(false)).To(HavePrefix("unproxied")) + Expect(pApi.ManualRecordAdvice(false)).To(ContainSubstring("ACME challenge")) +} diff --git a/pkg/clouds/pulumi/vm/driver.go b/pkg/clouds/pulumi/vm/driver.go index 9b9d2a189..c35f56197 100644 --- a/pkg/clouds/pulumi/vm/driver.go +++ b/pkg/clouds/pulumi/vm/driver.go @@ -59,11 +59,54 @@ type HostFacts struct { HostKeyPinned bool // Role is what the machine is being prepared for. Role vm.HostRole + // PrivateAddress is what the machine's SIBLINGS dial it on, where Address is + // what SC dials from outside. Equal to Address unless the driver knows better. + // + // It exists because k3s does not: SC sets `--node-ip` nowhere, so k3s advertises + // whichever address the default-route interface carries — correct on a cloud VM + // behind a floating/NAT IP, the PUBLIC address on a single-NIC VPS, which puts + // etcd peer traffic and flannel's unencrypted VXLAN overlay on the public + // internet. A multi-node k3s pool is refused unless this is known (see + // vm.VmHost.PrivateAddress); a compose fleet never needs it, because those + // machines do not talk to each other. + // + // An Output for the same reason Address is: a cloud driver learns the instance's + // internal IP only after creating it. Not secret — a private address is not a + // credential, and redacting it would hide the one value a reader needs to tell a + // correctly-addressed cluster from an exposed one. + // + // Read it through EffectivePrivateAddress, never directly: a driver written + // before this field existed — every out-of-tree plugin, today — leaves it as the + // zero Output, which has no OutputState and panics inside pulumi's own + // gatherJoinSet the moment it reaches sdk.All. + PrivateAddress sdk.StringOutput // DependsOn are the resources that must exist before anything connects. Empty // for byo: the machine predates the stack. DependsOn []sdk.Resource } +// EffectivePrivateAddress is the address this machine's siblings should dial, with +// Address as the fallback. +// +// The fallback is a compatibility guarantee, not a convenience. Every cloud VM +// driver is an out-of-tree plugin built against an older HostFacts, so +// PrivateAddress arrives as the ZERO Output — and a zero Output carries no +// OutputState, so passing it to sdk.All does not resolve to "" but panics with a nil +// dereference inside pulumi's gatherJoinSet. An accessor is the only place that can +// be fixed once for every consumer; comparing against sdk.StringOutput{} works +// because the type is a comparable struct wrapping that state pointer. +// +// Reading Address when a plugin supplied nothing is the pre-existing behaviour, and +// it is safe where it is reached: the config tier refuses a multi-node k3s pool +// whose private addresses are unknown (vm.VmHost.PrivateAddress), so a fleet that +// needs the distinction cannot get here by this path. +func (h HostFacts) EffectivePrivateAddress() sdk.StringOutput { + if h.PrivateAddress == (sdk.StringOutput{}) { + return h.Address + } + return h.PrivateAddress +} + // Driver is the per-cloud half of the VM path. // // Narrower than the nine-method interface sketched in vm-matrix.md §1.1 on purpose. diff --git a/pkg/clouds/pulumi/vm/plugin_authoring_test.go b/pkg/clouds/pulumi/vm/plugin_authoring_test.go index f83ae48f1..1a8918fcc 100644 --- a/pkg/clouds/pulumi/vm/plugin_authoring_test.go +++ b/pkg/clouds/pulumi/vm/plugin_authoring_test.go @@ -81,6 +81,17 @@ func TestPluginDriver_AcceptsWhatTheAuthoringHelperBuilds(t *testing.T) { Expect(address.Known).To(BeTrue()) Expect(address.Secret).To(BeFalse()) + // This plugin — like every plugin in the fleet today — said nothing about a + // private address, so the field arrives as the ZERO Output. Read through the + // accessor it resolves to Address; read directly it does not resolve at all, + // it panics (see EffectivePrivateAddress). + private := awaitString(t, ctx.Context(), facts.EffectivePrivateAddress()) + Expect(private.Value).To(Equal("203.0.113.9")) + Expect(private.Known).To(BeTrue()) + Expect(private.Secret).To(BeFalse(), + "a private address is not a credential, and redacting it hides the one "+ + "value that tells a correctly-addressed cluster from an exposed one") + // The helper marked it secret on the wire and core marks it secret again. // Both, deliberately — see PrivateKey's doc comment. key := awaitString(t, ctx.Context(), facts.PrivateKey) @@ -111,3 +122,42 @@ func TestPluginDriver_AcceptsWhatTheAuthoringHelperBuilds(t *testing.T) { Expect(facts.HostKeyPinned).To(BeFalse(), "TrustOnFirstUse is not a pinned key") Expect(facts.DependsOn).To(HaveLen(1)) } + +// The zero sdk.StringOutput is the hazard that made EffectivePrivateAddress an +// accessor rather than a plain field read, and it is worth a test of its own because +// the failure is a PANIC in pulumi's internals rather than an empty string: a zero +// Output carries no OutputState, so sdk.All nil-dereferences inside gatherJoinSet. +// +// Every cloud VM driver is an out-of-tree plugin compiled against an older +// HostFacts, so "the field is zero" is the normal case for the whole fleet, not an +// edge one. This test fails loudly if someone later swaps the accessor for a direct +// read, or drops the comparison on the theory that an unset Output behaves like "". +func TestHostFacts_ZeroPrivateAddressFallsBackInsteadOfPanicking(t *testing.T) { + RegisterTestingT(t) + + err := sdk.RunErr(func(ctx *sdk.Context) error { + // Exactly the shape a pre-field plugin produces: Address set, everything + // else left alone. + facts := HostFacts{Address: sdk.String("203.0.113.9").ToStringOutput()} + Expect(facts.PrivateAddress).To(Equal(sdk.StringOutput{}), + "the premise: an older plugin leaves this at its zero value") + + // sdk.All is where the panic happened — the k3s cluster builder joins the + // SAN list, the join URL and this value before rendering the install script. + joined := sdk.All(facts.EffectivePrivateAddress()).ApplyT(func(args []any) string { + return args[0].(string) + }).(sdk.StringOutput) + + Expect(awaitString(t, ctx.Context(), joined).Value).To(Equal("203.0.113.9")) + + // And an explicit value is passed through untouched. + set := HostFacts{ + Address: sdk.String("203.0.113.9").ToStringOutput(), + PrivateAddress: sdk.String("10.0.0.9").ToStringOutput(), + } + Expect(awaitString(t, ctx.Context(), set.EffectivePrivateAddress()).Value).To(Equal("10.0.0.9")) + + return nil + }, sdk.WithMocks("project", "stack", newVmMocks())) + Expect(err).ToNot(HaveOccurred()) +} diff --git a/pkg/clouds/pulumi/vm/plugin_driver_test.go b/pkg/clouds/pulumi/vm/plugin_driver_test.go index 5d729260e..dae2a86cb 100644 --- a/pkg/clouds/pulumi/vm/plugin_driver_test.go +++ b/pkg/clouds/pulumi/vm/plugin_driver_test.go @@ -6,6 +6,7 @@ package vm import ( "context" "encoding/json" + "fmt" "testing" "google.golang.org/grpc/codes" @@ -287,6 +288,65 @@ func TestPluginDriver_NoHostsIsAnError(t *testing.T) { Expect(err.Error()).To(ContainSubstring("portal-vm")) } +// The case this guard exists for: a plugin built before `nodes` existed. The field +// rides the marshalled config, so an older plugin's unmarshal drops it and it +// creates ONE machine for a three-machine resource — and a count mismatch is +// invisible from the operator's side, because the provision is green and the pool +// is simply a size nobody asked for. +func TestPluginDriver_WrongHostCountIsRefused(t *testing.T) { + RegisterTestingT(t) + + registerPluginVm(t, func(_ context.Context, _ *scpluginv1.VmHostsRequest) (*scpluginv1.VmHostsResponse, error) { + return &scpluginv1.VmHostsResponse{Hosts: []*scpluginv1.VmHostFacts{{ + Name: "node-0", User: "ubuntu", Port: 22, Role: string(vm.RoleCompose), + Address: wireLiteral("203.0.113.9"), + }}}, nil + }) + + cfg := pluginVmConfig() + cfg.Nodes = 3 + + _, err := provisionVm(cfg, newVmMocks()) + Expect(err).To(HaveOccurred()) + Expect(err.Error()).To(ContainSubstring("asked driver \"fake-cloud-vm\" for 3 machine(s) and got 1")) + // The likely cause, named — otherwise the operator reads this as their own + // config being wrong and goes looking in server.yaml. + Expect(err.Error()).To(ContainSubstring("older than the `nodes` field")) +} + +// And the same path with the count honoured: the guard must not fire on a plugin +// that does the right thing, which is the only way to know it is a guard and not a +// cap on the fleet size. +func TestPluginDriver_HonouredHostCountPasses(t *testing.T) { + RegisterTestingT(t) + + registerPluginVm(t, func(_ context.Context, req *scpluginv1.VmHostsRequest) (*scpluginv1.VmHostsResponse, error) { + var cfg vm.VmConfig + if err := json.Unmarshal(req.ConfigJson, &cfg); err != nil { + return nil, err + } + // What an up-to-date plugin does: read the count off the config rather than + // assume one machine. + hosts := make([]*scpluginv1.VmHostFacts, 0, cfg.NodeCount()) + for i := range cfg.NodeCount() { + hosts = append(hosts, &scpluginv1.VmHostFacts{ + Name: fmt.Sprintf("node-%d", i), User: "ubuntu", Port: 22, + Role: string(vm.RoleCompose), + Address: wireLiteral(fmt.Sprintf("203.0.113.%d", 10+i)), + }) + } + return &scpluginv1.VmHostsResponse{Hosts: hosts}, nil + }) + + cfg := pluginVmConfig() + cfg.Nodes = 3 + + out, err := provisionVm(cfg, newVmMocks()) + Expect(err).ToNot(HaveOccurred()) + Expect(out.Hosts).To(HaveLen(3)) + Expect(out.Ready).To(HaveLen(3), "every host of a fleet needs its own readiness gate") +} + // A host with no address: there is nothing for SSH to connect to, and the layers // above would otherwise build a connection to "". func TestPluginDriver_MissingAddressIsAnError(t *testing.T) { diff --git a/pkg/clouds/pulumi/vm/vm.go b/pkg/clouds/pulumi/vm/vm.go index 3efaf8165..39a769f29 100644 --- a/pkg/clouds/pulumi/vm/vm.go +++ b/pkg/clouds/pulumi/vm/vm.go @@ -85,6 +85,24 @@ func Vm(ctx *sdk.Context, stack api.Stack, input api.ResourceInput, params pApi. if len(hosts) == 0 { return nil, errors.Errorf("driver %q produced no hosts for vm %q", drv.Name(), input.Descriptor.Name) } + // The driver has to produce the pool SC asked for. `byo` satisfies this by + // definition (NodeCount is len(hosts) there), so this guards the cloud drivers — + // and every one of them is an out-of-tree plugin, built against whichever version + // of VmConfig existed when it was released. `nodes` reaches a plugin inside the + // marshalled config (plugin_driver.go), so a plugin that predates the field + // unmarshals it into nothing and creates ONE machine for a resource that asked + // for three: a green provision, one host, `-vm-host-count` of 1, and a k3s + // pool or compose fleet that is silently a single machine. Refused here rather + // than discovered later, for the same reason a multi-node pool with no + // `privateAddress` is refused (vm.VmHost.PrivateAddress). + if want := cfg.NodeCount(); len(hosts) != want { + return nil, errors.Errorf( + "vm %q asked driver %q for %d machine(s) and got %d. If that driver is a plugin, it is probably "+ + "older than the `nodes` field and ignored the count — upgrade it, or declare the pool size the "+ + "way that version understands", + input.Descriptor.Name, drv.Name(), want, len(hosts), + ) + } out := &VmOutput{Hosts: hosts, IngressEnabled: cfg.IngressEnabled()} for i, h := range hosts { @@ -144,6 +162,12 @@ func Vm(ctx *sdk.Context, stack api.Stack, input api.ResourceInput, params pApi. } ctx.Export(ToHostCountExport(input.Descriptor.Name), sdk.Int(len(hosts))) ctx.Export(ToIngressEnabledExport(input.Descriptor.Name), sdk.Bool(cfg.IngressEnabled())) + // The NEGATION of cfg.OriginTlsEnabled(), and that is deliberate. A child stack + // deployed against a parent provisioned before this export existed reads a missing + // output as the zero value, so the zero has to be the old behaviour — and the old + // behaviour is that the machine terminates TLS. Exporting the positive sense would + // make every not-yet-re-provisioned fleet silently switch to plain HTTP. + ctx.Export(ToIngressNoOriginTlsExport(input.Descriptor.Name), sdk.Bool(!cfg.OriginTlsEnabled())) // What SC actually opened, so a child stack can tell its operator that the host // port it is about to publish is not reachable. Empty when SC does not manage // this machine's firewall — the child must then say nothing rather than guess, @@ -189,6 +213,10 @@ func ToIngressEnabledExport(vmName string) string { return fmt.Sprintf("%s-vm-ingress-enabled", vmName) } +func ToIngressNoOriginTlsExport(vmName string) string { + return fmt.Sprintf("%s-vm-ingress-no-origin-tls", vmName) +} + func ToOpenPortsExport(vmName string) string { return fmt.Sprintf("%s-vm-open-ports", vmName) } diff --git a/pkg/clouds/pulumi/vm/vm_test.go b/pkg/clouds/pulumi/vm/vm_test.go index b24ae7014..6b6ded082 100644 --- a/pkg/clouds/pulumi/vm/vm_test.go +++ b/pkg/clouds/pulumi/vm/vm_test.go @@ -494,6 +494,7 @@ func TestVm_ExportsHostFacts(t *testing.T) { Expect(ToHostAddressExport("portal-vm", 0)).To(Equal("portal-vm-vm-host-0-address")) Expect(ToHostPrivateKeyExport("portal-vm", 0)).To(Equal("portal-vm-vm-host-0-private-key")) Expect(ToIngressEnabledExport("portal-vm")).To(Equal("portal-vm-vm-ingress-enabled")) + Expect(ToIngressNoOriginTlsExport("portal-vm")).To(Equal("portal-vm-vm-ingress-no-origin-tls")) } // Vm is reachable on its own, so it re-checks what Provider checked: a nil provider diff --git a/pkg/clouds/vm/caddy.go b/pkg/clouds/vm/caddy.go index eb785566a..57f11d72a 100644 --- a/pkg/clouds/vm/caddy.go +++ b/pkg/clouds/vm/caddy.go @@ -66,6 +66,27 @@ func RenderCaddyfile(cfg *VmConfig) (string, error) { b := &strings.Builder{} b.WriteString(caddyGeneratedHeader) b.WriteString("\n{\n") + if !cfg.OriginTlsEnabled() { + // `auto_https off` rather than relying on every site address carrying an + // explicit `http://`. Both would stop ACME today, and the scheme on the site + // address is what actually makes the port 80 listener happen — but this is a + // file an operator edits and a conf.d an operator drops files into, and one + // bare domain in one hand-written site block is otherwise enough to send the + // machine off to Let's Encrypt for a certificate it cannot validate. Off at + // the global level is the statement that this machine does no TLS, in the one + // place that cannot be contradicted from below. + // + // It also disables the HTTP→HTTPS redirect, which is the point: there is no + // HTTPS here to redirect to. The edge in front is what serves it. + fmt.Fprintf(b, "\t# `ingress.originTls: false` on this vm resource: something in front terminates TLS\n") + fmt.Fprintf(b, "\t# (Cloudflare's proxy, a load balancer), so this machine serves plain HTTP on :80,\n") + fmt.Fprintf(b, "\t# requests no certificate, and has no ACME account. Edge-to-origin is CLEARTEXT —\n") + fmt.Fprintf(b, "\t# the machine's firewall is what has to admit only the edge.\n") + b.WriteString("\tauto_https off\n") + b.WriteString("}\n\n") + writeCaddySnippets(b, snippets) + return b.String(), nil + } fmt.Fprintf(b, "\temail %s\n", caddyToken(cfg.Ingress.Email)) if cfg.Ingress.AcmeCa != "" { // An `acme_ca` that does not speak ACME, or is unreachable, presents as @@ -84,7 +105,16 @@ func RenderCaddyfile(cfg *VmConfig) (string, error) { writeCaddyAcmeStorage(b, cfg.Ingress.AcmeStorage) } b.WriteString("}\n\n") + writeCaddySnippets(b, snippets) + return b.String(), nil +} +// writeCaddySnippets writes everything below the global options block: the inlined +// snippet library and the conf.d import. Shared by both exits of RenderCaddyfile so the +// no-origin-TLS machine cannot drift into having a different snippet set — a +// `siteExtraHelpers` that resolves on one machine and not another is a 500 on one third +// of a fleet's requests. +func writeCaddySnippets(b *strings.Builder, snippets []byte) { fmt.Fprintf(b, "# Shared snippet library, inlined from %s in the SC binary.\n", "pkg/clouds/caddy/"+caddyassets.CaddyfilePath) fmt.Fprintf(b, "# Identical to the one the Kubernetes edge serves, so `lbConfig.extraHelpers`\n") @@ -94,7 +124,6 @@ func RenderCaddyfile(cfg *VmConfig) (string, error) { fmt.Fprintf(b, "# One file per stack-env, each written by that stack's own deploy.\n") fmt.Fprintf(b, "import %s/*.caddy\n", CaddyContainerConfigDir+"/conf.d") - return b.String(), nil } // writeCaddyAcmeStorage renders the global `storage s3` block — the thing that makes @@ -408,8 +437,13 @@ func RenderCaddySite(in *VmComposeInput, opts RenderOptions) (string, error) { proxy.WriteString("}") imports := []string{"import gzip", "import handle_static"} - // Unconditional, unlike Kubernetes' `useSSL`. There is nothing in front of this - // Caddy, so it is the thing that terminates TLS and the site is always HTTPS — + // Kept even when this machine serves plain HTTP, unlike Kubernetes' `useSSL`. + // With origin TLS the reasoning is the obvious one: this Caddy terminates, so the + // site is HTTPS and HSTS is its own to send. Without it, the header still reaches + // the browser over the EDGE's TLS, where it is honoured and stops a downgrade — + // and on the pathological arrangement (no origin TLS and no edge either) a + // browser ignores HSTS received over cleartext, so it is inert rather than + // harmful. There is no case where dropping it is better. // `lbConfig.https` describes what an external load balancer terminates and has // nothing to answer here. imports = append(imports, "import hsts") @@ -418,7 +452,19 @@ func RenderCaddySite(in *VmComposeInput, opts RenderOptions) (string, error) { b.WriteString(fmt.Sprintf(caddySiteHeader, opts.Stack, opts.Env, opts.Slot)) // The bare domain as the site address: Caddy's automatic HTTPS then provisions a // certificate for it and redirects HTTP to HTTPS by itself. - fmt.Fprintf(b, "%s {\n", sc.Domain) + // + // `http://` instead when the resource sets `ingress.originTls: false`. + // The scheme is not decoration — it is what suppresses automatic HTTPS for this + // site and puts the listener on :80. The global block's `auto_https off` says the + // same thing for the machine; both are rendered, because this file is written by + // the stack's deploy and that one by the resource's provision, and a machine that + // has not been re-provisioned since the flag was set would otherwise go asking a + // CA for a certificate on behalf of a stack that cannot answer the challenge. + if opts.NoOriginTls { + fmt.Fprintf(b, "http://%s {\n", sc.Domain) + } else { + fmt.Fprintf(b, "%s {\n", sc.Domain) + } if sc.Prefix == "" { writeIndented(b, proxy.String(), " ") for _, imp := range imports { diff --git a/pkg/clouds/vm/caddy_test.go b/pkg/clouds/vm/caddy_test.go index f2c70ff62..b9091baa1 100644 --- a/pkg/clouds/vm/caddy_test.go +++ b/pkg/clouds/vm/caddy_test.go @@ -485,3 +485,66 @@ func TestRenderCaddySiteRefusals(t *testing.T) { Expect(err).To(HaveOccurred(), "%s must be refused", name) } } + +// `ingress.originTls: false`: the machine serves plain HTTP for an edge that +// terminates, and the global block has to say so in a way that cannot be contradicted +// by a hand-written site block in conf.d. +func TestRenderCaddyfileNoOriginTls(t *testing.T) { + RegisterTestingT(t) + + cfg := ingressConfig() + cfg.Ingress.Email = "" + cfg.Ingress.OriginTls = lo.ToPtr(false) + + got, err := RenderCaddyfile(cfg) + Expect(err).ToNot(HaveOccurred()) + + // The whole point: no automatic HTTPS anywhere on this machine. Without it one + // bare domain in one conf.d file sends the machine off to a CA for a certificate + // whose challenge it cannot answer, and the fleet quietly earns a rate-limit. + Expect(got).To(ContainSubstring("auto_https off")) + // Nothing ACME-shaped is rendered, because nothing requests a certificate. + Expect(got).ToNot(ContainSubstring("email ")) + Expect(got).ToNot(ContainSubstring("acme_ca")) + Expect(got).ToNot(ContainSubstring("acme_eab")) + Expect(got).ToNot(ContainSubstring("storage s3")) + // The hop the operator is now responsible for, named in the file they will read. + Expect(got).To(ContainSubstring("CLEARTEXT")) + // The snippet library and the conf.d import are NOT a casualty of the early + // return — a `siteExtraHelpers` that resolves on an HTTPS machine and not on this + // one would be a 500 on a fraction of a fleet's requests. + for _, snippet := range []string{"(gzip)", "(hsts)", "(handle_static)", "(handle_server_error)"} { + Expect(got).To(ContainSubstring(snippet), "snippet %s must be inlined here too", snippet) + } + Expect(got).To(ContainSubstring("import " + CaddyContainerConfigDir + "/conf.d/*.caddy")) +} + +// The site file's scheme. Both halves are asserted against the SAME stack input, so a +// regression that renders one scheme unconditionally fails one of them. +func TestRenderCaddySiteSchemeFollowsOriginTls(t *testing.T) { + RegisterTestingT(t) + + in := renderTestInput(t) + domain := in.Deployment.StackConfig.Domain + Expect(domain).ToNot(BeEmpty()) + + https, err := RenderCaddySite(in, caddySiteOpts()) + Expect(err).ToNot(HaveOccurred()) + Expect(https).To(ContainSubstring(domain + " {")) + Expect(https).ToNot(ContainSubstring("http://" + domain)) + + opts := caddySiteOpts() + opts.NoOriginTls = true + plain, err := RenderCaddySite(in, opts) + Expect(err).ToNot(HaveOccurred()) + // The scheme is not decoration: it is what suppresses automatic HTTPS for this + // site and puts the listener on :80. + Expect(plain).To(ContainSubstring("http://" + domain + " {")) + // HSTS stays. It reaches the browser over the EDGE's TLS, where it is honoured; + // with no edge at all a browser ignores HSTS received over cleartext, so it is + // inert rather than harmful. There is no case where dropping it is better. + Expect(plain).To(ContainSubstring("import hsts")) + // And the upstream is untouched — the proxy hop was always plain HTTP over the + // docker network. + Expect(plain).To(ContainSubstring("reverse_proxy http://")) +} diff --git a/pkg/clouds/vm/cloudinit.go b/pkg/clouds/vm/cloudinit.go index 7c3348962..a8e9bb85a 100644 --- a/pkg/clouds/vm/cloudinit.go +++ b/pkg/clouds/vm/cloudinit.go @@ -30,6 +30,17 @@ const ( RoleK3sAgent HostRole = "k3s-agent" ) +// AllHostRoles is every spelling `hosts[].role` accepts, in the order the +// validation error lists them. Kept next to the constants so adding a role is one +// edit rather than two places that drift. +var AllHostRoles = []HostRole{RoleCompose, RoleK3sServer, RoleK3sAgent} + +// IsK3s reports whether the role makes the machine a node of a k3s cluster, as +// opposed to a host running stacks under `docker compose`. +func (r HostRole) IsK3s() bool { + return r == RoleK3sServer || r == RoleK3sAgent +} + // DefaultDockerInstallScript is the fallback way to get docker onto a fresh machine. // // It reaches out to get.docker.com, which is one of the four egress paths whose diff --git a/pkg/clouds/vm/convert.go b/pkg/clouds/vm/convert.go index 6a3985f1b..4b94e068b 100644 --- a/pkg/clouds/vm/convert.go +++ b/pkg/clouds/vm/convert.go @@ -99,13 +99,16 @@ func ToVmComposeSingleImageConfig(tpl any, stackCfg *api.StackConfigSingleImage) } composeCfg := &api.StackConfigCompose{ - Domain: stackCfg.Domain, - BaseDnsZone: stackCfg.BaseDnsZone, - Uses: stackCfg.Uses, - Runs: []string{SingleImageService}, - Env: stackCfg.Env, - Secrets: stackCfg.Secrets, - Version: stackCfg.Version, + Domain: stackCfg.Domain, + // Carried, not defaulted: every reader of this flag reads the compose config, + // so dropping it here is the same as the operator never having written it. + DomainProxied: stackCfg.DomainProxied, + BaseDnsZone: stackCfg.BaseDnsZone, + Uses: stackCfg.Uses, + Runs: []string{SingleImageService}, + Env: stackCfg.Env, + Secrets: stackCfg.Secrets, + Version: stackCfg.Version, // basePath is the path the application itself serves under, so the prefix has // to survive the proxy hop — which is exactly `proxyKeepPrefix: true`. Prefix: strings.TrimSuffix(stackCfg.BasePath, "/"), @@ -195,34 +198,47 @@ func toComposeTemplate(tpl any) (*ComposeTemplateConfig, error) { func (i *VmComposeInput) validateStackConfig() error { cfg := i.Deployment.StackConfig - if cfg.DomainProxied != nil { - return errors.Errorf( - "`domainProxied` is not supported by the `%s` template: it proxies the domain through "+ - "Cloudflare, and a VM serves TLS itself (Caddy obtains the certificate via ACME, which a "+ - "proxied domain breaks). Remove the field, or put the machine behind Cloudflare outside SC", - TemplateTypeVmCompose, - ) - } + // `domainProxied` is NOT checked here, and the absence is deliberate. It used to be + // refused outright with "a VM serves TLS itself, and a proxied domain breaks the + // ACME challenge" — true of every vm resource up to `ingress.originTls`, and now + // true of only some of them. Whether it holds is a property of the PARENT stack's + // resource, and this function runs in the convert tier with no parent reference at + // all, so the check moved to where the answer exists: VmComposeStack, after + // hostsFromParent has read the resource's exports (clouds/pulumi/vm/compose_stack.go). + // + // The cost of moving it is that an operator sees the refusal at `sc deploy` rather + // than at config load. The alternative — keeping a blanket refusal here — makes the + // one front-door arrangement that needs no shared certificate store unreachable, + // which is the opposite trade. + if cfg.ClusterIPAddress != nil { return errors.Errorf( "`clusterIPAddress` is not applicable to the `%s` template: there is no cluster. The stack is "+ - "reachable at the machine's address", TemplateTypeVmCompose, + "reachable at the address of each machine it was deployed onto", TemplateTypeVmCompose, ) } if s := cfg.Scale; s != nil { if s.Max > 1 { - return errors.Errorf( - "`scale: {min: %d, max: %d}` is not supported by the `%s` template: one machine runs one "+ - "replica of each service. Use `scale: {min: 1, max: 1}`, or move the stack to a k3s/Kubernetes "+ - "parent, which is what horizontal scaling needs", s.Min, s.Max, TemplateTypeVmCompose, - ) + // A warning and not a refusal, since a fleet made `scale.max > 1` an + // expressible thing to want: the stack gets one replica of each service + // per machine, so a three-machine vm resource genuinely serves three. + // What this tier cannot do is CHECK the number — the machines are declared + // in the PARENT stack, and a client stack's config is validated without + // it. So the number is compared against the real host count one tier + // down, in VmComposeStack, which is the first place both are in hand; here + // we only say where the replica count actually comes from. + i.warn("`scale: {min: %d, max: %d}`: this template runs one replica of each service per MACHINE, "+ + "so the replica count is the number of hosts on the `%s` resource in the parent stack, not this "+ + "field. Add hosts there to get %d, or move the stack to a k3s/Kubernetes parent, which is what "+ + "scaling WITHIN a machine needs.", s.Min, s.Max, ResourceTypeVm, s.Max) } if s.Min == 0 { i.warn("`scale.min: 0` has no effect on a VM: containers are started with `restart: always` " + "and stay up. There is no scale-to-zero on this delivery path.") } if s.Policy != nil { - i.warn("`scale.policy` is ignored on a VM: autoscaling needs more than one node to scale onto.") + i.warn("`scale.policy` is ignored on a VM: a fleet's size is declared in the parent stack and " + + "fixed at provision time. Nothing here grows or shrinks it in response to load.") } } if lo.FromPtr(cfg.StaticEgressIP) { @@ -443,7 +459,8 @@ func (i *VmComposeInput) finalize() error { // unhealthy`, then a rolled-back deploy, with the container answering // on 127.0.0.1:8080 the whole time. Test: []string{"CMD-SHELL", fmt.Sprintf( - "nc -z 127.0.0.1 %d || nc -z ::1 %d || exit 1", port, port)}, + "nc -z 127.0.0.1 %d || nc -z ::1 %d || exit 1", port, port, + )}, Interval: "10s", Timeout: "3s", StartPeriod: "20s", diff --git a/pkg/clouds/vm/convert_test.go b/pkg/clouds/vm/convert_test.go index 375f31052..8ca6c5a85 100644 --- a/pkg/clouds/vm/convert_test.go +++ b/pkg/clouds/vm/convert_test.go @@ -110,21 +110,13 @@ func TestToVmComposeConfigRefusals(t *testing.T) { stackCfg *api.StackConfigCompose msg string }{ - { - name: "domainProxied breaks ACME", - stackCfg: &api.StackConfigCompose{ - Runs: []string{"backend"}, DomainProxied: lo.ToPtr(true), - }, - msg: "`domainProxied` is not supported", - }, - { - // Even `false` is refused: accepting it would imply SC understood the field. - name: "domainProxied false is still refused", - stackCfg: &api.StackConfigCompose{ - Runs: []string{"backend"}, DomainProxied: lo.ToPtr(false), - }, - msg: "`domainProxied` is not supported", - }, + // `domainProxied` was refused here in both senses until `ingress.originTls` + // existed. It is now decided at deploy time against the parent resource — + // TestVmCompose_DomainProxiedNeedsNoOriginTls in clouds/pulumi/vm covers both + // outcomes — because this tier cannot see the parent. The two cases that used + // to live here are deliberately gone rather than relaxed to a warning: a + // warning on `domainProxied: true` would fire on the arrangement that is now + // correct. { name: "clusterIPAddress has no cluster", stackCfg: &api.StackConfigCompose{ @@ -132,13 +124,6 @@ func TestToVmComposeConfigRefusals(t *testing.T) { }, msg: "there is no cluster", }, - { - name: "scale.max > 1 needs more than one node", - stackCfg: &api.StackConfigCompose{ - Runs: []string{"backend"}, Scale: &api.StackConfigComposeScale{Min: 1, Max: 3}, - }, - msg: "move the stack to a k3s/Kubernetes parent", - }, } { t.Run(tt.name, func(t *testing.T) { _, err := ToVmComposeConfig(testTemplate(), readTestCompose(t), tt.stackCfg) @@ -171,6 +156,26 @@ func TestToVmComposeConfigWarnings(t *testing.T) { Expect(warnings).To(ContainElement(ContainSubstring("`alerts` is not wired"))) } +// `scale.max > 1` used to be refused outright, on the reasoning that one machine runs +// one replica. A vm resource can now hold a fleet, so the ask is satisfiable — but the +// machines are declared in the PARENT stack, which a client stack's config tier cannot +// see. So this tier states where the replica count comes from and the provisioning tier +// (VmComposeStack) compares it against the real host count, which is the first place +// both numbers exist. +func TestToVmComposeConfigScaleIsFleetGoverned(t *testing.T) { + RegisterTestingT(t) + + got, err := ToVmComposeConfig(testTemplate(), readTestCompose(t), &api.StackConfigCompose{ + Runs: []string{"backend"}, Scale: &api.StackConfigComposeScale{Min: 1, Max: 3}, + }) + Expect(err).ToNot(HaveOccurred()) + + warnings := toInput(got).Warnings + Expect(warnings).To(ContainElement(ContainSubstring("one replica of each service per MACHINE"))) + // The remedy names the place the number actually lives, not just the problem. + Expect(warnings).To(ContainElement(ContainSubstring(string(ResourceTypeVm)))) +} + func TestToVmComposeConfigMountRefusals(t *testing.T) { RegisterTestingT(t) @@ -352,19 +357,57 @@ func TestToVmComposeSingleImageConfig(t *testing.T) { Expect(input.HealthChecks[SingleImageService].Synthesized).To(BeTrue()) } +// Every target reads `domainProxied` off the COMPOSE config, so a field that the +// single-image conversion forgets to carry is a field the operator wrote and SC +// ignored. That is not a cosmetic loss on a vm fleet: `domainProxied: true` with +// `ingress.originTls: false` is the whole front door, and an unproxied record points +// clients straight at an origin that only speaks plain HTTP. +// +// Live-caught 2026-11-08 on the 3-VPS compose smoke — `domainProxied: true` in the +// client.yaml produced three `proxied=false` A records and no error anywhere. +func TestToVmComposeSingleImageConfig_CarriesDomainProxied(t *testing.T) { + RegisterTestingT(t) + + for _, tt := range []struct { + name string + in *bool + }{ + {name: "proxied", in: lo.ToPtr(true)}, + {name: "explicitly not proxied", in: lo.ToPtr(false)}, + // Unset must stay unset rather than becoming false: the readers all use + // lo.FromPtr, but a nil is what lets a future default change behave. + {name: "unset", in: nil}, + } { + t.Run(tt.name, func(t *testing.T) { + got, err := ToVmComposeSingleImageConfig(testTemplate(), &api.StackConfigSingleImage{ + Image: &api.ContainerImage{Name: "simplecontainer/atrium:latest"}, + Domain: "smoke.simple-container.com", + DomainProxied: tt.in, + }) + Expect(err).ToNot(HaveOccurred()) + Expect(toInput(got).Deployment.StackConfig.DomainProxied).To(Equal(tt.in)) + }) + } +} + func TestToVmComposeSingleImageConfigRefusalsAndWarnings(t *testing.T) { RegisterTestingT(t) _, err := ToVmComposeSingleImageConfig(testTemplate(), &api.StackConfigSingleImage{}) Expect(err).To(MatchError(ContainSubstring("`image` is required"))) - // `min`/`max` project onto `scale`, so the one-replica-per-machine rule applies to - // a single-image stack through the same gate. - _, err = ToVmComposeSingleImageConfig(testTemplate(), &api.StackConfigSingleImage{ + // `min`/`max` project onto `scale`, so the one-replica-per-machine rule reaches a + // single-image stack through the same gate — as a warning now that a vm resource + // can hold more than one machine, since the host count that decides whether the + // ask is met lives in the parent stack and is not visible from here. + gotScaled, err := ToVmComposeSingleImageConfig(testTemplate(), &api.StackConfigSingleImage{ Image: &api.ContainerImage{Name: "atrium:1"}, Min: 1, Max: 3, }) - Expect(err).To(MatchError(ContainSubstring("move the stack to a k3s/Kubernetes parent"))) + Expect(err).ToNot(HaveOccurred()) + Expect(toInput(gotScaled).Warnings).To(ContainElement( + ContainSubstring("one replica of each service per MACHINE"), + )) got, err := ToVmComposeSingleImageConfig(testTemplate(), &api.StackConfigSingleImage{ Image: &api.ContainerImage{Name: "atrium:1"}, diff --git a/pkg/clouds/vm/render.go b/pkg/clouds/vm/render.go index f019562fe..676e1c6c6 100644 --- a/pkg/clouds/vm/render.go +++ b/pkg/clouds/vm/render.go @@ -54,6 +54,16 @@ type RenderOptions struct { // Caddy exists (P5.2) and for `ingress.enable: false` (§8.4); 0 publishes nothing // and leaves the service reachable only over the docker network. IngressHostPort int + // NoOriginTls renders the site on `http://` instead of on the bare domain, + // because `ingress.originTls: false` is set on the vm resource this stack deploys + // onto and the machine terminates no TLS. + // + // It arrives here as a field rather than being read off the vm config because + // RenderCaddySite is given the STACK's input and the flag belongs to the parent's + // resource; the deploy reads it out of the parent's exports and passes it down. + // Negative-named to match that export, where the zero value has to mean the + // behaviour every resource had before the field existed. + NoOriginTls bool } // RenderedCompose is what the deploy writes to the machine. diff --git a/pkg/clouds/vm/vm.go b/pkg/clouds/vm/vm.go index a39156df9..c350a19c4 100644 --- a/pkg/clouds/vm/vm.go +++ b/pkg/clouds/vm/vm.go @@ -1,8 +1,8 @@ // SPDX-License-Identifier: MIT // Copyright (c) Simple Container -// Package vm is the cloud-independent description of a virtual machine (and, later, -// of a pool of them). It is the config tier behind three delivery paths that all +// Package vm is the cloud-independent description of a virtual machine, or of a +// pool of them. It is the config tier behind three delivery paths that all // want the same thing from a machine and would otherwise each grow their own copy // of cloud-init, SSH readiness and key handling: // @@ -117,8 +117,31 @@ type VmNetwork struct { // stacks share one machine and therefore one port 443 (compose-on-vm.md §3, D15). type VmIngress struct { Enable bool `json:"enable" yaml:"enable"` - // Email is the ACME account address. Required when Enable is set, because a - // certificate authority will not issue without one. + // OriginTls is whether THIS machine terminates TLS. Unset means true, which is + // every vm resource that existed before this field. + // + // Set it to false when something in front already terminates — Cloudflare's + // proxy, a cloud load balancer, an existing reverse proxy — and Caddy then serves + // plain HTTP on port 80 and runs no ACME at all. That is the ONE front-door + // arrangement a fleet can have without a shared certificate store (see + // AcmeStorage, which explains why per-machine ACME behind N A records cannot + // work), so on a cloud with no bucket resource and no appetite for running minio + // it is not a downgrade, it is the option. + // + // It is also what makes `domainProxied: true` legal on a stack: that refusal + // exists because a proxied domain breaks the HTTP-01 challenge Caddy runs itself, + // and here Caddy runs none. The refusal therefore moved out of the convert tier + // (which cannot see this field — it belongs to the parent stack) and into the + // deploy, where the parent's exports are readable. + // + // What it does NOT do is secure the hop. Edge-to-origin is cleartext over the + // public internet unless the machine's firewall admits only the edge's addresses + // — Cloudflare publishes theirs, and `ingressPorts` on this resource is where + // they go. Said plainly because "my site is HTTPS" is true for the visitor and + // false for the leg SC is responsible for. + OriginTls *bool `json:"originTls,omitempty" yaml:"originTls,omitempty"` + // Email is the ACME account address. Required when Enable is set and this machine + // terminates TLS, because a certificate authority will not issue without one. Email string `json:"email,omitempty" yaml:"email,omitempty"` // Image is the Caddy image to run. Defaults to the same `simplecontainer/caddy` // tag the Kubernetes edge uses, which is what makes `lbConfig.siteExtraHelpers` @@ -247,6 +270,20 @@ func (r *VmConfig) AcmeStorageEnabled() bool { return r.IngressEnabled() && r.Ingress.AcmeStorage != nil && r.Ingress.AcmeStorage.Bucket != "" } +// OriginTlsEnabled reports whether this machine's Caddy terminates TLS. +// +// Defaults to true on an unset field and on no ingress at all, so that every caller +// reads "yes" for every resource that predates `ingress.originTls` — the flag only ever +// turns behaviour off, never on. The parent stack exports the NEGATION of this for the +// same reason (ToIngressNoOriginTlsExport): a child reading a parent that has never +// heard of the field gets a zero bool, and the zero has to mean the old behaviour. +func (r *VmConfig) OriginTlsEnabled() bool { + if !r.IngressEnabled() { + return true + } + return lo.FromPtrOr(r.Ingress.OriginTls, true) +} + // UsePathStyle resolves PathStyle's default: on whenever there is an endpoint. func (s *VmAcmeStorage) UsePathStyle() bool { if s.PathStyle != nil { @@ -301,6 +338,75 @@ type VmHost struct { // Not a secret: it is the machine's PUBLIC key, and keeping it readable is what // lets an operator compare it against the console by eye. HostKey string `json:"hostKey,omitempty" yaml:"hostKey,omitempty"` + + // Role is what this machine is prepared for: one of the HostRole values + // (`compose`, `k3s-server`, `k3s-agent`). + // + // It exists because SC cannot infer which of three machines should run etcd. On + // a cloud driver the machines are interchangeable — SC creates them, so it + // assigns the roles by index. On byo they are not: the operator knows which box + // has the disk, the memory and the stable address, and that knowledge has no + // other way into the config. + // + // Optional, and optional in a load-bearing way: when every host leaves it empty + // the role is inferred from what consumes the vm resource, which is exactly + // what every single-host stack written before this field did (see + // EffectiveRole). Either every host names a role or none does — a pool where + // host 0 says `k3s-server` and host 1 says nothing is a pool whose host 1 is a + // guess, and the guess decides whether it joins the cluster at all. + Role string `json:"role,omitempty" yaml:"role,omitempty"` + + // PrivateAddress is the address this machine's SIBLINGS should use to reach it. + // `address` is what SC dials from outside; this is what the fleet dials from + // inside, and on a multi-node k3s pool they are not interchangeable. + // + // Required for a multi-host k3s pool, and the reason is worth stating here + // rather than only in the error: k3s is told nothing about addressing by SC + // (`--node-ip`, `--node-external-ip` and `--flannel-iface` are set nowhere), so + // it defaults node-ip to the address of the default-route interface. On a cloud + // VM reached through a floating/NAT IP that is already the private address and + // everything is correct. On a single-NIC VPS with a directly attached public IP + // it is the PUBLIC address — and then etcd peer traffic (the cluster's entire + // state, every Secret) and flannel's VXLAN overlay (unencrypted; see + // pkg/clouds/k3s/cluster.go) cross the public internet, on a `byo` host that has + // no firewall to stop them (pkg/clouds/pulumi/vm/byo.go). That deploy SUCCEEDS, + // which is why this is a refusal and not a warning. + // + // Not required for a compose fleet: those machines never talk to each other (the + // only shared state is the ACME store, reached outbound over HTTPS), so there is + // no node-to-node traffic to misdirect. + // + // Hosters without private networking have no value to put here. That case needs + // an SC-managed WireGuard mesh, which does not exist yet — see the roadmap item + // named in the error, and forge design 2026-11-07/multi-node-clusters §5.8. + PrivateAddress string `json:"privateAddress,omitempty" yaml:"privateAddress,omitempty"` +} + +// EffectivePrivateAddress is the address siblings of this machine should dial: +// `privateAddress` when the operator named one, else `address`. +// +// The fallback is only reached on shapes that do not need the distinction — a +// compose fleet, or a single-host pool — because validateHostRoles refuses a +// multi-host k3s pool that names none (see VmHost.PrivateAddress). +func (h VmHost) EffectivePrivateAddress() string { + if h.PrivateAddress != "" { + return h.PrivateAddress + } + return h.Address +} + +// EffectiveRole resolves the machine's role, falling back to RoleCompose — which +// is what the provisioning tier hardcoded for every byo host before `role:` +// existed, so a config that names no role behaves exactly as it did. +// +// A consumer that derives roles itself (the k3s cluster resource, which assigns by +// index when the pool declares none) must ask DeclaresHostRoles first: this method +// cannot distinguish "the operator said compose" from "the operator said nothing". +func (h VmHost) EffectiveRole() HostRole { + if h.Role == "" { + return RoleCompose + } + return HostRole(h.Role) } // EffectiveUser resolves the login name: the host's own, else the resource-level @@ -330,6 +436,18 @@ type VmConfig struct { Region string `json:"region,omitempty" yaml:"region,omitempty"` Zones []string `json:"zones,omitempty" yaml:"zones,omitempty"` + // Nodes is how many machines a cloud driver creates for this resource. Not + // valid with DriverByo, where the count is `len(hosts)`. + // + // Default (see NodeCount): `len(zones)` when zones are listed, else 1. So + // `zones: [a, b, c]` alone means three machines, one per zone, which is the + // common case and needs no second field — and `nodes: 3` with one zone means + // three machines in that zone, which is the other one. + // + // Machine i goes in zone i%len(zones) (ZoneForNode): deterministic and + // index-stable, so a later apply that raises the count never moves an existing + // machine to a different zone. Moving one would mean destroying it. + Nodes int `json:"nodes,omitempty" yaml:"nodes,omitempty"` // Image is an SC alias (see DefaultImage), resolved per cloud by the driver. Image string `json:"image,omitempty" yaml:"image,omitempty"` Size *VmSize `json:"size,omitempty" yaml:"size,omitempty"` @@ -481,13 +599,48 @@ func (r *VmConfig) validateIngress() error { if !r.IngressEnabled() { return nil } - if r.Ingress.Email == "" { - return errors.Errorf("`ingress.enable: true` requires `ingress.email` (the ACME account address)") - } if d := r.EffectiveIngressImageDelivery(); d != ImageDeliveryRegistry && d != ImageDeliverySshTar { return errors.Errorf("unknown `ingress.imageDelivery` %q; one of: %s, %s", d, ImageDeliveryRegistry, ImageDeliverySshTar) } + if !r.OriginTlsEnabled() { + // Every ACME setting is refused rather than ignored, which is this file's + // standing rule and earns its keep here: an operator who sets `originTls: + // false` and leaves `acmeStorage` behind believes the fleet still holds a + // certificate, and would not find out otherwise until something in front of + // the machine went away. The remedy is one deleted line and the message names + // it. + // + // `email` is in the list because it has no other purpose on this path — it is + // the ACME account address and there is no ACME account. + for _, f := range []struct{ name, value string }{ + {"email", r.Ingress.Email}, + {"acmeCa", r.Ingress.AcmeCa}, + {"acmeEabKeyId", r.Ingress.AcmeEabKeyID}, + {"acmeEabMacKey", r.Ingress.AcmeEabMacKey}, + } { + if f.value != "" { + return errors.Errorf( + "`ingress.%s` is set together with `ingress.originTls: false`: this machine serves plain "+ + "HTTP and requests no certificate, so nothing would read it. Remove `ingress.%s`, or "+ + "remove `originTls: false` and let Caddy terminate TLS itself", f.name, f.name, + ) + } + } + if r.Ingress.AcmeStorage != nil { + return errors.Errorf( + "`ingress.acmeStorage` is set together with `ingress.originTls: false`: the shared store holds " + + "certificates and ACME challenge state, and this machine requests neither. These are the two " + + "alternative front doors for a fleet, not a pair — keep the store and drop `originTls: false` " + + "to serve HTTPS from the machines, or keep `originTls: false` and drop the store to serve " + + "plain HTTP behind an edge that terminates", + ) + } + return nil + } + if r.Ingress.Email == "" { + return errors.Errorf("`ingress.enable: true` requires `ingress.email` (the ACME account address)") + } if (r.Ingress.AcmeEabKeyID == "") != (r.Ingress.AcmeEabMacKey == "") { return errors.Errorf( "`ingress.acmeEabKeyId` and `ingress.acmeEabMacKey` must be set together: External Account " + @@ -636,6 +789,11 @@ func (r *VmConfig) validateByo() error { if len(r.Zones) > 0 { ignored = append(ignored, "zones") } + // The pool's size is `len(hosts)` here: the machines already exist, so a second + // spelling of the count could only ever contradict the first. + if r.Nodes > 0 { + ignored = append(ignored, "nodes") + } if r.Image != "" { ignored = append(ignored, "image") } @@ -677,15 +835,6 @@ func (r *VmConfig) validateByo() error { if len(r.Hosts) == 0 { return errors.Errorf("`provider: %s` requires at least one entry under `hosts`", DriverByo) } - // One machine is all P5 (compose on a single VM) can place a stack on, and a - // multi-host byo pool only becomes meaningful with k3s. Refusing now is cheaper - // than deploying to the first host and leaving the rest idle. - if len(r.Hosts) > 1 { - return errors.Errorf( - "`provider: %s` currently supports exactly one host, got %d; multi-host pools arrive with the "+ - "k3s cluster resource (roadmap item sc-k3s-cluster-resource)", DriverByo, len(r.Hosts), - ) - } for i, h := range r.Hosts { if h.Address == "" { return errors.Errorf("hosts[%d] requires `address`", i) @@ -700,9 +849,226 @@ func (r *VmConfig) validateByo() error { return errors.Errorf("hosts[%d] (%s) has an invalid `port`: %d", i, h.Address, h.Port) } } + if err := r.validateHostRoles(); err != nil { + return err + } + if dup, ok := firstDuplicateAddress(r.Hosts); ok { + // Two entries for one machine is never what was meant, and it fails late and + // confusingly: both get the same cloud-init, the same ingress, and — for a k3s + // pool — the same node name, so the second node silently replaces the first in + // `kubectl get nodes` and the cluster reports N-1 members for an N-host pool. + return errors.Errorf("`hosts` lists %q more than once; one entry per machine", dup) + } return r.validateIngress() } +// DeclaresHostRoles reports whether the operator named `hosts[].role` at all. +// +// It is the question a consumer that derives roles itself has to ask before +// trusting VmHost.EffectiveRole, which cannot distinguish "the operator said +// compose" from "the operator said nothing" — RoleCompose is the answer to both. +// Validation guarantees all-or-none (validateHostRoles), so one host naming a role +// is enough to know the whole pool did. +func (r *VmConfig) DeclaresHostRoles() bool { + return lo.SomeBy(r.Hosts, func(h VmHost) bool { return h.Role != "" }) +} + +// validateHostRoles checks the role set of a byo pool as a whole. +// +// Per-host spellings are checked here too, but the rules that matter are the ones +// about the set — a pool is a single deployment target, and the shapes refused +// below each produce a deploy that half-works rather than an error. +func (r *VmConfig) validateHostRoles() error { + for i, h := range r.Hosts { + if h.Role == "" { + continue + } + if !lo.Contains(AllHostRoles, HostRole(h.Role)) { + return errors.Errorf( + "hosts[%d] (%s) has an unknown `role`: %q; expected one of %s", + i, h.Address, h.Role, strings.Join(quoteAll(lo.Map(AllHostRoles, func(role HostRole, _ int) string { + return string(role) + })), ", "), + ) + } + } + if !r.DeclaresHostRoles() { + // No role anywhere: inferred, exactly as before this field existed. A + // single-host config written against P5 lands here and is unaffected. + return nil + } + // All or none. See VmHost.Role — a half-declared pool is a pool whose remaining + // hosts are a guess, and the guess decides whether a machine joins the cluster. + missing := lo.FilterMap(r.Hosts, func(h VmHost, i int) (string, bool) { + return fmt.Sprintf("hosts[%d] (%s)", i, h.Address), h.Role == "" + }) + if len(missing) > 0 { + return errors.Errorf( + "either every entry under `hosts` names a `role` or none does; %s %s none", + strings.Join(missing, ", "), lo.If(len(missing) == 1, "names").Else("name"), + ) + } + k3sHosts := lo.Filter(r.Hosts, func(h VmHost, _ int) bool { return h.EffectiveRole().IsK3s() }) + if len(k3sHosts) == 0 { + // An all-compose fleet: every machine runs the full container set. Nothing + // further to check — the front door is the registrar's problem, not this + // validator's. + return nil + } + // A machine serves one thing on port 443. Mixing the two roles in one resource + // means the compose ingress and k3s both want :80 and :443 on the pool, and + // which one gets them depends on which host a request's DNS answer landed on — + // the single-node reasoning behind the servicelb/traefik disable logic in + // pkg/clouds/pulumi/k3s/cluster.go, now a cross-host rule. + if len(k3sHosts) != len(r.Hosts) { + return errors.Errorf( + "`hosts` mixes %q with k3s roles; one vm resource is one deployment target, so declare the "+ + "compose fleet and the k3s pool as two resources", RoleCompose, + ) + } + servers := lo.CountBy(r.Hosts, func(h VmHost) bool { return h.EffectiveRole() == RoleK3sServer }) + if servers == 0 { + return errors.Errorf( + "`hosts` declares %d k3s agent(s) and no %q; a cluster needs at least one control-plane node", + len(r.Hosts), RoleK3sServer, + ) + } + // etcd needs a quorum of its members. Two servers tolerate zero failures — + // strictly worse than one, because either machine going away takes the API + // server down — so an even count above one is always a mistake. + if servers > 1 && servers%2 == 0 { + return errors.Errorf( + "`hosts` declares %d %q; an embedded-etcd cluster needs an odd number (1, 3, 5) — %d members "+ + "tolerate no failures at all, so they are strictly worse than 1", + servers, RoleK3sServer, servers, + ) + } + // Node-to-node addressing. A single-node cluster has no node-to-node traffic, so + // it is unaffected and every pre-existing single-host k3s config stays valid. + // + // Refused rather than warned because the failure mode is a SUCCESS: the cluster + // comes up, `kubectl get nodes` is green, and etcd plus an unencrypted VXLAN + // overlay are on the public internet. See VmHost.PrivateAddress for why SC + // cannot infer the value — k3s picks the default-route interface and `HostFacts` + // carries no private address to override it with. + if len(r.Hosts) > 1 { + noPrivate := lo.FilterMap(r.Hosts, func(h VmHost, i int) (string, bool) { + return fmt.Sprintf("hosts[%d] (%s)", i, h.Address), h.PrivateAddress == "" + }) + if len(noPrivate) > 0 { + return errors.Errorf( + "a multi-node k3s pool requires `privateAddress` on every entry under `hosts`, and %s %s "+ + "none. Without it k3s advertises whichever address the machine's default route uses — the "+ + "public one on a single-NIC VPS — so etcd peer traffic and flannel's unencrypted VXLAN "+ + "overlay would cross the public internet, and `provider: %s` has no firewall to stop them. "+ + "That deploy succeeds, which is why this is refused rather than warned about. Give each host "+ + "the address its siblings reach it on; if these machines share no private network, this "+ + "topology needs the WireGuard mesh of roadmap item sc-vm-multi-node-clusters, which is not "+ + "built yet — declare a compose fleet instead, which needs no node-to-node traffic at all", + strings.Join(noPrivate, ", "), lo.If(len(noPrivate) == 1, "names").Else("name"), DriverByo, + ) + } + if dup, ok := firstDuplicatePrivateAddress(r.Hosts); ok { + // Same reasoning as the duplicate `address` check above, and the same + // late-and-confusing failure: two nodes claiming one node-ip. + return errors.Errorf("`hosts` gives %q as the `privateAddress` of more than one machine", dup) + } + } + return nil +} + +// firstDuplicateAddress returns the first address that appears twice, in the order +// the hosts are written. +func firstDuplicateAddress(hosts []VmHost) (string, bool) { + seen := make(map[string]bool, len(hosts)) + for _, h := range hosts { + if seen[h.Address] { + return h.Address, true + } + seen[h.Address] = true + } + return "", false +} + +// firstDuplicatePrivateAddress is firstDuplicateAddress over `privateAddress`. +// Empty values are skipped: a pool that names none is either refused by +// validateHostRoles or does not need them, and treating "" as a collision would +// turn both of those into this error instead. +func firstDuplicatePrivateAddress(hosts []VmHost) (string, bool) { + seen := make(map[string]bool, len(hosts)) + for _, h := range hosts { + if h.PrivateAddress == "" { + continue + } + if seen[h.PrivateAddress] { + return h.PrivateAddress, true + } + seen[h.PrivateAddress] = true + } + return "", false +} + +// NodeCount is how many machines this resource describes. +// +// It is the one place the default lives, so a driver never computes it from +// `nodes` and `zones` itself — two drivers disagreeing about whether `zones: [a, +// b]` means one machine or two would be a fleet that is one size in preview and +// another after apply. +func (r *VmConfig) NodeCount() int { + switch { + case r.Driver == DriverByo: + return len(r.Hosts) + case r.Nodes > 0: + return r.Nodes + case len(r.Zones) > 0: + return len(r.Zones) + default: + return 1 + } +} + +// ZoneForNode is where machine i goes: round-robin over `zones`, empty when the +// operator listed none (the driver then picks, normally the cloud's default zone). +// +// Index-stable on purpose — see VmConfig.Nodes. Raising `nodes` from 3 to 5 must +// leave machines 0..2 exactly where they are, because a machine cannot change zone +// without being destroyed and recreated. +func (r *VmConfig) ZoneForNode(i int) string { + if len(r.Zones) == 0 || i < 0 { + return "" + } + return r.Zones[i%len(r.Zones)] +} + +// validateNodeCount checks the shape of a cloud-created pool: how many machines, +// across which zones, and whether the cloud has that many zones to spread over. +func (r *VmConfig) validateNodeCount(drv DriverInfo) error { + if r.Nodes < 0 { + return errors.Errorf("`nodes` must be positive, got %d", r.Nodes) + } + // Fewer machines than zones means a zone nobody is placed in — an ignored + // field, which this package treats as a future incident rather than a + // convenience. The operator either wanted more machines or fewer zones, and + // guessing which would silently pick one. + if r.Nodes > 0 && len(r.Zones) > r.Nodes { + return errors.Errorf( + "`nodes` is %d but `zones` lists %d; every zone listed must hold at least one machine, so either "+ + "raise `nodes` or shorten `zones`", r.Nodes, len(r.Zones), + ) + } + // Capabilities.MultiAZInRegion is zero for "not measured yet" and the field's + // own doc says not to validate against it until a driver fills it in — so an + // unmeasured cloud is not refused here, it is simply not checked. + if az := drv.Capabilities.MultiAZInRegion; az > 0 && len(r.Zones) > az { + return errors.Errorf( + "`zones` lists %d entries but `provider: %s` has %d usable zone(s) in a region; placing a machine "+ + "in a zone the cloud does not have fails at create time, after the rest of the pool is billed", + len(r.Zones), r.Driver, az, + ) + } + return nil +} + // validateCloud holds the invariants of every driver that creates a machine. They // live here, once, rather than being rediscovered per cloud — the first driver // (`cloudru-vm`) is written against exactly these. @@ -766,14 +1132,8 @@ func (r *VmConfig) validateCloud(drv DriverInfo) error { "bastion or VPN path to a machine without a public address", r.Driver, ) } - // One zone, because one machine. Several zones only mean something once a vm - // resource can describe a pool, which arrives with the k3s cluster resource. - if len(r.Zones) > 1 { - return errors.Errorf( - "`provider: %s` currently supports at most one entry under `zones`, got %d; multi-zone pools "+ - "arrive with the k3s cluster resource (roadmap item sc-k3s-cluster-resource)", - r.Driver, len(r.Zones), - ) + if err := r.validateNodeCount(drv); err != nil { + return err } // `network.id` without the matching subnet is the one shape of a BYO network that // cannot be completed by guessing: a driver that picked "some subnet in the VPC" diff --git a/pkg/clouds/vm/vm_test.go b/pkg/clouds/vm/vm_test.go index 50ff7e4ff..574297474 100644 --- a/pkg/clouds/vm/vm_test.go +++ b/pkg/clouds/vm/vm_test.go @@ -92,13 +92,25 @@ func TestValidate(t *testing.T) { errMatch: "requires at least one entry under `hosts`", }, { - // Refused rather than deployed-to-the-first-host: a silently idle second - // machine is exactly the kind of thing nobody notices for a month. + // Accepted since multi-node (roadmap item sc-vm-multi-node-clusters): a byo + // pool with no role declared anywhere is a compose fleet, which is what the + // provisioning tier has looped over all along. This case used to assert the + // opposite — "supports exactly one host, got 2" — and the role-set rules + // that replaced it are in TestValidateHostRoles. name: "byo with two hosts", mutate: func(c *VmConfig) { c.Hosts = append(c.Hosts, VmHost{Address: "203.0.113.11", PrivateKey: "k"}) }, - errMatch: "supports exactly one host, got 2", + errMatch: "", + }, + { + // Two entries for one machine: the same cloud-init, the same ingress and — + // in a k3s pool — the same node name, so the cluster reports N-1 members. + name: "byo with a repeated address", + mutate: func(c *VmConfig) { + c.Hosts = append(c.Hosts, VmHost{Address: c.Hosts[0].Address, PrivateKey: "k"}) + }, + errMatch: `lists "203.0.113.10" more than once`, }, { name: "host without address", @@ -326,6 +338,279 @@ func TestAcmeStorageDefaults(t *testing.T) { Expect(plain.AcmeStorageEnabled()).To(BeFalse()) } +// TestValidateHostRoles is the role set of a byo pool, which is what replaced the +// old "exactly one host" refusal (roadmap item sc-vm-multi-node-clusters, phase 1a). +// +// Every refused shape here is one that otherwise deploys and half-works: a machine +// left out of the cluster, two services fighting over :443, or an etcd membership +// that tolerates no failures while reading as HA. +func TestValidateHostRoles(t *testing.T) { + RegisterTestingT(t) + + host := func(address, role string) VmHost { + return VmHost{Address: address, PrivateKey: "k", Role: role} + } + + // k3sHost also names the address the machine's SIBLINGS dial it on. Every valid + // multi-node k3s shape has to: without it k3s advertises its default-route + // address, which on a single-NIC VPS is the public one (VmHost.PrivateAddress). + // The refused shapes below deliberately do NOT use this helper — the addressing + // check runs after the topology checks, so a pool that is the wrong shape says so + // rather than complaining about addresses it would never have used. + k3sHost := func(address, private, role string) VmHost { + h := host(address, role) + h.PrivateAddress = private + return h + } + + cases := []struct { + name string + hosts []VmHost + errMatch string + }{ + { + // The pre-existing shape: one host, no role. Still valid, still inferred. + name: "single host, no role", + hosts: []VmHost{host("203.0.113.10", "")}, + }, + { + name: "three hosts, no roles — an inferred compose fleet", + hosts: []VmHost{ + host("203.0.113.10", ""), host("203.0.113.11", ""), host("203.0.113.12", ""), + }, + }, + { + name: "explicit compose fleet", + hosts: []VmHost{ + host("203.0.113.10", string(RoleCompose)), host("203.0.113.11", string(RoleCompose)), + }, + }, + { + name: "one server, two agents — the default k3s topology", + hosts: []VmHost{ + k3sHost("203.0.113.10", "10.0.0.10", string(RoleK3sServer)), + k3sHost("203.0.113.11", "10.0.0.11", string(RoleK3sAgent)), + k3sHost("203.0.113.12", "10.0.0.12", string(RoleK3sAgent)), + }, + }, + { + name: "three servers — embedded-etcd HA", + hosts: []VmHost{ + k3sHost("203.0.113.10", "10.0.0.10", string(RoleK3sServer)), + k3sHost("203.0.113.11", "10.0.0.11", string(RoleK3sServer)), + k3sHost("203.0.113.12", "10.0.0.12", string(RoleK3sServer)), + }, + }, + { + // A single-node cluster has no node-to-node traffic, so it is exempt — and + // has to be, or every k3s config written before `privateAddress` existed + // would stop validating. + name: "single k3s server needs no private address", + hosts: []VmHost{host("203.0.113.10", string(RoleK3sServer))}, + }, + { + // The whole point of the refusal: this shape otherwise DEPLOYS, green, with + // etcd and an unencrypted VXLAN overlay on the public internet. + name: "multi-node k3s with no private addresses", + hosts: []VmHost{ + host("203.0.113.10", string(RoleK3sServer)), + host("203.0.113.11", string(RoleK3sAgent)), + }, + errMatch: "a multi-node k3s pool requires `privateAddress` on every entry under `hosts`, " + + "and hosts[0] (203.0.113.10), hosts[1] (203.0.113.11) name none", + }, + { + // All-or-none, same as `role`: one machine left on its default route is one + // machine advertising a public node-ip, which is the whole failure. + name: "multi-node k3s with one private address missing", + hosts: []VmHost{ + k3sHost("203.0.113.10", "10.0.0.10", string(RoleK3sServer)), + host("203.0.113.11", string(RoleK3sAgent)), + }, + errMatch: "and hosts[1] (203.0.113.11) names none", + }, + { + // Two nodes claiming one node-ip: the second overwrites the first in the + // cluster, and the symptom appears nowhere near the config. + name: "multi-node k3s with a repeated private address", + hosts: []VmHost{ + k3sHost("203.0.113.10", "10.0.0.10", string(RoleK3sServer)), + k3sHost("203.0.113.11", "10.0.0.10", string(RoleK3sAgent)), + }, + errMatch: "`hosts` gives \"10.0.0.10\" as the `privateAddress` of more than one machine", + }, + { + // A compose fleet is exempt: those machines never talk to each other. The + // only shared state is the ACME store, reached outbound over HTTPS. + name: "compose fleet needs no private addresses", + hosts: []VmHost{ + host("203.0.113.10", string(RoleCompose)), + host("203.0.113.11", string(RoleCompose)), + host("203.0.113.12", string(RoleCompose)), + }, + }, + { + name: "unknown role", + hosts: []VmHost{host("203.0.113.10", "k3s-master")}, + errMatch: `unknown ` + "`role`" + `: "k3s-master"; expected one of "compose", "k3s-server", "k3s-agent"`, + }, + { + // The shape the all-or-none rule exists for: host 1's role would have to be + // guessed, and the guess decides whether it joins the cluster at all. + name: "half-declared pool", + hosts: []VmHost{ + host("203.0.113.10", string(RoleK3sServer)), host("203.0.113.11", ""), + }, + errMatch: "either every entry under `hosts` names a `role` or none does; hosts[1] (203.0.113.11) names none", + }, + { + name: "half-declared pool names every host that is missing one", + hosts: []VmHost{ + host("203.0.113.10", string(RoleK3sServer)), host("203.0.113.11", ""), host("203.0.113.12", ""), + }, + errMatch: "hosts[1] (203.0.113.11), hosts[2] (203.0.113.12) name none", + }, + { + name: "compose mixed with k3s", + hosts: []VmHost{ + host("203.0.113.10", string(RoleK3sServer)), host("203.0.113.11", string(RoleCompose)), + }, + errMatch: `mixes "compose" with k3s roles`, + }, + { + name: "agents with no server", + hosts: []VmHost{ + host("203.0.113.10", string(RoleK3sAgent)), host("203.0.113.11", string(RoleK3sAgent)), + }, + errMatch: `declares 2 k3s agent(s) and no "k3s-server"`, + }, + { + // Two members tolerate zero failures, so they are strictly worse than one. + name: "two servers", + hosts: []VmHost{ + host("203.0.113.10", string(RoleK3sServer)), host("203.0.113.11", string(RoleK3sServer)), + }, + errMatch: "needs an odd number (1, 3, 5)", + }, + { + name: "four servers", + hosts: []VmHost{ + host("203.0.113.10", string(RoleK3sServer)), host("203.0.113.11", string(RoleK3sServer)), + host("203.0.113.12", string(RoleK3sServer)), host("203.0.113.13", string(RoleK3sServer)), + }, + errMatch: "needs an odd number (1, 3, 5)", + }, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + RegisterTestingT(t) + + cfg := byoConfig() + cfg.Hosts = tc.hosts + + err := cfg.Validate() + if tc.errMatch == "" { + Expect(err).ToNot(HaveOccurred()) + return + } + Expect(err).To(HaveOccurred()) + Expect(err.Error()).To(ContainSubstring(tc.errMatch)) + }) + } +} + +// EffectiveRole's fallback is what keeps every config written before `role:` existed +// producing the same HostFacts — pkg/clouds/pulumi/vm/byo.go hardcoded RoleCompose. +func TestEffectiveRole(t *testing.T) { + RegisterTestingT(t) + + Expect(VmHost{}.EffectiveRole()).To(Equal(RoleCompose)) + Expect(VmHost{Role: string(RoleK3sServer)}.EffectiveRole()).To(Equal(RoleK3sServer)) + Expect(VmHost{Role: string(RoleK3sAgent)}.EffectiveRole()).To(Equal(RoleK3sAgent)) + + Expect(RoleK3sServer.IsK3s()).To(BeTrue()) + Expect(RoleK3sAgent.IsK3s()).To(BeTrue()) + Expect(RoleCompose.IsK3s()).To(BeFalse()) + Expect(VmHost{}.EffectiveRole().IsK3s()).To(BeFalse()) +} + +// EffectivePrivateAddress falls back to `address`, which is what keeps every +// single-host and compose-fleet config written before `privateAddress` existed +// producing the same HostFacts. A multi-node k3s pool cannot reach the fallback — +// validateHostRoles refuses one that names no private addresses. +func TestEffectivePrivateAddress(t *testing.T) { + RegisterTestingT(t) + + Expect(VmHost{Address: "203.0.113.10"}.EffectivePrivateAddress()).To(Equal("203.0.113.10")) + Expect(VmHost{Address: "203.0.113.10", PrivateAddress: "10.0.0.10"}.EffectivePrivateAddress()). + To(Equal("10.0.0.10")) +} + +// NodeCount is the single place the pool's size is decided, so that two drivers +// can never disagree about whether `zones: [a, b]` means one machine or two. +func TestNodeCount(t *testing.T) { + RegisterTestingT(t) + + // byo: the machines exist, so the count is `len(hosts)` and `nodes` is refused. + Expect(byoConfig().NodeCount()).To(Equal(1)) + threeHosts := byoConfig() + threeHosts.Hosts = append(threeHosts.Hosts, + VmHost{Address: "203.0.113.11", PrivateKey: "k"}, VmHost{Address: "203.0.113.12", PrivateKey: "k"}) + Expect(threeHosts.NodeCount()).To(Equal(3)) + + // Cloud: zones alone are a count, `nodes` alone is a count, and neither is 1. + Expect(cloudConfig().NodeCount()).To(Equal(1)) + + zonesOnly := cloudConfig() + zonesOnly.Zones = []string{"ru-moscow-1a", "ru-moscow-1b", "ru-moscow-1c"} + Expect(zonesOnly.NodeCount()).To(Equal(3)) + + nodesOnly := cloudConfig() + nodesOnly.Nodes = 3 + Expect(nodesOnly.NodeCount()).To(Equal(3)) + Expect(nodesOnly.ZoneForNode(0)).To(BeEmpty(), "no zones listed: the driver picks") + + // More machines than zones: round-robin, and `nodes` wins over `len(zones)`. + both := cloudConfig() + both.Nodes = 5 + both.Zones = []string{"ru-moscow-1a", "ru-moscow-1b"} + Expect(both.NodeCount()).To(Equal(5)) + Expect(lo.Map(lo.Range(both.NodeCount()), func(i int, _ int) string { return both.ZoneForNode(i) })). + To(Equal([]string{"ru-moscow-1a", "ru-moscow-1b", "ru-moscow-1a", "ru-moscow-1b", "ru-moscow-1a"})) + + // Index stability is the whole reason the assignment is i%len(zones) rather + // than anything smarter: growing the pool must not move an existing machine, + // because a machine cannot change zone without being destroyed. + grown := cloudConfig() + grown.Nodes = 7 + grown.Zones = both.Zones + for i := range both.NodeCount() { + Expect(grown.ZoneForNode(i)).To(Equal(both.ZoneForNode(i)), "machine %d moved zone when the pool grew", i) + } +} + +// The zone count is checked against the cloud's own measured capability — and NOT +// checked when that capability is still zero, which Capabilities.MultiAZInRegion +// defines as "not measured yet" rather than "none". +func TestValidateNodeCountAgainstZoneCapability(t *testing.T) { + RegisterTestingT(t) + + cfg := cloudConfig() + cfg.Zones = []string{"ru-moscow-1a", "ru-moscow-1b", "ru-moscow-1c"} + + unmeasured := DriverInfo{Capabilities: Capabilities{MultiAZInRegion: 0}} + Expect(cfg.validateNodeCount(unmeasured)).ToNot(HaveOccurred()) + + roomy := DriverInfo{Capabilities: Capabilities{MultiAZInRegion: 3}} + Expect(cfg.validateNodeCount(roomy)).ToNot(HaveOccurred()) + + tight := DriverInfo{Capabilities: Capabilities{MultiAZInRegion: 2}} + err := cfg.validateNodeCount(tight) + Expect(err).To(HaveOccurred()) + Expect(err.Error()).To(ContainSubstring("`zones` lists 3 entries but `provider: cloudru-vm` has 2 usable zone(s)")) +} + // A cloud driver's own invariants are checked before the not-implemented refusal: // an operator whose config is also wrong in shape should hear about the shape, so // that the config is already right when the driver lands. @@ -373,9 +658,27 @@ func TestValidateCloud(t *testing.T) { errMatch: "", }, { + // Accepted since multi-node (roadmap item sc-vm-multi-node-clusters): two + // zones means two machines, one each. This case asserted the refusal that + // used to stand here; the pool-shape rules are in TestValidateNodeCount. name: "two zones", mutate: func(c *VmConfig) { c.Zones = []string{"ru-moscow-1a", "ru-moscow-1b"} }, - errMatch: "at most one entry under `zones`, got 2", + errMatch: "", + }, + { + name: "negative node count", + mutate: func(c *VmConfig) { c.Nodes = -1 }, + errMatch: "`nodes` must be positive, got -1", + }, + { + // A zone nobody is placed in: the operator wanted either more machines or + // fewer zones, and guessing which silently picks one. + name: "fewer nodes than zones", + mutate: func(c *VmConfig) { + c.Nodes = 2 + c.Zones = []string{"ru-moscow-1a", "ru-moscow-1b", "ru-moscow-1c"} + }, + errMatch: "`nodes` is 2 but `zones` lists 3", }, { // Half a BYO network is the one shape that cannot be completed by @@ -647,3 +950,100 @@ func TestValidateRefusesUnverifiedProvider(t *testing.T) { Expect(err.Error()).To(ContainSubstring("`allowUnverifiedProvider` is not valid")) Expect(err.Error()).To(ContainSubstring("live-verified")) } + +// `ingress.originTls: false` and the ACME settings are alternatives, not a pair. Each +// refusal below replaces a belief an operator would otherwise keep: that the fleet +// still holds a certificate, or that a CA override still applies. Neither is visible +// until the thing in front of the machine goes away. +func TestValidateOriginTls(t *testing.T) { + RegisterTestingT(t) + + noTls := func(mutate func(*VmIngress)) *VmConfig { + cfg := byoConfig() + cfg.Ingress = &VmIngress{Enable: true, OriginTls: lo.ToPtr(false)} + mutate(cfg.Ingress) + return cfg + } + + // The whole point of the field: `email` stops being required, because there is no + // ACME account to name. + Expect(noTls(func(*VmIngress) {}).Validate()).ToNot(HaveOccurred()) + Expect(noTls(func(*VmIngress) {}).OriginTlsEnabled()).To(BeFalse()) + + for _, tc := range []struct { + name string + mutate func(*VmIngress) + errMatch string + }{ + { + name: "email", + mutate: func(i *VmIngress) { i.Email = "ops@example.com" }, + errMatch: "`ingress.email` is set together with `ingress.originTls: false`", + }, + { + name: "acmeCa", + mutate: func(i *VmIngress) { i.AcmeCa = "https://acme.example.com/directory" }, + errMatch: "`ingress.acmeCa` is set together with `ingress.originTls: false`", + }, + { + name: "acmeEab", + mutate: func(i *VmIngress) { + i.AcmeEabKeyID = "kid" + i.AcmeEabMacKey = "mac" + }, + errMatch: "`ingress.acmeEabKeyId` is set together with `ingress.originTls: false`", + }, + { + name: "acmeStorage", + mutate: func(i *VmIngress) { + i.AcmeStorage = &VmAcmeStorage{Bucket: "sc-acme", Region: "eu-central-1"} + }, + // Spelled out at length in the message because these are the two + // alternative front doors for a fleet and the operator has asked for both. + errMatch: "These are the two alternative front doors for a fleet, not a pair", + }, + { + // imageDelivery is checked BEFORE the origin-TLS branch, so a resource + // that is wrong in two ways still hears about the one that is wrong on + // every path. + name: "imageDelivery is still validated", + mutate: func(i *VmIngress) { i.ImageDelivery = "carrier-pigeon" }, + errMatch: "unknown `ingress.imageDelivery`", + }, + } { + t.Run(tc.name, func(t *testing.T) { + RegisterTestingT(t) + err := noTls(tc.mutate).Validate() + Expect(err).To(HaveOccurred()) + Expect(err.Error()).To(ContainSubstring(tc.errMatch)) + }) + } +} + +// Backwards compatibility, asserted rather than assumed: the operator's standing +// requirement is that nothing already deployed changes behaviour. Every way of NOT +// saying `originTls: false` has to read as "this machine terminates TLS", because that +// is what every vm resource written before the field did. +func TestOriginTlsDefaultsToOn(t *testing.T) { + RegisterTestingT(t) + + unset := byoConfig() + unset.Ingress = &VmIngress{Enable: true, Email: "ops@example.com"} + Expect(unset.OriginTlsEnabled()).To(BeTrue()) + + explicit := byoConfig() + explicit.Ingress = &VmIngress{Enable: true, Email: "ops@example.com", OriginTls: lo.ToPtr(true)} + Expect(explicit.OriginTlsEnabled()).To(BeTrue()) + Expect(explicit.Validate()).ToNot(HaveOccurred()) + + // No ingress at all: there is no Caddy to ask, and a caller that reads this must + // not take "false" for "an edge terminates" — it would render an http:// site for + // a machine that serves no sites. + none := byoConfig() + none.Ingress = nil + Expect(none.OriginTlsEnabled()).To(BeTrue()) + + disabled := byoConfig() + disabled.Ingress = &VmIngress{Enable: false, OriginTls: lo.ToPtr(false)} + Expect(disabled.OriginTlsEnabled()).To(BeTrue()) +} diff --git a/pkg/cmd/cmd_stack/cmd_outputs.go b/pkg/cmd/cmd_stack/cmd_outputs.go index 30605bfc6..56401e6a1 100644 --- a/pkg/cmd/cmd_stack/cmd_outputs.go +++ b/pkg/cmd/cmd_stack/cmd_outputs.go @@ -11,20 +11,45 @@ import ( ) func NewOutputsCmd(sCmd *stackCmd) *cobra.Command { + var compact bool cmd := &cobra.Command{ Use: "outputs", Short: "Displays outputs of a stack defined in stacks directory", RunE: func(cmd *cobra.Command, args []string) error { + // Silent for the whole operation: this command's stdout is a JSON + // document someone pipes into jq, and SC's logger writes to stdout + // too, so any INFO line would land inside the payload. The level + // rides the context from here down — including into the plugin + // LogStream consumers and go-plugin's own hclog. cmd.SetContext(sCmd.Root.Logger.Silent(cmd.Context())) - if res, err := sCmd.Root.Provisioner.Outputs(cmd.Context(), sCmd.Params); err != nil { + res, err := sCmd.Root.Provisioner.Outputs(cmd.Context(), sCmd.Params) + if err != nil { return err - } else if j, err := json.Marshal(*res); err != nil { + } + // Indented by default. These payloads are read by a person at a + // terminal at least as often as by a script — forge's infra stack + // returns dozens of keys, which as one line wraps into an + // unreadable block — and indenting is what every comparable CLI + // (`gh api`, `aws`, `pulumi stack output --json`) does. --compact + // restores the single line for anything that depended on it. + j, err := marshalOutputs(*res, compact) + if err != nil { return err - } else { - fmt.Println(string(j)) } + fmt.Println(string(j)) return nil }, } + cmd.Flags().BoolVar(&compact, "compact", false, + "Print the JSON on a single line instead of indented") return cmd } + +// marshalOutputs is split out of RunE so the format is testable without +// standing up a cobra command and a provisioner. +func marshalOutputs(res any, compact bool) ([]byte, error) { + if compact { + return json.Marshal(res) + } + return json.MarshalIndent(res, "", " ") +} diff --git a/pkg/cmd/cmd_stack/cmd_outputs_test.go b/pkg/cmd/cmd_stack/cmd_outputs_test.go new file mode 100644 index 000000000..3b973c894 --- /dev/null +++ b/pkg/cmd/cmd_stack/cmd_outputs_test.go @@ -0,0 +1,64 @@ +// SPDX-License-Identifier: MIT +// Copyright (c) Simple Container + +package cmd_stack + +import ( + "encoding/json" + "strings" + "testing" + + . "github.com/onsi/gomega" + + "github.com/simple-container-com/api/pkg/api" +) + +func TestMarshalOutputs(t *testing.T) { + RegisterTestingT(t) + + res := api.OutputsResult{ + StackName: "infra", + Outputs: map[string]any{ + "infra--mongodb--staging-mongo-uri": "mongodb+srv://cluster0.example.net", + "forge-storage--staging-bucket-name": "forge-storage-staging", + }, + } + + t.Run("indented by default", func(t *testing.T) { + RegisterTestingT(t) + + j, err := marshalOutputs(res, false) + Expect(err).ToNot(HaveOccurred()) + + // Two-space indent, one key per line. forge's infra returns ~20 keys; + // on a single line that wraps into a block nobody can read. + Expect(string(j)).To(ContainSubstring("\n \"outputs\": {")) + Expect(strings.Count(string(j), "\n")).To(BeNumerically(">", 3), + "output is not actually indented: %s", j) + }) + + t.Run("--compact restores the single line", func(t *testing.T) { + RegisterTestingT(t) + + j, err := marshalOutputs(res, true) + Expect(err).ToNot(HaveOccurred()) + + // The escape hatch for anything that depended on the old format. + Expect(string(j)).ToNot(ContainSubstring("\n")) + }) + + t.Run("both shapes parse to the same document", func(t *testing.T) { + RegisterTestingT(t) + + // Indenting is a presentation change only — a consumer piping either + // form into jq has to see identical data. + var pretty, oneLine map[string]any + j1, err := marshalOutputs(res, false) + Expect(err).ToNot(HaveOccurred()) + j2, err := marshalOutputs(res, true) + Expect(err).ToNot(HaveOccurred()) + Expect(json.Unmarshal(j1, &pretty)).To(Succeed()) + Expect(json.Unmarshal(j2, &oneLine)).To(Succeed()) + Expect(pretty).To(Equal(oneLine)) + }) +} diff --git a/pkg/plugin/discover.go b/pkg/plugin/discover.go index 71cb3022a..29ce13e9e 100644 --- a/pkg/plugin/discover.go +++ b/pkg/plugin/discover.go @@ -15,6 +15,7 @@ import ( "github.com/pkg/errors" "github.com/simple-container-com/api/pkg/api" + "github.com/simple-container-com/api/pkg/api/logger" scpluginv1 "github.com/simple-container-com/api/pkg/plugin/proto/v1" ) @@ -34,6 +35,40 @@ import ( // prefer CachedSchema first and only fall through to ResolveAndSchema // on cache miss — spawning a plugin per parse costs a 200-400 ms // handshake per plugin that CachedSchema sidesteps. +// defaultPluginHclog builds the hclog.Logger hashicorp/go-plugin logs its +// spawn handshake through, at a level derived from SC's own. +// +// Debug is the default and is load-bearing: it is what surfaces plugin panic +// traces, because go-plugin relays the child's stderr at Debug +// (client.go:1186). A Warn-level logger discards every byte of it and turns a +// panic into a bare `exit status 2` with no further diagnostics — live-caught +// 2026-10-30, forge-baas. So Info and Debug both keep Debug here; the level +// is only lowered when SC was explicitly asked to be quieter. +// +// Silent maps to Off rather than Error. A command that silences SC wants a +// machine-readable stdout (`sc stack outputs` emits a JSON document for jq), +// and while go-plugin writes here to stderr, "silent" that still prints two +// dozen handshake lines per plugin is not silent to the operator reading the +// terminal. Nothing diagnostic is lost: a plugin that fails to spawn still +// returns an error through ResolveAndSchema, and re-running with -v restores +// the full trace. +func defaultPluginHclog(ctx context.Context, name string) hclog.Logger { + level := hclog.Debug + switch logger.LevelFromContext(ctx) { + case logger.LogLevelSilent: + level = hclog.Off + case logger.LogLevelError: + level = hclog.Error + case logger.LogLevelWarn: + level = hclog.Warn + } + return hclog.New(&hclog.LoggerOptions{ + Name: "sc-plugin-" + name, + Level: level, + Output: os.Stderr, + }) +} + func ResolveAndSchema( ctx context.Context, name string, @@ -51,16 +86,7 @@ func ResolveAndSchema( return nil, nil, err } if logger == nil { - // Mirror the preengine's choice: Debug surfaces plugin panic - // traces via hashicorp/go-plugin's stderr-at-Debug logging - // (client.go:1186). A Warn-level logger discards every byte - // and turns panics into `exit status 2` with no further - // diagnostics. See loadOne comment. - logger = hclog.New(&hclog.LoggerOptions{ - Name: "sc-plugin-" + name, - Level: hclog.Debug, - Output: os.Stderr, - }) + logger = defaultPluginHclog(ctx, name) } cli, err = NewClient(ClientConfig{ Path: resolvedPath, diff --git a/pkg/plugin/plugin_hclog_level_test.go b/pkg/plugin/plugin_hclog_level_test.go new file mode 100644 index 000000000..0d1af2d6a --- /dev/null +++ b/pkg/plugin/plugin_hclog_level_test.go @@ -0,0 +1,88 @@ +// SPDX-License-Identifier: MIT +// Copyright (c) Simple Container + +package plugin + +import ( + "context" + "testing" + + hclog "github.com/hashicorp/go-hclog" + . "github.com/onsi/gomega" + + "github.com/simple-container-com/api/pkg/api/logger" +) + +// hashicorp/go-plugin logs the entire spawn handshake — "starting plugin", +// "waiting for RPC address", "using plugin: version=1", and the matching +// teardown lines — through the hclog.Logger it is handed. That is roughly a +// dozen lines per plugin, and `sc stack outputs` on a stack declaring five +// plugins printed sixty of them around a JSON document. +// +// Debug has to stay the DEFAULT, though: go-plugin relays a crashed plugin's +// stderr at Debug, so a quieter default turns a panic into a bare +// `exit status 2` (live-caught 2026-10-30, forge-baas). So the level follows +// SC's own, and only a command that explicitly asked for quiet gets it. +func TestDefaultPluginHclogFollowsSCLogLevel(t *testing.T) { + RegisterTestingT(t) + + log := logger.New() + + for _, tc := range []struct { + name string + ctx func() context.Context + want hclog.Level + why string + }{ + { + name: "silent", + ctx: func() context.Context { return log.Silent(context.Background()) }, + want: hclog.Off, + why: "`sc stack outputs` silences SC so its stdout is a clean JSON document; handshake spam is not silent", + }, + { + name: "bare context keeps Debug", + ctx: func() context.Context { return context.Background() }, + want: hclog.Debug, + why: "the default must stay Debug or a plugin panic loses its stderr trace", + }, + { + name: "explicit info keeps Debug", + ctx: func() context.Context { return log.SetLogLevel(context.Background(), logger.LogLevelInfo) }, + want: hclog.Debug, + why: "provision/deploy are unchanged by this fix", + }, + { + name: "warn", + ctx: func() context.Context { return log.SetLogLevel(context.Background(), logger.LogLevelWarn) }, + want: hclog.Warn, + }, + { + name: "error", + ctx: func() context.Context { return log.SetLogLevel(context.Background(), logger.LogLevelError) }, + want: hclog.Error, + }, + } { + t.Run(tc.name, func(t *testing.T) { + RegisterTestingT(t) + + got := defaultPluginHclog(tc.ctx(), "mongodb") + Expect(got.GetLevel()).To(Equal(tc.want), tc.why) + }) + } +} + +// The level has to be readable from the context by a non-Logger consumer, or +// the mapping above has nothing to read. +func TestLevelFromContext(t *testing.T) { + RegisterTestingT(t) + + log := logger.New() + + // A bare context must NOT read as silent — otherwise every caller that + // forgot to thread the command context would accidentally go quiet. + Expect(logger.LevelFromContext(context.Background())).To(Equal(logger.LogLevelInfo)) + Expect(logger.LevelFromContext(log.Silent(context.Background()))).To(Equal(logger.LogLevelSilent)) + Expect(logger.LevelFromContext(log.SetLogLevel(context.Background(), logger.LogLevelDebug))). + To(Equal(logger.LogLevelDebug)) +}