mirror of
https://github.com/cozystack/cozystack.git
synced 2026-11-06 03:57:52 +09:00
feat/ca-extraction-controller
1855
Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
 Aleksei Sviridkin
|
a121bec10b |
fix(controller): resolve an empty sourceSecretName to a condition
An entry with an empty sourceSecretName sent the reconciler to Get a Secret with an empty name. The apiserver answers that with "resource name may not be empty" — not NotFound — so it escaped the not-found branch, was returned from Reconcile, and wedged the sentinel on exponential backoff. The error path returns before any status write, so no Ready condition was ever recorded: the silent retry the sentinel was introduced to make visible. Pin sourceSecretName to MinLength=1 so a live apiserver rejects it at admission, and guard the reconcile so an object admitted by an older CRD resolves to Ready=False/SourceInvalid instead of an error. Signed-off-by: Aleksei Sviridkin <f@lex.la> |
||
|
Aleksei Sviridkin
|
9ac06cd1fc |
fix(controller): withdraw the CA projection and refuse contested releases
Two gaps in the sentinel-to-canonical-name lifecycle: Withdrawal — the reconciler published each declared entry but never deleted one. A sentinel that keeps existing with its CACert entry removed (a chart gating it on tls.enabled, say) left the old <release>.tenant-ca served forever, because owner-reference GC only fires when the whole sentinel is pruned. A sentinel that reconciles with zero CACert entries now deletes the projection at the canonical name, but only when it still carries that sentinel's owner reference, so a foreign Secret or another sentinel's projection is never touched. Contention — two sentinels in one namespace with the same release label both resolve to the single canonical name. Whichever reconciled first would publish an arbitrary CA and the two could flap ownership of one projection. Refuse both, mirroring the more-than-one-CACert-entry refusal, so nothing is published until the declaration lives on exactly one sentinel. The ClusterRole comments claimed the reconciler never deletes a Secret; corrected to describe the owner-scoped withdrawal delete. Signed-off-by: Aleksei Sviridkin <f@lex.la> |
||
|
Aleksei Sviridkin
|
93b496be95 |
feat(cozystack-basics): pin the TenantProjection writer to flux
Replace the publish-ca-cert label-leg admission control with a policy on the sentinel. A ValidatingAdmissionPolicy restricts TenantProjection CREATE and UPDATE to the flux ServiceAccount helm-controller runs as, so a tenant (who has no RBAC on internal.cozystack.io) cannot forge a declaration that publishes a CA of their choosing. DELETE stays ungated for owner-ref GC. cozystack runs its flux controllers under one shared cozy-fluxcd:flux ServiceAccount, so the pin admits any flux controller — a deliberate platform-tier widening, since only helm-controller renders charts and no tenant reaches this group. The literal is guarded by a bats check on its shape, name and namespace. Remove the label-leg policy, its unit test, its pin check and its chainsaw suite — the sentinel model retires that leg. Signed-off-by: Aleksei Sviridkin <f@lex.la> |
||
|
Aleksei Sviridkin
|
4503538f91 |
feat(postgres): render a TenantProjection for the CA trust anchor
Postgres is the first consumer of the sentinel model. The chart renders a TenantProjection named <release>-ca declaring CNPG's <release>-ca as the CACert source; the CA-extraction controller lifts ca.crt alone into the tenant-readable <release>.tenant-ca projection and owns it through the sentinel. Drop the spec.caCert declaration leg from the ApplicationDefinition API and from the postgres definition — the sentinel replaces it. The tenant-ca include selector that makes the projection tenant-visible is unchanged. Signed-off-by: Aleksei Sviridkin <f@lex.la> |
||
|
Aleksei Sviridkin
|
b41b6cce89 |
feat(controller): discover the CA source through a TenantProjection sentinel
Drive the CA-extraction reconciler off the namespaced TenantProjection sentinel a chart renders, replacing the publish-ca-cert label leg and the cluster-scoped spec.caCert declaration leg. The sentinel names the source Secret and key; the projected trust anchor takes an owner reference to the sentinel, so the lineage webhook resolves the tenant through sentinel -> HelmRelease and garbage collection prunes the projection when the chart stops rendering the sentinel. The source Secret is watched as metadata only, so the informer holds no private key material, and the one source actually read goes through the uncached reader. The sentinel's Ready status makes a missing or unpopulated source visible with kubectl instead of a silent requeue. The write-path guard, canonical-name contract, adoption rules and selectors-digest re-admission are unchanged. Signed-off-by: Aleksei Sviridkin <f@lex.la> |
||
|
Aleksei Sviridkin
|
564e1d95d9 |
feat(api): add TenantProjection CRD for declared tenant-visible projections
Introduce the namespaced internal.cozystack.io/v1alpha1 group and its TenantProjection kind. A chart renders this sentinel to declare which of an application's Secrets should be published to its tenant as a key-free copy; the controller reconciles the projected Secret from it. No tenant RBAC role grants any verb on internal.cozystack.io, so the sentinel cannot be forged from a tenant namespace. Signed-off-by: Aleksei Sviridkin <f@lex.la> |
||
|
Aleksei Sviridkin
|
d2944051c9 |
fix(cozystack-basics): gate ValidatingAdmissionPolicies on API availability
The four VAP policies in this chart (publish-ca-cert-writer plus the route, gateway, and ingress hostname policies) rendered unconditionally, so a cluster whose apiserver does not serve the ValidatingAdmissionPolicy API would receive an unrenderable resource and fail the cozystack-basics install or upgrade. Wrap all four in the same .Capabilities.APIVersions.Has guard the platform deletion-protection template already uses, and cover both the served and unavailable cases in helm-unittest. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <f@lex.la> |
||
|
Aleksei Sviridkin
|
7e7cf7a377 |
docs(cozystack-basics): record the source/destination gating asymmetry
The writer policy gates the CA-source surface but not the destination surface — the <release>.tenant-ca projection a tenant reads. Document why that is accepted rather than closed: a platform-tier writer that could forge a projection already has an ungated path through the declared-leg source, so it is unequal depth rather than an escalation; a destination gate could not be a clean single-writer pin because the tenant-ca label has two legitimate producers (this controller and helm-controller); and the surgical fix is a UID check in lineage owner resolution, tracked as separate follow-up work. Signed-off-by: Aleksei Sviridkin <f@lex.la> |
||
|
Aleksei Sviridkin
|
c106a28026 |
feat(api): constrain the application release prefix to dot-free characters
The CA-extraction controller projects a tenant's trust anchor to "<release>.tenant-ca" and relies on the dot being a separator no release name can contain: release names are "<prefix><app name>", app names are DNS-1035 labels with no dot, so the projection name is unambiguous only while the prefix is dot-free too. That property was asserted in the controller but never enforced on the field it depends on. Add a validation pattern restricting the prefix to lowercase DNS-1123 characters, which excludes the dot by construction. Every shipped application definition already conforms. Signed-off-by: Aleksei Sviridkin <f@lex.la> |
||
|
Aleksei Sviridkin
|
e8631ae920 |
feat(cozystack-basics): restrict CA-source Secret writes to cert-manager
The CA-extraction controller's label-driven leg treats a core Secret labelled internal.cozystack.io/publish-ca-cert as a trust-anchor source and lifts its ca.crt into the tenant-readable projection. That label is the whole attribution on that leg and sits on an ordinary Secret, so without a platform-side control any principal that can write a Secret in the namespace could forge a source or rewrite an existing one's ca.crt and swap the trust anchor a tenant is handed as vouched. Add a ValidatingAdmissionPolicy that allows a Secret carrying that label to be created or updated only by cert-manager's controller ServiceAccount, the sole legitimate writer of these Secrets. It covers both the forge-new (CREATE) and tamper-existing (UPDATE, including a relabel-away evasion via the old object) shapes, and by keying on the publish-ca-cert label it never touches the declared-leg source, the projection, or any unlabelled Secret. The allowed identity is pinned as a literal to the shipped cert-manager package's controller ServiceAccount; a comment marks it as the fragile seam that must move if cert-manager is repackaged. Signed-off-by: Aleksei Sviridkin <f@lex.la> |
||
|
Aleksei Sviridkin
|
f2c2abfe88 |
feat(postgres): publish the CA trust anchor to tenants
Declare CNPG's operator-created CA Secret as the postgres trust-anchor
source (spec.caCert names {{ .release }}-ca), and add the generic
internal.cozystack.io/tenant-ca selector to spec.secrets so the key-free
<release>.tenant-ca projection the extraction controller publishes becomes
readable by the tenant.
postgres is the first converged engine, and the hardest: CNPG creates the
CA Secret itself, key-bearing, asynchronously, and offers no way to label
it, so it exercises the name-driven leg end to end. The test drives the
shipped ApplicationDefinition rather than a fixture, so the wiring and the
controller cannot drift apart unseen: a CNPG-shaped key-bearing
<release>-ca appears, and the projection must be produced key-free AND
selected for tenant visibility by the shipped selector.
Signed-off-by: Aleksei Sviridkin <f@lex.la>
|
||
|
Aleksei Sviridkin
|
d7232831f4 |
feat(cozystack-controller): run the CA-extraction reconciler
Wire the reconciler into the platform controller manager and grant its ServiceAccount the Secret verbs the projection needs: create/update/patch to publish the trust anchor, and delete to withdraw it when an application stops publishing one. Read was already covered by the catch-all rule, and no tenant RBAC is widened. Give the reconciler its own dedicated, scoped Secret cache rather than narrowing the manager's shared cache. Only labelled CA sources are held in that informer, so a cluster-wide Secret watch never caches every Secret, and so every private key, in the cluster; the operator-owned name-driven sources and the projections are read through the uncached API reader instead. A private cache also keeps this scoping independent of any other controller on the same manager that needs to scope Secrets differently. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <f@lex.la> |
||
|
Aleksei Sviridkin
|
5da3fab457 |
feat(api): declare where an application's CA lives
The trust-anchor contract is a label on the CA Secret, but two engines cannot honour it. CloudNativePG builds its PKI Secrets without applying spec.inheritedMetadata, so the chart's labels never reach <release>-ca, and Percona Server for MongoDB exposes no secretTemplate at all and reconciles an out-of-band patch away. Add an optional ApplicationDefinition.spec.caCert declaring, per engine, the name of the Secret its operator creates and the key inside it that holds the certificate. Registering an engine is then data, not code. The declaration only names an object the platform already reads; nothing a tenant can read is widened by it, and a labelled Secret still takes precedence where an engine can label its own output. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <f@lex.la> |
||
|
myasnikovdaniil
|
741a5acc4a |
fix(opensearch-operator): use RollingUpdate maxSurge=0 instead of Recreate (#3319)
## What this PR does
Fixes a Helm-upgrade failure in the `opensearch-operator` chart that
blocks the new release-upgrade E2E lane (added in #3276).
Upgrading from a chart that shipped no explicit strategy (v1.5.x) to a
v1.6.0-rc build fails validation immediately, helm-controller retries
it, and the HelmRelease never becomes Ready:
```
Helm upgrade failed for release cozy-opensearch-operator/opensearch-operator:
Deployment.apps "opensearch-operator-controller-manager" is invalid:
spec.strategy.rollingUpdate: Forbidden: may not be specified when strategy type is Recreate
```
Observed in the ["Upgrade E2E Test" job of run
29480724982](https://github.com/cozystack/cozystack/actions/runs/29480724982)
— `UpgradeFailed ... (x19 over 11m)`, with the HelmRelease never
reaching Ready.
### Root cause
#3040 (commit
|
||
|
myasnikovdaniil
|
960d853706 |
feat(api): typed waitStrategy/healthCheckExprs for kstatus readiness (#2642) (#3273)
## What Adds typed `waitStrategy` + `healthCheckExprs` fields so a package/app can declare the Flux helm-controller v1.5.0 kstatus health gate for the custom resource it wraps — making `HelmRelease.Ready` mean "the CR is actually healthy" instead of "helm applied the CR". This is the operator/apiserver foundation of the kstatus epic (#2642); the API surface was reviewed and approved by the API owner in #3263. Commits (per the review's "isolated step-0" request): 1. `build(deps)` — the dependency bump (see below). 2. `feat(api)` — the fields, plumbing, tests, and regenerated code. 3. `fix(api)` — match the openapi code-generator pin to the 0.35 runtime (fixes an aggregated-apiserver crash the bump surfaced). 4. `fix(api)` — declare `OpenAPIModelName` on the apps types so SSA resolves catalog kinds. 5. `docs(api)` — note that `disableWait` silently defeats `healthCheckExprs`. ## API surface Typed fields on **both** HelmRelease-building paths: - `ApplicationDefinition.spec.release` (apiserver / catalog apps → `convertApplicationToHelmRelease`) - `ComponentInstall` (operator / platform packages → `buildHelmReleaseSpec`) ```go // WaitStrategy maps to HelmReleaseSpec.WaitStrategy.Name — a deliberate scalar // simplification of the upstream {name} object. One of poller|legacy. WaitStrategy string // HealthCheckExprs maps to HelmReleaseSpec.HealthCheckExprs (the upstream // kustomize.CustomHealthCheck type verbatim). HealthCheckExprs []kustomize.CustomHealthCheck ``` `config.ResolveWaitStrategy` is shared by both builders and **couples the default**: `healthCheckExprs` are only evaluated under the `poller` strategy, so when expressions are set and no strategy is given, the generated HelmRelease defaults to `poller`. A package that sets only `healthCheckExprs` is therefore self-contained, independent of the controller's global default. ## The dependency bump (commit 1) The `WaitStrategy` / `kustomize.CustomHealthCheck` / `HelmReleaseSpec.HealthCheckExprs` types do not exist below `helm-controller/api` v1.5.0 — the runtime is already v1.5.0 (shipped by #2602), so this closes a Go-types-vs-runtime skew. It is **not** an isolated bump: v1.5.x requires **k8s 0.35 + controller-runtime 0.23**, cascading the k8s stack 0.34 → 0.35 (and the go directive to 1.26). cozystack replaces `k8s.io/apimachinery` with a fork carrying the still-open upstream `ConvertToVersion` patch (k/k#135537). That single commit rebases cleanly onto apimachinery v0.35.0; published at `cozystack/apimachinery@release-1.35-pr135537`, and `go.mod` pins its pseudo-version. **The repo builds with zero source changes under the new stack.** > Maintainer note: please confirm the `release-1.35-pr135537` fork branch naming, or fold it into the `cozystack` branch as you prefer — the pin can be updated to match. ## Not included (conscious YAGNI) `disableWait` is intentionally omitted. The only known need (the kubernetes tenant-addon deadlock) was resolved child-side, and `waitStrategy: legacy` is not equivalent (legacy still waits Helm-v3-style; `disableWait` skips waiting entirely). Add it later when a concrete leaf needs a true no-gate escape hatch. ## Tests - `config.TestResolveWaitStrategy` — the couple-the-default logic (unset+exprs→poller; explicit legacy honored; unset+no-exprs→nil). - Operator + apiserver cases mirror each other (`TestBuildHelmReleaseSpecHealthCheckExprs` / `TestConvertApplicationToHelmRelease_HealthCheckExprs`). - `go build ./...` clean, `go vet` clean, `gofmt` clean, codegen regenerated and committed. The 4 backupstrategy CRDs regenerate with benign k8s-0.35 PodSpec description drift (DynamicResourceAllocation alpha→stable, toleration Lt/Gt, resizePolicy note) — no functional change. ## Follow-up Phase 3 (per-leaf adoption — postgres/kafka/mongodb/etc. declaring their CEL via these fields) builds on this. The full CRD→CEL catalog is maintained in the epic's readiness audit. Refs #2642 ```release-note feat(api): add `waitStrategy` and `healthCheckExprs` to ApplicationDefinition and ComponentInstall, so a HelmRelease reports Ready only once the custom resource it wraps is actually healthy ``` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Added configurable CEL health-check expressions for application releases and package components. * Added `waitStrategy` support (`poller` and `legacy`) with automatic `poller` selection when health checks are configured and no strategy is set. * Propagated these settings into generated Helm releases, affecting readiness/wait behavior. * **Documentation** * Updated Application and PackageSource schemas to include `healthCheckExprs` and `waitStrategy`. * Extended related Kubernetes CRD documentation, including projected pod certificate annotations and workload reference fields. <!-- end of auto-generated comment: release notes by coderabbit.ai --> |
||
|
Andrey Kolkov
|
e73ad68a85 |
fix(backups): support cnpg-operator with barman plugin (#3313)
<!-- Thank you for making a contribution! Here are some tips for you:
- Use Conventional Commits for the PR title: `type(scope): description`
- Types: feat, fix, docs, style, refactor, perf, test, build, ci, chore
- Scopes are not an exhaustive list — pick the most specific scope for
the change and extend the list when a genuinely new area appears.
Examples:
- System components: dashboard, platform, operator, cilium, kube-ovn,
linstor, fluxcd, cluster-api
- Managed apps: postgres, mariadb, redis, kafka, clickhouse,
virtual-machine, kubernetes
- Development and maintenance: api, hack, tests, ci, docs, maintenance
- Breaking changes: append `!` after type/scope (`feat(api)!: ...`) or
add a `BREAKING CHANGE:` footer
- If it's a work in progress, consider creating this PR as a draft.
- Don't hesistate to ask for opinion and review in the community chats,
even if it's still a draft.
- Add the label `backport` if it's a bugfix that needs to be backported
to a previous version.
-->
## What this PR does
Fixes #3300. Migrates CNPG Postgres backups off the deprecated native
`spec.backup.barmanObjectStore` (removed in CNPG 1.29; the
`barman-cloud-*` binaries it needs are absent from the `standard` image
variant keycloak-db pins, and the bare system-flavor tags apps/postgres
pins are deprecated upstream) onto the [CloudNativePG Barman Cloud
plugin](https://github.com/cloudnative-pg/plugin-barman-cloud), across
every Postgres backup surface: `apps/postgres`, `system/keycloak`, and
the `cozy-default-cnpg` BackupClass strategy driver. The chart renders
`spec.plugins` + a `barmancloud.cnpg.io/ObjectStore` CR; the
backup-controller driver SSA-applies an `ObjectStore` and patches
`spec.plugins`; `Backup`/`ScheduledBackup` use `method: plugin`. The
`postgres-operator` chart vendors and co-locates the
`plugin-barman-cloud` operator (with its cert-manager-issued mTLS) so
CNPG can discover it.
One platform-level change is required for the plugin to run and ships
with this PR (see the upgrade notes in
`docs/operations/backup-classes.md` and the release note below): the
CloudNativePG operator is bumped to 1.28.1 (also fixing an operator/CRD
version skew that broke all backups). The other prerequisite — the
LINSTOR scheduler admission webhook stripping
`initContainers[].restartPolicy` cluster-wide, which broke the plugin's
restartable sidecar — is already fixed on `main` (
|
||
|
Myasnikov Daniil
|
b16a51e60e |
test(seaweedfs): pin the naming guard's MultiZone zone-component classification
MultiZone (and Simple-with-pools) tenants render no plain `volume` component — extra/seaweedfs sets volume.enabled=false and the vendored chart emits one component per zone key with suffix `volume-<key>`, truncated at (62 - len(suffix)), which is SHORTER than the 56-char cut the guard's renamedVolumePrefix replays. The final review pass flagged this divergence as a guard bypass. For the supported instance the prefixes cannot diverge: the tenant module hardcodes the name `seaweedfs`, the fullname is 16 chars, and nothing truncates until a zone/pool key of ~40+ characters. Three new cases pin that supported path — legacy-only zones adopt, release-named-only zones refuse as class S, both generations refuse — since the suite previously had no zone shapes at all and the zone objects are the ONLY evidence on a MultiZone tenant. The divergence beyond that boundary (long instance names or absurd keys) is an accepted, documented limit, not a fix: decided in the 1.6 triage, recorded in _naming.tpl (both copies, kept in lockstep) and the runbook's Scope section. Instance names are fixed by the tenant module, so absurd keys are the only reachable trigger; revisit if instance naming is ever opened up. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com> |
||
|
Myasnikov Daniil
|
601f164c5f |
fix(seaweedfs): stop the naming guard guessing which volume generation holds the data
The guard told a class-S tenant from a D tenant by comparing PVC creationTimestamps,
on the premise that "PVCs are never recreated in place, so the older generation is
where the data was born". The premise is false, and this runbook is the counter-
example: Step 2's re-bind DELETES each release-named claim and recreates it under
the chart name against the same PV.
So a tenant interrupted part-way through Step 2 — after volume-0, before volume-1 —
has data1-seaweedfs-volume-0 created SECONDS ago holding real data, beside
data1-seaweedfs-system-volume-1 with the original timestamp also holding real data.
That is the exact inversion of the rule. Reproduced against the old template:
keeps its data on the release-named volume PVCs (created 2026-06-20T07:01:59Z,
OLDER than the chart-named data1-seaweedfs-volume-* claims created
2026-07-17T09:30:00Z) ... Remove the empty chart-named set ... (Step 2a, then Step 2)
Step 2a then runs `grep -E '^data1-seaweedfs-volume' | xargs kubectl delete pvc` —
deleting the claim Step 2 had just re-bound — and Step 2 had already restored the
PV's original reclaim policy, so on any Delete-policy StorageClass the volume and
the bytes go with it. The guard's own recovery path was a data-loss instruction.
Two more holes in the same classifier:
- The S-damaged branch was gated on $systemPVC, which requires the PVC name to
contain "seaweedfs". seaweedfs.componentName truncates the fullname to 56 chars
before appending -volume, so an instance name >= ~40 chars drops the chart name
off the PVC name — the very case fullname_override_named_test.yaml exists for.
Probed with `archive-of-quarterly-financial-statements-x1`: damaged + renamed set
Ready fell through to the D-split branch ("is running two live SeaweedFS sets ...
Stop the split"), sending the operator to quiesce the set holding ALL the data —
the exact misclassification the parent commit exists to remove. Damaged + not
Ready fired no branch at all and rendered through unguarded.
- readyReplicas == 0 was read as proof the duplicate never served. It is a
snapshot: a duplicate that crashed, was scaled down, or lost readiness AFTER
serving writes reads identically, and adoption strands whatever it wrote.
There is no sound signal here. Claim timestamps are mutable, StatefulSets are
recreated by the adoption hook, and liveness is a snapshot — nothing durable in the
object graph distinguishes "empty set a bad upgrade created" from "the data,
mid-re-bind". Rather than add a fourth heuristic, the guard now decides only what
is decidable:
legacy generation only (or nothing) -> render; adoption is correct
release-named generation only -> refuse, class S (Step 2)
BOTH generations -> refuse; an operator classifies
Both generation flags now OR the PVC and StatefulSet evidence, so the truncated
long-name tenant is seen via its label-matched StatefulSet.
The cost is bounded and lands only where it should. A duplicate exists only on a
tenant that passed through the broken 4.31 rename in 1.5.x — already damaged,
already needing an operator. A 1.4.x tenant upgrading straight to 1.6 never
renames, so it has one generation and is untouched. Confirmed on the stand:
tenant-l (1.4.5-era, never upgraded) is legacy-only and dry-runs clean; the four
both-generation tenants all already need an operator. What is lost is D-wedged
auto-adoption, which was resting on the readyReplicas assumption above.
Classification moves to the runbook, where a human has signals a template does not:
Step 1 now reads the ages of the PVs (durable — they survive the re-bind) via
.spec.volumeName instead of the claims, reports MIXED instead of picking, and walks
the operator through interrupted-Step-2 first, then strict age separation, then
verifying emptiness directly, then escalating rather than guessing. Step 2 stashes
the PV's original reclaim policy ON THE PV and leaves it Retain until Step 5
verifies the tenant, so a mis-step during recovery is survivable. Step 2a grows a
precondition that refuses unless every chart-named claim is bound to a strictly
newer PV.
Tests: 7 of 14 in the enforcing suite fail against the old template, including the
interrupted-Step-2 tenant and both truncated-long-name cases. hack/seaweedfs-guard-
parity.bats pins the two copies (system/ enforcing, extra/ sibling) against silent
drift — they cannot share a library, neither chart depends on cozy-lib, and "keep
these in sync" as a comment is the failure class this whole branch is about; it
also pins that creationTimestamp and readyReplicas never come back.
`make test` in both charts now runs helm template --is-upgrade and greps for the
refusal. The fail-closed canary — the headline safety property — had ZERO automated
coverage: guard_fail_closed_test.yaml passes with AND without the fix, because
helm-unittest 1.0.3 ignores release.isUpgrade. Verified the new target fails when
the canary is removed.
Review round 3 — the refusal was right; the justification was false and the
procedure that replaced the chart's judgement was broken on both axes.
THE COMMENT WAS WRONG. It said "nothing durable in the object graph distinguishes
the empty generation from the data mid-re-bind". A durable signal does exist, and
the reviewers were right to call that out: the Helm release history. Verified on
the stand — sh.helm.release.v1.<name>-system.v1 records the scheme the tenant was
BORN under (tenant-root/l/dsplit/named rev 1 render seaweedfs-master => pre-4.31;
tenant-fresh renders seaweedfs-system-master => 1.5.x), and info.first_deployed is
on every retained revision, so anchoring PV creationTimestamps against it survives
history pruning and the Step 2 re-bind (tenant-root: chart-named +10s vs
release-named +2084s; tenant-fresh: +5225s vs +6s).
BUT IT DOES NOT UNLOCK AUTO-ADOPTION, and that is the substantive disagreement
with the round-3 direction. Birth order answers "which generation is ORIGINAL".
Adoption needs "is the OTHER one EMPTY". Those come apart exactly where it
matters: D-wedged (duplicate never scheduled, safe to adopt) and D-split
(duplicate served writes, adopting strands them) are BOTH born pre-4.31 and are
therefore identical on every durable signal. Verified: tenant-root and
tenant-dsplit have the same rev-1 scheme and the same first_deployed deltas, yet
one duplicate served and the other did not. The only thing that ever separated
them was readyReplicas, which is a snapshot and not evidence. So the render keeps
refusing on presence, and the comment now says why — naming the signal, so nobody
re-derives the broken one.
The signal IS used where it belongs, in the audit.
- naming-guard.yaml / seaweedfs.yaml: match the release-named generation by
RECONSTRUCTING the name 4.31 gives this release's volume component
(templates/_naming.tpl, replaying seaweedfs.fullname + componentName), tested
FIRST because the prefixes nest. Fixes a blocker: an instance legitimately named
`seaweedfs-volume` renders seaweedfs-volume-system-volume and
data1-seaweedfs-volume-system-volume-0, which BOTH satisfy the chart-named
prefixes — a prefix-only guard read its live storage as legacy and would render
onto the empty chart-named claims, stranding the real generation. The parity
bats passed straight through it, because parity pins sameness, not correctness.
Reconstruction also removes the name-match blind spot at source: a truncated long
instance name now matches by PVC, not only by StatefulSet.
- hack/seaweedfs-naming-audit.sh + .bats: the classifier is a tested script now.
Two revisions of it shipped as an untested snippet inside the runbook and both
were wrong in ways that routed a live tenant into the step that strands its data:
a selector `^data1-(.*seaweedfs.*)-volume` that also matched the chart-named
claims, so the release-named range spanned BOTH generations, every tenant read as
"overlapping / mid-rebind", and a genuine S-damaged tenant was routed AWAY from
Step 2a into Step 2 — which deletes its data claim, fails to recreate it, then
deletes its StatefulSets; and a `grep seaweedfs` name match that reported
"L — nothing to do" for a long-named tenant the chart refuses. Both are covered.
The audit reports L/S/MIXED matching the guard, names the original generation
from both durable signals, detects MID-REBIND (both generations at
first_deployed), and states plainly that "original" is not "empty" and that
emptiness must be established from the duplicate's volume files before deleting.
A bats case renders _naming.tpl through helm and compares it to the script's
reconstruction, so the two implementations cannot drift.
- Runbook: Step 1 calls the script. Steps 3/4 were circular — Step 3 ended in an
upgrade the presence-gated guard refuses, and Step 4 told the operator to clear
duplicates AFTER an upgrade that cannot run until they are gone. Step 3 now
removes the duplicate BEFORE the upgrade and re-runs the audit to prove it; Step 4
starts from "every tenant reads L or S". D-wedged no longer claims to need
nothing. Step 3's "enumerate fids that resolve only on the duplicate" was not an
executable procedure and is now an explicit escalation: two masters allocate
volume IDs from independent sequences into one shared seaweedfs-db, so there is
no supported tool that reconciles two volume-ID spaces against one metadata
store. Step 2's re-bind restores the cleanup hook's labels on the recreated
claims, without which a recovered tenant leaks its volumes on a later delete.
Fixture corrected: data1-...-x1-system-volume-0 is not a name the chart renders;
componentName gives ...-x1-system-seaw-volume-0.
Assisted-By: Claude <noreply@anthropic.com>
Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com>
|
||
|
Myasnikov Daniil
|
ae93356ed1 |
fix(seaweedfs): name cluster-scoped RBAC per namespace, not after the pinned fullname
ClusterRoles and ClusterRoleBindings are cluster-scoped: one object per name, cluster-wide. This chart installs once per tenant, so every cluster-scoped name it renders must be unique per install. Four of them were not. Upstream 4.31 switched them from global.serviceAccountName to seaweedfs.fullname. That is fatal in combination with the #3282 pin: fullnameOverride: seaweedfs is what adopts the running NAMESPACED workloads across the rename, so fullname is by construction the SAME string in every tenant. Every SeaweedFS instance in the fleet therefore renders seaweedfs-objectstorage-provisioner, seaweedfs-rw-cr and their bindings — one object, five claimants. NOT a #3282 regression. Rendering v1.4.5, v1.5.3 and main for the same tenants: v1.4.5 tenant-root-seaweedfs-* tenant-named-seaweedfs-* unique v1.5.3 seaweedfs-system-* foo-system-seaweedfs-* COLLIDES (identical for every default-named instance; a non-default instance name was the only thing that escaped) main seaweedfs-* seaweedfs-* COLLIDES (nothing escapes: the pin removes the last distinguishing input) So the collision shipped in 1.5.0 with the 4.31 bump; #3282 widens it from "every default-named instance" to "every instance". Observed live on the upgrade stand: one ClusterRole/seaweedfs-objectstorage-provisioner annotated to a single owning tenant, four other tenants' COSI provisioners left with no RBAC, and tenant-dsplit's helm history recording Upgrade failed: original object ClusterRole "seaweedfs-objectstorage-provisioner" not found The binding is the sharp end: its subject names a ServiceAccount in the release namespace, so the single live ClusterRoleBinding grants cluster-wide secrets RBAC to whichever tenant reconciled last, and silently revokes it from the rest. Fix: put the four names back on global.seaweedfs.serviceAccountName, which extra/seaweedfs already sets to <namespace>-seaweedfs and which cosi-provisioner-sa-name.patch already uses for the ServiceAccount and the binding's subject — the cluster-scoped names were simply left behind. Three of the four come out byte-identical to pre-4.31, so a 1.4.x tenant adopts them in place; the fourth (rw-crb) replaces upstream's username-shaped system:serviceaccount:<sa>:default and is pruned from the release's own manifest. The namespaced pin is untouched: workloads still render seaweedfs-master/-filer/ -volume, so #3282's adoption is intact. Uniqueness now rests on a value, so assert it rather than trust it: templates/cluster-scoped-rbac-guard.yaml refuses to render when the name would not carry the release namespace. It sits behind the same namespace canary as the naming guard, because system/seaweedfs's own values.yaml ships a placeholder that only extra/seaweedfs overrides — a client-side render must not trip on it. Tests fail on unfixed code: cluster_scoped_names_test asserts two tenants get two names, and unpatched both render seaweedfs-objectstorage-provisioner (5 failed, 0 passed). They test the vendored chart templates directly, which is where the rename happens. The guard includes seaweedfs.compat before reading. extra/seaweedfs sets the OLD flat key global.serviceAccountName, and the shim is what folds it into the canonical global.seaweedfs.serviceAccountName; without it the guard compares against this chart's placeholder default and refuses every real upgrade. Caught by helm upgrade --dry-run=server against the stand, not by unit tests — the mocks never carry the flat key. Verified live with --dry-run=server on the stand: tenant-l (installed pre-4.31, never upgraded) already owns tenant-l-seaweedfs-objectstorage-provisioner and tenant-l-seaweedfs-rw-cr, and the fixed chart renders exactly those — adopted in place, no ownership error. tenant-root renders tenant-root-seaweedfs-* and also dry-runs clean. Leftovers on the release-based names that a 1.5.x tenant passed through are inert once every tenant is upgraded (no release renders them any more) but are not always pruned, since more than one release claimed them; the runbook adds the sweep. Review round 2: - The uniqueness guard used `contains .Release.Namespace $sa`, a substring test that accepts namespace `tenant-a` for service account `tenant-ab-seaweedfs` — a different namespace's. Anchored to hasPrefix "<namespace>-". It still asserts the VALUE the names are built from, not the rendered names, so it cannot catch the chart being reverted to name them after the pinned fullname; cluster_scoped_names_test.yaml covers that axis and is what actually bites. - Added the case that would have caught the compat bug: every test set the CANONICAL global.seaweedfs.serviceAccountName, while production sets the OLD FLAT global.serviceAccountName that only seaweedfs.compat folds in. Deleting the include left the suite passing 5/5 while every real upgrade refused. The new case sets the flat key; with the include removed it now fails with the production error. - Runbook sweep missed the fourth pre-4.31 name: 4.31 renamed the master-rw binding from upstream's username-shaped system:serviceaccount:<sa>:default to <sa>-rw-crb, and the old one carries neither suffix nor a tenant prefix, so the grep could not see it (live on the stand as system:serviceaccount:tenant-l-seaweedfs:default). Also documents the rolling- upgrade window: Helm prunes by name without checking ownership, so the first tenant onto the fixed chart deletes shared objects a not-yet-upgraded tenant is still bound through — COSI provisioning 403s until they reconcile, S3 traffic unaffected. Review round 3: - hasPrefix "<namespace>-" was still too loose. It accepts `tenant-a-b-seaweedfs` for namespace `tenant-a` — which is ALSO the natural value for namespace `tenant-a-b`, so two namespaces would accept one string and collide on the very object this guard exists to keep unique. extra/seaweedfs sets exactly `<namespace>-seaweedfs`, so assert equality with it and nothing looser. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com> |
||
|
Myasnikov Daniil
|
8499b5f4a1 |
fix(seaweedfs): move the naming-migration guard into the render that actually renames
The class-S guard from #3282 lives in extra/seaweedfs, but that render is not in the path of a platform upgrade: the <name>-system HelmRelease pulls system/seaweedfs from a platform-managed ExternalArtifact, so bumping the platform upgrades the release directly from the new artifact and applies fullnameOverride: seaweedfs without extra/seaweedfs ever re-rendering. On a tenant installed fresh on 1.5.x that stood up an empty chart-named set beside the live release-named data, deleted the renamed s3 Service, and left S3 readable only because the empty set's pods cannot start. Worse, the damage flips the guard's own classification: creating the chart-named artifacts makes $legacyData true, so the guard then reports D-split and sends the operator to runbook Step 3 — which quiesces the renamed set, i.e. the one holding ALL the data. - Add the ENFORCING guard to system/seaweedfs (templates/naming-guard.yaml): the only render guaranteed to sit between a platform upgrade and the tenant's workloads. It also refuses to render if the fullnameOverride pin is ever lifted. - Tell S from D by PVC AGE when both generations exist: PVCs are never recreated in place (StatefulSets are, by the adoption hook), so the older volume-PVC generation is where the data was born. Renamed-older ⇒ class S (damaged variant, new Step 2a), never D-split. - Fail closed: the release namespace is a lookup canary; an upgrade that cannot see the cluster refuses instead of classifying blind. Client-side installs (CI lint/unittest) still render — they never touch live data. - Mirror the same classification in extra/seaweedfs so the operator-facing SeaweedFS application reports the correct class. - Tests now model the ExternalArtifact upgrade path in the chart it actually runs through (the old ones mocked lookups only in extra/seaweedfs, which is why this shipped green), including the damaged-S tenant observed live. - Runbook: add S-damaged audit classification (PVC age), Step 2a cleanup of the empty chart-named set, and the D-wedged anti-affinity stall workaround observed on 3-node clusters. helm-unittest 1.0.3 ignores release.isUpgrade, so the canary refusal is verified with 'helm template --is-upgrade' instead of a unit test. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com> |
||
|
myasnikovdaniil
|
df0da594c0 |
fix(metrics-server): move the listener off the kubelet's port (#3361)
## What this PR does metrics-server is the third component serving on port 10250 — the kubelet's port — after the webhooks addressed in #3359 and #3360. It ships default-enabled in the platform bundle and is also installed into every tenant Kubernetes cluster with no value overrides, so a package-root override propagates to both. The hard defect here is different from the webhook cases. The chart has first-class `hostNetwork.enabled` support, documented upstream for clusters where the API server cannot reach pod IPs — and on the default port that mode cannot start at all: the kubelet already holds 10250, so the process dies with `bind: address already in use`. Upstream is aware (kubernetes-sigs/metrics-server#1026 carries the exact panic) and declined to change the chart default, advising hostNetwork users to set their own port. This override is that port, set ahead of need. The misleading-certificate angle of the sibling fixes mostly does not apply: the chart defaults `apiService.insecureSkipTLSVerify` to true, so a connection misrouted to a node would fail silently rather than with a wrong-certificate error — and with endpoints resolving to pod IPs such a misroute is not established to occur here at all. The comment states that as a hypothesis, not a fact. What is factual is the consumer set: `kubectl top`, tenant HPAs, and the console's cluster-usage page all read `metrics.k8s.io`; the shipped VPA does not — it is configured with Prometheus storage. The port is 4443 rather than the siblings' 10260, for a reason worth keeping: metrics-server is the one component of the three with supported hostNetwork, and parking it on 10260 would put a live host-network listener on exactly the port the webhook fixes rely on being silent — turning their honest connection refusals back into answered handshakes. 4443 is the value metrics-server's own manifests shipped through v0.6.4, before v0.7.0 moved them to 10250; it sits outside the 10248-10259 node-component block, and nothing in the tree binds it. No chart patch is needed: the Service, both probes and the ServiceMonitor already reference the port by name, so the rollout-gap that required a vendored patch in #3360 does not exist here. The package also gains the standard `test:` target it was missing. Not verified without a cluster: that the aggregated APIService stays Available through an upgrade, and that no supported substrate filters 4443 inbound on nodes — the tree configures no host firewall, but Talos and managed substrates are outside what a render can prove. There is no e2e coverage of `metrics.k8s.io` at all; a wrong guess here would surface as HPAs silently not scaling. ### Release note ```release-note fix(metrics-server): move the listener off port 10250, which the kubelet also holds. The default made hostNetwork mode unable to start at all, and the new port keeps node-port collisions out of the metrics path. ``` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Bug Fixes** * Configured the metrics server to listen on secure port 4443. * Updated service routing and health checks to use the named HTTPS port, improving reliability when port settings change. * Ensured pod networking does not rely on host networking. * **Tests** * Added automated chart tests covering container ports, service routing, and liveness/readiness probes. * Added a test command for running the chart test suite. <!-- end of auto-generated comment: release notes by coderabbit.ai --> |
||
|
myasnikovdaniil
|
8af4c1d600 |
fix(external-secrets): move the webhook off the kubelet's port (#3360)
## What this PR does The external-secrets webhook listens on port 10250 — the kubelet's port — with `failurePolicy: Fail`. This is the same trap being removed from cert-manager in #3359: a connection to the webhook Service that resolves to a node IP instead of the pod IP reaches the kubelet, which completes the handshake with its own node serving certificate, and the API server rejects the call with an x509 error that blames the webhook's PKI rather than the routing layer. The port collision does not cause the misroute; it turns a rare transient one into a confidently misattributed failure. The blast radius here is wider than admission alone: `crds.conversion.enabled` defaults to true and points every ExternalSecret CRD's conversion webhook at the same Service, so a misrouted connection also breaks CRD conversion — reads at non-storage versions, not just writes. The webhook moves to 10260, matching #3359. The controller-runtime default 9443 was considered and rejected: the etcd kube-rbac-proxy binds it on host network, which would reproduce exactly the same wrong-certificate failure with a different certificate. Nothing in the tree binds 10260. Unlike cert-manager, the upstream chart's Service targets the port numerically — `targetPort: {{ .Values.webhook.port }}` — and a numeric targetPort is copied into every endpoint verbatim rather than resolved per pod. With one replica, a port change would direct traffic at a port the still-running old pod does not serve while the replacement is not yet Ready, failing every admission and conversion call in the gap; surge-first rollout does not help because both pods sit behind the same number. A one-line vendored-chart patch points the Service at the `webhook` container-port name upstream already declares (the metrics port in the same Service is already named), which closes the gap: during the overlap each pod is routed on the port it actually serves. The patch is applied by `make update` with `--forward`, so an already-applied hunk fails loudly instead of being silently reversed. Upstream has already made the same change — merged as external-secrets/external-secrets#5108 and shipped since chart 0.19.1 — but the vendored chart here is 0.10.4, nine minor releases earlier, so the patch carries the fix until the chart is bumped past that point. The `--forward` guard is exactly what will flag that moment: on a bumped chart the patch fails loudly, signalling it should be dropped. external-secrets-operator ships disabled by default and is enabled per-cluster via `bundles.enabledPackages`, so this is defensive hardening for opt-in installs rather than a response to an observed incident. `metrics-server` is a third instance of the same port collision, left out here: it serves an APIService rather than an admission webhook, a lesser failure class deserving its own change. ### Release note ```release-note fix(external-secrets): move the webhook off port 10250, which the kubelet also serves, and make its Service target the port by name so the move rolls out without an admission gap ``` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Bug Fixes** * Improved webhook Service routing by switching to the named `targetPort` (`webhook`) for pod traffic. * Configured the webhook listener to run on port **10260** to avoid prior connection/TLS certificate misrouting issues. * Preserved admission validation behavior and CRD conversion routing. * **Tests** * Added Helm chart regression tests covering webhook port wiring, networking settings, failure policy behavior, and CRD conversion. * **Chores** * Added Helm chart unit testing to the development workflow and extended the chart update process to apply the webhook routing adjustment. <!-- end of auto-generated comment: release notes by coderabbit.ai --> |
||
|
Myasnikov Daniil
|
955f7ccaa6 |
fix(opensearch-operator): use RollingUpdate maxSurge=0 instead of Recreate
#3040 set the single-replica operator Deployment to strategy.type: Recreate to avoid two uncoordinated managers overlapping during a rollout while leader election is gated off. That breaks Helm upgrades of existing releases: the upgrade fails validation immediately, helm-controller retries it (x19 over 11m in Actions run 29480724982), and the HelmRelease never becomes Ready: Deployment.apps "opensearch-operator-controller-manager" is invalid: spec.strategy.rollingUpdate: Forbidden: may not be specified when strategy type is Recreate The trigger is server-side apply. A v1.5.x release shipped no strategy at all, so the apiserver defaulted spec.strategy.rollingUpdate to 25%/25%, and the helm-controller field manager never owned that block. helm-controller applies server-side by default (Install.ServerSideApply defaults to true, Upgrade.ServerSideApply to "auto"; the operator's package_reconciler.go sets neither), and SSA does not remove a field the applier never owned merely because the new intent omits it -- so sending type: Recreate leaves the defaulted rollingUpdate in place and the apiserver rejects the merged object. The client-side path is unaffected: DeploymentSpec.Strategy carries patchStrategy:"retainKeys", so a 3-way merge emits {"$retainKeys":["type"],"type":"Recreate"} and does clear rollingUpdate. That is why this surfaces through helm-controller's server-side apply but not a local 'helm upgrade'. Keep type: RollingUpdate and set rollingUpdate.maxSurge: 0 / maxUnavailable: 1 on the single-replica path instead. maxSurge: 0 caps total pods at replicas, so the old manager is scaled down before the new one is scaled up, rather than starting a surge pod while the old manager is still fully live as the default 25% would. It is not Recreate's exclusivity -- the deployment controller orders the ReplicaSet desired counts, not the pod lifecycles, so a draining old manager can still overlap the new pod -- but it keeps the type transition legal on every apply path, and recovers releases already on an rc build with Recreate. This matches the existing kube-ovn idiom (central-deploy.yaml). Also add a 'test' target to the package Makefile: hack/helm-unit-tests.sh runs a package's suite only when its Makefile defines one, so tests/leader_election_test.yaml had never executed. Multi-replica installs keep the default RollingUpdate (leader election on), unchanged. The change is carried in patches/leaderElection.diff so it survives 'make update'. Verified on kind v1.33.1: SSA of the v1.5.x manifest followed by SSA of the Recreate manifest reproduces the error, while the same pair applied client-side succeeds; SSA of the maxSurge:0 manifest succeeds both from v1.5.x and from a live Recreate Deployment. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com> |
||
|
Aleksei Sviridkin
|
aa2525a8d8 |
fix(metrics-server): move the listener off the kubelet's port
metrics-server scrapes every kubelet on 10250 and, by upstream default, serves its own aggregated API there too. Sharing that number makes hostNetwork unusable. The chart offers it for clusters whose apiserver cannot reach Pod IPs, but at the default port the process joins the node namespace where the kubelet already holds 10250 and exits with "bind: address already in use". Upstream has had this reported and kept the default anyway, on the view that hostNetwork is non-standard here and that those users can set a port themselves. It also decides how a misrouted connection would present. Were a connection to the Service to arrive at a node IP rather than a Pod IP it would reach the kubelet, and because the APIService sets insecureSkipTLSVerify the aggregation layer would never check whose certificate it got, leaving nothing in the error to name the wrong server. Nothing here establishes that such a misroute occurs; moving the port only makes one fail as a connection refusal rather than as a rejection from an unexpected daemon. The override gives up the firewall coverage 10250 buys on substrates permitting only 443 and 10250 to nodes. Cozystack's own do not filter it, but this package ships into every tenant cluster and no e2e exercises the metrics API. 4443 is the value upstream's manifests carried through v0.6.4, before v0.7.0 shipped 10250. It sits clear of the 10248-10259 block the node components occupy and nothing else in this repo binds it. The Service's targetPort and both probes reach the container by name, so the number moves without a rollout gap. The tests pin that alongside the value, since the vendored chart ships no schema that would reject an upstream rename of the key. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <f@lex.la> |
||
|
Aleksei Sviridkin
|
a90fa38fcb |
fix(external-secrets): move the webhook off the kubelet's port
The chart defaults the validating webhook to port 10250, which the kubelet also serves on. When a connection to the webhook Service resolves to a node IP rather than a Pod IP it reaches the kubelet, which completes the TLS handshake with its node serving certificate, and the apiserver rejects the call with an x509 error naming the node instead of the webhook Service. The certificate is valid, just the wrong server's, so the failure reads as a PKI fault and hides the misroute behind it. Moving the listener to a port nothing else answers on turns the same misroute into a connection refusal, which names the real problem. This does not prevent the misroute. 9443, the controller-runtime default this chart would otherwise take, is disqualified: the etcd kube-rbac-proxy binds it on host-network, so it would swap one wrong certificate for another. Nothing in the tree binds 10260 in a node network namespace. It remains CloudControllerManagerWebhookPort in k8s.io/cloud-provider, so a host-network cloud-controller-manager added later has to keep clear of it. Point the Service at the "webhook" container-port name so the move is safe. Upstream renders targetPort from webhook.port as a number, which every endpoint shares regardless of what its Pod listens on, so changing the port alone would aim the Service at a port the outgoing Pod does not serve until the replacement passes readiness. That window rejects admission for externalsecrets, secretstores and clustersecretstores, and breaks CRD conversion, which routes through the same Service. No values-only setting closes it; a named targetPort resolves per-Pod and does. Add unit tests pinning the listener flag, the containerPort and the named targetPort. This chart's values.schema.json allows unknown keys, so a renamed upstream value would be silently ignored and the port would revert to 10250 with nothing else noticing. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <f@lex.la> |
||
|
Aleksei Sviridkin
|
df85632c89 |
fix(cert-manager): move the webhook off the kubelet's port
The vendored chart defaults webhook.securePort to 10250, which is the port the kubelet serves on. Upstream picks that value deliberately, for GKE private clusters where apiservers may reach nodes only on 443 and 10250; it is free of consequence there and unsafe here. When a connection to the webhook Service resolves to a node IP instead of a Pod IP, it reaches the kubelet, which completes the TLS handshake with its node serving certificate. The apiserver then rejects every cert-manager admission call in the cluster with x509: certificate is valid for <node>, not cert-manager-webhook.cozy-cert-manager.svc and, because the webhook is failurePolicy: Fail, that stalls platform bootstrap. The certificate is valid; it is simply the wrong server's. The error names cert-manager and points away from the routing layer that actually failed, which makes a rare misroute expensive to diagnose. Serve on 10260 instead, the port upstream's troubleshooting guide prescribes for this exact error. It documents the failure on EKS Fargate, where a microVM forwards every port to the pod except the one its own kubelet already holds; the cause there is understood and ours is not, but the shape is the same, down to the certificate that comes back naming the node. What the fix needs of the port is that nothing answers on it in a node's network namespace, and nothing in the platform does. The same misroute is then refused at connect time, which is an honest and immediately diagnosable error. This does not prevent the misroute, whose cause is unestablished; it removes the collision that turns one into a cluster-wide failure attributed to the wrong component. The Service targets the container port by name, so it follows the change without an edit, and a rolling update resolves the old and new ports per Pod rather than dropping the endpoint. Tests pin the listener flag together with the advertised containerPort so the two cannot drift apart. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <f@lex.la> |
||
|
Timofei Larkin
|
7475491686 |
fix(keycloak): point HTTPRoute backends at the service port
The keycloak-http Service only exposes port 8080, but every HTTPRoute backendRef referenced port 80. The Gateway API resolves backendRefs against the Service's declared ports, so the route had no valid backend and envoy answered 503 for all Keycloak hostnames whenever gateway-enabled is true. The Ingress template is unaffected: it references the port by name (http). Signed-off-by: Timofei Larkin <lllamnyp@gmail.com> |
||
|
Andrey KolkovandClaude Fable 5
|
67db8f7da7 |
fix(backups): make the barman-cloud serverName stable across every flow
Two defects of the same class — the WAL-archive S3 prefix (serverName) changing or defaulting differently between components: Recovery default regression (review blocker): native CNPG defaulted an absent externalClusters serverName to the entry name (= bootstrap.oldName), but the barman-cloud plugin defaults it to the NEW cluster's name, so a legacy-flow restore with bootstrap.serverName unset would read the wrong S3 prefix (hard failure, or silently restoring another cluster's data). The chart now always emits serverName, defaulting to bootstrap.oldName. Live-cluster serverName flip (found by the gated postgres backup e2e on a real cluster): the chart renders spec.plugins without serverName (effective prefix = cluster name) while a strategy template may name a different one; the driver's ForceOwnership patch then flipped the prefix mid-stream, splitting the WAL archive across two prefixes — the WAL holding the backup's start was lost between them and the restore failed with "WAL not found". The driver now preserves the live Cluster's effective serverName (explicit parameter, or the cluster name when the plugin is attached without one) and only applies the strategy's value to a Cluster with no plugin attached; both charts pin serverName explicitly, and the example strategy no longer sets one. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: Andrey Kolkov <androndo@gmail.com> |
||
|
Andrey KolkovandClaude Opus 4.8
|
a1c135c2a1 |
fix(backups): omit empty retentionPolicy from the ObjectStore
Review follow-up (cozystack#3300, non-blocking): the barman-cloud ObjectStore CRD
validates spec.retentionPolicy against ^[1-9][0-9]*[dwm]$, so an operator who
explicitly sets backup.retentionPolicy="" would render retentionPolicy: "" and
the API server would reject the ObjectStore, breaking backup provisioning. Guard
the field in the apps/postgres and keycloak chart ObjectStores with
{{- if .Values.backup.retentionPolicy }} (the Go driver path is already safe via
omitempty). Add a helm-unittest asserting an empty retentionPolicy is omitted.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Signed-off-by: Andrey Kolkov <androndo@gmail.com>
|
||
|
Andrey KolkovandClaude Opus 4.8
|
5fbc90047d |
docs(apps/postgres): modernize backup annotations/README for the plugin flow
Review follow-up (cozystack#3300): the apps/postgres user-facing docs still described the removed native spec.backup.barmanObjectStore / archive_command model. Update the values.yaml @field/@typedef/@param annotations (useSystemBucket, schedule, S3CredentialsSecret, EndpointCA, endpointCA, the @param backup note, bootstrap.serverName) and the hand-written README Operations prose to the barman-cloud plugin + ObjectStore model, then regenerate values.schema.json, the README parameters table, the api types, and the postgres-rd ApplicationDefinition schema (which feed the dashboard form help + cozyrd schema). Field names, defaults, and the storageClass immutability marker are unchanged; only backup/ bootstrap description text changed. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: Andrey Kolkov <androndo@gmail.com> |
||
|
Andrey KolkovandClaude Opus 4.8
|
2212367842 |
fix(backups): own driver-created ObjectStore + correct stale barman references
Review follow-ups (cozystack#3300): - Lifecycle/leak: applyClusterPluginBackup now sets an ownerReference from the driver-created barmancloud.cnpg.io/ObjectStore to its cnpg.io/Cluster, so Kubernetes garbage-collects it when the Cluster is deleted (tenant app teardown or the RestoreJob purge). In the platform useSystemBucket flow the chart does not render this ObjectStore, so previously it was orphaned in the tenant namespace. Drop the now-accurate-only 'delete' verb on objectstores from the driver RBAC (GC handles removal; the driver never deletes it) and correct the RBAC comment that falsely claimed the purge deletes it. - Diagnostics/comments: the RestoreJob WAL-archive-deadline failure message and several doc comments still told operators to check spec.backup.barmanObjectStore / externalClusters[].barmanObjectStore, fields no longer emitted after the migration. Point them at the barman-cloud plugin (spec.plugins + ObjectStore, externalClusters[].plugin) instead so they don't mislead during an incident. - Test: TestApplyClusterPluginBackup_PatchesExistingCluster now asserts the ObjectStore carries an ownerReference to the Cluster (kind/name/uid). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: Andrey Kolkov <androndo@gmail.com> |
||
|
Andrey KolkovandClaude Opus 4.8
|
39bd8623c7 |
fix(keycloak): back up keycloak-db via the barman-cloud plugin
The keycloak-db backup added in #3174 uses the in-tree native spec.backup.barmanObjectStore, whose barman-cloud-* binaries ship only in the CNPG system image variant; keycloak-db is pinned to the standard variant, so base backups fail with 'barman-cloud-backup: executable file not found'. The proposed workaround (#3306) switches keycloak-db to the deprecated system image variant when backups are enabled. Migrate keycloak-db to the barman-cloud plugin instead, matching the rest of this PR: render spec.plugins referencing a barmancloud.cnpg.io ObjectStore (the S3/barman config moves there) and a method=plugin ScheduledBackup, keeping the Cluster on the standard image (the plugin runs the barman tooling in a sidecar). This removes the need for the system-variant workaround entirely, so #3306 can be closed. cozystack#3300. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: Andrey Kolkov <androndo@gmail.com> |
||
|
Andrey Kolkov
|
13ff340277 |
fix(backups): migrate CNPG backups to the barman-cloud plugin
Signed-off-by: Andrey Kolkov <androndo@gmail.com> |
||
|
Aleksei Sviridkin
|
a5cabca73e |
fix(ouroboros): render controller rollout strategy in every mode (#3323)
## What this PR does Bumps the ouroboros chart and controller image to 0.8.1, which fixes a latent upgrade break in the controller Deployment's rollout strategy, and pins the fixed contract from the cozystack side. Up to chart 0.8.0 the controller Deployment rendered a rollout strategy **only** in `external-dns` mode, as `type: Recreate`, and rendered no strategy block at all in the other modes. The apiserver defaults a `rollingUpdate` block onto any Deployment created without one, so switching a live release into `external-dns` laid `Recreate` on top of that defaulted block, and the apiserver rejected the object: ``` spec.strategy.rollingUpdate: Forbidden: may not be specified when strategy `type` is 'Recreate' ``` 0.8.1 renders the same `RollingUpdate` block, with no surge, in every mode. The strategy no longer changes when `controller.mode` does, so the rejected combination cannot arise on any transition. Rendering `Recreate` unconditionally would not have worked: existing `coredns`-mode releases already carry the defaulted `rollingUpdate` block and would hit the identical rejection on their first upgrade. Always-`RollingUpdate` is upgrade-safe from both existing states, and rollback works for the same reason. **Reachability, stated plainly:** this is a latent break, not an active one. The host pins `coredns` and tenants pin `coredns-import`, neither of which rendered a strategy in 0.8.0, and no shipped configuration selects `external-dns`. But `controller.mode` is a documented, user-settable value that the tenant wrapper exposes through `valuesOverride`, so the transition is reachable by configuration rather than only in theory. **Why it went unnoticed:** the break only surfaces under server-side apply, which helm-controller uses by default. `DeploymentSpec.Strategy` carries `patchStrategy:"retainKeys"`, so a client-side strategic merge clears `rollingUpdate` on the way through and a plain `helm upgrade` never reproduces it. Only the upgrade breaks, never the install, so nothing catches it at render time either. **Why no surge, in every mode:** the controller has no leader-election in any mode and no serving path — no ports, no probes — so surging a second pod buys no availability anywhere. In `external-dns` mode it also removes a real hazard: that reconciler `Update`s DNSEndpoints with no conflict retry, so two overlapping reconcilers race on Get→Update. The `coredns` paths wrap Get→Update in `RetryOnConflict` and converge instead, which is why only `external-dns` rejects `replicaCount > 1`; that asymmetry is deliberate and is now called out in the template so it does not get tidied away later. The upstream comment is explicit that `maxSurge: 0` is *not* Recreate-equivalent exclusivity — the surge budget counts desired replicas, not live pods — and rests the argument on what is actually guaranteed: the old pod's SIGTERM precedes the new pod's creation, and SIGTERM cancels the reconciler context. The proxy Deployment is untouched and keeps its default rolling surge, as the data-path component. The fix was made upstream in `lexfrei/ouroboros` and released as v0.8.1 rather than patched into the vendored tree here, since `make update` regenerates `charts/` wholesale. The vendored chart in this PR is a verbatim `make update` pull of the published 0.8.1 artifact, byte-identical to the registry. Chart manifest digest and controller image digest are bumped in lockstep with the version, as the package Makefile requires. 0.8.1 also carries an upstream Renovate bump of the cleanup-hook kubectl image to 1.36.1 (verified to resolve, multi-arch). The `update` target additionally drops the empty digest-named directory that `helm pull` leaves beside the untarred chart when pulling by digest. Git ignores empty directories, which is exactly why it lingered unnoticed. ### Testing `make -C packages/system/ouroboros test` → 17/17 pass. The three new cases pin the rendered strategy on `coredns`, `coredns-import` and `external-dns`. `external-dns` is included deliberately: it is the only mode that ever rendered `Recreate`, so a future bump that re-gated the block would slip past a coredns-only suite. Verified by re-gating the bug surgically against the new suite — it fails on exactly that case, and passes without it. Upstream additionally tests this contract across every mode in its own suite. ### Screenshots Not applicable — no UI change. ### Downstream repositories Walked the trigger map in `docs/agents/contributing.md` against the diff file by file. The diff touches only `packages/system/ouroboros/`: the package Makefile's version and digest pins, the vendored chart (regenerated), the image pin in `values.yaml`, and the package's own tests. It adds, renames and removes no package under `packages/apps/` or `packages/extra/`; touches no `packages/core/platform/values.yaml` key, no platform variant, bundle or component; changes no `ApplicationDefinition`, no `values.schema.json`, no version enum, no `release.prefix`; touches nothing under `hack/`, no namespace, and no node prerequisite. No trigger matches. - [x] No downstream repository is affected by this change - [ ] [cozystack/website](https://github.com/cozystack/website) - follow-up: - [ ] [cozystack/terraform-provider-cozystack](https://github.com/cozystack/terraform-provider-cozystack) - follow-up: - [ ] [cozystack/ansible-cozystack](https://github.com/cozystack/ansible-cozystack) - follow-up: - [ ] [cozystack/ccp](https://github.com/cozystack/ccp) - follow-up: - [ ] [cozystack/talm](https://github.com/cozystack/talm) - follow-up: - [ ] [cozystack/cozyhr](https://github.com/cozystack/cozyhr) - follow-up: - [ ] [cozystack/cozy-proxy](https://github.com/cozystack/cozy-proxy) - follow-up: - [ ] [cozystack/cozystack-telemetry-server](https://github.com/cozystack/cozystack-telemetry-server) - follow-up: - [ ] [cozystack/external-apps-example](https://github.com/cozystack/external-apps-example) - follow-up: - [ ] [cozystack/examples](https://github.com/cozystack/examples) - follow-up: ### Release note ```release-note fix(ouroboros): render the controller rollout strategy in every controller.mode. Up to chart 0.8.0 the strategy was rendered only in external-dns mode, as type: Recreate, so switching a live release into that mode applied Recreate on top of the rollingUpdate block the apiserver defaults onto a Deployment created without one, and the upgrade was rejected with "spec.strategy.rollingUpdate: Forbidden: may not be specified when strategy `type` is 'Recreate'". The rejection only occurs under server-side apply, which helm-controller uses by default. The strategy is now identical across all modes, so a mode switch changes no strategy field. ``` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Updated Ouroboros to version 0.8.1 with refreshed, pinned images. * Improved controller rollouts using a consistent rolling-update strategy across supported modes. * Updated cleanup hook tooling to version 1.36.1. * **Bug Fixes** * Prevented leftover chart directories from accumulating during chart updates. * Improved deployment behavior by limiting rollout disruption while maintaining controller availability. * **Tests** * Added coverage validating rollout behavior across all supported controller modes. <!-- end of auto-generated comment: release notes by coderabbit.ai --> |
||
|
myasnikovdaniil
|
4a00b502d7 |
fix(seaweedfs): adopt legacy workloads in place across the 4.31 rename (#3282)
## What this PR does Fixes a v1.5.0 upgrade regression where a tenant ends up with **two** SeaweedFS workload sets. **Root cause.** v1.5.0 bumped the vendored SeaweedFS chart `4.0.405 → 4.31.0`. Upstream changed resource naming: before 4.31 the chart named workloads after the **chart** (`seaweedfs-*`, ignoring the release name), and 4.31 names them after the **release**. The data-plane release is `<name>-system`, so every StatefulSet wanted to become `seaweedfs-system-*`. StatefulSet names are immutable, so Helm could not rename in place — it stood up a second, empty set beside the running one while the data stayed on the original `data1-seaweedfs-volume-*` PVCs. Where the node count let both sets run, they shared one `seaweedfs-s3` Service (identical pod labels) and one `seaweedfs-db` filer metadata database, so the empty set answered reads and the two master sets mixed volume IDs from independent sequences into one metadata table. **Fix.** Pin `fullnameOverride` in `system/seaweedfs` values so the rendered names stop following the release name, which is the convention already used by `cozy-proxy`, `flux-operator`, `grafana-operator`, `linstor-scheduler` and `victoria-metrics-operator`. The names go back to what every pre-1.5 cluster already runs, so the upgrade adopts the running workloads **and their volumes** in place. No data migration, no platform migration. **The one case that cannot be adopted.** A tenant installed *fresh* on 1.5.x wrote its data under the renamed workloads, so its volumes are not the chart-named ones. Pinning the chart name there would rename the workloads *away* from that data and bring up an empty cluster beside it. Helm cannot move data between PVCs, so `extra/seaweedfs` refuses to render for such a tenant and points at the recovery runbook, which re-binds the existing PVs onto the adopted PVC names (no copying — the claim is renamed, the volume is not touched). The guard is fail-closed: it fires only when renamed volumes are present and no adopted ones are. Two naming subtleties the guard has to respect, both verified by rendering the shipped 1.5 chart: - Before 4.31 the chart ignored the release name, so an instance running under a non-default name (`SeaweedFS` is a user-creatable kind) *also* stored its data on `data1-seaweedfs-volume-*`. Such a tenant is adopted, not blocked. - On 4.31 the fullname helper appends the chart name when the release name does not contain it, and the component helper truncates to 56 characters. A fresh 1.5.x instance named `archive` therefore wrote to `data1-archive-system-seaweedfs-volume-*`, and a long name loses the chart name from its tail entirely. The guard matches volumes by shape and by the chart labels on the StatefulSet instead of reconstructing those names, so both are caught. **On the duplicate set.** Once the fix reconciles, the `seaweedfs-system-*` StatefulSets and Deployments are no longer part of the release and Helm removes them on its own. What it cannot remove — PVCs it did not template, and cert-manager Secrets that carry no owner reference — is inert, and the runbook clears it explicitly. A pre-upgrade migration was considered and dropped: it would duplicate what Helm already does, while its only unique action (deleting PVCs) cannot distinguish "the duplicate never served" from "it served and is currently down", and any transient delete failure under `set -e` would fail the platform's pre-upgrade hook and block delivery of this very fix. ## Testing `helm unittest` — `system/seaweedfs` 21 tests, `extra/seaweedfs` 17 tests. The guard's `lookup` branches are covered by mocking the Kubernetes provider (the pattern already used in `core/platform` and `system/cozystack-api`): adopted-only, renamed-only, both sets, zoned volumes, a non-default instance name, a truncated long name, and an unrelated `data1-*-volume-*` PVC that must not trip the guard. The s3-consumer suite now re-creates the name/fullname divergence explicitly, so it keeps guarding that regression regardless of the shipped default. The remaining regression test for this class of bug is an upgrade E2E (seed a pre-4.31 named set, upgrade, assert a single set with data intact) — no fresh-install E2E or `helm template` can reproduce an upgrade over pre-existing state. That belongs in the chainsaw upgrade lane and is a follow-up. ### Screenshots N/A — no UI changes. ### Release note ```release-note fix(seaweedfs): adopt existing SeaweedFS workloads and volumes in place when upgrading past the 4.31 chart rename, instead of standing up a second, empty cluster beside them ``` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Added an audit & recovery runbook for SeaweedFS 4.31 rename impacts. * **Bug Fixes** * Stabilized SeaweedFS workload/PVC naming across upgrades, including non-default instance names. * Added preflight safeguards to prevent upgrade scenarios that could split or wedge existing data. * Updated post-delete cleanup to remove secrets from both legacy and current naming schemes. * **Tests** * Expanded Helm unit coverage for naming guards, cleanup behavior, and S3 service name consumers. <!-- end of auto-generated comment: release notes by coderabbit.ai --> |
||
|
Aleksei Sviridkin
|
0238231303 |
fix(linstor-scheduler): re-vendor to upstream 0.3.1 with native admission webhook (#3201)
## What this PR does Re-vendors the `linstor-scheduler` chart from upstream **0.3.1** (appVersion **v0.3.6**). Upstream now ships the admission webhook natively (contributed via [piraeusdatastore/helm-charts#94](https://github.com/piraeusdatastore/helm-charts/pull/94)), so the loose downstream admission templates — which round-tripped every Pod through an old `k8s.io/api` and silently stripped fields they did not know about, the `image` volume source and the native-sidecar `restartPolicy` among them, breaking KubeVirt containerDisk VMs and native sidecars cluster-wide — are dropped in favour of the upstream copy. - The admission webhook comes from the upstream chart now; the previous downstream copy and its patch are removed. `make update` pulls 0.3.1 and applies only `cozystack-scheduler-deployment.patch` (the scheduler pod component label and the distro-suffix-stripped kube-scheduler image tag, neither of which is upstream yet), so vendoring stays deterministic and reproducible. - The upstream admission Deployment keeps the same name but changes its immutable pod selector, so an in-place upgrade of an existing install would fail with `field is immutable`. Migration 52 deletes the old admission Deployment before the upgrade so the new one is recreated cleanly (`targetVersion` 52 → 53). - appVersion v0.3.6 also carries the webhook's TLS certificate hot-reload fix ([piraeusdatastore/linstor-scheduler-extender#22](https://github.com/piraeusdatastore/linstor-scheduler-extender/pull/22)): the webhook used to load its certificate once at startup, so after the mounted Secret was rotated it kept serving a stale certificate and — with `failurePolicy: Ignore` — silently stopped mutating until it was restarted. It now reloads on rotation, so no external cert-reload workaround is needed here. - Wrapper values keep `replicaCount: 2`, now that the admission Pods have their own PodDisruptionBudget and no longer share the scheduler's. - Adds a helm-unittest suite covering the admission resources, PodDisruptionBudget selector isolation, least-privilege RBAC, the TLS provisioning modes and their rejection paths, the scheduler image-tag strip, and backward compatibility with the webhook disabled. Closes #3198. ### Screenshots N/A — no UI changes. ### Downstream repositories I walked the trigger map against the diff, file by file. It touches `packages/system/linstor-scheduler/` and, under `packages/core/platform/`, only migration 52 and the `migrations.targetVersion` bump — an internal, release-managed counter, not a user-facing `spec.components.platform.values.*` key documented on the website. No trigger matches. - [x] No downstream repository is affected by this change - [ ] [cozystack/website](https://github.com/cozystack/website) - follow-up: - [ ] [cozystack/terraform-provider-cozystack](https://github.com/cozystack/terraform-provider-cozystack) - follow-up: - [ ] [cozystack/ansible-cozystack](https://github.com/cozystack/ansible-cozystack) - follow-up: - [ ] [cozystack/ccp](https://github.com/cozystack/ccp) - follow-up: - [ ] [cozystack/talm](https://github.com/cozystack/talm) - follow-up: - [ ] [cozystack/cozyhr](https://github.com/cozystack/cozyhr) - follow-up: - [ ] [cozystack/cozy-proxy](https://github.com/cozystack/cozy-proxy) - follow-up: - [ ] [cozystack/cozystack-telemetry-server](https://github.com/cozystack/cozystack-telemetry-server) - follow-up: - [ ] [cozystack/external-apps-example](https://github.com/cozystack/external-apps-example) - follow-up: - [ ] [cozystack/examples](https://github.com/cozystack/examples) - follow-up: ### Release note ```release-note fix(linstor-scheduler): the LINSTOR scheduler admission webhook no longer strips unknown Pod fields (the `image` volume source and native-sidecar `restartPolicy`), which had broken KubeVirt containerDisk VMs and native sidecars, and it now reloads its TLS certificate after rotation instead of silently failing. The chart is re-vendored from upstream 0.3.1; an automatic upgrade migration replaces the admission Deployment whose immutable selector changed. ``` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Expanded the optional Linstor Scheduler admission webhook with richer configuration, including automated `schedulerName` mutation for LINSTOR CSI-backed PVC Pods. * **Improvements** * Updated the Linstor Scheduler Helm chart to **0.3.1** (scheduler app **v0.3.6**), including updated admission webhook behavior, safer defaults, and **2 replicas**. * Platform migrations now advance to **version 53**, with upgrade-safe cleanup of the existing admission deployment. * **Documentation** * Updated Helm README and post-install notes with detailed guidance for admission webhook and TLS modes. <!-- end of auto-generated comment: release notes by coderabbit.ai --> |
||
|
Andrei Kvapil
|
fbbe4af7df |
refactor(network): drop ExposureClass/ServiceExposure for native loadBalancerClass (#3218)
## What Removes the `network.cozystack.io` API group — `ExposureClass` + `ServiceExposure` (#3081) — and its reconciler, in favor of native `Service` `type: LoadBalancer` + `loadBalancerClass`. ## Why Full rationale and discussion in #3164. For **managed applications** the chart already owns the Service, so: - `type: LoadBalancer` + `loadBalancerClass` + an admin-provisioned pool covers vendor-neutral external exposure without a dedicated API group and controller; - the `externalIPs` (node-IP) default never went through `ServiceExposure` (its backend is a no-op) and is left unchanged; - the only current consumer, the host ingress, keeps rendering `type: LoadBalancer` and just drops the `ServiceExposure`; the admin provisions the address pool / announcer out of band, as the install docs already assume. `ServiceExposure`'s one unique feature — dynamic per-namespace scoping of a shared VIP pool — is a convenience the managed-app path does not need, and it does not justify a second tenant-networking API group next to `sdn.cozystack.io`. ## Changes - Delete `api/network/v1alpha1` and `internal/controller/serviceexposure` (+ CRD definitions, e2e bats, controller registration, RBAC). - Host ingress: drop the `ServiceExposure` template; add an optional `publishing.loadBalancerClass` that sets `spec.loadBalancerClass` on the host-ingress LoadBalancer Service. - The `externalIPs` path and default behavior are unchanged. ## Verification - `go build ./...` — green. - `helm unittest` — `core/platform` 72/72, `extra/ingress` 25/25 (incl. host-vs-tenant `loadBalancerClass` scoping and the immutable-field migration guard). ## Migration None required. `network.cozystack.io` (#3081) has never shipped in a release — it is on `main` only (not backported to `release-1.5`; absent from v1.4.x / v1.5.x), so no upgrading cluster has these CRDs and this removes it before it is ever released. Clusters that track `main` and explicitly set `publishing.exposureClass` (opt-in, only since 2026-06-30) should provision their MetalLB / Cilium address pool themselves and set `publishing.loadBalancerClass`; any orphaned `cozystack-<class>` pool can be deleted by hand. Default clusters (externalIPs path) are unaffected. ## Status **Draft / RFC for the #3164 discussion** — this removes an API group that landed only days ago, so nothing here should merge before the design call. cc @lexfrei (author of #3081) and @lllamnyp (#3164). Deferred: when Kubernetes actually removes `Service.spec.externalIPs`, the node-IP default migrates via a MetalLB `IPAddressPool` with no `L2Advertisement` (allocate-but-don't-announce) — a separate change. <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Added support for configuring the ingress host Service with a native `loadBalancerClass` value. * Expanded chart tests to verify class wiring and immutability behavior across install and upgrade scenarios. * **Bug Fixes** * Removed the legacy exposure-class/service-exposure flow and related controller behavior. * Updated templates and CRD packaging so the platform no longer renders or depends on those deprecated resources. <!-- end of auto-generated comment: release notes by coderabbit.ai --> |
||
|
Aleksei Sviridkin
|
f4aef73053 |
fix(linstor-scheduler): re-vendor to upstream 0.3.1 with native admission webhook
Re-vendor the linstor-scheduler chart from upstream 0.3.1 (appVersion v0.3.6), which carries the admission webhook binary that stopped stripping unknown Pod fields — the image VolumeSource and native-sidecar restartPolicy, which broke KubeVirt containerDisk VMs and native sidecars cluster-wide — and now reloads its TLS certificate on rotation instead of serving a stale one until restarted. The admission webhook ships in the upstream chart, so the previous downstream copy is dropped: `make update` pulls it directly and applies only cozystack-scheduler-deployment.patch (pod component label and a distro-suffix-stripped image tag), keeping the vendoring deterministic. Because the webhook now reloads its own certificate, the external cert-reload workaround is dropped. The upstream admission Deployment keeps the same name but changes its immutable pod selector, so an in-place upgrade of an existing install would fail. A migration deletes the old admission Deployment before the upgrade so the new one is recreated cleanly, and bumps the migration target version. Add a helm-unittest suite covering the admission resources, PDB selector isolation, least-privilege RBAC, the scheduler image tag, and backward compatibility. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <f@lex.la> |
||
|
Aleksei Sviridkin
|
d542f66368 |
fix(ouroboros): render controller rollout strategy in every mode
Bump the chart and controller image to 0.8.1, which renders the controller Deployment's rollout strategy in every controller.mode instead of only in external-dns mode. Up to 0.8.0 the chart rendered a strategy only in external-dns mode, as type: Recreate, and nothing in the other modes. The apiserver defaults a rollingUpdate block onto a Deployment created without one, so switching a live release into external-dns laid Recreate on top of that defaulted block and the apiserver rejected the object: spec.strategy.rollingUpdate: Forbidden: may not be specified when strategy `type` is 'Recreate' The strategy is now identical across all modes, so no mode switch changes a strategy field at all. This is a latent break rather than an active one: the host pins coredns and tenants pin coredns-import, neither of which rendered a strategy in 0.8.0, and no shipped configuration selects external-dns. But controller.mode is user-settable and the chart supports the mode, so the transition is reachable by configuration. It surfaces only under server-side apply, which helm-controller uses by default, so no local helm upgrade reproduces it: a client-side strategic merge carries retainKeys on strategy and clears rollingUpdate on the way through. The rendered strategy is pinned on all three modes, external-dns included — it is the only mode that ever rendered Recreate, so a bump that re-gated the block would otherwise slip past a coredns-only suite. 0.8.1 also carries an upstream bump of the cleanup-hook kubectl image to 1.36.1. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <f@lex.la> |
||
|
Myasnikov Daniil
|
f1619215ff |
docs(api): note disableWait silently defeats healthCheckExprs
The field docs already say healthCheckExprs are only evaluated under the poller wait strategy, but that is only half the precedence rule. Upstream evaluates them only when the Helm action itself has wait enabled (helm-controller/api@v1.5.1 v2/helmrelease_types.go:193-199), so the release.cozystack.io/helm-install-disable-wait annotation makes them a silent no-op whatever waitStrategy says — no error, no warning, the release just reports Ready as before. This is reachable today: packages/system/kubernetes-rd already sets that annotation and is a plausible candidate for CEL expressions in phase 3. Document it on the ApplicationDefinition field (so it reaches the CRD description) and next to HelmInstallDisableWaitAnnotation. ComponentInstall has no disableWait, so its field doc is unchanged. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com> |
||
|
Myasnikov Daniil
|
96a9652fb1 |
feat(api): typed waitStrategy/healthCheckExprs for kstatus readiness
Add first-class fields so a package/app can declare the Flux helm-controller v1.5.0 kstatus health gate for the custom resource it wraps, so HelmRelease.Ready means "the CR is actually healthy" instead of "helm applied the CR" (#2642). API-owner approved in #3263. - api: WaitStrategy (scalar, maps to HelmReleaseSpec.WaitStrategy.Name — a deliberate simplification) + HealthCheckExprs ([]kustomize.CustomHealthCheck, the upstream type verbatim) on both ApplicationDefinition.spec.release (the apiserver path) and ComponentInstall (the operator path). - config.ResolveWaitStrategy: shared by both HelmRelease builders. Couples the default — healthCheckExprs imply the poller strategy when none is set, since exprs are only evaluated under poller. This removes the "must also set waitStrategy: poller" footgun and does not depend on the controller's global default. - thread through buildHelmReleaseSpec (operator) and convertApplicationToHelmRelease (apiserver); the apiserver loads the typed spec.release fields in start.go. - regenerate deepcopy, the ApplicationDefinition and PackageSource CRDs, and openapi. The backupstrategy CRDs also regenerate — benign k8s-0.35 PodSpec description drift, not a functional change. disableWait is intentionally not added (YAGNI): the only known need (the kubernetes tenant-addon deadlock) was resolved child-side, and waitStrategy: legacy is not equivalent (legacy still waits Helm-v3-style; disableWait skips waiting entirely). Add it later when a concrete leaf needs a true no-gate escape hatch. Refs #2642 Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com> |
||
|
myasnikovdaniil
|
f6a03e93f8 |
fix(etcd): complete v1alpha2 transition for in-cluster 1.5→1.6 upgrades (#3270)
Consolidates the etcd `v1alpha2` transition fix for **in-cluster 1.5 → 1.6 upgrades** into a single PR, rebased on current `main`. Supersedes #3265 and #3261 — their commits are carried here (authorship preserved), so those PRs can be closed once this lands. Fresh installs use the new shapes directly and are unaffected, so fresh-install CI does not catch these; the 1.5 → 1.6 e2e upgrade path is the authoritative regression guard. ## What this PR does ### 1. Keep the legacy `etcd-headless` Service alive during adoption — `packages/extra/etcd` `etcd-migrate` adopts legacy clusters **in place**: the Pods keep their original `spec.subdomain: etcd-headless` and are dialed at `etcd-<i>.etcd-headless.<ns>.svc` until they roll onto the operator's native `<member>.etcd.<ns>.svc` domain. The v1alpha2 operator only creates the native `etcd` Service and the legacy `etcd-headless` Service is pruned, so those per-pod names stop resolving (`no such host`), `MemberList` fails, and `status.readyMembers` never populates — the `EtcdCluster` never goes `Ready` even though etcd is healthy and in quorum. We ship a chart-managed transitional headless `etcd-headless` Service (selector mirrors the operator's native `etcd` Service via `etcd-operator.cozystack.io/cluster`, `publishNotReadyAddresses: true`) — the DNS counterpart of the legacy `*.etcd-headless.<ns>.svc` SAN already kept for this window. Removable together with that SAN once members roll onto the native subdomain. ### 2. Survive the immutable controller-Deployment selector on upgrade — `packages/system/etcd-operator` (#3242) #2859 replaced the upstream etcd-operator chart with the cozystack-authored one, changing `Deployment.spec.selector.matchLabels`. `spec.selector` is immutable, so `helm upgrade` cannot patch the existing Deployment and the whole HelmRelease upgrade fails (`field is immutable`). A **pre-upgrade hook** (`templates/pre-upgrade-selector-fix.yaml`: ServiceAccount + Role + RoleBinding + Job) deletes the Deployment **only** when its live selector is the pre-1.6 one, so Helm recreates it cleanly. No-op when the selector already matches, never runs on fresh install. ### 3. Raise the operator's memory cold-start floor — `packages/system/etcd-operator` Steady-state working set is ~250Mi; the static `limits.memory: 128Mi` OOMKills a Pod that starts before the VPA admission webhook rewrites it. Raise the floor to `256Mi` (and VPA `minAllowed` to match) as defense in depth so the operator never depends on VPA timing to avoid crashing. ### 4. Make migration 50 (etcd adoption) robust in-cluster — `packages/core/platform/images/migrations/migrations/50` (was #3261) Exact server/peer cert-SAN match, Secret-gated wait on the adoption Secret, and in-cluster (IPv6-safe) kubeconfig handling, so the migration script drives the adoption reliably from inside the cluster. Covered by `hack/migration-50-etcd-adopt.bats`. ### 5. Hardening / review fixes (this PR's original scope) - **Hook `runAsUser: 65532`.** `pre-upgrade-selector-fix.yaml` set `runAsNonRoot: true` but no numeric `runAsUser`; `clastix/kubectl`'s image user is the non-numeric name `nonroot`, which the kubelet cannot verify against `runAsNonRoot` — so the hook Pod fails admission and silently blocks the very upgrade it exists to unblock. Adds `runAsUser: 65532`, matching the postgres-operator webhook-ready hook that runs the same image. - **Digest-pin the kubectl image.** The values comment claimed digest-pinning but shipped a floating `v1.32` tag. Reuses the digest postgres-operator vendors, adds the `renovate` annotation, and templates `repo:tag@digest`. - **Tests.** `etcd-operator/tests/selector-fix-hook_test.yaml` (hook wiring, weight ordering, namespaced least-privilege RBAC, numeric-non-root security context, digest-pinned image), `etcd-operator/tests/deployment_test.yaml` (256Mi cold-start floor), `extra/etcd/tests/etcd-cluster_test.yaml` (transitional `etcd-headless` Service). Each assertion was mutation-tested. ### 6. Derive the default S3 endpoint from the provisioned bucket — `packages/system/backupstrategy-controller` Derive the default S3 endpoint (and per-driver scheme / TLS / `secure_connection`) from the provisioned bucket Secret instead of requiring it to be hand-set, so etcd (and other) backup strategies get a working endpoint by default. Docs in `docs/operations/backup-classes.md`; covered by `tests/endpoint_form_test.yaml`. ### Verification - `helm unittest` green on current `main`: etcd-operator **18/18**, extra/etcd **18/18**, backupstrategy-controller **11/11**. - Behaviours 1 & 2 were reproduced and confirmed live on a 1.5.2 → 1.6.0-rc.1 adoption (3-node cluster) — recreating the `etcd-headless` Service took the adopted cluster to `readyMembers=3 / Available=True`. ```release-note fix(etcd): complete the v1alpha2 transition on in-cluster 1.5→1.6 upgrades — keep the legacy etcd-headless Service alive so adopted members stay resolvable, delete the pre-1.6 operator Deployment via a pre-upgrade hook to get past the immutable selector, raise the operator's memory floor so it does not OOM before the VPA scales it, and make the etcd adoption migration robust in-cluster. ``` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Improved backup storage endpoint handling across supported backup drivers, including provisioned and external S3 storage. * Added compatibility support for legacy etcd pod discovery during migration. * Added an automated upgrade safeguard for etcd operator deployments. * **Bug Fixes** * Improved certificate SAN detection and etcd migration authentication. * Increased the etcd operator’s minimum startup memory to prevent early restarts. * **Documentation** * Clarified backup endpoint, TLS, and driver-specific behavior. <!-- end of auto-generated comment: release notes by coderabbit.ai --> |
||
|
Myasnikov DaniilandClaude Opus 4.8
|
4af5a82796 |
fix(etcd-operator,backups): address review nits — trim endpoint + hook resources
- backupstrategy-controller: trim() the decoded S3 endpoint before scheme stripping so a trailing newline in the Secret can't yield a malformed URL - etcd-operator: give the pre-upgrade selector-fix hook container resource requests/limits (matches postgres-operator webhook-ready), +unittest Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com> |
||
|
Myasnikov DaniilandClaude Opus 4.8
|
e160a0dc87 |
fix(backups): derive default S3 endpoint from the provisioned bucket
The default backupStorage.endpoint is http://seaweedfs-s3...svc:8333, but Cozystack ships SeaweedFS with global.seaweedfs.enableSecurity=true, so its in-cluster S3 serves TLS on :8333 behind the self-signed "SeaweedFS CA". Every cozy-default backup (etcd/mariadb/velero/fdb/cnpg) therefore hits a TLS listener over plaintext and fails the handshake. This is fatal for the etcd v1alpha2 adoption migration: its mandatory pre-upgrade safety snapshot writes to this endpoint, so migration 50 hard-fails and blocks the 1.5->1.6 upgrade on any cluster with a legacy etcd. The Etcd Strategy S3 schema has no caCert/insecureSkipVerify field, so it cannot target the self-signed in-cluster endpoint at all. Add a "backupstrategy-controller.endpoint" helper that, for a provisioned bucket, resolves the endpoint from the COSI bucket's system credentials Secret (backupStorage.systemSecretName) — the external S3 ingress with an ACME cert, the same trusted endpoint COSI advertises and every backup operator can verify — and forces the https:// scheme. All Strategy CRs, the Velero BackupStorageLocation and the controller Deployment env are routed through it. It falls back to .Values.backupStorage.endpoint for external S3 (provisionBucket=false) and for offline `helm template`/unit renders and the pre-reconcile first install, where the Secret lookup returns nothing (Flux re-renders on spec.interval once the Secret exists). helm-unittest extended to assert every consumer picks up the resolved endpoint (CNPG/Etcd/Velero keep the full URL, MariaDB/FDB strip the scheme and derive their secure flag) plus the external-S3 verbatim fallback. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com> |
||
|
Myasnikov DaniilandClaude Opus 4.8
|
ca4f99411f |
fix(etcd): harden v1alpha2 transition — hook runAsUser, digest-pin kubectl, tests
Addresses review on #3265: - pre-upgrade-selector-fix hook: add runAsUser: 65532. clastix/kubectl's image user is the non-numeric name `nonroot`, which the kubelet cannot verify against runAsNonRoot: true — so the Pod fails admission and the hook never runs, silently blocking the very 1.5->1.6 upgrade it exists to unblock. Matches postgres-operator's webhook-ready hook (same image). - Digest-pin the kubectl image (the comment already claimed digest-pinning but shipped a floating v1.32 tag). Reuse the digest postgres-operator already vendors, add the renovate annotation, and template repo:tag@digest. - Tests (both charts have CI helm-unittest suites, none previously covered these): - etcd-operator/tests/selector-fix-hook_test.yaml: hook wiring, namespaced least-privilege RBAC, numeric-non-root securityContext (guards the runAsUser regression above), digest-pinned image. - etcd-operator/tests/deployment_test.yaml: assert the 256Mi cold-start memory floor so it can't silently drop back to an OOMKilling value. - extra/etcd/tests/etcd-cluster_test.yaml: assert the transitional etcd-headless Service (headless, publishNotReadyAddresses, member selector, client/peer ports). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com> |
||
|
Andrey Kolkov
|
9daaa9ff5a |
fix(etcd): fix etcd-operator 1.5->1.6 upgrade (immutable selector + OOM floor)
Two defects that break the etcd-operator on an in-cluster 1.5 -> 1.6 upgrade (#2859 swapped the upstream chart for the cozystack-authored one): - #3242: the controller Deployment's spec.selector.matchLabels changed ({instance,name} -> {name,control-plane}). spec.selector is immutable, so `helm upgrade` cannot patch the existing Deployment and the whole HelmRelease upgrade fails ("field is immutable") -- the operator and the v1alpha2 CRDs it serves never come up, which blocks the etcd v1alpha2 adoption. Fresh installs use the new selector directly, so fresh-install CI does not catch it. Add a pre-upgrade hook that deletes the Deployment ONLY when its live selector is the pre-1.6 one (lacks control-plane=controller-manager), so Helm recreates it cleanly. No-op when the selector already matches (rc.1 -> later) and never runs on a fresh install (pre-upgrade only). Keeping the selector stable in the chart is not an option: rc.1 already shipped the new selector, so aligning it back would merely move the immutable break to rc.1 -> next. - Raise the manager's cold-start memory limit floor 128Mi -> 256Mi (and the VPA minAllowed to match). The steady-state working set is ~250Mi (the VPA's own recommendation); at 128Mi a Pod that starts before the VPA admission webhook rewrites it (e.g. a Deployment recreated out of band, or the webhook briefly unavailable during upgrade) OOMKills into a crash loop. Defense in depth so the operator never depends on VPA timing merely to avoid crashing; the VPA still scales it further under load up to maxAllowed. Verified live on a 1.5 -> 1.6 adoption: the delete-Deployment step plus the VPA re-applying 256Mi+ let the operator come up and the adoption complete. Refs: #3242, #3243 Signed-off-by: Andrey Kolkov <androndo@gmail.com> |
||
|
Ivan Okhotnikov
|
0164a6afa4 |
fix(monitoring): set groups_attribute_path=groups so allowed_groups actually works on Grafana v11.x
Verified end-to-end on dev3 with Grafana v11.6.15: OAuth flow reaches Grafana, id_token and userinfo response both carry the correct `groups` claim (`[cozystack-cluster-admin, tenant-root-super-admin]` for kvaps), `allowed_groups` in grafana.ini lists `tenant-root-super-admin`, and login still fails with `user not a member of one of the required groups`. Grafana debug log shows `Attributes: map[]` — the extracted user's Groups slice is empty. On v11.x+ Grafana does not auto-map the `groups` claim onto user.Groups unless `groups_attribute_path` is set explicitly (the field is documented as optional but the gate is a silent no-op without it — a claim-name-string is not enough). Fix: hard-wire `groups_attribute_path=groups` in the chart-forced System-mode block alongside `allowed_groups`. "groups" points at the top-level `groups` array from userinfo, matching the shape Keycloak's oidc-group-membership-mapper emits (verified on the same dev3 run). Two helm-unittest cases updated to assert `groups_attribute_path` lives next to `allowed_groups` in both the users-map-active and empty-users branches (rejecting the silent no-op regression). Docs also updated to name the requirement. Signed-off-by: Ivan Okhotnikov <xorokhotnikov@gmail.com> |
||
|
IvanHunters
|
b002c967ac |
fix(monitoring): close cross-tenant Grafana OIDC hole via tenant-group gate
Round-3 review from lllamnyp: any user in the shared `cozy` Keycloak
realm could sign into any tenant's Grafana. The audience mapper stops
replay of a minted token across releases, but does not gate who may
mint one — tenant-alice could complete the OAuth flow against
tenant-bob's Grafana. With `spec.oidc.users` non-empty the earlier
`allow_sign_up=false` fix mostly closed the door (though tenant-alice
still hit `error 401` after landing in Grafana, revealing the release
exists); with empty users under mode: System — a config the schema
permits and the docs describe as hands-off — the login succeeded and
minted a Viewer account for every cozy-realm identity.
The fix uses the KeycloakRealmGroups the tenant chart already
provisions (`<namespace>-{view,use,admin,super-admin}` in
packages/apps/tenant/templates/keycloakgroups.yaml) — no new
directory objects owned by the Monitoring release. Two chart edits:
1. oidc-keycloak.yaml: add the platform-managed realm-default `groups`
scope to `defaultClientScopes` explicitly. EDP's KeycloakClient
does NOT merge realm-level defaults into `defaultClientScopes`, so
the scope must be listed here or the `groups` claim would be
missing from the token.
2. grafana.yaml: in System mode, unconditionally emit
`allowed_groups: <ns>-view <ns>-use <ns>-admin <ns>-super-admin`
in `auth.generic_oauth` and add `groups` to `scopes`. Grafana
rejects any login whose token does not carry at least one of the
groups.
The gate is chart-owned and does not depend on the users-map — with
`users: []` tenant members still get a self-service Viewer through
`auto_assign_org_role`, cross-tenant users get 401 in Keycloak's OAuth
callback. Docs updated to reflect the two-layered contract
(tenant-membership gate + optional users-map).
Signed-off-by: IvanHunters <xorokhotnikov@gmail.com>
|
||
|
IvanHunters
|
f64ba91872 |
fix(monitoring): do not render users-Job when spec.oidc.users is empty
Address PR#3176 review round-2 gap-2: the users-Job's prune pass deletes every non-admin Main-Org member whose email is not in the desired list. When the desired list is empty every helm-upgrade wiped the whole org. Three empty-users states used to fall into this trap: * customConfig.secretRef: users[] is forbidden by assertCustomSecretRefUsersEmpty, so the desired list was structurally always empty and the chart wiped an org whose auth.ini fragment the operator advertised as authoritative. * System with users[] unset: the operator opted into OIDC but not into the chart-managed users-map, yet the Job still pruned every Main-Org member on their behalf. * CustomConfig-inline with users[] unset: same shape as System. The mode=None guard (from an earlier round) already had the same shape; extend the render gate to cover the other empty-list doors — now the Job renders only when `mode != None AND users non-empty`. The trade-off: taking `users:` back to `[]` no longer prunes the last entry — matching the already-documented `System → None` posture — called out explicitly in the docs. Signed-off-by: IvanHunters <xorokhotnikov@gmail.com> |
||
|
IvanHunters
|
ff4ef2c189 |
fix(monitoring): force allow_sign_up=false and gate isolation dict on users-map
Address PR#3176 review round-2 gap-1: the docs promise "users not in spec.oidc.users get nothing", but Grafana's default allow_sign_up=true combined with the chart-forced skip_org_role_sync=true was minting a Viewer account for every cozy-realm identity that completed the OAuth flow, then reaping it on the next helm-upgrade prune pass. The chart-forced isolation dict (now three keys: skip_org_role_sync, oauth_allow_insecure_email_lookup, allow_sign_up=false) is applied only when spec.oidc.users is non-empty. A BYO-IdP operator who does not use the users-map gets none of the three forced on top of their inline config, matching the review's minor: forcing insecure_email_lookup against an issuer that may not verify emails is a needless account- linking surface, and allow_sign_up=false with no pre-provisioned accounts would lock everyone out. Tests split the two branches: users-active asserts all three keys are present and the operator cannot override them via CustomConfig-inline; users-empty asserts the auth.generic_oauth block is emitted without any of the isolation keys. Signed-off-by: IvanHunters <xorokhotnikov@gmail.com> |