4 Commits
Author SHA1 Message Date
Myasnikov Daniil 0ba5337526 fix(migrations): key the etcd-adoption endpoint scheme off the claimed bucket
Deriving the safety-snapshot target from the projected cozy-backups-creds
Secret is right, but detecting the admin-managed external-S3 case
(provisionBucket: false) by "the projected endpoint is absent" is not:
that state does not exist. The projector substitutes
BACKUP_STORAGE_ENDPOINT and fails ReasonSourceMalformed when both sources
are empty, so a projected Secret always carries a non-empty,
scheme-stripped endpoint.

So for provisionBucket: false against a plaintext external S3 the
projected endpoint is a bare host, the script saw it non-empty, forced
https:// and never consulted the Strategy CR — a regression against main,
which read the CR verbatim and passed http:// through. Being a pre-upgrade
hook with no ETCD_ADOPT_SKIP_BACKUP plumbed through migration-hook.yaml,
that is a total upgrade block with no operator escape.

Classify on data instead: match the projected bucketName against the
.status.bucketName of any BucketClaim. The COSI driver assigns that name
and the projector republishes exactly it (packages/system/bucket
user-credentials.yaml reads BucketInfo spec.bucketName), so a match means
the creds describe a COSI bucket => the host is the always-TLS S3 ingress
=> force https, as the 1.6 chart helper does. No match means external S3.

Keying on data rather than on the claim's name and namespace matters
twice. backupStorage.namespace/.bucketName are supported Package-CR
overrides that this hook cannot see, so a name lookup would miss a renamed
bucket and fall back to the v1.5.x plaintext CR — the original P0. And a
BucketClaim wedged Terminating (its cosi bucketclaim-protection finalizer
outlives an uninstalled COSI controller) keeps answering a name lookup
while its status names some other bucket.

Split the two paths by which source is authoritative. On COSI the
projected coordinates win: the bucket is the COSI-assigned name the CR can
only reproduce through a live lookup that may never have run. On external
S3 the Strategy CR wins WHOLESALE, not just for the endpoint — the CR
renders backupStorage.* and points at this same cozy-backups-creds, so CR
coordinates plus these credentials is exactly what the cluster's own
BackupJobs already use, and the bucket an operator will look in. Keeping
the Secret's bucket while taking the CR's endpoint assembled a pairing
nothing else produces: the projector copies bucketName straight from the
admin-managed source Secret and never from backupStorage.bucketName, so
the two can disagree outright and the snapshot could land in a bucket the
platform never writes. Relying on the CR is safe on this path alone,
because bucketName short-circuits to values before the BucketClaim lookup
that races on the COSI path, so the CR is guaranteed present.

Match with a single jq `any`, never `jq -r | grep -q`: grep exits on the
first match and SIGPIPEs jq, and pipefail reports jq's 141 rather than
grep's 0, so a MATCH returns as a failure and the cluster is misclassified
external. It is a race on the list outgrowing one stdio write (~8KB) with
only a last-line match safe, so it strikes exactly the big multi-tenant
clusters that can least afford it, and backoffLimit 3 can decide
differently on each retry. Measured with the match first, 20 runs each:
300 claims went 0/20 correct (rc=141 every time) and 2000 likewise, while
`any` is 20/20 at 3, 300 and 2000.

Refuse rather than guess when the answer is not knowable. Three states
qualify: the BucketClaim API is unreadable; the list is not interpretable
(no producer emits that, but without a shape gate it reads as "no claim" =
external = the plaintext P0, silently); or the bucket is claimed ONLY by a
Terminating claim. That last one is ambiguous — an admin who moved COSI ->
external S3 reusing the bucket name, versus a still-COSI cluster whose
claim is mid-recreate — and both readings fail as the same confusing
handshake error when wrong, so a guess buys no reliability while reading
it as "external" specifically resurrects the plaintext-:8333 P0. A live
claim still wins over an unrelated Terminating one, so healthy clusters
are unaffected. Each refusal names the condition and the operator action;
the migration is idempotent, so re-running after resolving it is the
remedy.

Behaviour change worth noting: a cluster on external S3 that still has
COSI installed now refuses on a sustained BucketClaim-API failure where
main would have proceeded off the Strategy CR. That is the intended trade
— main proceeded only because it never had to tell the two apart — but it
does mean an apiserver outage can now block this hook where it previously
did not. ETCD_ADOPT_SKIP_BACKUP is still not plumbed through
migration-hook.yaml, so the messages deliberately point at resolving the
condition rather than at an escape hatch the operator cannot reach.

Force the scheme only onto a non-empty host: an absent endpoint must
degrade to the CR as main did, not to a literal "https://" that would pass
the final non-empty guard and reach etcd-migrate as a destination.

The old test modelled external S3 by dropping the projected endpoint
entirely — a state the projector cannot produce — so it green-lit the
regression. It now models what the projector really writes, and the suite
pins the stale-claim, overridden-coordinates, empty-endpoint,
300-claim-multi-tenant, Terminating-match, hybrid-coordinate and
unreadable/uninterpretable API cases.

Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com>
2026-07-20 11:29:09 +05:00
Myasnikov Daniil 9ceccd2b07 fix(migrations): derive the etcd-adoption snapshot target from the projected bucket creds
Migration 50 is a pre-upgrade hook, so every chart-rendered resource it reads
still belongs to the version being upgraded FROM. It read the snapshot target
from the live cozy-default-etcd Strategy CR, which on a v1.5.x cluster is broken
in one of two ways — and the platform upgrade is gated behind this snapshot, so
neither can self-heal first:

1. Absent. strategy-etcd-default.yaml is guarded by `if $bucketName`, whose
   helper looks up the BucketClaim status. That BucketClaim is created by the
   same chart, so on first render the lookup is empty and the Strategy is
   silently skipped. Helm lookup only runs at install/upgrade time, so a cluster
   that reached 1.5.x in one hop never grows the CR on reconcile.

2. Present, carrying the static v1.5.x default
   http://seaweedfs-s3.tenant-root.svc.cozy.local:8333. Cozystack ships SeaweedFS
   with enableSecurity=true, so :8333 is a TLS listener and plaintext against it
   fails the handshake. Fixing the scheme alone is not enough: the Etcd Strategy's
   S3 schema has no caCert/insecureSkipVerify, so the self-signed in-cluster
   endpoint is unusable at any scheme and a trusted-cert endpoint is required.

Either way migration 50 hard-fails and blocks the upgrade, and the documented
ETCD_ADOPT_SKIP_BACKUP escape hatch cannot be reached — migration-hook.yaml
passes only NAMESPACE/CURRENT_VERSION/TARGET_VERSION.

Take the coordinates from cozy-backups-creds instead. The projector writes it
from the COSI-provisioned bucket's system credentials — bucket provisioning, not
chart values — so it describes the live bucket regardless of which version
rendered the charts, and it carries endpoint, bucketName, region and
forcePathStyle alongside the AWS keys. resolve_platform_backup_args already read
this Secret to validate those keys, so preferring it adds no new dependency. The
endpoint is a bare host fronted by the always-TLS S3 ingress, so force https://,
exactly as the 1.6 chart helper backupstrategy-controller.endpoint does.

The Strategy CR remains the fallback for the admin-managed external-S3 case
(provisionBucket: false), where .Values.backupStorage.endpoint is authoritative
and may legitimately be plaintext against a private store; that path is detected
by the absence of a projected endpoint and its scheme is preserved verbatim.

The bats fixture modelled cozy-backups-creds as carrying only the AWS keys, and
test 1 asserted the broken plaintext endpoint as expected output — the test was
written against the implementation rather than the requirement, which is why the
defect shipped green. The fixture now carries the coordinates the real projected
Secret has, test 1 asserts the derived https endpoint, and two cases are added:
an absent Strategy CR still resolving, and external S3 keeping the CR endpoint
verbatim.

Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com>
2026-07-20 11:29:09 +05:00
Andrey Kolkov 47d239c519 fix(platform): address review on migration 50 (exact SAN match, Secret-gated wait, IPv6 kubeconfig)
Review feedback on #3261 (gemini-code-assist, coderabbitai, myasnikovdaniil):

- Exact SAN match. _san_present read the Certificate SANs as {.spec.dnsNames}
  (a bracketed JSON blob that `tr ','` cannot split) and matched with a
  substring `grep -qF`, so a wildcard that is a substring of a longer SAN
  (e.g. *.etcd.<ns>.svc inside *.etcd.<ns>.svc.cluster.local) could
  false-positive and skip the re-issue. Read {.spec.dnsNames[*]} (space
  separated) and match exactly with `grep -qxF`, matching the Secret-annotation
  branch.

- Wait on the re-issued Secret, not the patched spec. The post-patch wait loop
  called _san_present, which returns as soon as the Certificate spec.dnsNames
  contains the wildcard -- but the patch just added it there, so the loop broke
  on the first iteration before cert-manager re-issued the Secret, and the
  "re-issued" log printed unconfirmed. Add _secret_has_san (Secret alt-names
  annotation only, same empty-read retry + exact match) and gate the wait on it.

- IPv6-safe kubeconfig server. The synthesized kubeconfig used
  https://$KUBERNETES_SERVICE_HOST:$KUBERNETES_SERVICE_PORT, which is an invalid
  URL on an IPv6-only cluster (a bare IPv6 host must be bracketed). Use
  https://kubernetes.default.svc -- IP-family-agnostic, validated by the mounted
  SA CA, and the same server URL kubectl synthesizes in-cluster.

Tests (the review blocker): extend the bats suite (and its fake kubectl) with
two cases pinning these contracts -- a superstring dnsName must NOT skip the
re-issue patch, and etcd-migrate must be invoked with a synthesized in-cluster
kubeconfig whose server is kubernetes.default.svc (not the bare IPv6 host),
authenticating via the SA token file + CA. Both the kubeconfig path and the SA
directory are now env-overridable (ETCD_MIGRATE_KUBECONFIG, ETCD_ADOPT_SA_DIR)
so the synthesis is exercisable off-cluster; production defaults are unchanged.

Signed-off-by: Andrey Kolkov <androndo@gmail.com>
2026-07-15 19:11:35 +05:00
Andrey Kolkov d0740ed8c7 test(etcd): gate S3 backup round-trip behind ETCD_E2E_S3_ROUNDTRIP
Signed-off-by: Andrey Kolkov <androndo@gmail.com>
2026-07-06 14:51:48 +04:00